[CLI] Add `set_hardware_component_state` verb #1248

christophfroehlich · 2023-12-25T18:58:09Z

I updated the hardware lifecylce demo as part of the multi robot example ros-controls/ros2_control_demos#417 and realized that there is no cli verb for changing hardware lifecycle.

This PR adds the verb set_hardware_component_state to our CLI.

codecov · 2023-12-25T19:10:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.02%. Comparing base (35bb5f7) to head (5e42962).
Report is 7 commits behind head on master.

❗ Current head 5e42962 differs from pull request most recent head 6fa3492. Consider uploading reports for the commit 6fa3492 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1248       +/-   ##
===========================================
- Coverage   75.51%   48.02%   -27.49%     
===========================================
  Files          41       41               
  Lines        3328     3525      +197     
  Branches     1921     1912        -9     
===========================================
- Hits         2513     1693      -820     
- Misses        421      442       +21     
- Partials      394     1390      +996

Flag	Coverage Δ
unittests	`48.02% <ø> (-27.49%)`	⬇️

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

see 32 files with indirect coverage changes

olivier-stasse

LGTM.
Thanks a lot.

destogl

Few smaller comments and one bigger to discuss

ros2controlcli/ros2controlcli/verb/set_controller_state.py

ros2controlcli/ros2controlcli/verb/set_hardware_component_state.py

destogl · 2024-01-12T17:18:37Z

ros2controlcli/ros2controlcli/verb/set_hardware_component_state.py

+            if args.state == "inactive":
+                inactive_state = State()
+                inactive_state.id = State.PRIMARY_STATE_INACTIVE
+                inactive_state.label = "inactive"
+
+                if matched_hardware_components.state.label == "unconfigured":
+                    response = set_hardware_component_state(
+                        node, args.controller_manager, args.hardware_component_name, inactive_state
+                    )
+                    if not response.ok:
+                        return (
+                            "Error configuring hardware component, check controller_manager logs"
+                        )
+
+                    print(
+                        f"Successfully set {args.hardware_component_name} to state {response.state}"
+                    )
+                    return 0
+
+                elif matched_hardware_components.state.label == "active":
+                    response = set_hardware_component_state(
+                        node, args.controller_manager, args.hardware_component_name, inactive_state
+                    )
+                    if not response.ok:
+                        return "Error stopping hardware component, check controller_manager logs"
+
+                    print(
+                        f"Successfully set {args.hardware_component_name} to state {response.state}"
+                    )
+                    return 0
+
+                else:
+                    return (
+                        f'cannot put {matched_hardware_components.name} in "inactive" state '
+                        f"from its current state {matched_hardware_components.state}"
+                    )
+
+            if args.state == "active":
+                if matched_hardware_components.state.label != "inactive":
+                    return (
+                        f"cannot activate {matched_hardware_components.name} "
+                        f"from its current state {matched_hardware_components.state}"
+                    )


This logic is also written in the ResourceManager, should we just let any request come through and then return here error or success with a message. Not sure if doubling logic is good for maintainability, but it is good for efficiency. Still, not sure that this degree of efficiency is needed here.

I also think that we shouldn't duplicate the logic,
But what I want to avoid is something like with other controller-manager services, where one log-line is an error from controller-manager but the service returns OK. I'll have a look if response.ok is fine for giving useful feedback with the CLI

I removed the logic now..

mateusmenezes95

Just a minor minor fix

ros2controlcli/doc/userdoc.rst

Co-authored-by: Dr. Denis <[email protected]>

Co-authored-by: Mateus Menezes <[email protected]>

saikishor

LGTM

destogl

Thanks!

Co-authored-by: Dr. Denis <[email protected]> Co-authored-by: Mateus Menezes <[email protected]> (cherry picked from commit a902e27)

Co-authored-by: Dr. Denis <[email protected]> Co-authored-by: Mateus Menezes <[email protected]> (cherry picked from commit a902e27) Co-authored-by: Christoph Fröhlich <[email protected]>

github-actions bot requested review from bijoua29, bmagyar, destogl, olivier-stasse, peterdavidfagan, VanshGehlot and VX792 December 25, 2023 18:59

christophfroehlich added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron labels Dec 25, 2023

olivier-stasse approved these changes Dec 26, 2023

View reviewed changes

destogl reviewed Jan 12, 2024

View reviewed changes

christophfroehlich force-pushed the set_hardware_component_state branch from d8bf1d1 to 5e42962 Compare January 12, 2024 21:45

destogl previously approved these changes Jan 25, 2024

View reviewed changes

mateusmenezes95 reviewed Jan 25, 2024

View reviewed changes

ros2controlcli/doc/userdoc.rst Outdated Show resolved Hide resolved

christophfroehlich dismissed destogl’s stale review via 83eb0da January 26, 2024 17:10

christophfroehlich and others added 4 commits March 23, 2024 15:30

Add set_hardware_component_state_verb

3d44d4e

Apply suggestions from code review

0effe54

Co-authored-by: Dr. Denis <[email protected]>

Update ros2controlcli/doc/userdoc.rst

41ddf89

Co-authored-by: Mateus Menezes <[email protected]>

Remove checks and just call the service

6fa3492

christophfroehlich force-pushed the set_hardware_component_state branch from 83eb0da to 6fa3492 Compare March 23, 2024 15:44

christophfroehlich requested review from mateusmenezes95 and destogl March 23, 2024 15:44

christophfroehlich mentioned this pull request Mar 23, 2024

[rqt_controller_manager] Add hardware components #1455

Merged

mateusmenezes95 approved these changes Mar 24, 2024

View reviewed changes

saikishor approved these changes Mar 25, 2024

View reviewed changes

bmagyar approved these changes Apr 1, 2024

View reviewed changes

destogl approved these changes Apr 1, 2024

View reviewed changes

destogl merged commit a902e27 into ros-controls:master Apr 1, 2024
6 of 11 checks passed

mergify bot pushed a commit that referenced this pull request Apr 1, 2024

[CLI] Add set_hardware_component_state verb (#1248)

3786661

Co-authored-by: Dr. Denis <[email protected]> Co-authored-by: Mateus Menezes <[email protected]> (cherry picked from commit a902e27)

mergify bot mentioned this pull request Apr 1, 2024

[CLI] Add set_hardware_component_state verb (backport #1248) #1470

Merged

mergify bot pushed a commit that referenced this pull request Apr 1, 2024

[CLI] Add set_hardware_component_state verb (#1248)

40d080d

Co-authored-by: Dr. Denis <[email protected]> Co-authored-by: Mateus Menezes <[email protected]> (cherry picked from commit a902e27)

mergify bot mentioned this pull request Apr 1, 2024

[CLI] Add set_hardware_component_state verb (backport #1248) #1471

Merged

christophfroehlich deleted the set_hardware_component_state branch April 7, 2024 20:32

This was referenced Jun 25, 2024

[rqt_controller_manager] Add hardware components (backport #1455) #1586

Merged

[rqt_controller_manager] Add hardware components (backport #1455) #1587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLI] Add `set_hardware_component_state` verb #1248

[CLI] Add `set_hardware_component_state` verb #1248

christophfroehlich commented Dec 25, 2023 •

edited

Loading

codecov bot commented Dec 25, 2023 •

edited

Loading

olivier-stasse left a comment •

edited

Loading

destogl left a comment

destogl Jan 12, 2024

christophfroehlich Jan 12, 2024 •

edited

Loading

christophfroehlich Mar 23, 2024

mateusmenezes95 left a comment

saikishor left a comment

destogl left a comment

[CLI] Add set_hardware_component_state verb #1248

[CLI] Add set_hardware_component_state verb #1248

Conversation

christophfroehlich commented Dec 25, 2023 • edited Loading

codecov bot commented Dec 25, 2023 • edited Loading

Codecov Report

olivier-stasse left a comment • edited Loading

Choose a reason for hiding this comment

destogl left a comment

Choose a reason for hiding this comment

destogl Jan 12, 2024

Choose a reason for hiding this comment

christophfroehlich Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

christophfroehlich Mar 23, 2024

Choose a reason for hiding this comment

mateusmenezes95 left a comment

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

destogl left a comment

Choose a reason for hiding this comment

[CLI] Add `set_hardware_component_state` verb #1248

[CLI] Add `set_hardware_component_state` verb #1248

christophfroehlich commented Dec 25, 2023 •

edited

Loading

codecov bot commented Dec 25, 2023 •

edited

Loading

olivier-stasse left a comment •

edited

Loading

christophfroehlich Jan 12, 2024 •

edited

Loading