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

Service to get software version of robot #964

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

URJala
Copy link
Collaborator

@URJala URJala commented Apr 11, 2024

This PR implements a service to get the software version of the robot (useful for certain functionalities, for example tool contact can only work on e-series robots). This PR depends on PR for functionality and PR for the tests to work.

Copy link
Collaborator

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

I think we should come up with a better name than "GetVersion" in the message, the code and the interfaces. I proposed get_robot_software_version at some point, but maybe get_polyscope_version might also be good. What do you think @URJala @VinDp @urrsk ?

ur_robot_driver/urdf/ur.ros2_control.xacro Outdated Show resolved Hide resolved
ur_robot_driver/test/test_common.py Outdated Show resolved Hide resolved
Comment on lines 252 to 265
def test_tool_contact(self, tf_prefix):
if self._io_status_controller_interface.get_version().major < 5:
start_result = self._io_status_controller_interface.start_tool_contact()
self.assertEqual(start_result.success, False)

end_result = self._io_status_controller_interface.end_tool_contact()
self.assertEqual(end_result.success, False)
else:
start_result = self._io_status_controller_interface.start_tool_contact()
self.assertEqual(start_result.success, True)

end_result = self._io_status_controller_interface.end_tool_contact()
self.assertEqual(end_result.success, True)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem like it belongs here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a re-implementation of the tool contact test, so it will work on all versions of the robot. The one included in the Tool contact PR will only work on e-series robots, as it cannot see the robot's software version, and tool contact can only be used on e-series robots.
I could move this to it's own pull request if that makes more sense? I agree that it probably doesn't quite fit in here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fmauch I have removed the tool contact parts from both pull requests now. I will make a new PR to add the tests back when this has been merged.

ur_controllers/include/ur_controllers/gpio_controller.hpp Outdated Show resolved Hide resolved
ur_controllers/include/ur_controllers/gpio_controller.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it got in here accitentally

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this got in here accidentally.

Copy link
Collaborator

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

This is definitively going in the right direction!

ur_controllers/src/gpio_controller.cpp Outdated Show resolved Hide resolved
ur_controllers/src/ur_configuration_controller.cpp Outdated Show resolved Hide resolved
ur_controllers/src/ur_configuration_controller.cpp Outdated Show resolved Hide resolved
ur_controllers/src/ur_configuration_controller.cpp Outdated Show resolved Hide resolved
ur_controllers/src/ur_configuration_controller.cpp Outdated Show resolved Hide resolved
ur_controllers/src/ur_configuration_controller.cpp Outdated Show resolved Hide resolved
tf_prefix: {
type: string,
default_value: "",
description: "Urdf prefix of the corresponding arm"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: "Urdf prefix of the corresponding arm"
description: "URDF prefix of the corresponding arm"

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not go into this PR I guess.

ur_robot_driver/test/robot_driver.py Show resolved Hide resolved
Copy link
Collaborator

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

I've changed the RealTimeBox with RealTimeBoxBestEffort. Apart from that, please see my other comments / requests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should not get removed by this PR. Please fix that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should not get removed by this PR. Please fix that.

URJala and others added 11 commits August 23, 2024 10:02
Also moved the version information from being stored in the command interface to be stored in the state interface
Also moved the version information from being stored in the command interface to be stored in the state interface
And moved the get_robot_software_version service in to it.
Also minor cleanup and add testing of the robot software version service
the RealTimeBox used before is not really real-time safe and the way it was
implemented there was unnecessary data allocation in both, the activate method
and the service callback.

Using the RealTimeBoxBestEffort makes allocating additional memory unnecessary
and makes things really thread-safe.
Copy link
Collaborator

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'll merge this together with the message update.

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.

3 participants