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

Remove first handler update as queue update is blocked #974

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

lboorman
Copy link
Contributor

@lboorman lboorman commented Nov 12, 2024

The queue length for the subscriber isn't set as the code doesn't get that far after setting the throttle rate and returning the updated handler.

Public API Changes
None

Description
Remove the first handle update. The throttle and queue_length stacked methods are still added correctly when updating the second queue_length option, via the conditional statements.

Testing

Without the change: ROS2 debug messages were added (node handle passed to lower classes) and showed the queue_length wasn't being set in `subscription_modifiers.py/MessageHandler.set_queue_length()

With the change added: Ran the bridge with a ROS image publisher and a bridged image subscriber. Setting the throttling rate, changed the image update rates, with or without queue length. Then setting slow throttling (300ms) with a large existing queue showed a long delay in image updates. With a very small queue, throttling effects were immediate.

None

The queue length isn't set as the code doesn't get that far.

The throttle and queue stacked methods are still added when updating the second queue_length option
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Looking at the code, I can't tell why this wouldn't have worked in the first place...

But based on the tests, I guess this helps? And it doesn't look like it will hurt anyhow.

@sea-bass sea-bass merged commit 1de6896 into RobotWebTools:ros2 Nov 16, 2024
4 checks passed
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.

2 participants