Skip to content

WIP: New robot hardware interface design#80

Closed
destogl wants to merge 10 commits intoros-controls:masterfrom
destogl:hardware_interface_new_design
Closed

WIP: New robot hardware interface design#80
destogl wants to merge 10 commits intoros-controls:masterfrom
destogl:hardware_interface_new_design

Conversation

@destogl
Copy link
Copy Markdown
Member

@destogl destogl commented May 20, 2020

Moved ros2_control_core from demo repository.

This is related to:
ros-controls/ros2_control_demos#6
ros-controls/ros2_control_demos#7

@destogl destogl changed the title Added new robot hardware interface design WIP: New robot hardware interface design May 20, 2020
@destogl
Copy link
Copy Markdown
Member Author

destogl commented Jun 1, 2020

Puh, many lines of code.... sorry for that guys, this thing is getting very, very complex...
We should probably simplify some parts to get a minimal version working.

If possible just look at the main idea and it would be great to discuss it in WG meeting. The code is designed according to the design document in the roadmap.

@bmagyar @Karsten1987

@Karsten1987
Copy link
Copy Markdown
Contributor

@destogl thanks for getting thing started here. As you can imagine that PR is pretty massive and I think realistically to make this work we have to break this down into multiple PRs.

As you said it yourself, we might want to have a minimal set which is also directly usable with the current code architecture. I propose we start with only introducing the ros2_control_components package by itself. Introducing interfaces for actuators and sensors, having default implementations for actuators like position, velocity, torque and their equivalent sensors.

I still haven't fully understood why there is such a package as ros2_control_core and I don't understand why the general sensor and actuator interfaces are living in this package and not in ros2_control_components. The way I envision this to run for a first pass is that we introduce the ros2_control_components package (or maybe better called ros2_control_hardware_components) as explained above and integrate this into the existing hardware_interface package. The hardware_interface package can then use the components to set up the robot hardware. This has the benefit that we don't have to adopt anything else (controller manager or controlers) to make this work already. Does this make sense?

We can then in a second iteration introduce another level of complexity - always integrating it to a degree that it's actually being used. The current PR provides a lot of code, but it's not being used anywhere.


Generally, while looking at the code, I have the feeling we're drifting already in a direction which leads to template and inheritance madness. I would strongly recommend that we stick to simple inheritance, that is defining abstract interface classes and having final classes using pointers to these interfaces via dependency injection. What I mean by this:

class SensorInterface
{
   hardware_ret_t read(std::vector<uint64_t> & data) = 0; 
};

class PositionSensor : public SensorInterface
{
  hardware_ret_t read(std::vector<uint64_t> & data)
  {
      data = my_device->read();
  }
};

class Sensor final
{
  Sensor(std::unique_ptr<SensorInterface> impl)
  : impl_(std::move(impl))
  {}

  hardware_ret_t read(std::vector<uin64_t> & data)
  {
     return impl_->read(data);
  }
};

The above example can hopefully illustrate the point here. That's pretty ABI/API stable, doesn't use a hell of inheritance/templates but moreover each individual implementation can easily be tested. If you'd take it even more serious like others we could get rid of that inheritance as well, but I think that'd be too crazy for a first run.

The hardware interface could then simply be composed of a list of these Sensors and Actuators.


That's pretty much all I would see as a first PR here. I'd leave our the ros2_control_core package as well as the communication part for now.
I see two follow ups from there:

  • Load the sensors and actuators dynamically from the newly introduced tag inside the URDF
  • Integrating the list of sensors and actuators as the base for the dynamic joint state message and pass these to the joint handles. Or maybe get rid of the joint handles ;-)

@Karsten1987
Copy link
Copy Markdown
Contributor

Another thing I've just noticed:

In the context of ROS2, we have to be a bit more careful about choosing how many nodes we'd
like to set up. I've seen that you propagate the node interfaces (such as parameters, services, etc.) through the components.
While that works, I'd recommend to develop the components as much as possible in a ROS agnostic way. The configure function to that respect could just directly get all needed parameters extracted from the parameter interface on the caller side. That way, all ROS related things can be handled within the hardware interface or whatever instantiates the components.

@destogl
Copy link
Copy Markdown
Member Author

destogl commented Jun 11, 2020

@Karsten1987 thank you for your comments. This is exactly I was hoping for :)

I will use the time until the next meeting to hopefully implement the first functional version.

For the ROS agnostic part... I am not sure how to implement this completely. The parameters are not a problem, especially if we are allowed to use undeclared parameters. The issue I see is with logging functions. The logger should be available from all components.

@destogl
Copy link
Copy Markdown
Member Author

destogl commented Jun 12, 2020

Closed in favor of #101

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.

2 participants