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

Modify libYARP_robotinterface and yarprobotinterface to support passing using ${portprefix} in parameters #2819

Merged
merged 3 commits into from
May 18, 2022

Conversation

traversaro
Copy link
Member

@traversaro traversaro commented Mar 23, 2022

Fix half of #2492 by actually making use of the portprefix attribute.

For example, this can be used as in the following example XML example:

<?xml version="1.0" encoding="UTF-8" ?>
<!DOCTYPE robot PUBLIC "-//YARP//DTD yarprobotinterface 3.0//EN" "http://www.yarp.it/DTD/yarprobotinterfaceV3.0.dtd">

<robot name="robotinterfaceExample" portprefix="/icub" build="0" xmlns:xi="http://www.w3.org/2001/XInclude">
    <devices>
        <device name="fake_motor_device" type="fakeMotionControl">
        	<group name="GENERAL">
                <!-- Number of joints of the fake_motor_device -->
		        <param name="Joints">            3         </param>
	        </group>
        </device>
        <device name="fake_motor_nws_yarp" type="controlBoard_nws_yarp">
            <!-- See https://www.yarp.it/latest/classControlBoard__nws__yarp.html for parameter documentation -->
            <param name="name"> ${portprefix}/body </param>
            <param name="period"> 0.01 </param>
            <action phase="startup" level="5" type="attach">
                <!-- This is the same name that we specified with the name attribute of the device element of a previously created device -->
                <param name="device"> fake_motor_device </param>
            </action>
            <action phase="shutdown" level="5" type="detach" />
        </device>
    </devices>
</robot>

if this xml is put in file is called config.xml, then it would be possible to call:

yarprobotinterface --config config.xml

and the YARP ports /icub/body/stateExt:o will be created. If one calls instead:

yarprobotinterface --portprefix /icub02 --config config.xml

the YARP ports /icub02/body/stateExt:o will be created

Help robotology/icub-models-generator#215 by providing a way in which two identical models can be spawned in a simulation while changing only the ${portprefix}.

Note: until now the portprefix was used without initial / (see for example https://github.com/robotology/robots-configuration/blob/f3fef4277717ae87d825a9a46d994ccd21fef3ec/experimentalSetups/torsoSkinLaptop/torsoSkinLaptop.xml#L2). However, it seems to be more in line with the rest of the devices to be explicit and indicate the initial /. In any case, the existing configuration files would need to be modified to use the ${portprefix} attribute, so it should not be a big problem to add the / to the portprefix.

Progress:

  • Implementation ✅
  • Tests ✅
  • Documentation ✅ (I had also to add a first skeleton of docs on yarprobotinterface otherwise I would not know where to document the ${portprefix}option

@traversaro traversaro requested a review from drdanz as a code owner March 23, 2022 09:00
@update-docs
Copy link

update-docs bot commented Mar 23, 2022

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 traversaro changed the title Modify libYARP_robotinterface and yarprobotinterface to support passing using ${portprefix} in parameters [WIP] Modify libYARP_robotinterface and yarprobotinterface to support passing using ${portprefix} in parameters Mar 23, 2022
@traversaro traversaro changed the title [WIP] Modify libYARP_robotinterface and yarprobotinterface to support passing using ${portprefix} in parameters Modify libYARP_robotinterface and yarprobotinterface to support passing using ${portprefix} in parameters Mar 24, 2022
@traversaro traversaro requested a review from randaz81 March 24, 2022 18:32
@traversaro traversaro closed this Mar 24, 2022
@traversaro traversaro reopened this Mar 24, 2022
@traversaro traversaro temporarily deployed to code-analysis March 24, 2022 18:59 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 24, 2022 18:59 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 24, 2022 19:00 Inactive
@traversaro
Copy link
Member Author

traversaro commented Mar 25, 2022

The test failures seems unrelated, so I think that the PR is ready for review.
FYI @Nicogene @lrapetti @ste93 @randaz81

@traversaro traversaro temporarily deployed to code-analysis March 25, 2022 16:15 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 25, 2022 16:15 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 25, 2022 16:15 Inactive
@ste93
Copy link
Contributor

ste93 commented Mar 25, 2022

it seems that it has been killed, I've tried to re-run the first to see if it changes

@@ -0,0 +1,5 @@
robotinterface_portprefix {#yarp_3_6}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be on master @elandini84 @randaz81? (yarp 3.7)

Suggested change
robotinterface_portprefix {#yarp_3_6}
robotinterface_portprefix {#yarp_3_7}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have it on yarp 3.7 (master), but I guess this will delay gazebo-yarp-plugins 5 release (see robotology/gazebo-yarp-plugins#594) until YARP 3.7 is released.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me can be also merged in 3.6, @randaz81 @elandini84 do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the new feature is actually fixing part of a reported issue (#2492) and it doesn't affect the already existing code or xml files, for me it can be merged on yarp-3.6.

@traversaro traversaro temporarily deployed to code-analysis March 31, 2022 07:28 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 31, 2022 07:28 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 31, 2022 07:28 Inactive
@ste93
Copy link
Contributor

ste93 commented Mar 31, 2022

btw for me it is ok to merge

@traversaro traversaro temporarily deployed to code-analysis March 31, 2022 08:56 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 31, 2022 08:56 Inactive
@traversaro traversaro temporarily deployed to code-analysis March 31, 2022 08:56 Inactive
@traversaro traversaro temporarily deployed to code-analysis April 22, 2022 13:06 Inactive
@traversaro traversaro temporarily deployed to code-analysis April 22, 2022 13:06 Inactive
@traversaro traversaro temporarily deployed to code-analysis April 22, 2022 13:06 Inactive
@traversaro traversaro changed the base branch from yarp-3.6 to master April 29, 2022 13:14
@traversaro traversaro temporarily deployed to code-analysis April 29, 2022 13:25 Inactive
@traversaro traversaro temporarily deployed to code-analysis April 29, 2022 13:25 Inactive
@traversaro traversaro temporarily deployed to code-analysis April 29, 2022 13:25 Inactive
@traversaro
Copy link
Member Author

As agreed with @randaz81 face to face, we moved this PR to target master as no further release will be done on the yarp-3.6 so there is no point in getting this change in yarp-3.6.

@traversaro traversaro temporarily deployed to code-analysis April 29, 2022 13:30 Inactive
@traversaro traversaro temporarily deployed to code-analysis April 29, 2022 13:30 Inactive
@traversaro traversaro temporarily deployed to code-analysis April 29, 2022 13:30 Inactive
randaz81
randaz81 previously approved these changes Apr 29, 2022
@randaz81 randaz81 added this to the YARP 3.7.0 milestone Apr 29, 2022
@randaz81
Copy link
Member

/rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants