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

Add <yarpConfigurationString> parameter #396

Merged
merged 2 commits into from
Nov 13, 2018

Conversation

lrapetti
Copy link
Member

As discussed in #142 I am adding a yarpConfigurationString option that allows to simply define a configuration for a plugin without editing/writing an .ini.

The implementation is based on the Property::fromString(), so it is using the same format of Bottle.
e.g. in order to define the name property of a linkattacher plugin without passing trough a .ini:

    <model name="iCub">
      <include>
          <uri>model://icub_standup</uri>
      </include>
      <plugin name='link_attacher' filename='libgazebo_yarp_linkattacher.so'>
        <yarpConfigurationString>(name /iCub/linkattacher/rpc:i)</yarpConfigurationString>
      </plugin>
    </model>

Observations

  • In case of coexistence of yarpConfigurationFile and yarpConfigurationString, the parameters will be loaded from both the file and the string. If a parameter is present in both, it will be always used the one from yarpConfigurationString.
  • loadConfigSensorPlugin() and loadConfigModelPlugin() are almost identical a part from the fact that the sensor name is read using Name() instead of GetName() when GAZEBO_MAJOR_VERSION >= 7. Probably, in the future it would make sense to merge the two function or to wrap the common part in a single function.
  • At the moment I have decided to use fromString() instead of fromCommand() because it would require a conversion of std::string to argc and argv, but I have not found any compact implementation. If we decide to implement this type conversion, I think it would be better to do it in yarp::os::Property by defining there a method like fromCommand(std::String & command, bool skipFirst, bool wipe).
  • I have tested the yarpConfigurationString with different devices and options and it was working correctly. However, I have observed in the ControlBoard that when I change the gazeboYarpPluginsRobotName using yarpConfigurationString, the property in m_parameters changes accordingly. But if a different gazeboYarpPluginsRobotName was defined somewhere else (e.g. in the .ini), this old name is used:

e.g. if I am using this sdf

    <plugin name='controlboard_right_leg' filename='libgazebo_yarp_controlboard.so'>
      <yarpConfigurationFile>model://icub/conf/gazebo_icub_right_leg.ini</yarpConfigurationFile>
      <yarpConfigurationString>(gazeboYarpPluginsRobotName new_iCub)</yarpConfigurationString>
    </plugin>

I can print here m_parameters.findGroup("gazeboYarpPluginsRobotName").toString() -> new_iCub, but the port prefix that is used in the controlboard is icubSim like if the port is opened before loading the new property (if the gazeboYarpPluginsRobotName is not defined anywhere else, it is correctly using new_iCub as prefix).

@traversaro
Copy link
Member

Thanks @lrapetti . I know that the current status of the documentation in gazebo-yarp-plugins is quite bad, but to avoid it to get even worse I need to ask you to provide documentation for this feature, I am sorry to be that guy. : (

I think it could make sense to expand the "Using the plugins in Gazebo Models" section in https://github.com/robotology/gazebo-yarp-plugins/blob/master/doc/embed_plugins.md#using-the-plugins-in-gazebo-models to contain a reference to the new yarpConfigurationString parameter and the behavior in corner case scenarios, such as:

In case of coexistence of yarpConfigurationFile and yarpConfigurationString, the parameters will be loaded from both the file and the string. If a parameter is present in both, it will be always used the one from yarpConfigurationString.

@lrapetti
Copy link
Member Author

documentation updated 50aeb93

@traversaro
Copy link
Member

I have tested the yarpConfigurationString with different devices and options and it was working correctly. However, I have observed in the ControlBoard that when I change the gazeboYarpPluginsRobotName using yarpConfigurationString, the property in m_parameters changes accordingly.

Agh, this is ugly. To make sure that the variables contained in the yarpConfigurationString are expanded when evaluating the yarpConfigurationFile, we would need to load the yarpConfigurationString before the yarpConfigurationFile, but if the yarpConfigurationFile contains any variable that is expanded, it will overwrite the one specified in the yarpConfigurationString. This is quite problematic for the multi-robot scenario, and at the moment I can't think of a solution for this.

@traversaro
Copy link
Member

We can also merge the PR as it is to test it and then try to solve the gazeboYarpPluginsRobotName problem before the next release.

@traversaro
Copy link
Member

For handling the multi-robot case, we can also change the strategy we agreed on in #93 .

@lrapetti
Copy link
Member Author

For handling the multi-robot case, we can also change the strategy we agreed on in #93 .

Yes, I probably this solution should be re-discussed.
Btw I didn't get what did you mean with #396 (comment). Can we discuss it f2f?

@traversaro
Copy link
Member

Yes, I probably this solution should be re-discussed.

I agree, that is solution is bad nevertheless because you need to manually modify the model to achieve multi-robot integration.
To be honest my original plan was not to have the [include "gazebo_icub_robotname.ini"] in all the configuration files, but rather to just use the gazeboYarpPluginsRobotName defined by Gazebo, but there some problems related to that.

@traversaro
Copy link
Member

Can we discuss it f2f?

Yes, give me 5 minutes.

@traversaro
Copy link
Member

The problem related to multiple robots will be solved in a future PR, as this PR add the required functionality without any regression.

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

Successfully merging this pull request may close these issues.

None yet

2 participants