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 node name to diag name of hesai hw_monitor #217

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

kotaro-hihara
Copy link
Collaborator

@kotaro-hihara kotaro-hihara commented Nov 6, 2024

PR Type

  • Improvement

Related Links

https://tier4.atlassian.net/browse/RT0-34026

Description

Added namespace so that hessai_hw_monitor can diag each LiDAR
You can verify with a bench test that a diag is published containing the namespace of each LiDAR

Review Procedure

Remarks

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 self-requested a review November 6, 2024 12:12
@mojomex mojomex changed the base branch from develop-eval to main November 6, 2024 12:12
@mojomex mojomex changed the base branch from main to develop-eval November 6, 2024 12:13
@mojomex mojomex changed the title add node name to diag name of hesai hw_monitor feat(hesai): add node name to diag name of hesai hw_monitor Nov 6, 2024
@mojomex
Copy link
Collaborator

mojomex commented Nov 6, 2024

Thank you! I was very confused why ROS2 includes the node name and provides no option to make it fully qualified (include the namespace like you did). Digging into the source code, there it was:

https://github.com/ros/diagnostics/blob/bb392ee850b7d549e621c265b214c47ad9f2dd8f/diagnostic_updater/src/diagnostic_updater.cpp#L72-L81

The parameter diagnostic_updater.use_fqn makes the node name fully qualified without any effort on our side!

Before After
image image

@mojomex mojomex force-pushed the chore/diag_name_with_namespace branch from a71032c to f5800ee Compare November 6, 2024 12:53
@mojomex mojomex changed the base branch from develop-eval to main November 6, 2024 12:53
Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and working, see comment above.

@mojomex
Copy link
Collaborator

mojomex commented Nov 6, 2024

@kotaro-hihara Since this is a super useful feature, I've changed the PR to be merged into main and will cherry-pick the commit to also apply to develop-eval. Thanks again for contributing!

@mojomex mojomex merged commit c7456f9 into main Nov 6, 2024
4 of 5 checks passed
@mojomex mojomex deleted the chore/diag_name_with_namespace branch November 6, 2024 12:57
mojomex pushed a commit that referenced this pull request Nov 6, 2024
Signed-off-by: Max SCHMELLER <[email protected]>
Co-authored-by: Kotaro HIHARA <[email protected]>
@mojomex
Copy link
Collaborator

mojomex commented Nov 6, 2024

Cherry-picked into develop-eval: d04eac5

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