Resource manager parser by Karsten1987 · Pull Request #165 · ros-controls/ros2_control

Karsten1987 · 2020-09-26T03:34:01Z

That's a first initial PR towards #164

The PR parses the URDF, extracts the components and initializes their hardware instants accordingly.

Karsten1987 · 2020-09-26T03:34:50Z

This is a branch on top of #159 and has to be rebased once the other one is merged.

destogl

Just a few short comments.

you introduced some very nice concepts :)

Since the #147 a bit ahead of this PR. I propose to add there your ideas regarding ResourceStorage and tests.

I am currently working on "closing" the loop with the components, i.e. I am changing forward controller to use Joint-component. After that we have everything set-up for the test scenario I we should slowly add missing functionality and optimize the code.

Are you OK with my plan/idea?

controller_manager/src/resource_manager.cpp

Karsten1987 · 2020-09-28T18:21:11Z

controller_manager/test/test_hardware_resources/test_sensor_interface.cpp

+}
+
+hardware_interface::return_type
+TestSensor::read_sensors(


@destogl while implementing this, I was wondering why a sensor has to implement multiple sensors? Why not only a single component?

Karsten1987 · 2020-09-28T23:05:10Z

There's quite some changes to the hardware interfaces as well.
None of the functions in the components are marked as virtual, which made me struggle a bit to understand how these classes were originally designed to be used. @destogl fyi
My main concern here is that I thought we'll leave it up to the actual hardware to allocate memory for the state and commands. However, I've seen that the components have implementations for this. Which technically would require then (yet) another copy between controller and hardware. How is that supposed to be used?

What's further a bit unfortunate is that there a hardware type called "sensor" and a component type called "sensor". We might want to rename one of these, or both.

controller_manager/src/resource_manager.cpp

bmagyar

The concept looks good, a few minor comments, summary is

fprintf vs anything else, but consistency most importantly
unique_ptr may be overused
comment out function arg instead of e.g. (void) joint
some .cpp files could be wrapped in namespace hardware_interface to save a bunch of namespacing

bmagyar · 2020-09-29T06:21:42Z

controller_manager/src/resource_manager.hpp

+  // loan_sensor(const std::string & name);
+
+private:
+  std::unique_ptr<ResourceStorage> resource_storage_;


What is the reason for making this a unique_ptr?

http://oliora.github.io/2015/12/29/pimpl-and-rule-of-zero.html#scott-meyers-c11-pimpl

Do we need this though? Are we going to support ResourceManager inheritance or common interaction at the C++ API level? Is there an ABI to keep stable here? Not really a strong point from me, mostly curious about the motivation.

I'd imagine ResourceManager to be a private as you also pointed it out so PIMPL would be better applied to wrap resource manager instead?

controller_manager/test/test_hardware_resources/test_actuator_hardware.cpp

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

bmagyar · 2020-09-30T06:00:31Z

controller_manager/src/resource_manager.hpp

+
+  size_t system_interfaces_size() const;
+
+  // loan_joint(const std::string & name);


Alarm, commented code found

destogl · 2020-10-07T07:34:55Z

hardware_interface/CMakeLists.txt

+  target_link_libraries(test_component_parser components)
 endif()

-ament_export_dependencies(


May I ask, why did you remove those? Are they always available?

Karsten1987 self-assigned this Sep 26, 2020

Karsten1987 force-pushed the resource_manager_parser branch 2 times, most recently from 7b508fd to b207ae7 Compare September 26, 2020 17:44

bmagyar self-requested a review September 28, 2020 17:07

destogl reviewed Sep 28, 2020

View reviewed changes

controller_manager/src/resource_manager.cpp Show resolved Hide resolved

controller_manager/src/resource_manager.cpp Outdated Show resolved Hide resolved

Karsten1987 commented Sep 28, 2020

View reviewed changes

bmagyar reviewed Sep 29, 2020

View reviewed changes

controller_manager/src/resource_manager.cpp Show resolved Hide resolved

bmagyar requested changes Sep 29, 2020

View reviewed changes

Karsten1987 added 7 commits September 29, 2020 10:36

fix regression

e04463f

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

add main repos

0faa655

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

resource manager parses urdf

49dd98d

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

cleanup hardware_interface cmake

8d3f14b

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

rebase

05e4e0e

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

renaming

d6033ce

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

load hardware and components

ade2a99

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

Karsten1987 force-pushed the resource_manager_parser branch from 7a0cd2b to 60c000f Compare September 29, 2020 18:20

review comments

88b6c32

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

Karsten1987 force-pushed the resource_manager_parser branch from 60c000f to 88b6c32 Compare September 29, 2020 18:29

bmagyar reviewed Sep 30, 2020

View reviewed changes

bmagyar mentioned this pull request Sep 30, 2020

Hardware resource folder #170

Closed

Karsten1987 mentioned this pull request Oct 1, 2020

Pure Component Interfaces #171

Closed

destogl mentioned this pull request Oct 3, 2020

Add virtual modifier to the functions of Joint and Sensor component #178

Merged

Karsten1987 mentioned this pull request Oct 6, 2020

Realtime safe access to read & write components #174

Closed

destogl reviewed Oct 7, 2020

View reviewed changes

destogl mentioned this pull request Oct 7, 2020

User unique_ptr in ResourceManager #186

Closed

Karsten1987 closed this Oct 22, 2020

Karsten1987 deleted the resource_manager_parser branch November 11, 2020 05:11


		size_t system_interfaces_size() const;

		// loan_joint(const std::string & name);

Conversation

Karsten1987 commented Sep 26, 2020

Uh oh!

Karsten1987 commented Sep 26, 2020

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Karsten1987 Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bmagyar left a comment

Choose a reason for hiding this comment

Uh oh!

bmagyar Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

Karsten1987 Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

bmagyar Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmagyar Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

destogl Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Karsten1987 commented Sep 28, 2020 •

edited

Loading