Populate Transitions in Transition Events #1140
Conversation
Signed-off-by: CursedRock17 <[email protected]>
CursedRock17
requested review from
audrow,
ivanpauno and
wjwwood
as code owners
CursedRock17
changed the title
Signed-off-by: CursedRock17 <[email protected]>
Signed-off-by: CursedRock17 <[email protected]>
| const rcl_lifecycle_state_t * start, const rcl_lifecycle_state_t * goal) | ||
| { | ||
| // Get the current system time based on the rcl_clock | ||
| rcutils_time_point_value_t current_time; | ||
| rcutils_ret_t time_ret = rcutils_system_time_now(¤t_time); |
There was a problem hiding this comment.
The clock works, but will the result of rcutils_system_time_now serve as the correct time? The transition timestamp is meant to be
The time point at which this event occurred.
Which needs to be set during the period of notification. I can't image trying to pass down a clock through multiple layers of API so that the user could set the time that way though.
There was a problem hiding this comment.
this will publish the system time only, but i think LifecycleNode.clock_interface should be used. more like, rck_lifecycle_com_interface should be initialized with the same clock with lifecycle node. so that if the node is configured with simulation time, we can also publish the simulation time in the message.
There was a problem hiding this comment.
I definitely agree that this would be the best route to go, but will it be problematic making API changes? As the rcl_lifecycle_com_interface is changed that means all functions initializing com_interface must be changed to include a rcl_clock_t * argument. This would include adding an argument to rcl_lifecycle_state_machine_init and all the functions it calls down the line.
| const char * transition_label, uint8_t transition_id, | ||
| const rcl_lifecycle_state_t * start, const rcl_lifecycle_state_t * goal); |
There was a problem hiding this comment.
just a curiosity, does that make more sense if that takes const rcl_lifecycle_transition_t * transition which includes everything but com_interface?
There was a problem hiding this comment.
It would, but originally I just wanted more transparency with what I was adding, that'll be fixed.
| const rcl_lifecycle_state_t * start, const rcl_lifecycle_state_t * goal) | ||
| { | ||
| // Get the current system time based on the rcl_clock | ||
| rcutils_time_point_value_t current_time; | ||
| rcutils_ret_t time_ret = rcutils_system_time_now(¤t_time); |
There was a problem hiding this comment.
this will publish the system time only, but i think LifecycleNode.clock_interface should be used. more like, rck_lifecycle_com_interface should be initialized with the same clock with lifecycle node. so that if the node is configured with simulation time, we can also publish the simulation time in the message.
Meant to resolve #1019 this PR adds the timestamp and transition portion of a
TransitionEventto thercl_lifecycle_com_interface_publish_notificationfunctionUnfortunately, it looks like in order to do this we will have to break the ABI with either changes or additions to that function's parameters.