Send the next command after a failed one, even if it is 0 W

A command that failed or timed out may still have been applied. When the
last successful command was 0 W, a following 0 W command was skipped as
a repeat, so the battery could keep running the failed command's power.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Author: shsms

RELEASE_NOTES.md

@@ -14,3 +14,4 @@ This release updates the microgrid component graph library to v0.6.0, which stop
 ## Bug Fixes
 
 - The battery pool no longer drops the cached data of batteries that stop working. A battery that started working again was left out of the pool's metrics until its next data sample arrived. When every working battery had just started working again, `system_power_bounds` briefly reported no bounds, and if an actor had requested power, the power manager set the target power to zero.
+- After a power request to a battery's inverter fails or times out, the next request that includes that inverter is now always sent, even if it is 0 W. Before, a 0 W request was skipped when the last successful request to that inverter was also 0 W. The failed request may have been applied anyway, so the battery could keep running at that request's power. As before, a battery whose request failed is left out of new requests for a while: 1 second at first, and up to 30 seconds if its requests keep failing. This only happens while another battery in the new request is working. If none is, the failed battery is used again right away.

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

@@ -184,7 +184,12 @@ 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."""
+        """The last power value set to each battery's inverter.
+
+        An inverter is missing if it was never sent a request, or if its last
+        request failed. A failed request may still have been applied, so the
+        next request to that inverter is always sent.
+        """
 
     @override
     def component_ids(self) -> collections.abc.Set[ComponentId]:
@@ -772,6 +777,9 @@ def _parse_result(
             if failed:
                 failed_power += distribution[inverter_id]
                 failed_batteries.update(battery_ids)
+                # The command may still have been applied, so the next one must
+                # be sent even if it is 0 W.
+                self._last_set_powers.pop(inverter_id, None)
             else:
                 self._last_set_powers[inverter_id] = distribution[inverter_id]
 

tests/timeseries/_battery_pool/test_battery_pool_control_methods.py

@@ -618,3 +618,48 @@ async def test_no_resend_0w(self, mocks: Mocks, mocker: MockerFixture) -> None:
         assert sorted(set_power.call_args_list) == [
             mocker.call(inv_id, 25.0) for inv_id in mocks.microgrid.battery_inverter_ids
         ]
+
+    async def test_resend_0w_after_failure(
+        self, mocks: Mocks, mocker: MockerFixture
+    ) -> None:
+        """Test that 0W is sent again to an inverter whose last command failed.
+
+        A command that timed out may still have been applied. The battery status
+        is mocked, so the failed battery is not blocked before the next request.
+        """
+        set_power = cast(
+            AsyncMock,
+            microgrid.connection_manager.get().api_client.set_component_power_active,
+        )
+        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
+        )
+
+        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 == 4
+
+        # The command to the first inverter times out.
+        failing_inverter = mocks.microgrid.battery_inverter_ids[0]
+
+        async def side_effect(inv_id: int, _: float) -> None:
+            if inv_id == failing_inverter:
+                await asyncio.sleep(1000.0)
+
+        set_power.side_effect = side_effect
+        await battery_pool.propose_power(Power.from_watts(100.0))
+        await asyncio.sleep(20.0)  # Wait for the requests to time out.
+
+        set_power.side_effect = None
+        set_power.reset_mock()
+        await battery_pool.propose_power(Power.from_watts(0.0))
+        await asyncio.sleep(1.0)
+        assert sorted(set_power.call_args_list) == [
+            mocker.call(inv_id, 0.0) for inv_id in mocks.microgrid.battery_inverter_ids
+        ]