-
Notifications
You must be signed in to change notification settings - Fork 411
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
Implement validity checks for rclcpp::Clock #2040
Implement validity checks for rclcpp::Clock #2040
Conversation
92a4666
to
53779d0
Compare
58028ea
to
869e4ed
Compare
Changes incorporated! |
869e4ed
to
74282af
Compare
937c213
to
23271a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with green CI!
eb680b4
to
6a54627
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@methylDragon thanks for the contribution.
Could you explain the actual problem that you meet to address with this PR? I am not really following the what needs to be done here, thanks in advance!
Hey Tomoya (@fujitatomoya), thanks for the reviews (and the exception docs catches)! This PR is supposed to add I'm upstreaming this change because it came up in one of my porting efforts. Notably, this notion of validity is separate from the |
@ros-pull-request-builder retest this please |
@methylDragon thank you for iterating with me.
https://design.ros2.org/articles/clock_and_time.html, says above. thanks for pointing out the design.
this becomes my question that why these notions are different? by design, @clalancette @ivanpauno @wjwwood @sloretz what do you think? |
Yes, this is a good question. It turns out that ROS has been considering time==0 as invalid since practically forever. It is not ideal, since time==0 is actually valid; it is just the start of time, not invalid time. If we were totally redesigning this today, I think we might designate something like -1 as invalid, with 0 just being the start of time. But if we did that now, we'd risk subtly breaking a lot of simulation applications that made some assumptions about past behavior. So while it is not ideal, I think we should continue to treat time==0 as invalid.
But this is a good question; it would be nice if this policy were encoded in the |
agree on this. I was wondering the same thing, thanks for pointing this out. |
b431888
to
6face35
Compare
It'll be nice to put this in the We need a distinct way to check if the members of a clock are initialized (currently what So if I were to add it into the
If we can't resolve the naming issue, I think it'd be more intuitive to place it in the client libraries? Or maybe I'm missing something :V Alternatively, do I even need to care about breaking ABI/API if I'm implementing this for rolling, or especially given that the scope of downstream use is so small :o |
2d311e9
to
5d6d65b
Compare
I just had an idea, I will create an Implemented here: ros2/rcl#1018 I will update this PR to use that new implementation in the rcl layer. Edit: Updated. Builds and tests pass locally with that rcl PR's changes. |
rclcpp/include/rclcpp/clock.hpp
Outdated
*/ | ||
RCLCPP_PUBLIC | ||
bool | ||
wait_for_valid(Context::SharedPtr context = contexts::get_global_default_context()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the use case of wait_for_valid()?
It seems like a strange feature to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll block till the first valid time. In most cases this will be waiting for sim to start (and time to march forward)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivanpauno friendly ping on this comment.
Signed-off-by: methylDragon <[email protected]>
a7eec28
to
ab95f66
Compare
This PR has been updated to use this PR instead:
It doesn't make sense for a non-zero time point to be invalid, so the names and implementations have been changed to make this PR about checking for whether a clock has started or not |
407a4cc
to
36aad12
Compare
@ros-pull-request-builder retest this please |
@methylDragon thanks for iterating with patience, so this has been update and ready to review, right? |
Hey @fujitatomoya , this is ready, yep! Though it'll take awhile for |
Signed-off-by: methylDragon <[email protected]>
36aad12
to
25b8a81
Compare
Signed-off-by: methylDragon <[email protected]>
@methylDragon Can we backport this to Humble? I believe "new non-virtual methods on a class" is an ABI-stable change, so it seems to me like the answer is yes. The dependency ros2/rcl#1021 only introduces new free functions, which I believe is also ABI stable |
yeah, correct. i believe backport should be fine. besides, this is something we add, there is no user application behavior change. |
@Mergifyio backport humble |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
@Mergifyio backport humble |
✅ Backports have been created
|
(cherry picked from commit c091fe1)
(cherry picked from commit c091fe1) Co-authored-by: methylDragon <[email protected]>
(cherry picked from commit c091fe1) Co-authored-by: methylDragon <[email protected]>
…rt 0.15.7 (#3) * Implement validity checks for rclcpp::Clock (ros2#2040) (ros2#2210) (cherry picked from commit c091fe1) Co-authored-by: methylDragon <[email protected]> * rclcpp: check rcl version [humble] Signed-off-by: tomoya.kimura <[email protected]> --------- Signed-off-by: tomoya.kimura <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: methylDragon <[email protected]>
This PR implements the analogues of the isValid and waitForValid methods in rclcpp.
I also added the necessary tests.Note: In this case, I added a special case for treatingRCL_CLOCK_UNINITIALIZED
as invalid as well, since there isn't an analogous clock type in the ROS 1 API.Pinging @sloretz for visibility.
Edit: Waiting on: