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

Set twist_marker/use_stamped to default true, to match twist_mux #55

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

TWALL9
Copy link
Contributor

@TWALL9 TWALL9 commented Dec 29, 2024

Something I noticed when updating the workspace I've been using for my hobby projects. The use_stamped parameter in twist_marker is defaulting to false, but the same parameter in twist_mux must be explicitly set or the node will throw an exception and die.

I'm making an assumption that when use_stamped was added to the twist_mux node, it was intentionally added without a default value to force developers to evaluate their usage of the node. I actually agree with that direction, if that was the intention, but at the same time it's pretty annoying that the two nodes in this package have different behaviours where one is optional and the other is not.

This PR changes /twist_mux/use_stamped to default to false when the node is created, and declares it as a parameter for twist_mux instead of just blindly looking in the global namespace.

There is also the matter that Steve M brought up regarding the default value of this parameter with Nav2. . I don't address that issue here, it's orthoganal to the merits of having a parameter act uniformly across the package.

@bmagyar
Copy link
Member

bmagyar commented Dec 30, 2024

The marker should be updated on this case. The better half of the ROS universe has finally moved on to use stamped twist messages, let's not regress on that.

@TWALL9
Copy link
Contributor Author

TWALL9 commented Jan 1, 2025

The marker should be updated on this case. The better half of the ROS universe has finally moved on to use stamped twist messages, let's not regress on that.

Done deal. I figured that would be addressed in a different PR but it may as well be done here as long as we're declaring parameters.

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thank you!

@bmagyar bmagyar changed the title Set twist_mux/use_stamped to default false, to match twist_marker Set twist_marker/use_stamped to default true, to match twist_mux Jan 3, 2025
@bmagyar bmagyar merged commit 3037d5e into ros-teleop:rolling Jan 3, 2025
1 check passed
@TWALL9 TWALL9 deleted the use_stamped-default-value branch January 3, 2025 19:40
@efernandez
Copy link
Member

Thanks for covering me on reviewing this @bmagyar and thanks @TWALL9 for your contribution.

I wonder if I'm missing something in the settings so I can be a default reviewer, but it looks like there's no setting for that. Maybe we could add something with a template to at least get reviewers pinged 🤔

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.

4 participants