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.

Sign up for GitHub

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

Jump to bottom

Use fully qualified node name in parameter events #402

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Use fully qualified node name in parameter events #402

merged 1 commit into from
Apr 5, 2024

Conversation

bjsowa
Copy link
Contributor

@bjsowa bjsowa commented Apr 4, 2024

Parameter events expect fully qualified ROS node paths. This fixes handling of parameter changes in rqt_reconfigure.

fujitatomoya
fujitatomoya approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm, it should be fully qualified node name. (this aligns with rclcpp)

pablogs9
pablogs9 reviewed Apr 5, 2024
View reviewed changes
Copy link
Member

@pablogs9 pablogs9 left a comment

Choose a reason for hiding this comment

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

@bjsowa Has this been tested in the context of rclc + rcl + rmw_microxrcedds?

@bjsowa
Copy link
Contributor Author

bjsowa commented Apr 5, 2024

@bjsowa Has this been tested in the context of rclc + rcl + rmw_microxrcedds?

Yes, this is how I've been using it.

pablogs9
pablogs9 approved these changes Apr 5, 2024
View reviewed changes
Copy link
Member

@pablogs9 pablogs9 left a comment

Choose a reason for hiding this comment

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

Quick check and rcl_node_get_fully_qualified_name shall work properly on micro-ROS. Also @bjsowa reports correct behaviour in micro-ROS context.

If this raises a problem with rmw_microxrcedds I will address later.

LGTM

JanStaschulat
JanStaschulat approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@JanStaschulat JanStaschulat left a comment

Choose a reason for hiding this comment

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

LGTM

@JanStaschulat JanStaschulat merged commit 995cda8 into ros2:rolling Apr 5, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogs9 pablogs9 pablogs9 approved these changes

@JanStaschulat JanStaschulat JanStaschulat approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bjsowa @pablogs9 @JanStaschulat @fujitatomoya