-
Notifications
You must be signed in to change notification settings - Fork 38
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
Design the ROS2 implementation for Clock class #575
Conversation
I am supposed to wait for it being not Draft. |
Actually yes, there is a problem related to m_clock.sleep_for(...) function. |
7c5dd45
to
01dd947
Compare
The PR is ready |
|
||
void RosClock::yield() | ||
{ | ||
std::this_thread::yield(); |
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.
Does it make sense to mix ROS2 and std
calls? Is it better to simply do anything?
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.
You're right. Removed
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.
Not a big problem, but it could make sense to have std::this_thread::yield()
in this context. In the end, sleep
/sleepUntil
are functions that in the end are waiting on some kind of OS synchronization primitive (mutex/condition variables), so it could make sense that you want to yield even if you are waiting on those. Anyhow, I do not think it is a big problem.
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.
I just thought that the effect could have not been very predictable given the sleep implementation of ROS2, but maybe I am being paranoid.
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.
In ros we should use the command spin
so in theory yield is not needed. Shall we add a debug print that remember the user that this implementation of yield does nothing?
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.
Shall we add a debug print that remember the user that this implementation of yield does nothing?
I would avoid the print, as it would make writing generic code quite difficult (you would need to not call yield if you use the ROS2 clock). I would say let's merge it empty (safest choice), and then let's see if problems emerge.
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.
Ok merged thank you all as usual ❤️
Sorry, that was originally meant to be a question, I put a |
5a9972b
to
39885f5
Compare
This PR develops the ROS2 implementation for the Clock class in the system component. This PR uses plain
CMake
instead ofament_cmake
(provided by ROS2). The required ROS version is humble.The clock can be used as follows: