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

Update Ring field data type #95

Open
girvenavery2022 opened this issue Mar 10, 2022 · 5 comments
Open

Update Ring field data type #95

girvenavery2022 opened this issue Mar 10, 2022 · 5 comments

Comments

@girvenavery2022
Copy link

girvenavery2022 commented Mar 10, 2022

OS

  • Ubuntu 20.04

Ros2 version

  • Galactic

Branch

  • Galactic

Information

As of now, the Ring field is defined in the point.h file as a uint8_t. While this doesn't cause any issues in the node so to speak, but it can cause issues down stream like decreasing the portability of the Ros2 Ouster driver along with the reusability of the users code for those who use ignition gazebo for simulation or also have other 3D Lidars like the Velodyne because they define their ring datatype as a uint16_t.
This can be solved by changing the ring data type from a std::uint8_t to a std::uint16_t. I implemented this fix on my local computer as well as tested it on an Ouster OS1 16 sensor to confirm it fixes the issue and it indeed does.
I can make a PR with this change so please let me know!

@SteveMacenski
Copy link
Member

That sounds reasonable, but I think you should also file the ticket / PR with the ROS 1 drivers where this was taken from. We try to keep all of the "client" code the same between them so as Ouster makes updates we can ingest them easily here. If you can make them there, I'd be happy to pull them in here as well

@girvenavery2022
Copy link
Author

ok cool. I'll fill out an issue/PR with the ros1 Ouster driver as well. I've only skimmed the ros1 code so I'll have to find where they define it at

@SteveMacenski
Copy link
Member

There should be the exact same file elsewhere in the project

@girvenavery2022
Copy link
Author

that is correct. I made the fix on the ros1 driver and created an issue for it!

@girvenavery2022
Copy link
Author

Hi @SteveMacenski,
I wanted to give you an update on this issue. The maintainers of the Ros1 Repository just got back to me and it is considered a breaking change with the ros bags, So they said they will merge my PR in with the next Breaking change Update sometime this year.

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

No branches or pull requests

2 participants