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

[Example 10] RRbot with GPIO interfaces #256

Merged
merged 32 commits into from
Sep 25, 2023

Conversation

christophfroehlich
Copy link
Contributor

I took the description from a ros2_control test and added a simple GPIO controller.

Is there anything more to present with GPIO interfaces?

Closes #149

@christophfroehlich christophfroehlich added the New example This PR is proposing a new example. label Mar 17, 2023
example_10/README.md Outdated Show resolved Hide resolved
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.

rest LTGM

example_10/controllers/gpio_controller.cpp Outdated Show resolved Hide resolved
example_10/controllers/gpio_controller.cpp Outdated Show resolved Hide resolved
example_10/controllers/gpio_controller.cpp Outdated Show resolved Hide resolved
example_10/controllers/gpio_controller.cpp Outdated Show resolved Hide resolved
example_10/controllers/gpio_controller.cpp Outdated Show resolved Hide resolved
example_10/hardware/rrbot.cpp Outdated Show resolved Hide resolved
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

Copy link
Collaborator

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

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

LGTM and tested on rolling.

@christophfroehlich christophfroehlich merged commit 60b788f into ros-controls:master Sep 25, 2023
@christophfroehlich christophfroehlich deleted the example_10 branch September 26, 2023 02:28
@christophfroehlich
Copy link
Contributor Author

@Mergifyio backport humble

@mergify
Copy link
Contributor

mergify bot commented Sep 26, 2023

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Sep 26, 2023
* gpio controller for single interface

* Use consistent topics for gpio_controller

* Add parameters for gpio_controller interface names

* Add URDF checks to hw interface

* Add example_10 to CI

* Add readme in example folder

(cherry picked from commit 60b788f)

# Conflicts:
#	.github/workflows/ci-coverage-build.yml
#	.github/workflows/ci-ros-lint.yml
christophfroehlich added a commit that referenced this pull request Sep 26, 2023
* gpio controller for single interface

* Use consistent topics for gpio_controller

* Add parameters for gpio_controller interface names

* Add URDF checks to hw interface

* Add example_10 to CI

* Add readme in example folder

(cherry picked from commit 60b788f)

# Conflicts:
#	.github/workflows/ci-coverage-build.yml
#	.github/workflows/ci-ros-lint.yml

Co-authored-by: Christoph Fröhlich <[email protected]>
for (auto state_if : info_.gpios.at(i).state_interfaces)
{
state_interfaces.emplace_back(hardware_interface::StateInterface(
info_.gpios.at(i).name, state_if.name, &hw_gpio_in_[ct++]));
Copy link

@tonynajjar tonynajjar Oct 4, 2023

Choose a reason for hiding this comment

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

I see that for GPIOs we don't really have a standard interface name so we're using whatever name is specified in the URDF. Would it make sense to create a new HW_IF_BOOLEAN interface type? I can create the issue.

I found myself needing this interface also when controlling an actuator so I think it could be generally useful. Opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we discussed something related to this in the last WG meeting: GPIO is now only for semantic reasons, and is of type double. Supporting other datatypes (boolean, or even ROS msgs) is a repeating topic but no one made a sustainable suggestion on how to implement it yet (or had time to make a PR)

You mean that we add a dedicated boolean GPIO in addition to "position", "velocity",..? then we would have only one possible boolean-gpio-interface per gpio-tag. Or you'd like to add a boolean-gpio-interface to the joints?

Copy link

@tonynajjar tonynajjar Oct 4, 2023

Choose a reason for hiding this comment

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

Let me clarify by using the example 10, more specifically

      <gpio name="flange_vacuum">
        <command_interface name="vacuum"/>
        <state_interface name="vacuum"/> <!-- Needed to know current state of the input -->
      </gpio>

It doesn't make much sense to me how the command and state interface names of this GPIO are vacuum. Correct me if I'm wrong but to me a command/state interface is the "physics" type of the value (position, velocity.... possibly voltage, temperature....).
vacuum is ambiguous; does the value represent the suction strength? or whether it's on or off?

What I'm proposing is adding in hardware_interface_type_values.hpp a HW_IF_BOOLEAN (or similarly named) to represent a common state/command interface not only for GPIOs, but also to joints and sensors.

I guess this leads to what you mentioned then:

Supporting other datatypes (boolean, or even ROS msgs) is a repeating topic but no one made a sustainable suggestion on how to implement it yet (or had time to make a PR)

Copy link
Contributor Author

@christophfroehlich christophfroehlich Oct 4, 2023

Choose a reason for hiding this comment

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

I totally agree, the interface vacuum could be named more specifically.

But by just adding a boolean to the list would only allow one state- or command interface of this type per sensor/joint/GPIO component in this form:

  <gpio name="flange_vacuum">
    <command_interface name="boolean"/>
    <state_interface name="boolean"/>
  </gpio>

Using a component with a set of boolean gpios is not possible then (think of an 8 bit digital input card)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New example This PR is proposing a new example.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add example with GPIO interfaces used
5 participants