fix(nebula_hw_interfaces_hesai): correct ptp config format for at128 tcp api#360
fix(nebula_hw_interfaces_hesai): correct ptp config format for at128 tcp api#360Ayrton2718 wants to merge 2 commits intotier4:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
==========================================
- Coverage 34.06% 32.62% -1.44%
==========================================
Files 126 126
Lines 10614 10515 -99
Branches 5481 4499 -982
==========================================
- Hits 3616 3431 -185
- Misses 6342 6485 +143
+ Partials 656 599 -57
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the PTP configuration format for the Hesai AT128 Lidar's TCP API to comply with the AT128 TCP API specification. Previously, the incorrect format caused PTP initialization to fail, and this change enables successful PTP configuration for the AT128.
- Refactored PTP configuration structures into a polymorphic design with sensor-specific formats
- Updated set_ptp_config() to handle different sensor models with appropriate payload formats
- Changed get_ptp_config() return type to support multiple PTP config formats
- Removed AT128-specific HTTP fallback code in favor of proper TCP API support
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hesai_hw_interface.cpp | Implements sensor-specific PTP config handling and removes HTTP fallback for AT128 |
| hesai_hw_interface.hpp | Updates get_ptp_config() return type to polymorphic base pointer |
| hesai_cmd_response.hpp | Refactors PTP config structs into polymorphic hierarchy with sensor-specific implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| if (sensor_configuration->sensor_model != SensorModel::HESAI_PANDARAT128) { | ||
| set_flg = true; |
There was a problem hiding this comment.
The removal of the conditional check if (sensor_configuration->sensor_model != SensorModel::HESAI_PANDARAT128) means this code now executes for all sensor models including AT128. This could be problematic if AT128 doesn't support the sync angle configuration that follows, as the original code specifically excluded AT128 from this block.
There was a problem hiding this comment.
According to the spec sheet, the AT128 supports sync angle configuration.
I also tested it on a real device.
There was a problem hiding this comment.
Ah, it seems that you are trying to run Nebula with Hesai AT128P, not the older AT128. We currently do not officially support AT128P since we don't have access to the sensor, and since it's not used in any of our projects.
If your change is all it takes, and you can provide a before/after (best as short videos of you trying to launch Nebula and showing a pointcloud), then we'd be more than happy to support it.
Related to #341.
Description
This PR fixes the PTP configuration format for the Hesai AT128 Lidar's TCP API to comply with the AT128 TCP API specification.
Previously, the incorrect format caused PTP initialization to fail over TCP. This fix allows the PTP settings to be configured successfully on the AT128 Lidar via the TCP API.
Tests performed
Operation has been confirmed with the Hesai AT128 and XT32 Lidars.
Effects on system behavior
Enables correct PTP time synchronization configuration for the Hesai AT128 Lidar via the TCP API.
Interface changes
The following C++ interfaces within the
nebula_hw_interfaceslibrary have been changed. There are no changes to external ROS interfaces such as topics or services.HesaiHwInterface::get_ptp_config()was changed fromHesaiPtpConfigtostd::unique_ptr<PtpConfigBase>.HesaiPtpConfigstruct was refactored into a polymorphic structure withPtpConfigBaseas the base class to handle sensor-specific formats.Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.