feat(Ouster): implement nebula's Ouster port as a submodule#436
feat(Ouster): implement nebula's Ouster port as a submodule#436Samahu wants to merge 22 commits into
Conversation
a08f210 to
b4ef2ac
Compare
|
Hi, I have attached a rosbag file showing a pre-recorded pointcloud osdome128.bag.zip. Once you unpack the zip file you may view its content as follows:
ros2 bag play osdome128.bag --loop
ros2 run rviz2 rviz2
Once you complete these steps you should get the following output: |
|
I noticed the build is failing due to the error: CMake Error at CMakeLists.txt:9 (message):
nebula_ouster_decoders: bundled ouster-sdk is missing
(/__w/nebula/nebula/src/nebula_ouster/nebula_ouster_decoders/../../../ouster-sdk)Does the gitflow build actions run the command: |
|
@Samahu I see. It seems that none of the I have no experience with |
…ule-fix-zipcmp add ouster-sdk deps and suppress libfmt stringop-overflow warning
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (62.96%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 40.23% 38.93% -1.30%
==========================================
Files 131 139 +8
Lines 10280 10677 +397
Branches 5393 5478 +85
==========================================
+ Hits 4136 4157 +21
- Misses 3793 4206 +413
+ Partials 2351 2314 -37
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
- Add nebula_ouster, nebula_ouster_common, nebula_ouster_decoders, and nebula_ouster_hw_interfaces flags to .codecov.yml following the pattern used by Hesai, Robosense, and Continental vendors. - Add GTest-based unit tests for OusterHwInterface covering: start without callback, empty callback registration, lifecycle, idempotent double start, stop without start, callback replacement while running, scan packet callback, and error code string conversion. - Update CMakeLists.txt with BUILD_TESTING block matching other vendors. Agent-Logs-Url: https://github.com/Samahu/nebula/sessions/e06d9bd5-4b3a-484a-bd8a-a5a3b002b63c Co-authored-by: Samahu <606033+Samahu@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Samahu/nebula/sessions/e06d9bd5-4b3a-484a-bd8a-a5a3b002b63c Co-authored-by: Samahu <606033+Samahu@users.noreply.github.com>
… on separate calls Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…support test: add Ouster code coverage flags and unit tests for hw_interface
|
Hi team, thanks for the work on this PR. I've been working on a parallel effort that adds Ouster support (OS-0/OS-1/OS-2, any beam count including 128) without requiring the Ouster SDK, to keep Nebula's dependency footprint minimal and consistent with the existing Hesai/Velodyne/Robosense vendor integrations. Packet parsing, per-beam XYZ projection, and metadata handling are all implemented natively. I've opened it here: #440 I'm open to combining efforts if that would be useful, happy to collaborate with @Samahu on aligning the two designs. Please let me know what else I'd need to do to get this ready for the codebase. |
Thanks @guthabharadwaj for your contribution .. I understand the appeal to not use ouster-sdk due to its size. I can't decide what path nebula team could take but not depending on the (also note this PR already supports OS0, OS1, OS2 and OSDOME, with any beam count 32/64/128. I haven't created launch files for any of these configurations thought as I am not sure whether it is necessary since the main launch file works for all) |
|
@guthabharadwaj @Samahu |
|
@drwnz @Samahu The concern is valid - and the ROS 2 buildfarm docs (that I found) don't specify what happens with submodules. However, the official Ouster driver ouster_ros has the SDK as a submodule. I'm not sure if Ouster is doing any trickery to get it to work, so that would be interesting to know before continuing the discussion. As for whether we generally want to use vendor SDKs, I'd personally answer that with a definite yes in high quality cases like Ouster's SDK. As pointed out by @Samahu, maintenance overhead per vendor can be significant, especially when the sensor docs don't document everything. |
|
For ouster-ros I do have a separate branch that directly embeds the required files from ouster-sdk rather than using the submodule when I want to release the package via I guess we have 3 approaches each with its out specific:
There are two further takes I would like us to consider:
I can get back to you on these two options in the next week. |
|
Thanks @Samahu for the breakdown, and the FW-support concern is completely fair coming from your vantage point. I agree a path that stays close to the SDK is the stronger long-term bet for Nebula. I'd be genuinely happy to help move this forward by contributing to trimming #433 further. Just let me know where I can be useful. |
Thanks @guthabharadwaj , I think did drop few unnecessary dependencies on that branch but haven't pushed my changes. Let me push these changes and copy some of the changes I did here for coverage and then we restart that branch .. the biggest contender in terms of number of added files was There are multiple pathways to go about how we trim this dependency but let me restart the branch by the end of the week. |

PR Type
Related Links
Closes #212
Related #433
Description
The PR adds support for all current generation Ouster Lidars running FW 2.2 through 3.2
The PR imports OusterSDK as a submodule in contrast to #433
Review Procedure
vcs importcommand to clone ouster-sdk into thenebulamain foldersensor_model.param.yamland set thesensor_ipanddest_ipto match your environmentRemarks
Pre-Review Checklist for the PR Author
PR Author should check the checkboxes below when creating the PR.
Checklist for the PR Reviewer
Reviewers should check the checkboxes below before approval.
Post-Review Checklist for the PR Author
PR Author should check the checkboxes below before merging.
CI Checks