PowerService/Power msgs (ADC values)

[Apehaenger]

This PR is related to xtech/fw-openmower-v2#35 and is based on (includes) https://github.com/ClemensElflein/open_mower_ros/tree/feat/input-context-and-high-level-service.

Summary by CodeRabbit

• Refactor
  • Power data message restructured with expanded voltage sensor support
  • Enhanced docking and behavior systems with multi-source sensor reading capability
  • Improved input button handling with context-aware command routing

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/sabo

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/mower_logic/src/mower_logic/mower_logic.cpp (1)

490-524: ⚠️ Potential issue | 🟠 Major

Guard against invalid/zero ADC readings when selecting last_battery_v.

ROS float32 fields default to 0.0, not NaN. When battery_voltage_adc is unpublished, std::isnan(0.0f) returns false, forcing last_battery_v to 0.0. This yields battery_percent = (0 - 23.5) / (28.5 - 23.5) ≈ -4.7, clamped to 0.0, which then triggers false docking since the critical voltage check (0 < 22.5V) becomes true. Additionally, if both ADC and CHG default to 0.0, the clamping logic never corrects the invalid value.

Switch to std::isfinite(v) && v > 1.0f to filter out both zero and uninitialized values, implement a fallback chain to battery_voltage_bms, and handle the "no valid voltage" case explicitly. <cmath> and <limits> are required but likely already available transitively.

🛠️ Suggested guard for invalid voltage readings

-  // Prefer ADC because it's assumed to be more accurate
-  const float last_battery_v =
-      !std::isnan(last_power.battery_voltage_adc) ? last_power.battery_voltage_adc : last_power.battery_voltage_chg;
-
-  double battery_percent = (last_battery_v - last_power_config.battery_empty_voltage) /
-                           (last_power_config.battery_full_voltage - last_power_config.battery_empty_voltage);
+  // Prefer ADC because it's assumed to be more accurate
+  const auto is_valid_v = [](float v) { return std::isfinite(v) && v > 1.0f; };
+  const float last_battery_v =
+      is_valid_v(last_power.battery_voltage_adc) ? last_power.battery_voltage_adc
+    : is_valid_v(last_power.battery_voltage_chg) ? last_power.battery_voltage_chg
+    : is_valid_v(last_power.battery_voltage_bms) ? last_power.battery_voltage_bms
+    : std::numeric_limits<float>::quiet_NaN();
+
+  double battery_percent = 0.0;
+  if (std::isfinite(last_battery_v)) {
+    battery_percent = (last_battery_v - last_power_config.battery_empty_voltage) /
+                      (last_power_config.battery_full_voltage - last_power_config.battery_empty_voltage);
+  } else {
+    ROS_WARN_STREAM_THROTTLE(5, "Battery voltage unavailable; battery_percent set to 0.");
+  }

This pattern also appears in IdleBehavior.cpp and DockingBehavior.cpp.

🤖 Fix all issues with AI agents

In `@src/mower_logic/src/mower_logic/behaviors/IdleBehavior.cpp`:
- Around line 87-92: The code uses std::isnan in IdleBehavior.cpp (variables
last_battery_v and last_charge_v) but does not include <cmath>, causing
build/portability issues; add `#include` <cmath> at the top of IdleBehavior.cpp
and likewise add the same include to the other files that use std::isnan
(mower_logic.cpp and DockingBehavior.cpp) so the calls to std::isnan compile
reliably across toolchains and standards modes.

🧹 Nitpick comments (2)

src/mower_comms_v2/src/HighLevelServiceInterface.cpp (1)

45-49: Follow up on the TODO for action handling.

There’s a TODO for action routing; happy to help implement or file a follow-up issue if needed.

src/mower_comms_v2/src/HighLevelServiceInterface.h (1)

27-41: Keep a NodeHandle member to make subscriber lifetime explicit.

The local ros::NodeHandle nh; is destroyed after the constructor. In roscpp, subscriptions created from a NodeHandle can be dropped if no handle remains, so it’s safer to store one as a member (or accept one from the caller).

♻️ Suggested adjustment

 class HighLevelServiceInterface : public HighLevelServiceInterfaceBase {
  public:
   HighLevelServiceInterface(uint16_t service_id, const xbot::serviceif::Context& ctx)
-      : HighLevelServiceInterfaceBase(service_id, ctx) {
-    ros::NodeHandle nh;
-    high_level_status_sub_ =
-        nh.subscribe("mower_logic/current_state", 1, &HighLevelServiceInterface::highLevelStatusReceived, this);
-  }
+      : HighLevelServiceInterfaceBase(service_id, ctx), nh_() {
+    high_level_status_sub_ =
+        nh_.subscribe("mower_logic/current_state", 1, &HighLevelServiceInterface::highLevelStatusReceived, this);
+  }
@@
  private:
   void highLevelStatusReceived(const mower_msgs::HighLevelStatus::ConstPtr& msg);
+  ros::NodeHandle nh_;
   ros::Subscriber high_level_status_sub_;
 };