Skip to content
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

Add support for rev7 fw v3.0.1 ouster sensors #137

Closed
wants to merge 11 commits into from

Conversation

Imaniac230
Copy link

@Imaniac230 Imaniac230 commented Jun 6, 2023

We recently bought the a new rev7 OS-0-128 sensor with firmware v3.0.1. Using the available ros2 drivers form ouster currently still seems suboptimal even when increasing the qos buffer sizes.

I think it would be useful to have support for the newer sensors here as well, so i started an attempt to port the new additions from ouster's sdk to here. This relates to #133.

This PR is currently a heavy work in progress and the code will probably still change. I'll also probably clean up the commits and make a new PR once it is fully review ready.

At this point, however, the updated driver with all 4 processors is functional with our fw3 ouster using all of its lidar resolution and frequency modes. Unfortunately, we do not posses any older sensors, so I'm not able to verify breaking compatibility with older versions. Since this involved quite substantial changes in the client as well as the parsing code, I would find helpful any feedback to push this further to completion correctly with respect to the design goals.

Used platform:

  • Ubuntu 20.04 with ros2 foxy

I initially started with these ports on the foxy-devel branch. However, I realized that this branch is also compilable on my system and works through foxy without any changes.

What has been changed:

  1. The client code was almost completely refactored according to the ouster-sdk implementation, which should be backwards compatible down to fw 2.0 (compatibility has not been tested).
  2. Most refactors were required in the LidarScan, ScanBatcher, and packet_format structures, and the XYZLut construction.
  3. The processors were updated to support these changes, however their internal function should remain unchanged.
  4. Similar additions have been made to the sensor and ouster_driver code.

Has been added and is working:

  1. The http client used to communicate with the fw3 sensor was ported as is from the ouster-sdk and works.
  2. Support for the different lidar packet profile types is functional.
  3. All four processors produce correctly parsed data using the packet profiles.
  4. The additional 4096x5 lidar mode is working.
  5. Apart from maintaining backwards compatibility, all previous functionality should have remained unobstructed.

What currently still isn't implemented or doesn't work and is in progress:
1. There seems to be a problem with some map field accesses when using the reduced data rate profile RNG15_RFL8_NIR8, which currently throws out of bounds exceptions. Something in the parsing code was probably not ported fully correctly. #137 (comment)
2. Parsing packets with the RNG19_RFL8_SIG16_NIR16_DUAL profile works, however the additional publishers on the driver side have not yet been implemented. This shouldn't be much of a problem to implement.
3. The http interfaces for fw versions 2.1 and 2.2 as they are in the ouster-sdk are not ported yet. This should also be quite straightforward, however I will not be able to test it out.
4. The tcp client implementation has also been ported as is from the ouster-sdk, however it has not been tested. This should be testable on our sensor since the new rev should still support the tcp commands.
5. The reset and reactivate sensor semantics using ros2 lifecycle state transitions have not been enabled. I haven't yet verified whether the lifecycle transitions as they are used by ouster are compatible with this implementation.
6. The private helper methods added to sensor and full_rotation_accumulator may require more sensible placement.
7. The reflectivity image output has not yet been added and the range image output doesn't seem to be quite right. The used _range_multiplier value is probably not compatible with the changes.

Finally, is there any clang-format config that I could use to keep the formatting consistent with the current code base? I haven't quite found any suitable settings yet.

@Imaniac230
Copy link
Author

Imaniac230 commented Jun 8, 2023

Problem 1. was fixed in commit f0ccedd.

Parsing should now be correct for all combinations of packet profiles and lidar resolutions in all processors.

@SteveMacenski
Copy link
Member

There are some discussions with ouster right now about deprecating this version and having them support their own version from now on. So, its not likely that anything of this nature will be merged here because it'll be entirely rewritten / superseded in the coming weeks to month(s).

@Imaniac230
Copy link
Author

Interesting, thanks for the info. Are they planning to merge the current concepts from this one or do they have a completely new version in the works?

I'll continue to keep playing with this a bit in the mean time, if it's no problem, since it behaves more reliably for us than the current ouster version.

@SteveMacenski
Copy link
Member

Are they planning to merge the current concepts from this one or do they have a completely new version in the works?

Completely new. It is their current version in the github under the ROS 2 branch. There's an open PR with a number of changes and improvements (ouster-lidar/ouster-ros#146) that you should check out and give feedback on it ASAP since that will be effectively what replaces this (minut any issues brought up by me or the community during reintregration)

@Samahu
Copy link

Samahu commented Jun 8, 2023

Thanks Steve for linking the PR: ouster-lidar/ouster-ros#146. Yes the plan is to fully combine the two drivers into one implementation moving forward. The linked PR bridges the gap between the two drivers and and improves many aspects on both sides.

@Imaniac230
Copy link
Author

Closing this, as it is not valid anymore.

@Imaniac230 Imaniac230 closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants