search

uos / sick_tim

A ROS driver for the SICK TiM series of laser scanners.
http://wiki.ros.org/sick_tim
47 stars 90 forks source link

Hydro improved tim551 #13

Closed EgbertW closed 10 years ago

EgbertW commented 10 years ago

We had a requirement to get the intensity data out of the SICK TIM551 laser scanner. The current hydro_catkin branch supports it, but the support is limited. It is fixed at 306 fields. If you change the device name in the SOPAS ET program, the number of fields may change and this renders the driver useless. If you include intensities or change the scanning range, the number of fields will change and also breaks the driver.

The proposed merge fixes this by looking to some essential fields in the package to confirm that it is valid data instead of relying on the total amount of fields.

Also, by including intensities the package size may increase above the maximum MTU which breaks the old approach using select. The proposed merge fixes this by using boost::asio for communications instead of raw socket operations.

jspricke commented 10 years ago

Hi MadEgg, thanks for your pull request, we will make some comments inline. Would be great if you could answer them and (force) push a new version.

EgbertW commented 10 years ago

I'm sorry, I'm not quite sure what you mean by:

Would be great if you could answer them and (force) push a new version.

Do you want me to do a git rebase to squash these commits into the previous commits or do you want me to submit a new pull request?

jspricke commented 10 years ago

You can use git rebase -i to fix your patches and git push -f to update this pull request.

EgbertW commented 10 years ago

I rebased the commits and removed the superfluous lines etc. Is this better?

mintar commented 10 years ago

Nice work! I didn't even know the scanner supported intensities; we should probably enable them on our scanner, too. I just tested the parser on recorded datagrams from our scanner (without intensities), and it works.

Setting the device label was only a problem before if it included spaces, since the string isn't quoted in the datagram (which is a weakness of the datagram protocol, IMO). But nice to see that the parser can now handle that.

I didn't test the networking code, but it also looks like an improvement. @jspricke is going to test it later on our setup.

From my point, this can be merged. Thanks!

EgbertW commented 10 years ago

Thanks!

You are right about the device label. The default value of "not defined" would work, but if you would change it to something without spaces or with more than one space the driver would stop working. It is indeed a severe weakness, but fortunately, no interesting data follows the device name in the message so it's not important to get it right in my opinion.

mintar commented 10 years ago

Yes, I agree it's not super important. Also, your implementation already gets it right, whereas the old one didn't.