Skip to content

Add battery_state_broadcaster - #2086

Merged
christophfroehlich merged 46 commits into
masterfrom
feature/battery_state_broadcaster
Aug 3, 2026
Merged

Add battery_state_broadcaster#2086
christophfroehlich merged 46 commits into
masterfrom
feature/battery_state_broadcaster

Conversation

@christophfroehlich

@christophfroehlich christophfroehlich commented Dec 31, 2025

Copy link
Copy Markdown
Member

This moves https://github.com/ipa320/ros_battery_monitoring/tree/main/battery_state_broadcaster to our repository.

For discsussion about this, see #1888 and especially #1888 (comment)

ottojo and others added 18 commits July 22, 2024 16:37
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
…et_dependencies

Replace ament_target_dependencies with target_link_libraries
Don't make a temporary copy of semantic component
Recent commit in realtime_tools [1] causes the following compilation
failure:

    battery_state_broadcaster/src/BatteryStateBroadcaster.cpp:28:24:
    error: ‘sensor_msgs::msg::BatteryState_<std::allocator<void> >
    realtime_tools::RealtimePublisher<sensor_msgs::msg::BatteryState_<std::allocator<void>
    > >::msg_’ is private within this context

This commit fixes that.

[1]: ros-controls/realtime_tools@9e26e41
Fix compile error with latest realtime_tools
@christophfroehlich christophfroehlich added backport-jazzy Triggers PR backport to ROS 2 jazzy. backport-kilted Triggers PR backport to ROS 2 kilted. labels Dec 31, 2025
@codecov

codecov Bot commented Dec 31, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.57584% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.82%. Comparing base (be6f504) to head (cd12b2b).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
...tate_broadcaster/src/battery_state_broadcaster.cpp 77.21% 29 Missing and 25 partials ⚠️
...roadcaster/test/test_battery_state_broadcaster.hpp 92.59% 1 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2086      +/-   ##
==========================================
- Coverage   86.85%   86.82%   -0.03%     
==========================================
  Files         148      152       +4     
  Lines       16267    16656     +389     
  Branches     1367     1411      +44     
==========================================
+ Hits        14128    14462     +334     
- Misses       1632     1656      +24     
- Partials      507      538      +31     
Flag Coverage Δ
unittests 86.82% <84.57%> (-0.03%) ⬇️

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

Files with missing lines Coverage Δ
...roadcaster/test/test_battery_state_broadcaster.cpp 100.00% <100.00%> (ø)
...aster/test/test_load_battery_state_broadcaster.cpp 100.00% <100.00%> (ø)
...roadcaster/test/test_battery_state_broadcaster.hpp 92.59% <92.59%> (ø)
...tate_broadcaster/src/battery_state_broadcaster.cpp 77.21% <77.21%> (ø)

... and 1 file with indirect coverage changes

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

@MichaelT64

Copy link
Copy Markdown

@christophfroehlich are there any further plans for this?

@christophfroehlich

Copy link
Copy Markdown
Member Author

Please have a look in #1888 and leave your review there.

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Comment thread battery_state_broadcaster/src/battery_state_broadcaster.cpp Outdated
Comment thread battery_state_broadcaster/doc/userdoc.rst Outdated
Comment thread battery_state_broadcaster/doc/userdoc.rst Outdated
Comment thread battery_state_broadcaster/src/battery_state_broadcaster_parameters.yaml Outdated

@saikishor saikishor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some nitpicks

Comment thread battery_state_broadcaster/src/battery_state_broadcaster_parameters.yaml Outdated
Comment on lines +334 to +348
for (size_t i = 0; i < params_.batteries.size(); ++i)
{
const auto & interfaces = params_.interfaces.batteries_map.at(params_.batteries.at(i));

raw_battery_states_msg_.battery_states[i].header.stamp = time;

raw_battery_states_msg_.battery_states[i].voltage = static_cast<float>(
state_interfaces_[interface_cnt].get_optional<double>().value_or(kUninitializedValue));
sums_.voltage_sum += raw_battery_states_msg_.battery_states[i].voltage;
interface_cnt++;

if (interfaces.battery_temperature)
{
raw_battery_states_msg_.battery_states[i].temperature = static_cast<float>(
state_interfaces_[interface_cnt].get_optional<double>().value_or(kUninitializedValue));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the math here, I've a question, what if one of the battery doesn't have the interface, then we are summing NaN and resulting in the bad mean right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But interfaces.battery_temperature only is true if the interface exists?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What I mean is, what if it might be true for some of the batteries but not each of them?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

interfaces is a map with an entry for every battery?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Image

I believe so

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

that's what I mean: If it is not available for certain batteries and configured properly, than it won't accumulate NaN here?

@github-project-automation github-project-automation Bot moved this from Needs review to WIP in Review triage Jul 31, 2026
Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
@christophfroehlich christophfroehlich moved this from WIP to Needs review in Review triage Aug 1, 2026

@saikishor saikishor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left some comments

@christophfroehlich christophfroehlich added the check-prerelease Runs the pre-release workflow without downstream dependencies label Aug 1, 2026
@christophfroehlich
christophfroehlich merged commit 5b2e1ca into master Aug 3, 2026
46 of 49 checks passed
@christophfroehlich
christophfroehlich deleted the feature/battery_state_broadcaster branch August 3, 2026 19:56
@github-project-automation github-project-automation Bot moved this from Needs review to Done in Review triage Aug 3, 2026
christophfroehlich added a commit that referenced this pull request Aug 4, 2026
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Jonas Otto <jonas.otto@ipa.fraunhofer.de>
Co-authored-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Co-authored-by: Jonas Otto <jonas@jonasotto.com>
Co-authored-by: Michal Sojka <michal.sojka@cvut.cz>
Co-authored-by: Yara Shahin <58101871+YaraShahin@users.noreply.github.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
christophfroehlich added a commit that referenced this pull request Aug 4, 2026
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Jonas Otto <jonas.otto@ipa.fraunhofer.de>
Co-authored-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Co-authored-by: Jonas Otto <jonas@jonasotto.com>
Co-authored-by: Michal Sojka <michal.sojka@cvut.cz>
Co-authored-by: Yara Shahin <58101871+YaraShahin@users.noreply.github.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-jazzy Triggers PR backport to ROS 2 jazzy. backport-kilted Triggers PR backport to ROS 2 kilted. check-prerelease Runs the pre-release workflow without downstream dependencies

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

8 participants