Skip to content

Add a struct for Interface information, update the test URDF#167

Merged
bmagyar merged 9 commits intoros-controls:masterfrom
AndyZe:andyz/update_urdf_parsing2
Oct 20, 2020
Merged

Add a struct for Interface information, update the test URDF#167
bmagyar merged 9 commits intoros-controls:masterfrom
AndyZe:andyz/update_urdf_parsing2

Conversation

@AndyZe
Copy link
Copy Markdown
Contributor

@AndyZe AndyZe commented Sep 27, 2020

Closes #132

@AndyZe AndyZe force-pushed the andyz/update_urdf_parsing2 branch 2 times, most recently from 01fc30d to 0c89bfc Compare September 27, 2020 04:45
Copy link
Copy Markdown
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Thanks for the effort!

I hope I gave you some good suggestions how to continue. By applying a few suggestions everything will get simpler :)

@AndyZe AndyZe force-pushed the andyz/update_urdf_parsing2 branch 3 times, most recently from 23c82e0 to 7af10f7 Compare October 4, 2020 04:19
@AndyZe AndyZe force-pushed the andyz/update_urdf_parsing2 branch from 7af10f7 to 18ed973 Compare October 5, 2020 13:22
@AndyZe AndyZe changed the title WIP: Add a struct for Interface information, update the test URDF Add a struct for Interface information, update the test URDF Oct 5, 2020
Copy link
Copy Markdown
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few small corrections.

Thanks for language and grammar corrections 👍

Comment on lines +369 to +370
joint_info_.parameters["max_position"] = "3.14";
joint_info_.parameters["min_position"] = "-3.14";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
joint_info_.parameters["max_position"] = "3.14";
joint_info_.parameters["min_position"] = "-3.14";
joint_info_.parameters["max"] = "3.14";
joint_info_.parameters["min"] = "-3.14";

Copy link
Copy Markdown
Contributor Author

@AndyZe AndyZe Oct 7, 2020

Choose a reason for hiding this comment

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

See next comment, below

EXPECT_EQ(hardware_info.joints[0].state_interfaces[0], "position");
EXPECT_EQ(hardware_info.joints[0].state_interfaces[0].name, "position");
ASSERT_THAT(hardware_info.joints[0].parameters, SizeIs(2));
EXPECT_EQ(hardware_info.joints[0].parameters.at("max_position_value"), "${PI}");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should also change those into "min" and "max"

Copy link
Copy Markdown
Contributor Author

@AndyZe AndyZe Oct 7, 2020

Choose a reason for hiding this comment

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

If I understand correctly, the key of joints[0].parameters can be any string. I believe you are thinking of joints[0].command_interfaces[0].min and joints[0].command_interfaces[0].max

For example, this is perfectly valid:

EXPECT_EQ(hardware_info.joints[0].parameters.at("serial_number"), "A12345C63");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A lot of the example URDF has this:

    <joint name="joint1">
      <classType>ros2_control_components/PositionJoint</classType>
      <param name="min_position_value">-1</param>
      <param name="max_position_value">1</param>
    </joint>

Maybe these examples are deprecated and they need to be deleted? Hopefully in a follow-up PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, than leave it as it is.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let us see than later, what we do.

Probably this Example URDFs should go into test_robot_hatdware package.

Copy link
Copy Markdown
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

I am happy now :)

@destogl destogl mentioned this pull request Oct 12, 2020
4 tasks
Copy link
Copy Markdown
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Another round of parser update coming shortly, let's merge this in the mean time. Thanks @AndyZe for this work, we'll sort out the min_position_value situation in a follow-up PR.

@bmagyar bmagyar merged commit 891f524 into ros-controls:master Oct 20, 2020
destogl pushed a commit to b-robotized-forks/ros2_control that referenced this pull request Aug 11, 2022
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.

Update component parsing to cover changes to min-max

3 participants