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

Populate Transitions in Transition Events #1140

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Populate Transitions in Transition Events #1140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f26a78d
Populate Transitions
CursedRock17 Mar 26, 2024
15e01e3
Suspending timestamp until I find an ansewr
CursedRock17 Mar 26, 2024
1f7898a
Adding timestamp
CursedRock17 Mar 28, 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
15 changes: 15 additions & 0 deletions rcl_lifecycle/src/com_interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,23 @@ rcl_lifecycle_com_interface_fini(
rcl_ret_t
rcl_lifecycle_com_interface_publish_notification(
rcl_lifecycle_com_interface_t * com_interface,
const char * transition_label, uint8_t transition_id,
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(&current_time);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (time_ret != RCUTILS_RET_OK) {
rcutils_error_string_t error = rcutils_get_error_string();
rcutils_reset_error();
RCL_SET_ERROR_MSG(error.str);
time_ret = RCL_RET_ERROR;
return time_ret;
}

com_interface->msg.timestamp = current_time;
com_interface->msg.transition.id = transition_id;
rosidl_runtime_c__String__assign(&com_interface->msg.transition.label, transition_label);
com_interface->msg.start_state.id = start->id;
rosidl_runtime_c__String__assign(&com_interface->msg.start_state.label, start->label);
com_interface->msg.goal_state.id = goal->id;
Expand Down
1 change: 1 addition & 0 deletions rcl_lifecycle/src/com_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ rcl_ret_t
RCL_WARN_UNUSED
rcl_lifecycle_com_interface_publish_notification(
rcl_lifecycle_com_interface_t * com_interface,
const char * transition_label, uint8_t transition_id,
const rcl_lifecycle_state_t * start, const rcl_lifecycle_state_t * goal);
Comment on lines +82 to 83
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a curiosity, does that make more sense if that takes const rcl_lifecycle_transition_t * transition which includes everything but com_interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would, but originally I just wanted more transparency with what I was adding, that'll be fixed.


#ifdef __cplusplus
Expand Down
3 changes: 2 additions & 1 deletion rcl_lifecycle/src/rcl_lifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,8 @@ _trigger_transition(

if (publish_notification) {
rcl_ret_t fcn_ret = rcl_lifecycle_com_interface_publish_notification(
&state_machine->com_interface, transition->start, state_machine->current_state);
&state_machine->com_interface, transition->label, transition->id,
transition->start, state_machine->current_state);
if (fcn_ret != RCL_RET_OK) {
rcl_error_string_t error_string = rcl_get_error_string();
rcutils_reset_error();
Expand Down