Gpio command controller

[Wiktor-99]

As I discussed with @christophfroehlich I've opened new pr with gpio controller.

This PR is a follow-up to the thread and implements a controller to send commands to GPIO interfaces, allowing specific command interfaces for each GPIO.

I have made the following changes compared to the original PR:

• I utilized the 'DynamicJointState' message for both the command and state interfaces (this allows sending commands with specific GPIO values without having to worry about the order of ports in the command message).
• I added a parameters file, and the new input parameters file will look something like this:

gpios:
  - gpio1
  - gpio2
command_interfaces:
  - gpio1:
    - ports:
      - dig1
      - dig2
  - gpio2:
    - ports:
      - ana1

• I've done significant refactoring and have added many new UTs.

[Wiktor-99]

There was some issue with includes I've fixed it so jobs can be run more time.

[Wiktor-99]

I've fixed docks and pre-commit jobs other two jobs failed due to pid controller and ackermann steering controller. I believe that now CI should be green.

[Wiktor-99]

@christophfroehlich could you rerun the CI?

[Wiktor-99]

@christophfroehlich could you rerun the CI?

It appears that the CI is unstable and is failing on the diff drive and pid controller.

[christophfroehlich]

@christophfroehlich could you rerun the CI?

It appears that the CI is unstable and is failing on the diff drive and pid controller.

Binary builds due to a recent API breaking change. Semi-binary seems to be happy.

[Wiktor-99]

@christophfroehlich could you rerun the CI?

It appears that the CI is unstable and is failing on the diff drive and pid controller.

Binary builds due to a recent API breaking change. Semi-binary seems to be happy.

Okay, so what steps should be taken to merge the pr?

[christophfroehlich]

Someone has to find time to test and review it..

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 93.89474% with 29 lines in your changes missing coverage. Please review.

Project coverage is 83.05%. Comparing base (2e57917) to head (bc3eb30). Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
gpio_controllers/src/gpio_command_controller.cpp	85.31%	21 Missing and 5 partials :warning:
..._controllers/test/test_gpio_command_controller.cpp	98.97%	0 Missing and 3 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1251      +/-   ##
==========================================
+ Coverage   82.55%   83.05%   +0.50%     
==========================================
  Files         112      115       +3     
  Lines        9922    10399     +477     
  Branches      869      897      +28     
==========================================
+ Hits         8191     8637     +446     
- Misses       1438     1459      +21     
- Partials      293      303      +10     

Flag	Coverage Δ
unittests	83.05% <93.89%> (+0.50%)	:arrow_up:

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

Files with missing lines	Coverage Δ
...rollers/test/test_load_gpio_command_controller.cpp	100.00% <100.00%> (ø)
..._controllers/test/test_gpio_command_controller.cpp	98.97% <98.97%> (ø)
gpio_controllers/src/gpio_command_controller.cpp	85.31% <85.31%> (ø)

... and 2 files with indirect coverage changes

[Wiktor-99]

Thanks for feedback!

Renaming ports to interfaces is reasonable.

I followed first options and implement parameterizable state interfaces. Currently you can configure command interfaces and state interfaces separately. When no state interfaces are specified then controller fetches them from urdf (only for configured gpios). With such implementations you don't have to specify command or state interfaces if you want to broadcast all states of configured gpios.

I've tested it with mentioned example and UTs and seems to work.

[Wiktor-99]

I've fixed it. Now the CI no should be green.

[Wiktor-99]

Great! I don't know code of conduct but if I can I could help with reviewing linked example.

[christophfroehlich]

@saikishor raised a legitimate question: Should we allow zero command interfaces for this controller (what happens actually then?)? or should we set the limit size>0 here, and create a gpio_controllers/GpioStateBroadcaster for broadcasting states only?

[christophfroehlich]

Great! I don't know code of conduct but if I can I could help with reviewing linked example.

Your review is very welcome!

[Wiktor-99]

When we don't configure any command interface then controller will just behave as broadcaster and will publish state of configured gpios (explicitly configured or taken from urdf). Probably we should't create commands topic it might fixed.

If we want to separate to controller/broadcaster we could fallback to initial version and use same interface name for commands and states and then add broadcaster.

[christophfroehlich]

One last comment @saikishor brought up: Using DynamicJointStates for the commands feels semantically more than wrong here. Should we add a copy and name it more appropriate? DynamicResourceValuesor something better. Even for the broadcaster it is questionable.

[Wiktor-99]

Yes, I totally agree with you. Speaking of naming I would propose to add new message DynamicComponentValues, resource sounds like it could be acquired.

I've posted pr in the control_msgs repo https://github.com/ros-controls/control_msgs/pull/155. When it merges, I'll align this pr.

[saikishor]

@Wiktor-99 @christophfroehlich may be this PR is also missing dependency in the ros2_controllers metapkg

[christophfroehlich]

@Wiktor-99 @christophfroehlich may be this PR is also missing dependency in the ros2_controllers metapkg

you are right!

[Wiktor-99]

Yep, it had been missing. I've aligned it to use DynamicInterfaceGroupValues and fixed dependency in the meta pkg.

[christophfroehlich]

@Wiktor-99 could you please review https://github.com/ros-controls/ros2_control_demos/pull/627 ?

[Wiktor-99]

@christophfroehlich Yep I'll do it, can't promise I'll do it today but tomorrow seems feasible.