More uniform hardware_interface API

[adolfo-rt]

The different interfaces implemented in the hardware_interface package have a very similar look and feel between them, the main differences being mostly aesthetic, eg.

JointStateInterface: getJointNames(), registerJoint(...), getJointStateHandle()
ActuatorCommandInterface: getActuatorNames(), registerActuator(...), getActuatorHandle()
ImuSensorInterface: getSensorNames(), registerSensor(), getSensorHandle()

Implementations are also very similar, so e55fd29 tried to refactor common code to avoid some duplication. Still, I'm not very satisfied with the current situation.

I'd like to propose an API-breaking change ( lots of code will break), that would address the code duplication issue and make unit testing much much easier along the way.

The idea would be to have something like:

// Feel free to propose alternative names
template <class ResouceHandle>
class ResourceManager
{
  std::vector<std::string> getNames() const;
  void registerHandle(const std::string& name, const ResouceHandle& handle);
  ResouceHandle getHandle(const std::string& name) const;
};

typedef ResourceManager<JointStateHandle> JointStateInterface;
typedef ResourceManager<ActuatorCommandHandle> ActuatorCommandInterface;
typedef ResourceManager<ImuSensorHandle> ImuSensorInterface;

The main things that changes apart from a uniform API across hardware interfaces is that to register a resource, you must first create a handle object instead of the current practice of providing the requisites to the register[Joint|Actuator|Sensor|Transmission] methods.

I don't see a loss in code clarity by going from joint_state_iface.registerJoint(name, pos, vel, eff) to joint_state_iface.registerHandle(handle), but YMMV.

Thoughts?.

[adolfo-rt]

I'd like to add that I've been procrastinating writing tests for the hardware_interface package because of this. I wrote one, but never got to writing all others, as the code of each test would end up being an almost exact copy of the other. API differences are enough to disallow writing a single test for them all.

When writing tests is inconvenient or difficult, I take that as a sign ;-)

[yamokosk]

We went down the exact same path here. But then later scrapped the idea of messing with ros_control directly as it would have broken everything and everyone.

But I'd be up for it..

On 06/12/2013 08:48 AM, Adolfo Rodriguez Tsouroukdissian wrote:

The different interfaces implemented in the hardware_interface package have a very similar look and feel between them, the main differences being mostly aesthetic, eg.

JointStateInterface: getJointNames(), registerJoint(...), getJointStateHandle()
ActuatorCommandInterface: getActuatorNames(), registerActuator(...), getActuatorHandle()
ImuSensorInterface: getSensorNames(), registerSensor(), getSensorHandle()

Implementations are also very similar, so e55fd29e55fd29 tried to refactor common code to avoid some duplication. Still, I'm not very satisfied with the current situation.

I'd like to propose an API-breaking change ( lots of code will break), that would address the code duplication issue and make unit testing much much easier along the way.

The idea would be to have something like:

// Feel free to propose alternative names
template
class ResourceManager
{
std::vectorstd::string getNames() const;
void registerHandle(const std::string& name, const ResouceHandle& handle);
ResouceHandle getHandle(const std::string& name) const;
};

typedef ResourceManager JointStateInterface;
typedef ResourceManager ActuatorCommandInterface;
typedef ResourceManager ImuSensorInterface;

The main things that changes apart from a uniform API across hardware interfaces is that to register a resource, you must first create a handle object instead of the current practice of providing the requisites to the register[Joint|Actuator|Sensor|Transmission] methods.

I don't see a loss in code clarity by going from joint_state_iface.registerJoint(name, pos, vel, eff) to joint_state_iface.registerHandle(handle), but YMMV.

Thoughts?.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/45.

[jbohren]

When writing tests is inconvenient or difficult, I take that as a sign ;-)

That's really the first test, right?

I really like the increased consistency of the proposed API, even if it does break things. That was one of the hurdles I encountered when originally figuring out ros_control.

+1

[adolfo-rt]

@yamokosk I saw you tried something similar in the loadable_Transmissions branch. I also wanted to shy away from an API break with such repercussions, but every time I write a new hardware interface I end up repeating myself, hence this issue...

When writing tests is inconvenient or difficult, I take that as a sign ;-)

That's really the first test, right?

@jbohren I don't know if I understand the question. It's the first test I write for the hardware_interface package, yes, not the first in ros_control.

[jbohren]

@adolfo-rt Haha, no I mean that the difficulty of writing tests for an API is the first test of an API, like a meta-test.

[jbohren]

@davetcoleman might be interested

[wmeeusse]

@adolfo-rt It's take me some time, but I think I've come around to the new design for registering handles. I was struggling a bit with the extra step we'll require from users to first create a handle and then register that handle, but there seems to be a common pattern between HardwareInterfaces that themselves contain a number of interfaces to individual joints/actuators/etc. I assume in your example the ResourceManager will inherit from HardwareInterface?

[adolfo-rt]

@adolfo-rt It's take me some time, but I think I've come around to the new design for registering handles. I was struggling a bit with the extra step we'll require from users to first create a handle and then register that handle, but there seems to be a common pattern between HardwareInterfaces that themselves contain a number of interfaces to individual joints/actuators/etc.

I agree that the current design is convenient in that you don't have to go through the intermediate step of explicitly creating a handle, but it's only one extra line of code per resource registration. The main thing that blocks harmonizing hardware interfaces is that the signature of the resource registering method is different for every interface. I think the benefits are worth the extra overhead.

I assume in your example the ResourceManager will inherit from HardwareInterface?

Ah, yes.

If the general feeling towards the proposal is favorable, I'll go ahead and implement it.

[adolfo-rt]

Another comment concerning the convenience (or not) of having to explicitly create a resource handle. Resources like IMUs, which have many data fields --some of which are optional-- are a mess to add to a hardware interface through a registerSensor(list, of, eight, arguments, 0, 0, how, long) or similar method. In such cases, constructing the handle in advance is great, and unused optional fields are dealt with more elegantly than with default parameter values (zeros in the example).

[davetcoleman]

I don't have a enough grasp of the code layout to comment on its necessity, it seems to makes sense. We should get this in soon so that we can stabilize the Hydro release.

This change does add another layer of complexity to the code base - on a side note, do you think you could make some sort of diagram showing the inheritance layers of the various classes? I'll happily contribute to fixing it up/adding to it, and in fact I'm working on diagramming the data flow aspect of ros_control at the moment. Thanks!

[adolfo-rt]

I don't have a enough grasp of the code layout to comment on its necessity, it seems to makes sense. We should get this in soon so that we can stabilize the Hydro release.

I agree that time is a scarce resource, but it's important that the dust settles a bit as well, especially concerning interfaces with the most user-facing visibility (like the joint interfaces).

This change does add another layer of complexity to the code base

I would say verbosity or length rather than complexity. You can even think of it as it "making some lines wider" ;-), for instance, this

JointStateInterface js_iface;
EffortJointInterface e_iface;
// ...
js_iface.registerJoint(name[i], &pos[i], &vel[i], &eff[i]); 
e_iface.registerJoint(js_iface.getJointStateHandle(name[i]), &eff[i]);

becomes

JointStateInterface js_iface;
EffortJointInterface e_iface;
// ...
js_iface.registerHandle(JointStateHandle(name[i], &pos[i], &vel[i], &eff[i])); 
e_iface.registerHandle(JointHandle(js_iface.getJointStateHandle(name[i]), &eff[i]));

which is 85 chars wide. You may want to break up handle creation to separate lines for the sake of clarity, but writing ros_control code should not be any harder/easier than it already is.

• on a side note, do you think you could make some sort of diagram showing the inheritance layers of the various classes? I'll happily contribute to fixing it up/adding to it, and in fact I'm working on diagramming the data flow aspect of ros_control at the moment. Thanks!

Would Doxygen-generated dependency graphs be enough?. The following image is taken from my hardware_interface_rework branch.

[adolfo-rt]

These pull requests address the issue at hand for ros_control and ros_controllers.

User-facing changes

In the pull requests, the public API of the joint, actuator, transmission (and soon sensor) interfaces changes. Consider the EffortJointInterface example:

EffortJointInterface iface;

iface.registerHandle(JointHandle(js_handle, &eff_cmd));
// was iface.registerJoint(js_handle, &eff_cmd);

JointHandle h = iface.getHandle("foo");
// was JointHandle h = iface.getJointHandle("foo");

vector<string> names = iface.getNames();
// was vector<string> names = iface.getJointNames();

Other interfaces change in a similar way. Notice that now they all share a similar API, except for the handle type. If you're a ros_control user and don't plan on creating new interfaces, this is all you need to know.

Internal changes

Resource management

• Methods for registering and getting handles, as well as for querying resource names have been factored out in the ResourceManager class in hardware_interface/internal. This is used not only by the hardware_interface package, but also by transmission_interface.
• Hardware resource management ,ie. the above plus the resource-claiming and conflict-checking API is implemented in the HardwareResourceManager class in hardware_interface/internal.
• The implementations of hardware interfaces are now quite minimal, less than one page (without the license header) and emphasize the structure of the handle type, rather than that of the interface, which remains consistent across interface types.

Raw data validation

• Default constructors of Handle classes now initialize raw data pointers to null. Otherwise they hold random addresses and there is no way to check if the pointed-to data is valid (without a crash).
• Non-default constructors of handle objects now validate raw data pointers to be non-null, and throw exceptions with descriptive messages when null pointers are found.
• Handle class accessors have runtime assertions (debug only) on the raw data.
• See Validate raw data wrapped by hardware_interface #47 for a detailed discussion of this problem.

Unit tests

The hardware_interface package now has a complete unit test suite. This is important for a header-only package, because otherwise you need to rely on external code that depends on it to verify if the code compiles (and works as expected).

Open questions & TODO

• The JointCommandInterface class no longer serves a purpose as before. It's not explicitly used in ros_control or ros_controllers, but I don't know if it's used elsewhere. Should it stay or should it go?.
• Add default constructors to the transmission_interface handles for consistency with hardware_interface?. I think we can figure this one out while implementing the parser from XML specification.

More

Refer to the pull request commit messages.

[wmeeusse]

@adolfo-rt Could you update the wiki here https://github.com/willowgarage/ros_control/wiki/hardware_interface?
Thanks!

[davetcoleman]

@adolfo-rt, in your above example:

e_iface.registerJoint(js_iface.getJointStateHandle(name[i]), &eff[i]);
becomes
e_iface.registerHandle(JointHandle(js_iface.getJointStateHandle(name[i]), &eff[i]));

You missed changing getJointStateHandle to getHandle in the second part.

Maybe this helps others as an example - I just updated the gazebo plugin demo and had to make these changes:

js_interface_.registerJoint(joint_name_[j], &joint_position_[j], &joint_velocity_[j], &joint_effort_[j]);
ej_interface_.registerJoint(js_interface_.getJointStateHandle(joint_name_[j]), &joint_effort_command_[j]);
vj_interface_.registerJoint(js_interface_.getJointStateHandle(joint_name_[j]), &joint_velocity_command_[j]);

becomes

js_interface_.registerHandle(hardware_interface::JointStateHandle(joint_name_[j], &joint_position_[j], &joint_velocity_[j], &joint_effort_[j]));
ej_interface_.registerHandle(hardware_interface::JointHandle(js_interface_.getHandle(joint_name_[j]), &joint_effort_command_[j]));
vj_interface_.registerHandle(hardware_interface::JointHandle(js_interface_.getHandle(joint_name_[j]), &joint_velocity_command_[j]));

[adolfo-rt]

@adolfo-rt Could you update the wiki here https://github.com/willowgarage/ros_control/wiki/hardware_interface?

No problem, I'll take care of it today. I was waiting for the merge in case some additional API changes came out during the final review.

@davetcoleman Yes, I made a mistake two examples ago. Good catch. The last example explaining the pull request should be good. Also, I'll take a look at the ros_control_gazebo packages sometime between now and early next week.

[davetcoleman]

Also, I'll take a look at the ros_control_gazebo packages sometime between now and early next week.

fyi I've just started working on a fairly different version of the ros_control_gazebo packages to make it more generic to any robot.

[adolfo-rt]

fyi I've just started working on a fairly different version of the ros_control_gazebo packages to make it more generic to any robot.

How would you compare the existing implementation with the one you're preparing?. What part of ros_control_gazebo have you found to be limiting generality?. I'm very interested in hearing out the difficulties/limitations others are encountering with ros_control and its surrounding packages (to then make it better, of course).

[davetcoleman]

This one populates its joints within the hardware interface based on the <transmission> tags.