Altering liveliness lease_duration #755
Conversation
Signed-off-by: CursedRock17 <[email protected]>
Signed-off-by: CursedRock17 <[email protected]>
There was a problem hiding this comment.
nit: title would be Altering liveliness lease_duration? announcement period sounds really different qos setting as domain participant, and that is not what we are changing here. (https://fast-dds.docs.eprosima.com/en/latest/fastdds/discovery/general_disc_settings.html#discovery-lease-announ)
Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Lucas Wendland <[email protected]>
CursedRock17
changed the title
|
@clalancette friendly ping 🧇 |
| 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); |
There was a problem hiding this comment.
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.
|
@CursedRock17 #755 (comment) makes sense to me. do we close this one out? |
|
Apologies, I wanted to go through solutions in |
This pull request is supposed to solve an issue noted in this
RCUTILSpr in which a qos line undergoes an incorrect conversion. The original intent was to pass anint64_tand sacrifice some accuracy, but I don't believe it's possible due to our limited constructors forDuration_t. Instead, I converted to a nanosecond value, then operated on that value by making it 2/3s the original. Then, converting that value to a sec (double) which should fill thelong doubleargument. We shouldn't need to operate on the sec (double), after having converted, because while our new version will have less accuracy,RCUTILS_NS_TO_Sis undergoing changes to become more accurate.