Make rcl_lifecycle transition errors more readable

[alsora]

Feature request

Currently rcl_lifecycle transition errors are printed as follows in a C++ application

[rcl_lifecycle]: No transition matching 3 found for current state unconfigured
[]: Unable to start transition 3 from current state unconfigured: Transition is not registered.

It would be very helpful if rather than a number 3 there was the actual name of the transition being invoked, to avoid developers a constant lookup into the message definition (which is not even linked, so this error may seem extremely confusing for not-experienced people)

[CursedRock17]

Can I ask what the context of your transition error is? I know there are two methods which return the first message of "No transition matching 3", this is the method rcl_lifecycle_get_transition_by_id that takes in a uint8_t id. Then, there is method, rcl_lifecycle_get_transition_by_label that takes inconst char * label. These methods are carried underneath multiple functions when trying to get the state of any transition, so there's a good chance that it gets lost as an state id somewhere, in your case.

[fujitatomoya]

@CursedRock17
CC: @alsora

i think @alsora just wants the message like

[rcl_lifecycle]: No transition matching "activate" (ID:3) found for current state unconfigured

i think that this is nice-to-have.

label and id map can be referred to state_machine.transition_map, but this requires the API change for rcl_lifecycle_get_transition_by_label and rcl_lifecycle_get_transition_by_id to pass the state_machine. if that is okay, we can add rcl_lifecycle_get_transition_label_by_id and rcl_lifecycle_get_transition_id_by_label from the state_machine.transition_map, and print both of them.

but i am not convinced for not major enhancement with breaking API... and it is easy to look it up with https://github.com/ros2/rcl_interfaces/blob/rolling/lifecycle_msgs/msg/Transition.msg#L22

We do not want to move the warning message either for this.

[alsora]

i think @alsora just wants the message like
[rcl_lifecycle]: No transition matching "activate" (ID:3) found for current state unconfigured

yes exactly.

That error gets printed whenever you attempt an invalid lifecycle transition (for example the node is already active and you try to activate again or the node is active and you try to cleanup, skipping deactivation).
These often indicate bugs caused by race conditions or incorrect error handling.

Having a nicer log would simplify the identification of the bug.

[CursedRock17]

Oh I understand, I was thinking that the current state listed was the one being invoked, that makes much more sense.

but i am not convinced for not major enhancement with breaking API... and it is easy to look it up

Since this would be used for identification of bugs, this would be a solid reason to wait, but I see the value in creating messages like how @alsora wants. I agree that we could pass a state_machine.transition_map as another argument to both our functions in the future, but could we currently make a new method and pass a rcl_lifecycle_transition_t? Since rcl_lifecycle_transition_t has both the id and label this would allow the user to get both if they choose. For example:

const rcl_lifecycle_transition_t * 
  rcl_lifecycle_get_transition(const rcl_lifecycle_state_t * state,
  const rcl_lifecycle_transition_t * current_transition
)
{
  for(int i = 0; i < state->valid_transition_size; ++i) {
     if(state->valid_transitions[i].id == current_transition.id) {
        return &state->valid_transitions[i];
     }
  } 
  RCUTILS_LOG_WARN_NAMED(
  ROS_PACKAGE_NAME,
  "No transition matching %s (id: %d) found for current state %s",
   current_transition.label, current_transition.id, state->label
  );

}

const rcl_lifecycle_transition_t * current_transition;
current_transition.id = 3
current_transition.label = "activate"

rcl_lifecycle_get_transition(some_state_machine.current_state, current_transition)

Maybe I'm thinking of the incorrect usage of this function still, but this would allow us to avoid breaking API until we want to phase out the old types and prevent us from having to pass a state machine. I guess one caveat is if the user doesn't have both pieces of information, but it could kind of resort back to the old method and only use one if that's the case.