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

remove logical components, move hardware resources #201

Merged
merged 2 commits into from Oct 26, 2020

Conversation

Karsten1987
Copy link
Contributor

Addresses the first block of #164

The logical components such as Joint and Sensors are removed. There are only physical components now, namely Actuator, Sensor and System. These three are moved under the components namespace accordingly.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Copy link
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.

Only two small comments, feel free to convert into follow-up issues if you prefer

@destogl
Copy link
Member

destogl commented Oct 23, 2020

Please wait for my review this evening... thanks!

@bmagyar bmagyar requested a review from destogl October 23, 2020 16:12
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.

I have a few questions before merging:

  1. I see there are read/write functions missing? How are you planning to enable those?
  2. Since the hardware is parsing its interfaces, they should be somehow returned to the ResourceManager. I do not see those functions. Or are you planning to do something different?
  3. We agreed, we want to enable some kind of logical components, how we should call them then? Just "Joint"/"Sensor"?

*/
HARDWARE_INTERFACE_PUBLIC
virtual
return_type read_joint(std::shared_ptr<components::Joint> joint) const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

How the read is done now? Shouldn't we read to the "handles" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #203

Copy link
Member

Choose a reason for hiding this comment

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

OK

HARDWARE_INTERFACE_PUBLIC
std::vector<std::string> get_state_interface_names() const;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have those now in hardware? Or how are we doing interface-matching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #207

Copy link
Member

Choose a reason for hiding this comment

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

OK

@Karsten1987
Copy link
Contributor Author

I see there are read/write functions missing? How are you planning to enable those?

#164 depicts the roadmap I am planning to implement. The read functions are introduced in the second PR here: #203

Since the hardware is parsing its interfaces, they should be somehow returned to the ResourceManager. I do not see those functions. Or are you planning to do something different?

The resource loaning is going to be implemented similar to #187 in which handles loaned to the controllers are unregistering themselves upon scope exit.

We agreed, we want to enable some kind of logical components, how we should call them then? Just "Joint"/"Sensor"?

I think we agreed on the opposite to remove logical components (as the title of this PR says) and consider only hardware resources as components. The access to these handles is then done by re-using the existing "handles" we currently have.

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.

I think we agreed on the opposite to remove logical components (as the title of this PR says) and consider only hardware resources as components. The access to these handles is then done by re-using the existing "handles" we currently have.

I think we are talking about the same thing, but currently I don't see anymore how we will implement those, or even better to say, how we should call them.

Just for the record:

  • I am not 100% happy/convinced with renaming and removing "hardware" from the name, even though it is hardware representation on the end.
  • I am afraid, we are getting to lock us in again as in ROS1

These are just my "feelings" and I cannot give you any sensible reason not to do this change, so I am approving this.

Thanks for the work and the explanations!

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

Successfully merging this pull request may close these issues.

None yet

3 participants