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

Allow timer rate to be configurable inmessage_lost_talker #679

Open
wants to merge 4 commits into
base: rolling
Choose a base branch
from

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented May 5, 2024

By default the talker publishes an 8mB message every 3 seconds. I haven't been able to reproduce any message lost callback triggers with such a low frequency. It helps to increase the publishing rate.
This PR

  • Adds support to pass a -r CLI arg to message_lost_talker to configure the timer's period. Default behavior is still the same 3s period.
  • Sets sub's reliability QoS to best_effort to increase the probability of dropping a message. -> I can undo this change if needed.

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Command line arg looks wrong.

quality_of_service_demo/rclcpp/src/message_lost_talker.cpp Outdated Show resolved Hide resolved
Yadunund and others added 2 commits May 5, 2024 18:16
@@ -156,8 +156,8 @@ and the "Deadline missed" messages will no longer be printed.
This demo shows how to get a notification when a subscription loses a message.

This feature is not available in all RMW implementations.
`rmw_cyclonedds_cpp` and `rmw_connextdds` do support this feature.
CycloneDDS partially implements the feature and it only triggers the event under some limited circumstances, thus it's recommended to try the demo with Connext.
`rmw_zenoh_cpp`, `rmw_cyclonedds_cpp` and `rmw_connextdds` do support this feature.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think RMW_EVENT_MESSAGE_LOST is supported in rmw_fastrtps, i am not sure why that is not listed here?

https://github.com/ros2/rmw_fastrtps/blob/21e6a44a2ef25ce51ece019b255427a0248fad65/rmw_fastrtps_shared_cpp/src/custom_subscriber_info.cpp#L449

Suggested change
`rmw_zenoh_cpp`, `rmw_cyclonedds_cpp` and `rmw_connextdds` do support this feature.
`rmw_fastrtps_cpp`, `rmw_zenoh_cpp`, `rmw_cyclonedds_cpp` and `rmw_connextdds` do support this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I also think it is premature to add in rmw_zenoh_cpp here, as it is not a Tier-1 RMW.

input_stream >> timer_rate;
if (!input_stream) {
print_usage();
std::cout << "\n-s must be followed by a positive number, got: '" <<
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
std::cout << "\n-s must be followed by a positive number, got: '" <<
std::cout << "\n-r must be followed by a positive number, got: '" <<

std::exit(0);
}
timer_period_ = std::chrono::duration_cast<std::chrono::milliseconds>(
std::chrono::duration<double, std::ratio<1>>(1.0 / timer_rate));
Copy link
Collaborator

Choose a reason for hiding this comment

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

if -r 0, this could generate 0 division exception?

@sloretz
Copy link
Contributor

sloretz commented May 16, 2024

@Yadunund friendly ping 🧇

@Yadunund
Copy link
Member Author

Sorry for the delay. Will have this cleaned up by end of this week

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.

5 participants