From 00bfdb7b1a089af7eab88b4eff194cac831ca198 Mon Sep 17 00:00:00 2001
From: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>
Date: Tue, 14 Mar 2023 10:36:57 +0100
Subject: [PATCH] Check battery capacity in BatteryStatus

Battery with missing capacity will be considered as not working.
It was discussed that capacity is very important metric
and we can't guess its value.

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>
---
 RELEASE_NOTES.md                              |  2 +-
 .../power_distributing/_battery_status.py     | 22 +++++++++++++++++++
 tests/actor/test_battery_status.py            | 13 ++++++++++-
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index afb10a8ef..89bf41679 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -6,7 +6,7 @@
 
 ## Upgrading
 
-<!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with --> 
+* Update BatteryStatus to mark battery with unknown capacity as not working (#263)
 
 ## New Features
 
diff --git a/src/frequenz/sdk/actor/power_distributing/_battery_status.py b/src/frequenz/sdk/actor/power_distributing/_battery_status.py
index f3f885986..4fcd7752b 100644
--- a/src/frequenz/sdk/actor/power_distributing/_battery_status.py
+++ b/src/frequenz/sdk/actor/power_distributing/_battery_status.py
@@ -6,6 +6,7 @@
 
 import asyncio
 import logging
+import math
 from dataclasses import dataclass
 from datetime import datetime, timedelta, timezone
 from enum import Enum
@@ -269,6 +270,7 @@ def _update_status(self, select: Select) -> Optional[Status]:
                 self._is_message_reliable(msg.inner)
                 and self._is_battery_state_correct(msg.inner)
                 and self._no_critical_error(msg.inner)
+                and self._is_capacity_present(msg.inner)
             )
             self._battery.last_msg_timestamp = msg.inner.timestamp
             self._battery.data_recv_timer.reset()
@@ -352,6 +354,26 @@ def _get_current_status(self) -> Status:
 
         return Status.WORKING
 
+    def _is_capacity_present(self, msg: BatteryData) -> bool:
+        """Check whether the battery capacity is NaN or not.
+
+        If battery capacity is missing, then we can't work with it.
+
+        Args:
+            msg: battery message
+
+        Returns:
+            True if battery capacity is present, false otherwise.
+        """
+        if math.isnan(msg.capacity):
+            if self._last_status == Status.WORKING:
+                _logger.warning(
+                    "Battery %d capacity is NaN",
+                    msg.component_id,
+                )
+            return False
+        return True
+
     def _no_critical_error(self, msg: Union[BatteryData, InverterData]) -> bool:
         """Check if battery or inverter message has any critical error.
 
diff --git a/tests/actor/test_battery_status.py b/tests/actor/test_battery_status.py
index 12f4c02da..056fe2ad3 100644
--- a/tests/actor/test_battery_status.py
+++ b/tests/actor/test_battery_status.py
@@ -37,12 +37,13 @@
 from ..utils.mock_microgrid import MockMicrogridClient
 
 
-def battery_data(
+def battery_data(  # pylint: disable=too-many-arguments
     component_id: int,
     timestamp: Optional[datetime] = None,
     relay_state: BatteryRelayState.ValueType = BatteryRelayState.RELAY_STATE_CLOSED,
     component_state: BatteryState.ValueType = BatteryState.COMPONENT_STATE_CHARGING,
     errors: Optional[Iterable[BatteryError]] = None,
+    capacity: float = 0,
 ) -> BatteryData:
     """Create BatteryData with given arguments.
 
@@ -66,6 +67,7 @@ def battery_data(
 
     return BatteryDataWrapper(
         component_id=component_id,
+        capacity=capacity,
         timestamp=datetime.now(tz=timezone.utc) if timestamp is None else timestamp,
         _relay_state=relay_state,
         _component_state=component_state,
@@ -353,6 +355,15 @@ async def test_sync_update_status_with_messages(
         )
         assert tracker._update_status(select) is None  # type: ignore[arg-type]
 
+        # Check if NaN capacity changes the battery status.
+        select = FakeSelect(battery=battery_data(component_id=BATTERY_ID))
+        assert tracker._update_status(select) is Status.WORKING  # type: ignore[arg-type]
+
+        select = FakeSelect(
+            battery=battery_data(component_id=BATTERY_ID, capacity=float("NaN"))
+        )
+        assert tracker._update_status(select) is Status.NOT_WORKING  # type: ignore[arg-type]
+
         await tracker.stop()
 
     async def test_sync_blocking_feature(