[cob_sick_s300] fix bug reading not all scan points #317
Conversation
|
Testing with S300 Expert and the new protocol revealed some issues. Will reinvestigate this next week. |
mgruhler
changed the title
mgruhler
changed the title
|
@ipa-fxm @ipa-josh please review |
cob_sick_s300/README.md
Outdated
| Thus, the old Sick S300 Professional CMS as well as the new Sick S300 Expert are supported. | ||
|
|
||
| However, it does not cover the full functionality of the protocol: | ||
| - It only handle distance measurements properly |
| // Also, the size is reported as 16bit-words = 2 bytes... | ||
| // | ||
| // For the old protocol/compatability mode: | ||
| // "The telegram size is calculated ... starting with the ... 5. byte ... up to and including the ... CRC." | ||
| if(tc2_.protocol_version==0x102) |
There was a problem hiding this comment.
would it make sense to have some kind of if(0x102)-elseif(0xXXX)-else(unsupported-protocol) construct?
There was a problem hiding this comment.
Well, probably yes? However, then we could just throw a warning and stop the node. There is no other way to do this. New protocol number is 0x103 (if the documentation is correct at least this once...).
Should I add this?
There was a problem hiding this comment.
Hmm, do we have a chance to "try" the protocols supported and then exit in case it is an unsupported protocol? How would we detect whether the protocol is not supported? Would "total message length" be an indicator?
There was a problem hiding this comment.
no, total message length is not an indicator for the protocol. This changes depending on what you configure in the laser scanner itself.
I could throw an exception when there is an unsupported protocol, and "try" in the ROS (or the driver) part. What would you prefer?
There was a problem hiding this comment.
Detecting if the protocol is not supported could IMO only be done via checking the protocl number. However, this node could use some more error handling in this parser :-)
However, this would then also be another issue...
| @@ -188,7 +188,7 @@ class NodeClass | |||
| } | |||
| else | |||
| { | |||
| ROS_WARN("No params for the Sick S300 fieldset were specified --> will using default, but it's deprecated now, please adjust parameters!!!"); | |||
| //ROS_WARN("No params for the Sick S300 fieldset were specified --> will using default, but it's deprecated now, please adjust parameters!!!"); | |||
There was a problem hiding this comment.
You know how I don't like commenting warnings...
In case it's just a deprecated parameter syntax, the user should see this warning and then after setting the parameter correctly this warning won't occur anymore...
There was a problem hiding this comment.
I do. However, this is a newly introduced parameter which should deprecate the old one. But this is buggy/not properly implemented and undocumented. See issue #318
The warning pops up every time you use a laserscanner without the new parameter layout and it took me quite some time to get the syntax right. It is also not yet used with cob (at least, I did not find anything).
Thus, instead of telling people they have a deprecated parameter set, we should first make sure this one is working correctly. Also, there is another issue with this parameter as it could leed to ambiguities. I'll gladly discuss this in person...
|
@ipa-fxm @ipa-josh @ipa-fmw ping |
There was a problem hiding this comment.
FR: OK
cannot say much about the implemented functionality. I'll open new issues for the comments from @ipa-fxm: https://github.com/ipa320/cob_driver/issues/325
|
I'll approve as soon as there are a little more details in #318 |
This PR fixes #256 and fixes #276 by (more) correctly implementing the size calculations acoording to the (updated) Sick telegram listing.
Tested with:
This PR also comments out a warning about "old" parameter sets, as those parameter sets actually are buggy --> #318
@ipa-josh