Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Fix throttle publish first message#1943

Closed
efernandez wants to merge 2 commits intoros:noetic-develfrom
efernandez:fix-throttle-publish-first-message
Closed

Fix throttle publish first message#1943
efernandez wants to merge 2 commits intoros:noetic-develfrom
efernandez:fix-throttle-publish-first-message

Conversation

@efernandez
Copy link
Copy Markdown
Contributor

The throttle node from topic_tools fails to publish the first message because it doesn't wait for the subscribers.

Technically, it doesn't have to wait for the subscribers, but it should at least wait some time for them. I've added a 1.0s wait time, which seems reasonable to me, but I'm happy to make that a param if that makes more sense. I'm open to discuss that part of this change.

The use case is illustrated in a new rostest, that basically tries to only publish the first message, by using a very low frequency with throttle messages.

@efernandez efernandez force-pushed the fix-throttle-publish-first-message branch from 3e92a5e to df92c5c Compare May 1, 2020 07:54
@efernandez
Copy link
Copy Markdown
Contributor Author

I've just sent another commit that improve the wait for subscribers logic. The previous approach was only able to wait for a single subscriber, that could be anyone. The new one allows to wait for specific subscribers provided in a ROS param list: ac841a3

@efernandez efernandez force-pushed the fix-throttle-publish-first-message branch from 7836df4 to e3096ac Compare May 14, 2020 14:04
@dirk-thomas
Copy link
Copy Markdown
Member

@ros-pull-request-builder retest this please

@dirk-thomas
Copy link
Copy Markdown
Member

@efernandez Please check the failing test which seems to be related to the patch.

... before publishing the first message, so it does not get lost.
This is important so the first message is received by the subscribers,
otherwise it is always lost.
@efernandez efernandez force-pushed the fix-throttle-publish-first-message branch from 5fb6888 to 41196ed Compare July 21, 2020 10:20
@efernandez
Copy link
Copy Markdown
Contributor Author

Rebased and fixed conflicts

@dirk-thomas
Copy link
Copy Markdown
Member

Please check the failing test which seems to be related to the patch.

This comment is still pending.

@dirk-thomas
Copy link
Copy Markdown
Member

Closing for now due to no further replies. Please feel free to comment on the closed ticket once the feedback has been addressed and the ticket can be reopened.

@efernandez
Copy link
Copy Markdown
Contributor Author

@dirk-thomas Sorry for not addressing the broken test sooner. I just updated the branch and openned #2088

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants