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.

Sign up for GitHub

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

Altering liveliness lease_duration #755

Closed
wants to merge 3 commits into from
Closed

Altering liveliness lease_duration #755

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2168603
altering announcement_period
CursedRock17 May 2, 2024
3f932d2
altering announcement_period
CursedRock17 May 2, 2024
ffb4386
Typo causing error fix
CursedRock17 May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rmw_fastrtps_shared_cpp/src/qos.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ bool fill_entity_qos_from_profile(
// Docs suggest setting no higher than 0.7 * lease_duration, choosing 2/3 to give safe buffer.
// See doc at https://github.com/eProsima/Fast-RTPS/blob/
// a8691a40be6b8460b01edde36ad8563170a3a35a/include/fastrtps/qos/QosPolicies.h#L223-L232
double period_in_ns = entity_qos.liveliness().lease_duration.to_ns() * 2.0 / 3.0;
int64_t period_in_ns = entity_qos.liveliness().lease_duration.to_ns() * 2 / 3;
double period_in_s = RCUTILS_NS_TO_S(period_in_ns);
Comment on lines +144 to 145
Copy link
Contributor

Choose a reason for hiding this comment

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

Just coming back to look at this again, and I'm realizing that these two lines together are wrong, at least, with the current implementation of RCUTILS_NS_TO_S.

The problem is in switching period_in_ns to an int64_t. Because RCUTILS_NS_TO_S currently uses longs in the divisor, then everything is an integer. So if you have less than 1 second (1000000000 ns), then we use integer division here and always end up with zero. That's likely why period_in_ns was a double to begin with, since everything will be promoted to a double.

If we put in ros2/rcutils#460 along with this one, then I guess that particular issue goes away. But this is making me much more nervous about ros2/rcutils#460 in general. While this macro isn't used very heavily, I think that will change the semantics enough that it is concerning. I'll leave a comment over there.

As far as this PR goes, I actually think that what is here is fine, at least for now. So I'm actually going to suggest that we close this PR out.

entity_qos.liveliness().announcement_period = eprosima::fastrtps::Duration_t(period_in_s);
}
Expand Down