-
Notifications
You must be signed in to change notification settings - Fork 75
add BatteryStates msg #250
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: master
Are you sure you want to change the base?
add BatteryStates msg #250
Conversation
a979993
to
4984db1
Compare
For reference, there is an ongoing discussion in extending the upstream message |
Thanks for the pointer! I had a look at ros2/common_interfaces#299, from what I see, that discussion is about extending the fields inside sensor_msgs/BatteryState. My PR here is introducing BatteryStates, which is just a container for multiple BatteryState messages. As long as the upstream keeps the existing fields (and only adds new ones), this stays fully compatible. In my use case (Battery State Broadcaster), I can later extend the broadcaster to also publish any new fields that get added upstream, but that doesn’t affect the current functionality of this message. |
As part of the work @cmccrave-CPR is working on we also need a topic with a list of battery states. So this work will be useful to us as well |
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.
Shouldn't this be in the sensor_msgs itself?.
I understand that this is made for the broadcaster, but we can always create multiple instances of the broadcaster to publish info on multiple topics in the meantime
Thanks @saikishor for your comment. I see what you mean, but running multiple broadcaster instances wouldn't quite work for this use case. The broadcaster not only publishes multiple battery states, but also performs aggregation across all batteries. Specifically, it publishes:
With separate broadcaster instances, you'd only get the individual battery states and lose the synchronized, aggregated system-level view — which is one of the core features. I kept |
This PR introduces a new message definition:
BatteryStates
.BatteryStates
contains an array ofsensor_msgs/msg/BatteryState
messages.battery_state_broadcaster
inros2_controllers
.Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:
To send us a pull request, please:
colcon test
andpre-commit run
(requires you to install pre-commit bypip3 install pre-commit
)