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.

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

Gpio command controller #342

Closed
wants to merge 10 commits into from
Closed

Conversation

mcbed
Copy link
Member

@mcbed mcbed commented Apr 26, 2022

Controller to send commands to gpio interfaces allowing specific command interfaces for each gpio.

@mcbed
Copy link
Member Author

mcbed commented Apr 29, 2022

Do you think there could be some interest in such a controller ? Currently I use it in galactic so is it interesting to adapt it for rolling ?

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Thank you for the great proposal!

Just few comments to discuss, making this even more generic.

gpio_controllers/doc/userdoc.rst Outdated Show resolved Hide resolved
gpio_controllers/src/gpio_command_controller.cpp Outdated Show resolved Hide resolved
gpio_controllers/src/gpio_command_controller.cpp Outdated Show resolved Hide resolved
@mcbed mcbed force-pushed the gpio_controllers branch from a106a4c to 0ea1f49 Compare August 3, 2022 13:02
@mcbed mcbed requested a review from destogl August 3, 2022 13:10
@boilerbots
Copy link

I am building a system with Advantech EtherCAT modules and I want to use this controller or something similar. I think this gets a little confusing with comand_interfaces and state_interfaces.

What about dividing up Digital vs Analog interfaces instead? This would allow digital interfaces to have a single bit represent their state and floats for analog ports. The digital interfaces could define their direction and enforce that or just always be treated as bidirectional and depending on the hardware behind them you either read the value of the port or the internal state of the register.

The same thing would work for Analog ports.

@mcbed
Copy link
Member Author

mcbed commented Nov 2, 2022

I am building a system with Advantech EtherCAT modules and I want to use this controller or something similar. I think this gets a little confusing with comand_interfaces and state_interfaces.

What about dividing up Digital vs Analog interfaces instead? This would allow digital interfaces to have a single bit represent their state and floats for analog ports. The digital interfaces could define their direction and enforce that or just always be treated as bidirectional and depending on the hardware behind them you either read the value of the port or the internal state of the register.

The same thing would work for Analog ports.

Hi, I think that splitting Digital and Analog IO would reduce the generality of the controller but I am open to discussion.
Also, in its current version, the controller is intended to only provide commands to the interfaces tagged in the command_interface parameter. The state_interface flag does not exist yet in this controller, it was just a discussion argument, but the feedback of all command_interfaces is published in the ~/gpio_states topic as control_msgs::msg::DynamicJointState, thus my previous question: do you think it is the most relevant message type, even if the type joint is confusing, or should we add a new control_msgs type like DynamicGPIOState?
Do you think we should add an gpio_broadcaster as well ? Btw, in all cases, all other GPIO states are published by the joint_state_broadcaster.

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Merging #342 (72b56d7) into master (e7f9962) will decrease coverage by 6.03%.
The diff coverage is 20.21%.

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   35.78%   29.74%   -6.04%     
==========================================
  Files         189        7     -182     
  Lines       17570      743   -16827     
  Branches    11592      428   -11164     
==========================================
- Hits         6287      221    -6066     
+ Misses        994      162     -832     
+ Partials    10289      360    -9929     
Flag Coverage Δ
unittests 29.74% <20.21%> (-6.04%) ⬇️

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

Impacted Files Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <ø> (ø)
...ontroller/test/test_load_diff_drive_controller.cpp 12.50% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp 42.16% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 17.62% <12.08%> (ø)
diff_drive_controller/src/speed_limiter.cpp 46.55% <13.33%> (ø)
...troller/include/diff_drive_controller/odometry.hpp 20.00% <20.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 32.13% <24.11%> (ø)
...mand_controller/src/forward_command_controller.cpp
...ectory_controller/test/test_trajectory_actions.cpp
...int_trajectory_controller/test/test_trajectory.cpp
... and 192 more

@Wiktor-99
Copy link
Contributor

Hi @destogl @mcbed,

I notice that the PR is still open, so I assume that this feature is not the top priority. If you are short on time and there are tasks that need completion, I am willing to continue working on this.

@christophfroehlich
Copy link
Contributor

Hi @destogl @mcbed,

I notice that the PR is still open, so I assume that this feature is not the top priority. If you are short on time and there are tasks that need completion, I am willing to continue working on this.

Let's see what the CI is telling us about compatibility now.
We had some API breaking changes, at least they have to be addressed.
From what I see now, we also should add the generate_parameter_library for the parameter handling. @Wiktor-99 Do you want to take this over? Just start from this branch and open a new PR. If @mcbed is still around, you could also target your PR against his branch.

@Wiktor-99
Copy link
Contributor

Hi @destogl @mcbed,
I notice that the PR is still open, so I assume that this feature is not the top priority. If you are short on time and there are tasks that need completion, I am willing to continue working on this.

Let's see what the CI is telling us about compatibility now. We had some API breaking changes, at least they have to be addressed. From what I see now, we also should add the generate_parameter_library for the parameter handling. @Wiktor-99 Do you want to take this over? Just start from this branch and open a new PR. If @mcbed is still around, you could also target your PR against his branch.

Hi @destogl @mcbed,
I notice that the PR is still open, so I assume that this feature is not the top priority. If you are short on time and there are tasks that need completion, I am willing to continue working on this.

Let's see what the CI is telling us about compatibility now. We had some API breaking changes, at least they have to be addressed. From what I see now, we also should add the generate_parameter_library for the parameter handling. @Wiktor-99 Do you want to take this over? Just start from this branch and open a new PR. If @mcbed is still around, you could also target your PR against his branch.

Yes, I would like to take this over if @mcbed is okay with it.

@christophfroehlich
Copy link
Contributor

Superseded by #1251

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants