-
Notifications
You must be signed in to change notification settings - Fork 405
Add Battery State Broadcaster controller #1888
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 Battery State Broadcaster controller #1888
Conversation
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.
Thank you for your contribution.
Unfortunately, there is already an existing package with this name.
https://index.ros.org/p/battery_state_broadcaster/#rolling
Can you please highlight the difference between yours and the existing one? If there is a benefit, we could either rename yours or ask the maintainers/authors of the original one to move it to our repository and merge your additions on top. As an alternative, you could also open a PR there.
<maintainer email="[email protected]">Dr. Denis Štogl</maintainer> | ||
|
||
<license>Apache License 2.0</license> | ||
|
||
<url type="website">https://control.ros.org</url> | ||
<url type="bugtracker">https://github.com/ros-controls/ros2_controllers/issues</url> | ||
<url type="repository">https://github.com/ros-controls/ros2_controllers/</url> |
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.
please use the usual set of maintainers here from other packages
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.
Done
<build_depend>sensor_msgs</build_depend> | ||
<build_depend>control_msgs</build_depend> |
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.
anything you find in "build_depend" and also "exec_depend", it could be shortened to just a single "depend" entry
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.
Done
battery_state_broadcaster/include/battery_state_broadcaster/battery_state_broadcaster.hpp
Outdated
Show resolved
Hide resolved
float get_or_nan(int interface_cnt); | ||
char get_or_unknown(int interface_cnt); |
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.
please make sure that the return type from these align with where and how you use them. Internally we use doubles for a lot of stuff, using float
may cause a narrowing conversion.
for char, I see you used uint8_t
below. If that is what you are looking for, I suggest using that here as return type too.
Also please double check if the windows compilation (probably on the Ci is enough) is fine with the uint8_t
type or maybe you need to include stdint.h
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.
Thank you for pointing this out. I mainly have to do these conversions to match the types of the BatteryState msg which uses float
and int8
.
Regarding your suggestions:
- For
get_or_nan
, I now usevalue_or
with a cast tofloat
, so the internal double stays until assignment to the message field:
static_cast<float>(state_interfaces_[interface_cnt].get_optional<double>().value_or(kUninitializedValue));
- For
get_or_unknown
, I cast touint8_t
instead ofchar
:
static_cast<uint8_t>(state_interfaces_[interface_cnt].get_optional<double>().value_or(
sensor_msgs::msg::BatteryState::POWER_SUPPLY_STATUS_UNKNOWN));
-
Replaced all
char
byuint8_t
-
Since other broadcasters are also using uint8_t, I believe it should be fine. I added the
<cstdint>
anyways for safety.
This should keep the double type used by ros2 control till the end where it has to be assigned to the float/int8 message field.
|
||
std::vector<bool> battery_presence_; | ||
|
||
private: |
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.
I bet some of the above could be moved to the private section ;)
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.
I just moved what I could to private; unfortunately I am using some of them in test so they have to stay protected.
auto opt = state_interfaces_[interface_cnt].get_optional<double>(); | ||
if (opt.has_value()) | ||
{ | ||
return static_cast<float>(*opt); |
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.
return static_cast<float>(*opt); | |
return opt->value(); |
I won't highlight all of them, please fix the rest of the static_casts
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.
Fixed as mentioned in the comment above. Thanks for the tip!
{ | ||
return static_cast<char>(*opt); | ||
} | ||
return 0; |
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.
I'd think a little more carefully about this. What does return 0;
mean in the scope of a char
? I understand how it works in the number-crunching way of computers and bytes but what does it tell us from a semantic perspective? How do you want to handle an error from here vs an actual 0 value?
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.
For all the interfaces in which I was using this function, 0 was the enum value for UNKNOWN. So, that's its semantic value as well in case of an error.
After your recent comment, I changed this to
static_cast<uint8_t>(state_interfaces_[interface_cnt].get_optional<double>().value_or(
sensor_msgs::msg::BatteryState::POWER_SUPPLY_STATUS_UNKNOWN));
which illustrates this concept better.
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.
A few small cleanup steps, otherwise looks good!
Thank you @christophfroehlich for your feedback. I’ve reached out to the maintainer of the original In the meantime, I’ve prepared a summary of the differences and benefits of this implementation, as follows:
|
Hi! Creator of https://github.com/ipa320/ros_battery_monitoring here! As already realized my package is pretty bare bones, and admittedly not in active use by us at the moment (in a previous robot we only had battery state information from the motor controller - now we have a dedicated package receiving CAN messages from the battery and publishing that as BatteryState). I have only briefly skimmed over this PR, a technical question that i still have is: Is the battery state now coupled to joints? IIRC, i had one hardware interface plugin which implemented both the joint interfaces and the battery data interfaces as "sensor" tags beside the "joint"s in the XML. But maybe my setup is the one out of the ordinary here... Anyways, i myself don't have an issue giving the battery_state_broadcaster package name to the ros2_control project, if thats the question, although i do wonder if anybody is using my package and will have their battery monitoring unexpectedly broken... |
Thanks for that offer. According to index.ros.org there is no released package dependent on it at least. I haven't checked if the new proposed broadcaster can be rewritten to support the existing broadcaster config? Maybe @YaraShahin can evaluate that. If this does not make sense, we can break it on rolling but leave the other distros as they are now. |
Thanks a lot @ottojo for your feedback 🙏 To your question: in our implementation the battery state is indeed exposed via state interfaces that are grouped under state_joints. This keeps it consistent with other broadcasters in ros2_control (IMU, force-torque, etc.), but it does mean batteries are defined alongside joints in the URDF. If I understand correctly, your case should still be supported, since you could list your sensor_name under state_joints parameter. The state interface exposed is the same in both cases, for example: I’ll also take a look at the differences between our current implementation and the PR you mentioned in your repo, to see if there’s anything we should carry over here. Thanks again for the clarification and for being open to handing over the package name — we’ll make sure to handle the transition carefully so existing users aren’t left behind. |
Thanks @christophfroehlich for checking the dependency situation. I’ll review whether the configuration style from |
@YaraShahin any updates? |
Thanks for the reminder, I’ll push the fixes this week. |
d5255d9
to
860e8ea
Compare
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
…ttery_state_broadcaster.hpp Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Thanks for the feedback! I’ve fixed the points you mentioned. |
This Pull Request introduces the Battery State Broadcaster, a controller for publishing battery status information in ROS2. The broadcaster reads battery-related state interfaces from hardware and exposes them in standardized ROS messages for easy integration with monitoring tools, logging systems, and higher-level decision-making nodes.
Dependency: This PR depends on control_msgs#250 which introduces the BatteryStates message.
Features
Aggregated and Raw Outputs:
Publishes a combined
sensor_msgs::msg::BatteryState
message representing the overall system status.Publishes per-joint
control_msgs::msg::BatteryStates
messages containing raw values.Flexible Interface Support: Reads from interfaces such as
battery_voltage
,battery_current
,battery_temperature
,battery_charge
,battery_percentage
, and others.Parameterization: Configurable through YAML using generate_parameter_library.
Interfaces
Published Topics
~/battery_state
(sensor_msgs::msg::BatteryState
) — aggregated battery status across all configured joints.~/raw_battery_states
(control_msgs::msg::BatteryStates
) — raw per-joint battery state values.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
)