IJointCoupling: an interface for handling coupled joints

[Nicogene]

Warning
Please this PR is not meant to be merged, it is a starting point of discussion, a lot of stuff is still missing, I will arrange a meeting for discussing about it

I created this PR in order to show the people involved/interested a first proposal for creating a sw infrastructure for publishing the quantities related to physical joints when they are coupled.
Here are the quantities we want to publish for those joints:

• Position
• Velocities
• Torque
• Names

The idea is to create an interface called IJointCoupling.h, and a ImplementJointCoupling class that implements the functions that are common between all the coupling handlers (e.g. initialization).
The coupling handlers, e.g. HandMk5CouplingHandler (see icub-tech-iit/ergocub-software#178), will derive from ImplementJointCoupling and DeviceDriver.

In this way the data of coupled joints will be accessible via a IJointCoupling pointer after opening a PolyDriver, and after the usual view() on the interface. These coupling handlers are not actually "devices" but we want to use the DeviceDriver in order to be able to use for example HandMk5CouplingHandler in gazebo-yarp-plugins without having a compile dependency to ergocub-software.

The idea is to move all the coupling handlers hosted here to icub-main, ergocub-software, cer etc.

Once done we can think about creating a YARP device (random name JointCouplingHandler) that has-a Polydriver and has-a IJointCoupling pointer and derives from a list of interfaces in order to be "attachable" from a controlboard_nws_yarp. At this point, we should have the data published also from the physical joints, but I consider it a second step, first it is important to define the interface and the coupling handlers.

Here is the uml of the architecture:

classDiagram
direction RL
IJointCoupling<|--ImplementJointCoupling
ImplementJointCoupling<|--HandMk5Coupling
DeviceDriver<|--HandMk5Coupling
IJointCoupling *--GazeboYarpControlBoardDriver
DeviceDriver*--GazeboYarpControlBoardDriver
IJointCoupling *--ControlBoardCouplingHandler
DeviceDriver*--ControlBoardCouplingHandler

Loading

And here is the mathematical formalization done by @traversaro : https://mathb.in/76391

cc @randaz81 @pattacini @maggia80 @traversaro @xEnVrE @elandini84

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in doc/release/<target_branch>, based on your changes.

[traversaro]

fyi @mebbaid

[Nicogene]

In the latest commit I added the documentation of the interface and I changed some names form coupled to physical since in our formalization we speak about physical joints and actuated axis.

I am not super convinced about the names and the documentation of these methods, because it is not clear to me what in gyp are actually doing (maybe @traversaro can help with this)

    yarp::sig::VectorOf<size_t> getPhysicalJoints();
    std::string getPhysicalJointName(size_t joint);
    bool checkPhysicalJointIsCoupled(size_t joint);
    bool setPhysicalJointLimits(size_t joint, const double& min, const double& max);
    bool getPhysicalJointLimits(size_t joint, double& min, double& max);

In any case I kindly ask you if you can review what I have done so far in order to be able to finalize this PR and move with the implementation of coupling devices, thanks!

cc @xEnVrE @randaz81 @traversaro @mebbaid @elandini84 @pattacini

[randaz81]

Hi @Nicogene, the interface seems good to me (I'm going to add a fwe notes about the methods name in my review comments).

About your question about the methods:

    bool setPhysicalJointLimits(size_t joint, const double& min, const double& max);
    bool getPhysicalJointLimits(size_t joint, double& min, double& max);

the idea is that they retrieve the limits of the physical joint, because they are generally different from the actuated ones. For example, the gears of a differential drive may introduce a reduction ratio (e.g. 1:10). However finding this number sometimes is not easy, and they are probably obtained by the configuration file, and not by calculation. Also, I'm not sure about what a set method should do, I think that the method is there just for debug purposes in gazebo, since physical limits should be not changed in realtime by the user...

I think that the uml diagram is missing/not showing the relationship with the following other entities:

• controlboard nwc (should implement the interface and therefore also the nws?)
• what is jointcouplingpubisher? something different from controlboardnws?
• The releationship between HandMk5Coupling and a standard motorcontrol device?
  Could you upgrade the diagram for better comprehension?

Generally speaking, could you add a fake device (e.g. fakeCoupledJoint) that implements your interface, such as:
https://github.com/robotology/yarp/tree/master/src/devices/fakeMotionControl

Then we can have a test that tests the methods of the interface, as shown in: https://github.com/robotology/yarp/blob/master/src/devices/fakeMotionControl/tests/fakeMotionControl_test.cpp

[Nicogene]

• controlboard nwc (should implement the interface and therefore also the nws?)
• what is jointcouplingpubisher? something different from controlboardnws?
• The releationship between HandMk5Coupling and a standard motorcontrol device?
  Could you upgrade the diagram for better comprehension?

jointcouplingpubisher is an unfortunate and misleading name, because that device actually will not publish anything, if you have a better name we can change it now or put a placeholder and think about it later.
This device should just transform the actuated axis quantities in the physical joint domain(and vice versa) given the coupling laws.

During the meeting we had, we discussed the possibility of making this device "attachable" by controlBoard_nws_yarp (or ros, or ros2) and then also by controlBoard_nwc_yarp.
We looked at the code of controlBoard_nws_yarp and controboardremapper to see which interfaces are mandatory and here is the list:

yarp::dev::IPositionControl
yarp::dev::IVelocityControl
yarp::dev::IEncodersTimed

yarp::dev::IEncodersTimed is an interface the new device may want to implement, but since it has to act only in reading, not controlling, we evaluated to make yarp::dev::IPositionControl and yarp::dev::IVelocityControl not mandatory using a dedicated configuration option of controlBoard_nws_yarp (read-only ?).

In any case, these are all things concerning the second phase of the development that can be re-discussed in another meeting when we tackle it, because it is a little bit tricky

Generally speaking, could you add a fake device (e.g. fakeCoupledJoint) that implements your interface, such as:
master/src/devices/fakeMotionControl
Then we can have a test that tests the methods of the interface, as shown in: master/src/devices/fakeMotionControl/tests/fakeMotionControl_test.cpp

Sure!

[traversaro]

I am not super convinced about the names and the documentation of these methods, because it is not clear to me what in gyp are actually doing (maybe @traversaro can help with this)

After discussion on teams, with @Nicogene we think that we should also add the following methods:

    // As part of the interface, we assume that we have m actuated axis, 
    // and n physical joints. We assume that m <= n . 
    // We call "physical joint index" the number from 0 to n-1 that identifies
    // the location of a "physical joint" in a physical joint vector, while we
    // We call "actuated axis index" the number from 0 to m-1 that identifies
    // the location of a "actuated axis" in a actuated axis vector.

    // Map a "physical joint index" to the corresponding name
    std::string getPhysicalJointName(size_t physicalJointIndex);

    // Map a "actuated axis index" to the corresponding name
    std::string getActuatedAxisName(size_t actuatedAxisIndex);

    // Get the minimum and maximum position for a given physical joint 
    bool getPhysicalJointLimits(size_t physicalJointIndex, double& min, double& max);

    // In some case, for a given coupling several "physical joints" ad "actuated axis"
    // may be related in a obvious way, i.e. the position and torque of given physical 
    // joint could be equal to the position and torque of given actuated axis.
    // In that case "physical joints" ad "actuated axis" are typically identified by the
    // same name. The getCoupled***() methods return the indices of the "actuated axis"
    // and "physical joints" that are coupled in a non-obvious way

    // Return the vector of "physical joints indices" (i.e. numbers from 0 to n-1)
    // that are related to actuated axis in a non-obvious way
    yarp::sig::VectorOf<size_t> getCoupledPhysicalJoints();

    // Return the vector of "actuator axis indices" (i.e. numbers from 0 to m-1)
    // that are related to physical joints in a non-obvious way
    yarp::sig::VectorOf<size_t> getCoupledActuatedAxis();

@Nicogene perhaps we could convert this to the methods returning bool for coherence with the rest of the interface?

[traversaro]

jointcouplingpubisher is an unfortunate and misleading name, because that device actually will not publish anything, if you have a better name we can change it now or put a placeholder and think about it later.

I would call it jointcouplingremapper, it may be misleading but less then publisher.

[traversaro]

yarp::dev::IEncodersTimed is an interface the new device may want to implement, but since it has to act only in reading, not controlling, we evaluated to make yarp::dev::IPositionControl and yarp::dev::IVelocityControl not mandatory using a dedicated configuration option of controlBoard_nws_yarp (read-only ?).

I think @elandini84 was ok with that, however probably we can have a separate issue to discuss about this?

[Nicogene]

@Nicogene perhaps we could convert this to the methods returning bool for coherence with the rest of the interface?

Actually, I am not a fan of the methods returning bool and using the agument list for returning other values, I'd use it as less as possible but it is my personal taste

[traversaro]

@Nicogene perhaps we could convert this to the methods returning bool for coherence with the rest of the interface?

Actually, I am not a fan of the methods returning bool and using the agument list for returning other values, I'd use it as less as possible but it is my personal taste

And how do you return an error without that? Use a specific value of std::string to indicate the error?

[Nicogene]

@Nicogene perhaps we could convert this to the methods returning bool for coherence with the rest of the interface?

Actually, I am not a fan of the methods returning bool and using the agument list for returning other values, I'd use it as less as possible but it is my personal taste

And how do you return an error without that? Use a specific value of std::string to indicate the error?

For the return as string I'd use something as invalid or ``, for the vector an empty vector, but as I was saying this is my personal taste. The best for me would be std::pair<bool,T> but in YARP interfaces usually the return is a bool

[traversaro]

Ok. Just a comment, in place of std::pair<bool, ..> one can use std::optional

[randaz81]

yarp::dev::IEncodersTimed is an interface the new device may want to implement, but since it has to act only in reading, not controlling, we evaluated to make yarp::dev::IPositionControl and yarp::dev::IVelocityControl not mandatory using a dedicated configuration option of controlBoard_nws_yarp (read-only ?).

Yes, we can do that. (even without a specific configuration option, I think they should be not mandatory by default).

[randaz81]

@Nicogene perhaps we could convert this to the methods returning bool for coherence with the rest of the interface?

Actually, I am not a fan of the methods returning bool and using the agument list for returning other values, I'd use it as less as possible but it is my personal taste

And how do you return an error without that? Use a specific value of std::string to indicate the error?

For the return as string I'd use something as invalid or ``, for the vector an empty vector, but as I was saying this is my personal taste. The best for me would be std::pair<bool,T> but in YARP interfaces usually the return is a bool

What about staying on a standard bool return value for now, keeping the consistency with other Yarp interfaces, and aiming for a general change in a second moment? For example, a back-compatible option for all interfaces could be to define a custom Yarp class as a return value, that can be implicitly convertible to Bool.

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

44.6% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint