Registration of multiple callbacks on state transition

[LastStarDust]

Feature request

Feature description

It would be nice if it was possible to register multiple callbacks to be called on a certain state transition.
The callbacks should be called in the order of insertions.

For example, in the current implementation every time the method register_on_activate(callback) is called, the previous callback is overwritten.

Implementation considerations

This is because the callback container is implemented as a map.
See lifecycle_node_interface_impl.hpp#L149-L151:

  std::map<
    std::uint8_t,
    std::function<node_interfaces::LifecycleNodeInterface::CallbackReturn(const State &)>> cb_map_;

Why do we need it?

I feel this question might be coming.

For example, imagine a MotherNode class that is extended by a ChildNode class. The MotherNode might want to register a basic callback, but the ChildNode might want to register another callback to be called after that. In the current implementation, the ChildNode callback would overwrite the MotherNode callback.

BTW this happened to me personally today, and it took me a while to understand the cause of the issue.

[alsora]

This use case can easily be implemented on the application side (e.g. your mother node has a list of callbacks and it register a callback that iterates through them).

This also seems a better approach to me, since it allows you to hide the lifecycle details from the child nodes.

[LastStarDust]

@alsora Thank you for the suggestion. I might do something like that for the time being.

Nevertheless, I still think this feature would be useful. What if I do not want to hide the lifecycle details from the child nodes? What if I want the child node to be able to register callbacks for all the possible transitions?

Moreover, the semantic of the name register_on_configure is vaguely evocative of a "callback addition" more than a "callback overwrite".

[tgroechel]

@LastStarDust I think this is a good idea in theory but agree with @alsora that this should probably live as application-side behavior.

Nevertheless, I still think this feature would be useful. What if I do not want to hide the lifecycle details from the child nodes? What if I want the child node to be able to register callbacks for all the possible transitions?

I would argue:

1. default behavior would be to hide the lc details (sort of the point of lc in the first place in my opinion)
2. if the child were to register callbacks with the parent via user implementation (as opposed to the backend map), then the parent could (and should in my opinion) own the child callbacks / their ordering which will likely be context/application dependent. The parent would more likely be responsible for ordering of callback calls as opposed to insertion order (which is flimsy/difficult to re-order/could have constructor order issues depending on implementation).

I.e.
ParentNode::register_post_on_configure_cb(std::function) with the ChildNode registering their callback here. The ParentNode would then be responsible for calling this ChildNode callback when appropriate. Note this depends on what is meant by "parent/child" etc but hopefully this demonstrates my point, happy to clarify if not.

3. having more than one callback in the lc backend (the map you were referring to) would be problematic. By default, the lc creates callbacks that just return CallbackReturn::SUCCESS so users don't need to implement boiler plate for transition functions they do not need. This would require a mechanism for ids for callbacks, how to prune them, when to do so etc

Moreover, the semantic of the name register_on_configure is vaguely evocative of a "callback addition" more than a "callback overwrite".

This is a good point, possibly future consideration of renaming to override_on_configure or equivalent

[LastStarDust]

@tgroechel Point taken. I am going to implement this at the application level. As you say, it takes a little effort but it has its merits to manage this at the application level.

[LastStarDust]

Feel free to close this issue.

[fujitatomoya]

@alsora @LastStarDust @tgroechel

This also seems a better approach to me, since it allows you to hide the lifecycle details from the child nodes.
ParentNode::register_post_on_configure_cb(std::function) with the ChildNode registering their callback here.

i am not sure how this is related to multiple callbacks support? this sounds to me one of the application designs.
i mean even we have multiple callbacks supported in LifecycleNode, that can be application design if we want to? (i may be mistaken...)

i think that as LifecycleNode class itself, having multiple callbacks make senses and flexible for user application.

if a LifecycleNode node configures multiple sensors to configure on_configure transition, those functions would be provided sensor libraries.
those can be easily registered using multiple callback support without application wrapping those functions into single callback in the application code?

By default, the lc creates callbacks that just return CallbackReturn::SUCCESS so users don't need to implement boiler plate for transition functions they do not need.

this still can be supported with multiple callbacks? if no callbacks are registered in the map, we can return CallbackReturn::SUCCESS?

This would require a mechanism for ids for callbacks, how to prune them, when to do so etc

true.

besides, my major concern was what if 1st callback succeeds and 2nd callback fails? with multiple callbacks for a transition state.
probably we cannot roll back to previous primary state completely.
i believe that this is the case why it supports the single callback for each transition state.
i do not know the design history, so might be missing something.

what do you think?

[tgroechel]

@fujitatomoya Thanks for the follow up, my thoughts below. Apologies for large comment, I wish Github had threads.

ParentNode::register_post_on_configure_cb(std::function) with the ChildNode registering their callback here.

i am not sure how this is related to multiple callbacks support? this sounds to me one of the application designs.
i mean even we have multiple callbacks supported in LifecycleNode, that can be application design if we want to? (i may be mistaken...)

Ah wrote my response a bit quickly. I was meaning this to be an example of how a user could design their application. I still think single callback in the lifecycle backend is "better" design (see below).

This would require a mechanism for ids for callbacks, how to prune them, when to do so etc

true.

besides, my major concern was what if 1st callback succeeds and 2nd callback fails? with multiple callbacks for a transition state.
probably we cannot roll back to previous primary state completely.
i believe that this is the case why it supports the single callback for each transition state.
i do not know the design history, so might be missing something.

I don't know the history but I would hedge this - the lifecycle backend not knowing how to handle rollbacks - is the core reason (and better than my given reasons #2216 (comment)).

I think a core idea is that the user is the only one who knows how to "rollback" a transition (unloading, flipping flags etc.) with it being the user's responsibility to return a CallbackReturn::FAILURE. The responsibility of the LifecycleNode backend, in my opinion, is to:

1. offer guarantees / manage underlying state+transitions logic
2. offer services (e.g., GetState.srv, ChangeState.srv) to introspect / request changes to the state
3. manage ManagedEntities
4. offer user transition flexibility via user defined transition functions (e.g., on_configure)

Therefor it is the user's responsibility to rollback anything that they may have done during a transition. I have an open Issue / PR for these responsibilities outlined here: #2212

i think that as LifecycleNode class itself, having multiple callbacks make senses and flexible for user application.

if a LifecycleNode node configures multiple sensors to configure on_configure transition, those functions would be provided sensor libraries.
those can be easily registered using multiple callback support without application wrapping those functions into single callback in the application code?

I think this is likely a common design pattern (loading multiple dependencies, often external such as parameters & hardware), especially in on_configure. I think it brings up 2 things that have come up recently:

1. loading these external dependencies often require calling a service within a service which is unsupported / blocking (outlined in LifeCycle pattern dosn't work (for me at least) #2057 (comment) from today, @fujitatomoya thanks for the response).

I think a good way to address this is via adding response deferral functionality to lifecycle transitions: #2213

2. [Separate but related] It can be cumbersome to do a bulk set of service requests. Something possibly worth thinking about is creating a BatchServiceRequester utility that could send out a set of requests using a batch of client, moving SharedFuture callbacks to be waited on. It would be able to track all requests, their fail/success, have a callback given the full batches fail/success, and do clean up. I'm working on outlining something like this at the moment so this is still very much a theoretical idea / separate issue.

[ros-discourse]

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/deferrable-canceleable-lifecycle-transitions/32318/1