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

Make controller manager update rate optional #404

Merged
merged 3 commits into from
Apr 30, 2021
Merged

Conversation

Karsten1987
Copy link
Contributor

I believe it's still good practice to declare a desired update rate, but we shouldn't necessarily require it and actually default it to 100 if not declared.
The current logic was flawed in a way that it would always raise the exception if the parameter wasn't set.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

I agree that throwing an exception is not a good thing. What do you think about adding a warning when the update rate is not set and that the default value of 100 Hz is used? It would be great to see some yellow/orange output when this happens.

controller_manager/src/ros2_control_node.cpp Outdated Show resolved Hide resolved
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.

witha slightly better name and print this is good to go

controller_manager/src/ros2_control_node.cpp Outdated Show resolved Hide resolved
Karsten1987 and others added 2 commits April 30, 2021 08:42
I believe it's still good practice to declare a desired update rate, but we shouldn't necessarily require it and actually default it to 100 if not declared.
The current logic was flawed in a way that it would always raise the exception if the parameter wasn't set.
@bmagyar
Copy link
Member

bmagyar commented Apr 30, 2021

@Karsten1987 I applied the suggested changes + rebased the branch. Feel free to merge if you like it this way.

@destogl destogl merged commit 2c78bc2 into master Apr 30, 2021
@destogl destogl deleted the Karsten1987-patch-2 branch April 30, 2021 14:05
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.

None yet

3 participants