Updated rqt_controller_manager controller state color scheme to match list_controllers color scheme

[soham2560]

Brief

In this PR

• Updated Colour Scheme for rqt_controller_manager(refer 8098fb7)
  • Fixes Update rqt_controller_manager controller state color scheme to match list_controllers color scheme #2132
• Updated rqt_controller_manager image in documentation (refer 2f4b4b5)

How was this tested?

• Launch ros2_control_demo_example_14 with

  ros2 launch ros2_control_demo_example_14 rrbot_modular_actuators_without_feedback_sensors_for_position_feedback.launch.py

• Launch rqt_controller_manager with

  ros2 run rqt_controller_manager rqt_controller_manager

Screenshots:

[soham2560]

Couple of Notes:

• Wasn't able to find a cyan led for inactive, best I could find was blue, should I edit the image to make it closer to cyan?
• the not loaded state shows no led, rather than led_off, is this intended behaviour?
  

[soham2560]

Not entirely certain why the checks are failing, will have to look into it, these changes shouldn't be causing this imo, let me know if I'm missing something

[christophfroehlich]

Not entirely certain why the checks are failing, will have to look into it, these changes shouldn't be causing this imo, let me know if I'm missing something

the checks are failing because of #2134

[saikishor]

I'm not sure, when this can happen. Ideally, we shouldn't be able to show an unlisted one right?

[soham2560]

I'm not sure, when this can happen. Ideally, we shouldn't be able to show an unlisted one right?

if theres is a controller that was loaded before, and then you unload it for some reason, I think you should have the option to reload it right?

[saikishor]

if theres is a controller that was loaded before, and then you unload it for some reason, I think you should have the option to reload it right?

I don't think there is a way to reload it manually from this GUI

[soham2560]

I don't think there is a way to reload it manually from this GUI

I was able to do this once unloaded, and the controller did seem to activate and work afterwards, let me know if I've misunderstood

[soham2560]

how to proceed with this PR?

[saikishor]

how to proceed with this PR?

I'll check and get back to you

[saikishor]

how to proceed with this PR?

@soham2560 I just tested it. I understand how it is working now. The parameters inside the controller manager are still present, that's why it is working. This means proper cleanup is not done.

As it is functional this way, let's make it nicer. For this case is it possible for you to change from "not loaded" to "unloaded" and use a black icon?

[soham2560]

@saikishor that sounds like a good idea, I have added the changes! (refer 284076c) let me know how it looks

[soham2560]

also, since the led_red.png is not being used anymore, should I remove it?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.29%. Comparing base (90d1c59) to head (6e4b8ca).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2143      +/-   ##
==========================================
- Coverage   89.30%   89.29%   -0.02%     
==========================================
  Files         139      139              
  Lines       15502    15502              
  Branches     1318     1318              
==========================================
- Hits        13844    13842       -2     
- Misses       1155     1156       +1     
- Partials      503      504       +1     

Flag	Coverage Δ
unittests	89.29% <ø> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[saikishor]

LGTM.

Yes, let's remove the red one

[soham2560]

Yes, let's remove the red one

Done! refer 4bc3907

[christophfroehlich]

thanks for the cleanup. However, I think that the yellow button from inactive state should be called cleanup and trigger cleanup transition to unconfigured state? Like it is with the hardware components. Or it is the wrong icon and should be cleanup -> unload.

And actually, but not related to this PR? why don't we have the

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

[soham2560]

thanks for the cleanup. However, I think that the yellow button from inactive state should be called cleanup and trigger cleanup transition to unconfigured state? Like it is with the hardware components. Or it is the wrong icon and should be cleanup -> unload.

@christophfroehlich sorry for the late reply, I missed that completely, for this PR I think renaming it to "Deactivate, Cleanup and Unload" and changing the icon to match the unloaded state would solve make it technically right, though still not a complete solution (for that refer below comment), let me know if I should move forward with the same

[soham2560]

And actually, but not related to this PR? why don't we have the
unload transition from unconfigured
deactivate->unload transition from active?

Good catch, I think we can add that for the controllers, 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

Let me know how it sounds, I can take care of this in another PR if you want

[christophfroehlich]

Let me know how it sounds, I can take care of this in another PR if you want

Sounds great. We could discuss if it is necessary to have the multi-transition options, or only expose one transition at a time? At least it should be consistent for all, otherwise it gets confusing.

[christophfroehlich]

thanks for the cleanup. However, I think that the yellow button from inactive state should be called cleanup and trigger cleanup transition to unconfigured state? Like it is with the hardware components. Or it is the wrong icon and should be cleanup -> unload.

@christophfroehlich sorry for the late reply, I missed that completely, for this PR I think renaming it to "Deactivate, Cleanup and Unload" and changing the icon to match the unloaded state would solve make it technically right, though still not a complete solution (for that refer below comment), let me know if I should move forward with the same

Let's rename it to cleanup and call only the cleanup transition. Let's fix the rest in the already mentioned future PR.

[soham2560]

Let's rename it to cleanup and call only the cleanup transition. Let's fix the rest in the already mentioned future PR.

sounds good, have added change in 74a0a86, since there is no direct cleanup service (waiting on #1236 I believe), had to do

 elif action is action_cleanup:
     self._deactivate_controller(ctrl.name)
     unload_controller(self._node, self._cm_name, ctrl.name)
     load_controller(self._node, self._cm_name, ctrl.name)

You should now get cleanup option in inactive state, transitions the controller to unconfigured state

let me know if its good, I'll open up the other issue till then

[christophfroehlich]

Controllers: From unconfigured state we don't have the unload option, but let's fix that in #2151
components: "Deactivate and Cleanup" should be yellow, right?

[soham2560]

components: "Deactivate and Cleanup" should be yellow, right?

Yep, missed that, updated in 9ec1db7

[soham2560]

one more thing, the led on Deactivate and Unload is grey(for finalised), and the unload state is actually black, do we really need a finalised led?

[christophfroehlich]

Thanks, LGTM!

[christophfroehlich]

one more thing, the led on Deactivate and Unload is grey(for finalised), and the unload state is actually black, do we really need a finalised led?

you are right. As we don't have a finalized state, let's use the same led (choose which).

[saikishor]

Thanks guys. LGTM