Skip to content

Add data_type field to the HardwareInterfaces message #2204

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

Merged
merged 9 commits into from
May 15, 2025

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Apr 26, 2025

This would be interesting to have when we extend the data types in the handles. Now, the CLI will also show the type of the interface, so it is much easier for the users to understand the types from a closed system. Below will be the output with these changes

$ ros2 control list_hardware_interfaces 
command interfaces
	joint1/position [available] [claimed]
	joint1_position_controller/joint1/position [available] [unclaimed]
	joint2/position [available] [claimed]
	joint2_position_controller/joint2/position [available] [unclaimed]
state interfaces
	joint1/position
	joint2/position

$ ros2 control list_hardware_interfaces -v
command interfaces
	joint1/position [double] [available] [claimed]
	joint1_position_controller/joint1/position [double] [available] [unclaimed]
	joint2/position [double] [available] [claimed]
	joint2_position_controller/joint2/position [double] [available] [unclaimed]
state interfaces
	joint1/position [double]
	joint2/position [double]
$ ros2 control list_hardware_components 
Hardware Component 1
	name: RRBot
	type: system
	plugin name: ros2_control_demo_example_12/RRBotSystemPositionOnlyHardware
	state: id=3 label=active
	read/write rate: 10 Hz
	is_async: False
	command interfaces
		joint2/position [available] [claimed]
		joint1/position [available] [claimed]

$ ros2 control list_hardware_components -v
Hardware Component 1
	name: RRBot
	type: system
	plugin name: ros2_control_demo_example_12/RRBotSystemPositionOnlyHardware
	state: id=3 label=active
	read/write rate: 10 Hz
	is_async: False
	command interfaces
		joint2/position [double] [available] [claimed]
		joint1/position [double] [available] [claimed]
	state interfaces
		joint2/position [double] [available]
		joint1/position [double] [available]

@saikishor saikishor changed the title Add data_type field to the HardwareInterfaces message type Add data_type field to the HardwareInterfaces message Apr 26, 2025
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

I like the idea, but should we only add it with verbose mode maybe? (so we need to add this arg to list_hardware_interfaces verb).

Please also update https://github.com/ros-controls/ros2_control/blob/master/ros2controlcli/doc/userdoc.rst (or make sphinx auto-fill this page ;))

@saikishor
Copy link
Member Author

I like the idea, but should we only add it with verbose mode maybe? (so we need to add this arg to list_hardware_interfaces verb).

Right now, it is in the same mode where the information of claimed or unclaimed is printed. It's just extra in same line. Do you think it's better to do it only in verbose?. If you think so, I can make the changes.

Please also update https://github.com/ros-controls/ros2_control/blob/master/ros2controlcli/doc/userdoc.rst (or make sphinx auto-fill this page ;))

Sorry, I forgot🙈

@christophfroehlich
Copy link
Contributor

I like the idea, but should we only add it with verbose mode maybe? (so we need to add this arg to list_hardware_interfaces verb).

Right now, it is in the same mode where the information of claimed or unclaimed is printed. It's just extra in same line. Do you think it's better to do it only in verbose?. If you think so, I can make the changes.

it was just an idea, to avoid overloading users with info. some users never might see anything other from double. on the other hand, so they might get aware of the variants.

@saikishor
Copy link
Member Author

it was just an idea, to avoid overloading users with info. some users never might see anything other from double. on the other hand, so they might get aware of the variants.

You are right. Makes sense. I'll change it for verbose mode only. Thanks for your insights

Copy link

codecov bot commented Apr 26, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (af0d776) to head (cbfb5d3).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
hardware_interface/src/resource_manager.cpp 66.66% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2204      +/-   ##
==========================================
+ Coverage   89.08%   89.12%   +0.03%     
==========================================
  Files         139      139              
  Lines       16118    16140      +22     
  Branches     1389     1391       +2     
==========================================
+ Hits        14359    14385      +26     
+ Misses       1228     1224       -4     
  Partials      531      531              
Flag Coverage Δ
unittests 89.12% <75.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
controller_manager/src/controller_manager.cpp 76.55% <100.00%> (+0.17%) ⬆️
hardware_interface/src/resource_manager.cpp 77.07% <66.66%> (-0.13%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@christophfroehlich christophfroehlich 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 great, thanks!

bmagyar
bmagyar previously approved these changes May 15, 2025
Copy link
Contributor

mergify bot commented May 15, 2025

This pull request is in conflict. Could you fix it @saikishor?

@bmagyar bmagyar dismissed stale reviews from christophfroehlich and themself via 6b9f2ef May 15, 2025 18:39
@bmagyar bmagyar merged commit 0c7cb99 into ros-controls:master May 15, 2025
18 of 26 checks passed
@bmagyar bmagyar deleted the add/data_type/field branch May 15, 2025 19:21
@saikishor saikishor added the backport-jazzy This label should be used by maintainers only! Label triggers PR backport to ROS 2 jazzy. label May 15, 2025
mergify bot pushed a commit that referenced this pull request May 15, 2025
(cherry picked from commit 0c7cb99)

# Conflicts:
#	doc/release_notes.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-jazzy This label should be used by maintainers only! Label triggers PR backport to ROS 2 jazzy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants