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(hcm): setup callback groups for high hz subscriptions #460

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

texhnolyze
Copy link
Contributor

using a MutuallyExclusiveCallbackGroup for each one, so that they do not block each other but each subscription's callback is still called in order of received messages.

See: https://docs.ros.org/en/rolling/How-To-Guides/Using-callback-groups.html

Fixes: #313

Checklist

  • Run colcon build
  • Write documentation
  • Test on your machine
  • Test on the robot
  • Create issues for future work
  • Triage this PR and label it

@texhnolyze texhnolyze added bug Something isn't working motion labels May 16, 2024
@texhnolyze texhnolyze self-assigned this May 16, 2024
@texhnolyze
Copy link
Contributor Author

texhnolyze commented May 16, 2024

I tested in simulation and on robot and everything seems to work.
@Flova do you have an idea how I could benchmark if this lead to a performance improvement?

@texhnolyze texhnolyze marked this pull request as ready for review May 17, 2024 15:38
Copy link
Member

@timonegk timonegk left a comment

Choose a reason for hiding this comment

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

Code-wise this looks fine but I cannot judge the performance increase.

using a `MutuallyExclusiveCallbackGroup` for each one, so that they do
not block each other but each subscription's callback is still called in
order of received messages.

See: https://docs.ros.org/en/rolling/How-To-Guides/Using-callback-groups.html

Fixes: #313
@texhnolyze texhnolyze force-pushed the feature/parallel-hcm-callbacks branch from 192c7fe to f3824f4 Compare June 11, 2024 18:12
@texhnolyze
Copy link
Contributor Author

I took a look at the EventsExecutor implementation and determined, that it is single threaded and utilizing a single FIFO queue, no matter the callback groups.
To actually utilize parallel execution, we would need to either need to utilize an MultiThreadedExecutor with a multiple threads or assign different callback groups to separate instances of EventsExecutor utilizing the ->add_callback_group method.

In the way it was implemented right now, it will most likely not have any effect on the execution of callbacks in the HCM, but will also not break anything.

Until some clarification and benchmarking, I would put this on hold.
@Flova did we even still have issues with speed of joint_states and imu/data in the HCM in the last months?

@texhnolyze texhnolyze marked this pull request as draft June 11, 2024 18:17
@Flova
Copy link
Member

Flova commented Jun 11, 2024

I don't think so tbh. as discussed in person. (Just writing it here as a protocol for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working motion
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

HCM callback groups
3 participants