Add consistent transitions for controllers in rqt_controller_manager

[soham2560]

Brief

This issue is in context to a converstion at #2143, where @christophfroehlich pointed out that the transitions for the controllers are not consistent (refer #2143 (review))

Currently in rqt_controller_manager you can alter the state of the controller from one to another, but some transitions are missing in some states, notably

• unload transition from unconfigured
• deactivate->unload transition from active?
  and more

A potential solution is to do as suggested in #2143 (comment)

.......basically every controller should be in one of 4 states (atleast in context of rqt_controller_manager)
- active
- inactive
- unconfigured
- unloaded
and you should have 3 options at each state to go to the other ones
Each transition consists of a seqential combination of elements of either
Load -> Configure-> Activate
or
Deactivate-> Cleanup-> Unload

This is up for discussion however, since too many options can also be confusing, and you can always switch one by one between the states

[soham2560]

@christophfroehlich thought about what you said in #2143 (comment), I think the most minimal transition setup you can have will be to have the following options in each state

• active - [deactivate]
• inactive - [activate,cleanup]
• unconfigured - [configure,unload]
• unloaded - [load]

I am leaning more towards the minimal method, because the all transitions method does seem like it will create more confusion

[christophfroehlich]

I agree with the minimal solution, but IMHO we should stick to the "official" transition wording from https://design.ros2.org/articles/node_lifecycle.html

• We don't have the finalized state, and hence no create/destroy transition. Instead, we have the on_init() callback for creation, but none for destruction. I'm unsure if we should stay with load name, or use load/create to have both worlds?
• Let's add unload/shutdown transition from every state, calling the unload_controller service (which itself calls the shutdown transition of the node from the CM).
• To make that foolproof for users not familiar with the lifecycle transitions (like me, I always get confused with cleanup and shutdown), we could add the target state in parentheses?

@saikishor what do you think?

[saikishor]

• We don't have the finalized state, and hence no create/destroy transition. Instead, we have the on_init() callback for creation, but none for destruction. I'm unsure if we should stay with load name, or use load/create to have both worlds?

I think it is better to stick with the controller terminology of load alone

• Let's add unload/shutdown transition from every state, calling the unload_controller service (which itself calls the shutdown transition of the node from the CM).

@christophfroehlich Do you also mean with active state?

• To make that foolproof for users not familiar with the lifecycle transitions (like me, I always get confused with cleanup and shutdown), we could add the target state in parentheses?

I don't know if it better to include lifecycle transitions here. For me, it is clear from the controller states. Moreover, controllers use the lifecycle node, but they are not completely lifecycle node, so for this reason I'm not sure

[christophfroehlich]

• We don't have the finalized state, and hence no create/destroy transition. Instead, we have the on_init() callback for creation, but none for destruction. I'm unsure if we should stay with load name, or use load/create to have both worlds?

I think it is better to stick with the controller terminology of load alone

ok

• Let's add unload/shutdown transition from every state, calling the unload_controller service (which itself calls the shutdown transition of the node from the CM).

@christophfroehlich Do you also mean with active state?

I see that we don't allow this in the CM, so we could call deactivate -> unload in this case.

• To make that foolproof for users not familiar with the lifecycle transitions (like me, I always get confused with cleanup and shutdown), we could add the target state in parentheses?

I don't know if it better to include lifecycle transitions here. For me, it is clear from the controller states. Moreover, controllers use the lifecycle node, but they are not completely lifecycle node, so for this reason I'm not sure

We list the transitions as text, and the color code gives the target state. What I meant, let's also write the target state there, like 🟢 activate (active), or 🟡 cleanup (unconfigured)

[saikishor]

We list the tranistions as text, and the color code gives the target state. What I meant, let's also write the target state there, like 🟢 activate (active), or 🟡 cleanup (unconfigured)

Sure. 👍🏾

[soham2560]

Sorry for the late reply

To make that foolproof for users not familiar with the lifecycle transitions (like me, I always get confused with cleanup and shutdown), we could add the target state in parentheses?

this makes sense to me as well, since new users would also prefer to use the GUI and this would make it easier for them

We list the transitions as text, and the color code gives the target state. What I meant, let's also write the target state there, like 🟢 activate (active), or 🟡 cleanup (unconfigured)

Sounds good! I do feel the wording can be better, for me it does not give the clear impression of "this transition would lead to this state" upon first read, maybe because the words for transition and state are sometimes similar, sometimes not, but I think we can atleast set it up, the words can be changed later

Now we will have following transitions at each stage

• active 🟢
  • 🔵 deactivate (inactive)
  • ⚫ deactivate and unload (unloaded)
• inactive 🔵
  • 🟢 activate (active)
  • 🟡 cleanup (unconfigured)
  • ⚫ unload (unloaded)
• unconfigured 🟡
  • 🔵 configure (inactive)
  • ⚫ unload (unloaded)
• unloaded ⚫
  • 🟡 load (unconfigured)

Why am I getting the feeling this also looks a bit inconsistent? Let me know what your opinion is

[saikishor]

It probably should be something like the following:

• active 🟢
  • 🔵 deactivate (inactive)
  • ⚫ deactivate and unload (unloaded)
• inactive 🔵
  • 🟢 activate (active)
  • ⚫ unload (unloaded)
• unconfigured 🟡
  • 🟢 configure & activate (active)
  • 🔵 configure (inactive)
  • ⚫ unload (unloaded)
• unloaded ⚫
  • 🟡 load (configured)

I'm still not sure, if we should support the unloaded option, in future it shouldn't work this way. Right now, it is working as some cleanup is missing inside the CM.

[soham2560]

• activate transition from unconfigured makes sense
• Why remove the cleanup transition from inactive? wouldn't this mean that to go to an unconfigured state the user would have to manually unload and load again? Any reason we can't do it internally?

[soham2560]

@saikishor @christophfroehlich any opinions on the above?

[saikishor]

• activate transition from unconfigured makes sense

You mean to do unconfigured -> inactive -> active?

* Why remove the `cleanup` transition from inactive? wouldn't this mean that to go to an unconfigured state the user would have to manually unload and load again? Any reason we can't do it internally?

Right now, the cleanup controller service doesn't exist yet (#1236), so you cannot go to unconfigured state

[soham2560]

You mean to do unconfigured -> inactive -> active?

yes, I'm saying what you wrote makes sense (adding configure and activate transition in unconfigured)

Right now, the cleanup controller service doesn't exist yet (#1236), so you cannot go to unconfigured state

yes that was noted in #2143 (comment) (ignore the extra deactivate call), we can use the same work around, basically if anyone want to go to unconfigured from inactive, they would unload and load eitherways, what if we do it internally, till cleanup is introduced, then we can add that

[soham2560]

@saikishor @christophfroehlich I've added a draft PR at #2163 so that we can atleast get down to a visual representation, do check it out
The logic in the PR is

• active 🟢
  • 🔵 deactivate (inactive)
  • ⚫ deactivate and unload (unloaded)
• inactive 🔵
  • 🟢 activate (active)
  • 🟡 cleanup (unconfigured)
  • ⚫ unload (unloaded)
• unconfigured 🟡
  • 🟢 configure and activate (active)
  • 🔵 configure (inactive)
  • ⚫ unload (unloaded)
• unloaded ⚫
  • 🟡 load (unconfigured)

It is basically what @saikishor suggested in #2151 (comment) but with cleanup transition in inactive state, as I think that should also be there

but do give me your opinion!

Also, to be technically right we can rename the cleanup transition to unload and load i.e

• inactive 🔵
  • 🟢 activate (active)
  • 🟡 unload and load (unconfigured)
  • ⚫ unload (unloaded)

[soham2560]

@christophfroehlich @saikishor let me know how it looks

[saikishor]

For me, it looks good. I'm just not sure about unload and load operation. I don't have a strong opinion here, if everyone is ok with it. Let's do it

[soham2560]

@christophfroehlich any opinions?