Skip to content

Components architecture and examples.#22

Merged
bmagyar merged 17 commits intoros-controls:masterfrom
destogl:control_components_design
Aug 21, 2020
Merged

Components architecture and examples.#22
bmagyar merged 17 commits intoros-controls:masterfrom
destogl:control_components_design

Conversation

@destogl
Copy link
Copy Markdown
Member

@destogl destogl commented Aug 4, 2020

No description provided.

@destogl destogl requested review from Karsten1987 and bmagyar August 4, 2020 16:22
destogl and others added 10 commits August 11, 2020 11:00
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Copy link
Copy Markdown
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

In principle this looks good to me. I think we need more flesh to this though to describe things in more detail.

I think use-cases can be shifted around, maybe even combined - thinking about 1 & 4 as well as 6 & 7 & 8. Within these blocks, only the difference could be then highlighted.


![ros2_control Components Achitecture][ros2_control_arch_resource_manager]

[ros2_control_arch_resource_manager]: images/components_architecture.png "ROS2 Control - Components Architecture"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure how to comment on the image, so I'll do it here:
I think it's an implementation detail to mention "ActuatorHardwareInterface" and would remove that row of boxes with "template to implement ..."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As for wording, I know it's a constant bike shedding, but I don't see the point to add "Hardware" to "ActuatorHardware". Same for sensors and systems.

However, what this document lacks in my opinion is a general introduction/motivation and explanation of wording. Basically saying what's meant with "actuator", what's a system etc. Basically explaining the vision. Saying that a robot hardware with a basic main loop had to be written for ROS1, the goal here with ROS2 is to create an architecture to dynamically compose a robot.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure how to comment on the image, so I'll do it here:
I think it's an implementation detail to mention "ActuatorHardwareInterface" and would remove that row of boxes with "template to implement ..."

I wanted to have this implementation detail in it. The image is something between implementation UML and architecture and I hope for a user would be clear where to starts its implementation.

As for wording, I know it's a constant bike shedding, but I don't see the point to add "Hardware" to "ActuatorHardware". Same for sensors and systems.

I did it mainly to make a difference between "Joint" and "Sensor" on the right side, and those implementations of interfacing the hardware directly. This wording is in line with ROS1 ros_control with RobotHardware and RobotHardwareInterface.

However, what this document lacks in my opinion is a general introduction/motivation and explanation of wording. Basically saying what's meant with "actuator", what's a system etc.

I agree with you. My intention of current state of PR is to clarify the URDF structures, so we can start implement this in the main repository.


## URDF Examples

ros2_control implementation examples are presented for the following robot/robot-cell architectures:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what's a robot cell?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What is the proper English term for a fenced area where robot is, usually with other machines like feeders, doing his thing? image

In German is called "RoboterZellle". Therefore, I thought "cell" is a good term.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Caged robots is the common term in English I believe


Note:
* Everything within the `<classType>` tag is implemented as a plugin.
* The examples below have some `<param>` tags defined for each plugin which are primarily for demonstration, not part of a pre-defined xml schema. Components can define their own parameters.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand this. The param tag should be part of the xml schema - otherwise this tag wouldn't be allowed. I think what you want to mention is that the attributes and values are free to be chosen per plugin, is that right?
But we have to maybe clarify how these parameters are being used. Saying how each plugin is supposed to obtain these parameters.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I reword this. Is it better now?

But we have to maybe clarify how these parameters are being used.

This would be done in the example implementation.

<param name="example_param_write_for_sec">2</param>
<param name="example_param_read_for_sec">2</param>
</hardware>
<joint name="joint1">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know we've talked about this and you don't like it, but I think it is clearer here to replace "joint" with "joint_handle" or something similar to emphasize that this joint is purely virtual and abstract to any hardware specific implementation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this joint is purely virtual and abstract to any hardware specific implementation

I do not agree. On this level, in the RessourceManager all joints are actually the real joints of the hardware. First when we go into ControllerManager virtual joints are used. (This is also shown, not in full detail, in the image)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then I still don't understand what's happening with joints. In my understanding, the term joint here described something abstract which encapsulates common pattern like joint limits etc before these values are then finally passed to the system implementation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's the forward facing XML format we are trying to set up and the code that this will eventually tie into. I would 100% consider joint_handle an implementation detail nobody should care about when defining/reading a URDF. No matter how this is implemented, the URDF should never refer to handles, let's keep it at the abstract terms level.

Comment on lines +53 to +58
Note:
* `ros2_control_components/PositionJoint`type has implicitly:
```xml
<commandInterfaceType>position</commandInterfaceType>
<stateInterfaceType>position</stateInterfaceType>
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this note should come later in the document as we've never mentioned "commandInterface" up to this point.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We should add some general description at the beginning.

<param name="max_values">1</param>
</joint>
</ros2_control>
<ros2_control name="2DOF_System_Robot_ForceTorqueSensor" type="sensor">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it deserves a bit more context or description that we have two separate ros2_control tags in here and thus treat the two components individually.

destogl and others added 5 commits August 12, 2020 07:19
Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
after testing them with parser in ros-controls/ros2_control#127
@Karsten1987 Karsten1987 changed the title Components archtiecture and examples. Components architecture and examples. Aug 17, 2020
@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Aug 21, 2020

follow-up: #23

Copy link
Copy Markdown
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

This is now ready to merge, please open follow-up issues/PRs with changes you wish.

@bmagyar bmagyar merged commit 6002b60 into ros-controls:master Aug 21, 2020
@destogl destogl deleted the control_components_design branch September 8, 2020 11:13
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.

3 participants