Skip to content

Conversation

@YaraShahin
Copy link

Description

This PR introduces a new message definition: BatteryStates.

  • BatteryStates contains an array of sensor_msgs/msg/BatteryState messages.
  • The purpose is to support broadcasting the status of multiple batteries from a single source.
  • This message will be used by the battery_state_broadcaster in ros2_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 BatteryStates message was initially proposed for control_msgs, but @saikishor recommended placing it in sensor_msgs instead, since:

  • It's built on top of sensor_msgs/BatteryState.
  • It represents sensor data rather than control-specific info.
  • Other projects beyond ros2_controllers may have similar use cases for representing multiple battery states.

Yara Shahin added 2 commits October 24, 2025 13:07
Signed-off-by: Yara Shahin <yara.shahin@b-robotized.com>
Signed-off-by: Yara Shahin <yara.shahin@b-robotized.com>
@gavanderhoorn
Copy link

Perhaps it was already discussed in the issues/PRs you link, but would publishing multiple BatteryState messages "at the same time" not be an option (perhaps even with a shared timestamp)?

@nnarain
Copy link

nnarain commented Oct 24, 2025

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.

@gavanderhoorn
Copy link

gavanderhoorn commented Oct 24, 2025

Couldn't you publish to the same topic?

There is no 1-to-1 requirement for this to work.

@YaraShahin
Copy link
Author

Perhaps it was already discussed in the issues/PRs you link, but would publishing multiple BatteryState messages "at the same time" not be an option (perhaps even with a shared timestamp)?

Thank you @gavanderhoorn for the great question! You're right that we could technically publish individual BatteryState messages in the same update loop. However, there are some important considerations, particularly from the ros2_control perspective:

  • Guaranteed delivery and synchronization: With an array, subscribers receive all battery states in a single callback, guaranteed to be from the same sampling cycle. If we instead published separate messages (even with identical timestamps), subscribers would need extra logic (message filters, buffers) to synchronize them, and risk processing incomplete sets if one message drops.

  • Deterministic ordering and lookup: 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). With individual messages, users would need to parse frame IDs to match the batteries, as the message arrival order is not guaranteed even if they are published sequentially in the same loop.

  • Consistent pattern: Similar to messages like sensor_msgs/JoyFeedbackArray, diagnostic_msgs/DiagnosticArray, and visualization_msgs/MarkerArray, this approach groups related elements into a single message where order and synchronization matter.

While this design was motivated by ros2_control, it also benefits other applications that need to handle multiple batteries as one system. This structure also leaves room for future extensions, such as including common system-level metadata.

I hope this clarifies the reasoning! Happy to discuss further if you have other thoughts.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a 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.


set(msg_files
"msg/BatteryState.msg"
"msg/BatteryStates.msg"
Copy link
Member

Choose a reason for hiding this comment

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

🧇 I'm requesting a review from Michael, but I can already share one bit of feedback: based on existing naming conventions, this should be msg/BatteryStateArray.msg.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@YaraShahin
Copy link
Author

Thanks @fujitatomoya for the feedback! A few thoughts on your points:

i would use serial number or location as identification instead of application specific and static index order, so that subscription can be more flexible?

The array approach doesn't prevent you from using location or serial number; it just provides another (faster) way of access using index.

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.

A subscriber interested in one battery would still need to process at least N individual messages to get one complete set of data, instead of a single message containing N elements. That also adds serialization/deserialization overhead. In that sense, the array approach reduces bandwidth and processing load for the current use case.

I agree that publishing individual battery states could be better when different batteries publish at different rates. But for our setup, where updates are synchronized, the array-based message fits more efficiently.

Signed-off-by: Yara Shahin <yara.shahin@b-robotized.com>
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

As a top level response a message should be something that is self contained and provides additional information. Creating a datatype which is effectively the same as sending FooArray which can effectively be written as Foo[] what is the additional meta information that you're capturing. If you don't add additional context it shouldn't be a message. We created a bunch of these datatypes early on for every datatype and that was a mistake that we've basically deprecated ever since.

A few specifics:

Deterministic ordering and lookup: 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). With individual messages, users would need to parse frame IDs to match the batteries, as the message arrival order is not guaranteed even if they are published sequentially in the same loop.

This statement is not supported by the message definition. If I receive this message there is no information included in it to connect it to any of these other things. This sounds very application specific. If you want to use this as a justification it needs to be expressed in the message.

Each battery state item already has location and serial number too.

The array approach doesn't prevent you from using location or serial number; it just provides another (faster) way of access using index.

This is very fragile and relies on assuming the behavior of the publisher.

I agree that publishing individual battery states could be better when different batteries publish at different rates. But for our setup, where updates are synchronized, the array-based message fits more efficiently.

This is again an argument powered by a specific use case and there's edge cases all over. Another case is if the number of batteries available changes during runtime, the list will potentially change length. Or if a new one comes online.

Adding this message datatype wlll also have knock on effects on every publisher and subscriber of BatteryState as they will all now potentially have to subscribe to either a BatteryState or a BatteryStateArray. And you'll have to manage two side by side topics.

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

I don't think that this message is mature enough for us to bring it into common_interfaces. It could be expanded to add more metadata, but it doesn't sound like that's vaulable. And if there is a standard solution which can resolve the issue as suggested by @fujitatomoya we shouldn't add a new message which will cause redundancy and migration requirements. There is a cost to setting up a message filter synchronizer, but I do not expect BatteryState publishing to be high enough bandwidth to worry about the overhead of this. It's got a low characteristic frequency and if you're optimizing further it's likely you don't want to use the standard messages.

I might suggest that a small helper library which can setup the message filter based synchronizers in one line for you might be a better investment.

@destogl
Copy link
Contributor

destogl commented Nov 13, 2025

@fujitatomoya and @tfoote I have to mix myself here to add more reasoning behind this.

I think what @YaraShahin was trying to explain with index is a bit misleading, but this is my fault, as I didn't express myself clearly when discussing the approach.

The main idea of the contribution is to support the standard broadcaster for robots within ros2_control. We have realized in the recent project that there are more and more robots that have multiple batteries (yes, also industrial ones—just today we installed one). We want to provide the information about hardware to the higher-level application at once. There are two topics in the broadcaster, one that calculates average and overall information across batteries using the BatteryState message. Still, often times we find it useful that the user can get all information about batteries, for example, if one is removed or which one is charging or being used at the current moment.

There are, of course, multiple approaches to tackle this. We have experienced that this one is the easiest on the “client”-side of the high-level application, as we always get all data and can process them immediately. This enables writing reusable components, as otherwise we would need to wait for multiple messages, not knowing how many to gather all the information.

Generally, not having the possibility to publish messages at once, we are repeating the same code snippets over and over again across applications, and this makes it quite annoying. Making a synchronizer is a nice idea, but again, one more component in sea of components in complex application doesn't make it simpler. And further is the question, how to synchronize them the best—probably. Based on time, which brings its issues.

The first approach was to add this to control_msgs, but of course this package is a much better place.

@tfoote I understand that your decision might be final and respect that. Nevertheless, as this becomes more and more relevant don't take it personally if we come again 😉

@tfoote
Copy link
Contributor

tfoote commented Nov 14, 2025

I would say that I'm a strong no for add it's proposed in the PR right now. But understanding the use case more, I don't think that it's impossible to revise this and iterate.

What I'm hearing from your description sounds more like wanting to have more details about internal cells that may effectively have full data to report. Where you want to both report a whole battery aggregated state, but then potentially want to give components of the battery further information.

This could potentially be a separate message with the aggregate. Or maybe we would want to create a subcell message and add an array of them to the main message which would allow you to embed this information if you have it, but without changing the top level aggregate data that will allow all the existing implementations to keep working and avoid the fragmentation challenges.

This is why it's important to capture the use cases in the discussions to help understand the proposals.

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.

9 participants