Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gpio command controller #1251

Merged
merged 68 commits into from
Nov 18, 2024
Merged

Gpio command controller #1251

merged 68 commits into from
Nov 18, 2024

Conversation

Wiktor-99
Copy link
Contributor

@Wiktor-99 Wiktor-99 commented Aug 17, 2024 •
edited
Loading

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.

mcbed and others added 30 commits July 26, 2024 09:25
@mcbed @christophfroehlich
added gpio_command_controller
5fecf27
@mcbed @christophfroehlich
added doc to gpio controllers
6ffda9d
@mcbed @christophfroehlich
changed parameter interface_names to command_interfaces
f88fc7d
@mcbed @christophfroehlich
added test
56fc1e5
@mcbed @christophfroehlich
code formating
9e73ab7
@christophfroehlich
added gpio state publisher
1cbd3a6
@christophfroehlich
fixed broken node_state
7228e06
@christophfroehlich
added DynamicJointState message for Gpio state
880aeea
@christophfroehlich
fixed tests and added gpio state tests
94befb0
@christophfroehlich
fixed format
882c936
@Wiktor-99
Apply pre-commit fix
efce87a
@Wiktor-99
Fix compilation on ros rolling
b53bff6
@Wiktor-99
Add parameters file, use generated gpio_command_controller_parameters…
67b6069 
… params and align UTs
@Wiktor-99
Remove unnecessary members: interface_names_, gpios_names_ and use pa…
30d2604 
…rams struct instead.

Use default member initializer.
@Wiktor-99
Refactor of storing interface types
4c824e8
@Wiktor-99
Remove unnecessary includes
dd86d9e
@Wiktor-99
Refacotr on_activate method. Add validate_configured_interfaces and i…
4ac938d 
…nitialize_gpio_state_msg methods
@Wiktor-99
Refactor update method, split update of state if and command if
aa59297
@Wiktor-99
Move test fixture form hpp to cpp
6276617
@Wiktor-99
Update docks and package.xml
66bd971
@Wiktor-99
Add state validation
7b70159
@Wiktor-99
Store command and state ifs in the map
58c1afc
@Wiktor-99
Use interface maps for command and state update. Use DynamicJointStat…
e5c65db 
…e as cmd type
@Wiktor-99
Use ordered state if vector instead of map
0dc5c87
@Wiktor-99
Restore state if validation
765a3cb
@Wiktor-99
Align logging
a1c8fa3
@Wiktor-99
Update code layout
de8acba
@Wiktor-99
Simplifie test fixture
aebbf0b
@Wiktor-99
Update tests. Stop using pointer, set members as public
0deb417
@Wiktor-99
Add set of test for init
3e7a12a
@christophfroehlich
Copy link
Contributor

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 Wiktor-99 mentioned this pull request Nov 12, 2024
Merged
@Wiktor-99
Copy link
Contributor Author

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 ros-controls/control_msgs#155. When it merges, I'll align this pr.

@saikishor
Copy link
Member

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

@christophfroehlich
Copy link
Contributor

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

you are right!

@Wiktor-99
Copy link
Contributor Author

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

christophfroehlich
christophfroehlich requested changes Nov 17, 2024
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I once again tested it successfully with the demo PR. One last request from my side: please add a new section to the release notes.

gpio_controllers/doc/userdoc.rst Outdated Show resolved Hide resolved
gpio_controllers/doc/userdoc.rst Outdated Show resolved Hide resolved
christophfroehlich
christophfroehlich approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

saikishor
saikishor approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for addressing all the comments @Wiktor-99 and @christophfroehlich

@mergify mergify bot mentioned this pull request Nov 18, 2024
Merged
@christophfroehlich @saikishor
Update compile flags
4cd5772 
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
@christophfroehlich christophfroehlich merged commit 0590c6a into ros-controls:master Nov 18, 2024
15 of 22 checks passed
mergify bot pushed a commit that referenced this pull request Nov 18, 2024
@Wiktor-99 @mergify
Gpio command controller (#1251)
dfa58f6 
---------

Co-authored-by: m.bednarczyk <m.bednarczyk@unistra.fr>
Co-authored-by: Maciej Bednarczyk <macbednarczyk@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
(cherry picked from commit 0590c6a)

# Conflicts:
#	doc/release_notes.rst
@mergify mergify bot mentioned this pull request Nov 18, 2024
Draft
@christophfroehlich
Copy link
Contributor

@Wiktor-99 could you please review ros-controls/ros2_control_demos#627 ?

@Wiktor-99
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saikishor saikishor saikishor approved these changes

@christophfroehlich christophfroehlich christophfroehlich approved these changes

@bmagyar bmagyar Awaiting requested review from bmagyar

@destogl destogl Awaiting requested review from destogl

@mcbed mcbed Awaiting requested review from mcbed

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

@progtologist progtologist Awaiting requested review from progtologist

@olivier-stasse olivier-stasse Awaiting requested review from olivier-stasse

Assignees
No one assigned
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Wiktor-99 @christophfroehlich @saikishor @olivier-stasse @mcbed @bmagyar