Making it possible to add Custom Hardware to the HardwareInterface

ugocupcic commented 11 years ago

Adding this CustomHWMap makes it possible for people using this code to add new types of hardware to the hardware interface without having to fork many of the pr2 repos. By subclassing the CustomHW classes, we're able to fit in any hardware we want in this map which makes it very handy.

ahendrix commented 11 years ago

I feel like you haven't thought this all of the way through.

Why can't you subclass one of the existing hardware classes?
I don't see any code that uses this. Do you have any examples?

To be fair, I'm new to this code base, so I don't have an excellent grasp of what's going on. If I've overlooked something, feel free to enlighten me.

ugocupcic commented 11 years ago

We could subclass one of the existing hardware classes but this would be very hackish. We're developing the driver for a new type of device which can definitely not be classified as any of the other hardware maps that are in the Hardware Interface.

I've got some example of the code but it's unfortunately private for now. Here is some relevant part of the code:

This goes in our own hardware interface package.

namespace ronex
{
  class GeneralIOCommand
    : public pr2_hardware_interface::CustomHWCommand
  {
  public:
    std::vector<bool> digital_;

    struct PWM
    {
      unsigned short int period;
      unsigned short int on_time_0;
      unsigned short int on_time_1;
    };

    std::vector<PWM> pwm_;
    unsigned short int pwm_clock_speed_;
  };

  class GeneralIOState
    : public pr2_hardware_interface::CustomHWState
  {
  public:
    std::vector<bool> digital_;
    std::vector<unsigned short int> analogue_;
  };

  class GeneralIO
    : public pr2_hardware_interface::CustomHW
  {
  public:
    std::string name_;
    GeneralIOState state_;
    GeneralIOCommand command_;
  };
}

And this code goes in the driver: we add our custom hardware to the hardware interface in the init of the driver. We can then use the getCustomHW and unique name to access the hardware we want.

  //in the driver header
  boost::shared_ptr<ronex::GeneralIO> general_io_;

  //add the RoNeX to the hw interface (in the driver init)
  general_io_.reset( new ronex::GeneralIO() );
  general_io_->name_ = device_name_;
  general_io_->command_.pwm_clock_speed_ = static_cast<int16u>(tmp);

  hw->addCustomHW( general_io_.get() );

ahendrix commented 11 years ago

Creating new member variables with the same names as member variables in the parent class is very bad practice.

ugocupcic commented 11 years ago

OK removed the state / command from the CustomHW class. The example is the same as above except that GeneralIOState doesn't inherit from CustomHWState (same for command) obviously.

ahendrix commented 11 years ago

Released into Hydro beta as pr2_mechanism 1.8.2: https://github.com/ros/rosdistro/pull/1218

ugocupcic commented 11 years ago

Excellent. Would it also be possible to merge that into the groovy version? We're depending on it for a new product and are developing for groovy for now.

ahendrix commented 11 years ago

No, this will not be released into Groovy. The released Groovy API needs to remain stable, and I'm pretty sure this breaks it.

ugocupcic commented 11 years ago

Does it really break it? With this change we've just added something to the HardwareInterface without modifying the rest of the interface, so the rest of the code which depends on HardwareInterface will continue running happily.

On 4 July 2013 09:01, Austin Hendrix notifications@github.com wrote:

No, this will not be released into Groovy. The released Groovy API needs to remain stable, and I'm pretty sure this breaks it.

— Reply to this email directly or view it on GitHubhttps://github.com/PR2/pr2_mechanism/pull/315#issuecomment-20461961 .

ahendrix commented 11 years ago

Yes. By adding new member variables to a class, you change the size of that class, and also potentially affect the layout of that class in memory.

Any binaries compiled against the old version of the header will be incompatible with binaries compiled against the new version of the header.

See REP-9 for more details: http://ros.org/reps/rep-0009.html

ugocupcic commented 11 years ago

Ho OK, thanks I didn't know about this REP. We'll probably have to migrate to Hydro sooner than expected then...

Cheers,

Ugo

On 8 July 2013 20:47, Austin Hendrix notifications@github.com wrote:

Yes. By adding new member variables to a class, you change the size of that class, and also potentially affect the layout of that class in memory.

Any binaries compiled against the old version of the header will be incompatible with binaries compiled against the new version of the header.

See REP-9 for more details: http://ros.org/reps/rep-0009.html

— Reply to this email directly or view it on GitHubhttps://github.com/PR2/pr2_mechanism/pull/315#issuecomment-20627170 .

------------------------------------------- Shadow Robot Company Ltd. 251 Liverpool Road, N1 1LX, UK

PR2 / pr2_mechanism

Making it possible to add Custom Hardware to the HardwareInterface #315

Registered Number 3308007 (England & Wales)