sick mrs1000 driver

[spuetz]

The pull request concerns the sick mrs 1000 support. The initialization of the device has to be different, due to that I have made the methods for initialization virtual and now the mrs1000 driver runs different init code. Also the support for PointCloud2 is new. Sry for not having enough separate commits for the different stages of the code, I could not really test these interdependent code fragments, also I saw a limit in spending my time in a driver which needs – in a long term perspective – a rewrite. Probably I will have time for that in 2018, sometime. cheers

[spuetz]

Thank you both. It's much faster and saver with six eyes. I had not the time for a better version. And yes the structure is meant for laser scans which can be received in one single parsing action, so that caused the accumulation of the cloud. So it would be better to change that structure in the future. A huge disadvantage of publishing each line for it's own is that one then needs to synchronise and assemble it again, that's not really practical. I will have time next week for answering the comments and changing stuff. Thank you!

[jspricke]

Am 10. Januar 2018 23:44:10 MEZ schrieb "Sebastian Pütz" notifications@github.com:

A huge disadvantage of publishing each line for it's own is that one then needs to synchronise and assemble it again, that's not really practical.

You could use a laser assembler for that. ;)

[jspricke]

What about extracting the angle offset of the other layers into tf and let ROS compute the points?

[spuetz]

You could use a laser assembler for that. ;)

The current version of the laser_assembler is a bit slow and ugly, it converts all point clouds to PC1 and back to PC2, I had a pull request for that, but there were no reaction on that. Also it means that you always have the overhead of assembling. Because in nearly all cases you want to have the whole cloud. I like a version for a whole cloud more, it is much more intuitive. Then it would be better to add an optional channel for point stamps,

[spuetz]

What about extracting the angle offset of the other layers into tf and let ROS compute the points?

For that you would need a transformation for each point, because they do not lie in a plane. Or: you could publish ~>180 vertical sensor_msgs::LaserScan msgs combining four layers each time.

[jspricke]

For that you would need a transformation for each point, because they do not lie in a plane. Or: you could publish ~>180 vertical sensor_msgs::LaserScan msgs combining four layers each time.

That's not a requirement for a LaserScan. It only needs to be taken from one point, so we can transform it into Cartesian using the right coordinate frames.

[spuetz]

That's not a requirement for a LaserScan. It only needs to be taken from one point, so we can transform it into Cartesian using the right coordinate frames.

Aaa you mean the four point clouds? -> four tf transformations? that's fine. Otherwise, I don't understand. For now we have only one LaserScan (in the xy-plane) The other points can not represented by a LaserScan, because they are measured in a Spherical coordinate system.

[jspricke]

Aaa you mean the four point clouds? -> four tf transformations? that's fine.

No four LaserScans.

Otherwise, I don't understand. For now we have only one LaserScan (in the xy-plane) The other points can not represented by a LaserScan, because they are measured in a Spherical coordinate system.

That's really not a requirement!

I see two ways for the non planar points:

1. Transform them inside the scanner and expose them as individual point clouds.

2. Expose them as LaserScans together with the correct frame, so ROS can do the transformation into Cartesian.

[spuetz]

1. Transform them inside the scanner and expose them as individual point clouds.

Possible!

2. Expose them as LaserScans together with the correct frame, so ROS can do the transformation into Cartesian

Impossible! For a sensor_msgs::LaserScan there is the requirement, that the points of one LaserScan lie in one plane and that they have a fixed angle increment

Btw. that is the reason why the code in the sick repo is wrong. The transformation there is not correct.

[mintar]

Oops, accidentally clicked on "approve changes". Anyhow, @spuetz , do you have time to do this? It doesn't seem that there's a lot of work remaining.

[spuetz]

This afternoon... ;)

[spuetz]

Hey guys, I did the requested changes -- thx again for the detailed review! I'll test it with the scanner on Monday, just to be sure that everything works and get back to you then. Cheers -- Sebastian

[mintar]

I like what you've done with current_config_! I think that's the cleanest way to make sure that the config is consistent during the 4 scans from the same sweep.

[jspricke]

Apart from the frequency discussion above, I'm fine. Thanks for working on this! One future wish: Have the intensity field in the point cloud as well.

[spuetz]

Japp I'll integrate the intensity soon.

[spuetz]

But it works fine:

[jspricke]

No need to merge, we will do it once it's ready ;). I would propose to publish on a global topic, as the scan is global as well (although I think a local one would be better in general).

[spuetz]

Done.

[mintar]

AFAICT, these are the last remaining TODOs from the comments above. Correct?

• [x] change expectedFrequency_ from 50 to 12.5 Hz, test with rosrun rqt_robot_monitor rqt_robot_monitor

Edited to add:

• [x] cloud time stamp should be the ROS time when the first message was received, not the last

Once that's done, this is ready to merge from my part.

[jspricke]

I just had a look at the timestamp the cloud would get and as far as I read it would be the ros time when the last message was received. Shouldn't it be the time when the first one was there?

[mintar]

Shouldn't it be the time when the first one was there?

Yep, it should. I'll add it to the list so we can tick it off when it's done.

[mintar]

@spuetz must hate us by now... sorry! :)

[spuetz]

All fine! 👍

[jspricke]

@mintar feel free to do a squash and merge if you are fine with it.