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

robotinterface: Allow replacing values from command line #2608

Merged
merged 5 commits into from
Jun 15, 2021

Conversation

drdanz
Copy link
Member

@drdanz drdanz commented Jun 15, 2021

The goal of this patch is to allow us to replace some of the values of the parameters passed to the devices, with arguments passed from the command line. This saves us from having hundreds of identical files, with just one different parameter.
On the other hand we want to avoid the complexity of xacro and similar, and we want to have a valid xml that can be started even if these parameter are not set, and that are compatible with the old yarprobotinterface

As a result, we decided to add a new extern-name attribute to the param tag (suggestions for a better name are welcome), that contains a global name, that is compared and eventually replaced with a value taken from a Searchable that can now be optionally passed to the getRobotFromFile and getRobotFromString methods.


Libraries

robotinterface

XMLReader

  • The getRobotFromFile and getRobotFromString now accept a Searchable that
    is used to replace params with the attribute extern-name.
    The signatures are now:

        XMLReaderResult getRobotFromFile(const std::string& filename,
                                         const yarp::os::Searchable& config = yarp::os::Property());
        XMLReaderResult getRobotFromString(const std::string& filename,
                                           const yarp::os::Searchable& config = yarp::os::Property());
    
  • DTD version is now 3.1.

Tools

yarprobotinterface

  • Arguments taken from the command line are now handled and used to replace
    parameters marked with the attribute extern-name.

@drdanz drdanz requested review from diegoferigo and traversaro June 15, 2021 09:02
@drdanz drdanz self-assigned this Jun 15, 2021
@drdanz drdanz temporarily deployed to code-analysis June 15, 2021 09:05 Inactive
@drdanz drdanz temporarily deployed to code-analysis June 15, 2021 09:05 Inactive
@drdanz drdanz temporarily deployed to code-analysis June 15, 2021 09:05 Inactive
@drdanz drdanz requested a review from randaz81 June 15, 2021 09:22
@sonarcloud
Copy link

sonarcloud bot commented Jun 15, 2021

@traversaro
Copy link
Member

It seems quite cool, thanks! Do you have any example on how to do overload a parameter via the yarprobotinterface command line?

@traversaro
Copy link
Member

In the future (but this is more complex, so for the moment not need to block this PR) it would be cool to be able to overload parameters without manually specifying the extern-name attribute, re-using some already existing identified of the parameter, such as XPath or something equivalent. On the Ignition/Gazebo/SDF side, there is a lot of work going on a similar topics, see gazebosim/sdformat#278 for a reference.

@drdanz
Copy link
Member Author

drdanz commented Jun 15, 2021

There is one test hiddend inside the src/yarprobotinterface folder, you can try it running

yarprobotinterface --config src/yarprobotinterface/tests/fakeFrameGrabber/grabber.xml --mode line

There is also an unit test in tests/libYARP_robotinterface/RobotinterfaceTest.cpp that shows how to do it from the library

@drdanz
Copy link
Member Author

drdanz commented Jun 15, 2021

In the future (but this is more complex, so for the moment not need to block this PR) it would be cool to be able to overload parameters without manually specifying the extern-name attribute, re-using some already existing identified of the parameter, such as XPath or something equivalent. On the Ignition/Gazebo/SDF side, there is a lot of work going on a similar topics, see osrf/sdformat#278 for a reference.

I agree that using XPath or similar would be cool, but this probably means re-write the whole parsing with a different library, since I doubt that tinyxml is able to handle XPath (we already had to fake support for the DTD and for xi:include, since they are not supported).
Anyway the 2 options are not mutually exclusive, this could be added in the future.

I think this is somehow different, because in order to replace 2 parameters with the same value, you don't have to pass 2 different paths, you just need to "export" them externally with the same name (but I don't know much about XPath, perhaps you can have wildcards or similar, that would make them equivalent, assuming that you write the path properly)

@traversaro
Copy link
Member

Anyway the 2 options are not mutually exclusive, this could be added in the future.

I think this is somehow different, because in order to replace 2 parameters with the same value, you don't have to pass 2 different paths, you just need to "export" them externally with the same name (but I don't know much about XPath, perhaps you can have wildcards or similar, that would make them equivalent, assuming that you write the path properly)

Yes, I totally agree!

I agree that using XPath or similar would be cool, but this probably means re-write the whole parsing with a different library, since I doubt that tinyxml is able to handle XPath (we already had to fake support for the DTD and for xi:include, since they are not supported).

In SDF they are using tinyxml2, and they are doing something similar (even if perhaps they are not doing exactly XPath), I will check it more in detail in the future.

@drdanz drdanz added the PR Status: Continuous Integration - OK Continuous Integration for this PR passed (invalid if commits were added or modified after this) label Jun 15, 2021
@drdanz

This comment has been minimized.

drdanz added 5 commits June 15, 2021 13:43
… getRobotFromString

This is used to replace params with the attribute "extern-name".
DTD version is now 3.1.
The signatures are now:

  XMLReaderResult getRobotFromFile(const std::string& filename,
                                   const yarp::os::Searchable& config = yarp::os::Property());
  XMLReaderResult getRobotFromString(const std::string& filename,
                                     const yarp::os::Searchable& config = yarp::os::Property());
@drdanz drdanz force-pushed the robotinterface_param branch from c24a5e6 to a24fa00 Compare June 15, 2021 13:43
@drdanz drdanz merged commit 547e85c into robotology:master Jun 15, 2021
@drdanz drdanz deleted the robotinterface_param branch June 15, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Library - YARP_robotinterface Component: Tool - yarprobotinterface PR Status: Continuous Integration - OK Continuous Integration for this PR passed (invalid if commits were added or modified after this) PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Resolution: Merged Target: YARP v3.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants