-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Small changes #4427
base: main
Are you sure you want to change the base?
Small changes #4427
Conversation
} | ||
} | ||
} | ||
|
||
// Take average | ||
effort /= stall_joint_names_.size(); | ||
velocity /= stall_joint_names_.size(); | ||
if (match_count > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just to avoid a divide by zero issue? If so, we could just have a bool
instead which would be more intuitive unless we're going to use the count
rather than the size
(or check that count == size
or something to check we have all the stuff needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is really targeting a divide by zero, we probably should update the configure() to not create the subscriber if stall_joint_names_ is empty() - it already prints an error, but there is no reason to have the subscriber run the callback if there are no names to match
Please sign off with DCO! If you click on the job, it'll tell you how to ammend your commit for it ( |
@xianglunkai can you update the PR as we asked in the review so we can merge this? 😄 |
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Future work that may be required in bullet points
For Maintainers: