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

All interfaces should be easy to extend #1

Open
jordan-palacios opened this issue Dec 4, 2018 · 7 comments
Open

All interfaces should be easy to extend #1

jordan-palacios opened this issue Dec 4, 2018 · 7 comments
Assignees

Comments

@jordan-palacios
Copy link

Right now actuator, joint and transmission interfaces are hardcoded for position, velocity and effort.

See:

What we would like is to this not be longer the case and to be able to easily add new types of data. For example effort can be reported both as current and torque. Currently there is now way to differentiate them and, actually, we want both. Same thing happens if we want access more than one position source (incremental encoders, absolute encoders, etc).

We should discuss possible solutions to this. Proposed changes should not in any way affect negatively current ros_control features. Existing controllers will probably need to be adapted too.

@jordan-palacios jordan-palacios self-assigned this Dec 4, 2018
@bmagyar
Copy link
Member

bmagyar commented Jan 7, 2019

I see a couple of ways to make this possible, either by using a template-based mechanism mixed with some string magic or an entirely dictionary-based solution but there is a high chance that wouldn't comply with most realtime requirements.

Do you expect all of the new interfaces to be exported everywhere or we have to think of a policy to limit visibility? I'm thinking about how relevant imus and encoders are when one is browsing the command line interface for control interfaces.

A similar question is whether there should be different levels of interfaces. The current trio of position, velocity and effort are on the same level but we already have some sensor interfaces that are showed into the same category as the purely control ones. Should there be some sort of grouping for these?

@jordan-palacios
Copy link
Author

jordan-palacios commented Jan 7, 2019

I see a couple of ways to make this possible, either by using a template-based mechanism mixed with some string magic or an entirely dictionary-based solution but there is a high chance that wouldn't comply with most realtime requirements.

It really depends on how much of the current code we want to keep imho.

Do you expect all of the new interfaces to be exported everywhere or we have to think of a policy to limit visibility? I'm thinking about how relevant imus and encoders are when one is browsing the command line interface for control interfaces.

I'd rather visibility be limited to the interfaces intended for a said controller. Otherwise the amount of cluttering over time could be significant.

A similar question is whether there should be different levels of interfaces. The current trio of position, velocity and effort are on the same level but we already have some sensor interfaces that are showed into the same category as the purely control ones. Should there be some sort of grouping for these?

Yes. We should address this someway too.

@jordan-palacios
Copy link
Author

Expanding on the previous first point. As an example, current Transmission class implements the basic three interfaces: position, velocity and effort.

class Transmission
{
public:
  virtual ~Transmission() {}
  virtual void actuatorToJointEffort(const ActuatorData& act_data, JointData&    jnt_data) = 0;
  virtual void actuatorToJointVelocity(const ActuatorData& act_data, JointData&    jnt_data) = 0;
  virtual void actuatorToJointPosition(const ActuatorData& act_data, JointData&    jnt_data) = 0;
  virtual void jointToActuatorEffort(const JointData&    jnt_data, ActuatorData& act_data) = 0;
  virtual void jointToActuatorVelocity(const JointData&    jnt_data, ActuatorData& act_data) = 0;
  virtual void jointToActuatorPosition(const JointData&    jnt_data, ActuatorData& act_data) = 0;
  ...
};

One possible approach would then to make Transmission generic base class.

class Transmission
{
public:
  virtual ~Transmission() {}
  virtual void actuatorToJoint(const ActuatorData& act_data, JointData&    jnt_data) = 0;
  virtual void jointToActuator(const JointData&    jnt_data, ActuatorData& act_data) = 0;
  ...
};

And then have different types of transmission as derived classes:

class EffortTransmission   : public Transmission { ... }
class VelocityTransmission : public Transmission { ... }
class PositionTransmission : public Transmission { ... }

Same thing could we used for JointStateInterface and ActuatorStateInterface. This would mean that we would need to start treating interfaces as a set of objects instead of a single object.

For the JointStateController for example, the init(...) function would go from this:

bool init(hardware_interface::JointStateInterface* hw, ros::NodeHandle& root_nh, ros::NodeHandle&  controller_nh);

To something like this:

bool init(std::vector<hardware_interface::JointStateInterface> hw, ros::NodeHandle& root_nh, ros::NodeHandle&  controller_nh);

What do you think?

@bmagyar
Copy link
Member

bmagyar commented Jan 10, 2019

I've been thinking on ways to move this forward but I couldn't come up with a version yet where we retain types at all times.

First I thought of a version where Transmission is templated as such:

class EffortTransmission   : public Transmission<EffortActuatorInterface> { ... }
class VelocityTransmission : public Transmission<VelocityActuatorInterface> { ... }
class PositionTransmission : public Transmission<PositionActuatorInterface> { ... }

where they'd have a common base class, HardwareResourceManager<ActuatorHandle> so theoretically a layer of ResourceManager could be added to handle the string-based lookups the usual way.

I'm not entirely certain this would fully work though as a template approach (with derived types) would require all derived types to be known at that very point while a base-class transmission type approach would only carry this information in the dynamic type and a corresponding string. Do you have an idea of bridging this gap without dropping the type information momentarily?

I quite like the shape of the init function you posted but I would refrain from using a naked vector.

bool init(std::vector<hardware_interface::JointStateInterface> hw, ros::NodeHandle& root_nh, ros::NodeHandle&  controller_nh);

Once we find a suitable abstraction for handling a collection of transmissions, the same could be used to replace the std::vector..

What should the policy be for extending the set of state handles? Ideally we'd maintain the current set of handles with the possibility of adding new, common ones. At the same time I'd like to keep the option open for 3rd parties to add their own state or command handles within their local scopes. This could be achieved either with the current setup or if need be we could add a "second class handles" mechanism where we entirely let go of the type and trust users to fully know what they are doing in their lookups.

@bmagyar
Copy link
Member

bmagyar commented Jan 10, 2019

I think that this discussion is soon reaching a point where we'll need some use-cases to guide it along.
Can you/should I create a stripped down example of the encoder use case? One I imagine using a single encoder and the other using both. The single encoder case is already viable through position feedback, the two encoders version is going to be the subject through which we can discuss/discover other challenges.

@jordan-palacios
Copy link
Author

I think that this discussion is soon reaching a point where we'll need some use-cases to guide it along.

Agreed.

Can you/should I create a stripped down example of the encoder use case? One I imagine using a single encoder and the other using both. The single encoder case is already viable through position feedback, the two encoders version is going to be the subject through which we can discuss/discover other challenges.

Please do, you are more familiar with the code base than me.

@bmagyar
Copy link
Member

bmagyar commented Mar 5, 2019

I gave a few thoughts to the approach over and over again and I think I may have changed my mind.

The problem is specifically about interfaces. The current plugin-based system builds upon the shared set of C++ interfaces between RobotHW, transmissions and controllers.

First question is how to implement a string-based lookup for C++ classes which may dynamically change. There is a possibility of turning joint and actuator interfaces into plugins as well. These plugins would then have a standard interface where string-ification is defined per plugin locally.

This leads to another layer of plugin-ification in ros_control which I honestly would like to avoid. Even without thinking about the realtime-safety aspects of loading those binaries instead of using plain headers...

My proposal is the following: take a step back and define a new set of fixed interfaces to avoid massive parametrization of the entire framework for the good of all users and maintainers alike. Robotics has a finite set of concepts, we only have to provide support for those, allowing for concept-specific parametrization where necessary (eg.: encoder types and more).

What do you think of this approach? The upside is that it'd require less restructuring over the entire framework and would get us out of the current design phase where not much visible things happen, only I have a lot of broken code with prototypes that don't work :)

On easy extendability I think there's a lot of value in allowing users to provide their own HW Interfaces when defining a RobotHW and a controller while still being compatible with the existing controllers. Most of this should already be possible, we may only need to figure out a way to funnel this info back to the controller manager itself where stringification could come into play (all done at the RobotHW side at compile time!).

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

No branches or pull requests

2 participants