-
Notifications
You must be signed in to change notification settings - Fork 640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dual Return #208
Dual Return #208
Conversation
@JWhitleyAStuff ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review my suggestions in the code. I definitely agree with your overall approach and I think the changes make sense. However, there is at least one bug preventing CI from passing and, as you mentioned, it needs testing on other sensors.
@@ -327,7 +329,9 @@ inline float SQR(float val) { return val*val; } | |||
float x, y, z; | |||
float intensity; | |||
|
|||
const raw_packet_t *raw = (const raw_packet_t *) &pkt.data[0]; | |||
const raw_packet_vlp16_t *raw = (const raw_packet_vlp16_t *) &pkt.data[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead and change the VLP-16-specific stuff to be sensor-generic and I'll test the changes on a VLP-32C and an HDL-64E.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible for you to send me a PCAP of each? I made the raw_packet_vlp16_t to access the factory bytes (the packet definition is different from the rest of the lidars) I could technically just follow the datasheets to modify the other unpack
function but it will likely be wrong without a pcap for me to test.
Also I will squash all the commits once it is ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Andre. I'm working on getting you PCAPs for the two sensors I mentioned.
6a56d52
to
1898524
Compare
@andre-nguyen - Sorry I haven't been able to get those pcap files yet. It's still on the list for my support team but they're pretty bogged down at the moment. On another note, I've changed CI vendors for this repo and applied |
1efe00d
to
285a013
Compare
@JWhitleyAStuff Rebased and squashed on master. Still wouldn't merge until we have tested with multiple datasets. Also, I sort of figured out through trial and error that my build wasn't going through due to roslint failures but I couldn't see that in the build logs. May I suggest changing the build scripts to put roslint as a separate stage in the pipeline and explicitely building Side note: May I also suggest we change the CircleCI build script line |
Suggestions implemented in #231. Please merge again and verify that it looks good. |
285a013
to
7b1d0a6
Compare
Done deal, looks much neater! Let me know when those extra datasets come in. |
@JWhitleyAStuff |
@andre-nguyen I'm very sorry I haven't been able to provide the datasets. This PR is still valid and I'd like to get to it in the future but my priorities keep shifting. Can you please rebase it on |
🤷♂️ Glad you're still interested in this PR! The next few weeks are a bit loaded on my end, can you ping me again when you have data and I'll rebase and test everything in one go? |
Hi. Issue 2: There is a dramatic change in dual return characteristics across the 0 degree angle of the sensor within the same frame of data (cable coming out of the back is considered +180 degrees). Description of the images Not sure you are still working this, but any advice or help would be greatly appreciated. Thanks. |
Off the top of my head I cant see why data from one side of the lidar would be treated differently from the other such that you would get that response. Unfortunately I'm in the process of moving right now and I won't be able to look at this for another two weeks. 2 things would help:
|
@andre-nguyen I'm sorry that I was unable to get the data you requested. The priorities at AS just never made it possible. Unfortunately, I have now moved on to another job, making it impossible for me to gather the requested data. Given that and the large amount of changes that have been made to |
I am, though I might just close the whole MR outright and open a new one. Funnily enough, I too have moved to a new job and had access to an HDL-64 until covid hit. However, I do not have access to my origin VLP16-dual return data set because it belonged to my previous job. I'll get in contact with you by email. |
A much easier way to get point clouds in the dual return mode is: Just set It should be same for other types of sensors. |
@JWhitleyAStuff Hi There,
happy new year. This PR isn't complete but I'm posting this up to start a discussion. With this PR dual return works for my vlp16 dataset. The flashing in rviz is gone and all the points are tagged with their return type. There is also a check to see between two blocks to see if there was only one return and to prevent adding the same points in the cloud.
Since the return is encoded in the point cloud, you can visualize the returns in rviz as follows.
Here are the key implementation details:
I'd be interested in generalizing this to other lidars however I don't have any datasets and I only have access to a vlp16.