-
Notifications
You must be signed in to change notification settings - Fork 136
Add BatteryStates msg #302
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
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Yara Shahin <[email protected]>
Signed-off-by: Yara Shahin <[email protected]>
|
Perhaps it was already discussed in the issues/PRs you link, but would publishing multiple |
|
At my work, we originally subscribed to a list of individual battery state topics. However what we are moving to now is a list of battery states like this because it makes for a cleaner interface. |
|
Couldn't you publish to the same topic? There is no 1-to-1 requirement for this to work. |
Thank you @gavanderhoorn for the great question! You're right that we could technically publish individual
While this design was motivated by I hope this clarifies the reasoning! Happy to discuss further if you have other thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some comments, i do not have any objections against this suggestion, especially for consistent pattern.
risk processing incomplete sets if one message drops.
i do not think this is not responsibility for the message type, this sounds pretty much for QoS Reliability.
The array provides a defined and stable order, allowing users to look up each battery state by index (aligned with the controller’s state_joints parameter).
i would use serial number or location as identification instead of application specific and static index order, so that subscription can be more flexible?
it also benefits other applications that need to handle multiple batteries as one system.
and there can be also applications that just want to receive the specific battery state message only, right?
in that case, they also need to receive unnecessary messages from the array... i think this is wasting the resources.
after all, i would do,
- publish indivisual battery state to single battery topic.
- set QoS Reliability for battery topic.
- if necessary, configure the content filtering based on location or serial number on the subscriptions.
Description
This PR introduces a new message definition:
BatteryStates.BatteryStatescontains an array ofsensor_msgs/msg/BatteryStatemessages.battery_state_broadcasterinros2_controllers: ros-controls/ros2_controllers#1888.Is this user-facing behavior change?
No
Did you use Generative AI?
No
Additional Information
This PR is being opened following discussion in ros-controls/control_msgs#250. The
BatteryStatesmessage was initially proposed forcontrol_msgs, but @saikishor recommended placing it insensor_msgsinstead, since:sensor_msgs/BatteryState.ros2_controllersmay have similar use cases for representing multiple battery states.