Don't repeat zero commands to inverters (#1299)

Closes #1259

Author: shsms

RELEASE_NOTES.md

@@ -14,4 +14,6 @@
 
 ## Bug Fixes
 
+- Doesn't repeat zero commands to battery inverters anymore, to not interfere with lower level logic that might want to do things only when there are no actors trying to use the batteries.
+
 <!-- Here goes notable bug fixes that are worth a special mention or explanation -->

src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py

@@ -183,6 +183,9 @@ def __init__(
         )
         """The distribution algorithm used to distribute power between batteries."""
 
+        self._last_set_powers: dict[ComponentId, Power] = {}
+        """The last power value set to each battery's inverter."""
+
     @override
     def component_ids(self) -> collections.abc.Set[ComponentId]:
         """Return the set of component ids."""
@@ -266,29 +269,13 @@ async def _distribute_power(
             Result from the microgrid API.
         """
         distributed_power_value = request.power - distribution.remaining_power
-        battery_distribution: dict[frozenset[ComponentId], Power] = {}
         battery_ids: set[ComponentId] = set()
-        for inverter_id, dist in distribution.distribution.items():
+        for inverter_id in distribution.distribution:
             for battery_id in self._inv_bats_map[inverter_id]:
                 battery_ids.add(battery_id)
-            battery_distribution[self._inv_bats_map[inverter_id]] = dist
-        if _logger.isEnabledFor(logging.DEBUG):
-            _logger.debug(
-                "Distributing power %s between the batteries: %s",
-                distributed_power_value,
-                ", ".join(
-                    (
-                        str(next(iter(cids)))
-                        if len(cids) == 1
-                        else f"({', '.join(str(cid) for cid in cids)})"
-                    )
-                    + f": {power}"
-                    for cids, power in battery_distribution.items()
-                ),
-            )
 
         failed_power, failed_batteries = await self._set_distributed_power(
-            distribution, self._api_power_request_timeout
+            request, distribution, self._api_power_request_timeout
         )
 
         response: Success | PartialFailure
@@ -632,12 +619,14 @@ def _get_power_distribution(
 
     async def _set_distributed_power(
         self,
+        request: Request,
         distribution: DistributionResult,
         timeout: timedelta,
     ) -> tuple[Power, set[ComponentId]]:
         """Send distributed power to the inverters.
 
         Args:
+            request: Request to set the power for.
             distribution: Distribution result
             timeout: How long wait for the response
 
@@ -652,8 +641,34 @@ async def _set_distributed_power(
                 api.set_power(inverter_id, power.as_watts())
             )
             for inverter_id, power in distribution.distribution.items()
+            if power != Power.zero()
+            or self._last_set_powers.get(inverter_id) != Power.zero()
         }
 
+        if not tasks:
+            if _logger.isEnabledFor(logging.DEBUG):
+                _logger.debug("Not repeating 0W commands to batteries.")
+            return Power.zero(), set()
+
+        if _logger.isEnabledFor(logging.DEBUG):
+            battery_distribution = {
+                self._inv_bats_map[inverter_id]: distribution.distribution[inverter_id]
+                for inverter_id in tasks
+            }
+            _logger.debug(
+                "Distributing power %s between the batteries: %s",
+                request.power - distribution.remaining_power,
+                ", ".join(
+                    (
+                        str(next(iter(cids)))
+                        if len(cids) == 1
+                        else f"({', '.join(str(cid) for cid in cids)})"
+                    )
+                    + f": {power}"
+                    for cids, power in battery_distribution.items()
+                ),
+            )
+
         _, pending = await asyncio.wait(
             tasks.values(),
             timeout=timeout.total_seconds(),
@@ -722,6 +737,8 @@ def _parse_result(
             if failed:
                 failed_power += distribution[inverter_id]
                 failed_batteries.update(battery_ids)
+            else:
+                self._last_set_powers[inverter_id] = distribution[inverter_id]
 
         return failed_power, failed_batteries
 

src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py

@@ -370,8 +370,6 @@ def __init__(self, distributor_exponent: float = 1) -> None:
             ValueError: If distributor_exponent < 0
 
         """
-        super().__init__()
-
         if distributor_exponent < 0:
             raise ValueError("Distribution factor should be float >= 0.")
         self._distributor_exponent: float = distributor_exponent

tests/timeseries/_battery_pool/test_battery_pool_control_methods.py

@@ -551,3 +551,48 @@ async def test_case_4(self, mocks: Mocks, mocker: MockerFixture) -> None:
                 result, _power_distributing.Success
             ),
         )
+
+    async def test_no_resend_0w(self, mocks: Mocks, mocker: MockerFixture) -> None:
+        """Test that 0W command is not resent unnecessarily."""
+        set_power = typing.cast(
+            AsyncMock, microgrid.connection_manager.get().api_client.set_power
+        )
+        await self._patch_battery_pool_status(mocks, mocker)
+        await self._init_data_for_batteries(mocks)
+        await self._init_data_for_inverters(mocks)
+
+        battery_pool = microgrid.new_battery_pool(priority=5)
+        bounds_rx = battery_pool.power_status.new_receiver()
+
+        self._assert_report(
+            await bounds_rx.receive(), power=None, lower=-4000.0, upper=4000.0
+        )
+
+        # First send 0W command.  This should result in API calls.
+        await battery_pool.propose_power(Power.from_watts(0.0))
+
+        self._assert_report(
+            await bounds_rx.receive(), power=0.0, lower=-4000.0, upper=4000.0
+        )
+        await asyncio.sleep(1.0)  # Wait for the power to be distributed.
+        assert set_power.call_count == 4
+        assert sorted(set_power.call_args_list) == [
+            mocker.call(inv_id, 0.0) for inv_id in mocks.microgrid.battery_inverter_ids
+        ]
+
+        set_power.reset_mock()
+
+        # Sending 0W again should not result in any API calls.
+        await battery_pool.propose_power(Power.from_watts(0.0))
+        await asyncio.sleep(1.0)  # Wait for the power to be distributed.
+        assert set_power.call_count == 0
+
+        set_power.reset_mock()
+
+        # Sending a different power should result in API calls.
+        await battery_pool.propose_power(Power.from_watts(100.0))
+        await asyncio.sleep(1.0)
+        assert set_power.call_count == 4
+        assert sorted(set_power.call_args_list) == [
+            mocker.call(inv_id, 25.0) for inv_id in mocks.microgrid.battery_inverter_ids
+        ]