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

feat(hesai): add high resolution mode setting for Pandar128 and OT128 #142

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

mojomex
Copy link
Collaborator

@mojomex mojomex commented Apr 24, 2024

PR Type

  • New Feature

Related Links

TIER IV INTERNAL LINK -- Jira ticket

This PR depends on the following PR being merged first:

Description

This PR adds a launch parameter hires_mode (boolean) which is used to enable/disable high resolution mode for Pandar128 and OT128.
The hardware interface has been extended to send the corresponding PTC commands.

Standard Mode High Resolution Mode
image image

Review Procedure

Test with Pandar128 and OT128 and confirm that horizontal resolution changes like in the images above:

  • launching with hires_mode:=false
  • launching with hires_mode:=true
  • running ros2 param set /hesai_hw_driver hires_mode true during runtime
  • running ros2 param set /hesai_hw_driver hires_mode false during runtime

Remarks

🟡 I currently do not have a Pandar128 to test with, and also no access to its TCP documentation. It still needs to be confirmed if the command is the same as for OT128.

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

@mojomex mojomex requested review from amc-nu and drwnz April 24, 2024 09:42
@mojomex mojomex force-pushed the hires_mode_support branch from e563e9e to b5acdea Compare April 24, 2024 09:55
@mojomex
Copy link
Collaborator Author

mojomex commented Apr 25, 2024

🟢 Evaluation

🟢 OT128

Tested:

  • starting Nebula with hires_mode:=false, then setting it to true, false, true via ros2 param set /hesai_hw_driver hires_mode true/false

  • starting Nebula with hires_mode:=true, then setting it to false, true, falseviaros2 param set /hesai_hw_driver hires_mode true/false`

  • Confirmed TCP communication is correct using Wireshark and other tools.

  • Confirmed visually that hires mode is indeed changed

🟢 Pandar128

Not tested yet, but after consulting with @drwnz, merging is still fine.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 240 lines in your changes are missing coverage. Please review.

Project coverage is 10.95%. Comparing base (eb5e1b7) to head (55ce98a).
Report is 4 commits behind head on main.

Files Patch % Lines
.../nebula_hesai_hw_interfaces/hesai_hw_interface.cpp 0.00% 165 Missing ⚠️
.../nebula_hw_interfaces_hesai/hesai_cmd_response.hpp 0.00% 52 Missing ⚠️
...a_ros/src/hesai/hesai_hw_interface_ros_wrapper.cpp 0.00% 12 Missing ⚠️
...ula_ros/src/hesai/hesai_hw_monitor_ros_wrapper.cpp 0.00% 10 Missing ⚠️
.../nebula_hw_interfaces_hesai/hesai_hw_interface.hpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##            main     #142      +/-   ##
=========================================
+ Coverage   4.84%   10.95%   +6.10%     
=========================================
  Files        249       81     -168     
  Lines      19210     9424    -9786     
  Branches    1075     1203     +128     
=========================================
+ Hits         931     1032     +101     
+ Misses     17579     7536   -10043     
- Partials     700      856     +156     
Flag Coverage Δ
differential 10.95% <0.00%> (?)
total ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mojomex mojomex self-assigned this Jun 13, 2024
@mojomex mojomex force-pushed the hires_mode_support branch from 55ce98a to d996e03 Compare July 4, 2024 11:38
@mojomex mojomex changed the base branch from main to develop July 4, 2024 11:39
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.

2 participants