Fix succeeded_power calculation in PV power distribution

This was reported incorrectly earlier using a variable that was never
set.  That variable and related (unused) channels have been removed.

This also updates the PV pool tests to ensure that the distribution
results are accurate.

Signed-off-by: Sahas Subramanian <[email protected]>

Author: shsms

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

@@ -8,7 +8,7 @@
 import logging
 from datetime import timedelta
 
-from frequenz.channels import Broadcast, LatestValueCache, Sender
+from frequenz.channels import LatestValueCache, Sender
 from frequenz.client.microgrid import (
     ApiClientError,
     ComponentCategory,
@@ -67,9 +67,6 @@ def __init__(
         self._component_data_caches: dict[
             ComponentId, LatestValueCache[InverterData]
         ] = {}
-        self._target_power = Power.zero()
-        self._target_power_channel = Broadcast[Request](name="target_power")
-        self._target_power_tx = self._target_power_channel.new_sender()
         self._task: asyncio.Task[None] | None = None
 
     @override
@@ -241,7 +238,7 @@ async def _set_api_power(  # pylint: disable=too-many-locals
                     failed_components=failed_components,
                     succeeded_components=succeeded_components,
                     failed_power=failed_power,
-                    succeeded_power=self._target_power - failed_power,
+                    succeeded_power=request.power - failed_power - remaining_power,
                     excess_power=remaining_power,
                     request=request,
                 )
@@ -250,7 +247,7 @@ async def _set_api_power(  # pylint: disable=too-many-locals
         await self._results_sender.send(
             Success(
                 succeeded_components=succeeded_components,
-                succeeded_power=self._target_power,
+                succeeded_power=request.power - remaining_power,
                 excess_power=remaining_power,
                 request=request,
             )

tests/timeseries/_pv_pool/test_pv_pool_control_methods.py

@@ -151,6 +151,8 @@ async def test_setting_power(  # pylint: disable=too-many-statements
             bounds_rx,
             lambda x: x.bounds is not None and x.bounds.lower.as_watts() == -100000.0,
         )
+        dist_results_rx = pv_pool.power_distribution_results.new_receiver()
+
         self._assert_report(latest_report, power=None, lower=-100000.0, upper=0.0)
         await pv_pool.propose_power(Power.from_watts(-80000.0))
         await self._recv_reports_until(
@@ -172,6 +174,18 @@ async def test_setting_power(  # pylint: disable=too-many-statements
             mocker.call(inv_ids[2], -25000.0),
             mocker.call(inv_ids[3], -25000.0),
         ]
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.from_watts(-80000.0)
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(18),
+            ComponentId(28),
+            ComponentId(38),
+        }
 
         set_power.reset_mock()
         await pv_pool.propose_power(Power.from_watts(-4000.0))
@@ -194,6 +208,18 @@ async def test_setting_power(  # pylint: disable=too-many-statements
             mocker.call(inv_ids[2], -1000.0),
             mocker.call(inv_ids[3], -1000.0),
         ]
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.from_watts(-4000.0)
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(18),
+            ComponentId(28),
+            ComponentId(38),
+        }
 
         # After failing 1 inverter, bounds should go down and power shouldn't be
         # distributed to that inverter.
@@ -205,6 +231,17 @@ async def test_setting_power(  # pylint: disable=too-many-statements
         self._assert_report(
             await bounds_rx.receive(), power=-4000.0, lower=-80000.0, upper=0.0
         )
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.from_watts(-4000.0)
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(28),
+            ComponentId(38),
+        }
 
         set_power.reset_mock()
         await pv_pool.propose_power(Power.from_watts(-70000.0))
@@ -227,6 +264,17 @@ async def test_setting_power(  # pylint: disable=too-many-statements
             mocker.call(inv_ids[2], -30000.0),
             mocker.call(inv_ids[3], -30000.0),
         ]
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.from_watts(-70000.0)
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(28),
+            ComponentId(38),
+        }
 
         # After the failed inverter recovers, bounds should go back up and power
         # should be distributed to all inverters
@@ -238,17 +286,29 @@ async def test_setting_power(  # pylint: disable=too-many-statements
         self._assert_report(
             await bounds_rx.receive(), power=-70000.0, lower=-100000.0, upper=0.0
         )
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.from_watts(-70000.0)
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(18),
+            ComponentId(28),
+            ComponentId(38),
+        }
 
         set_power.reset_mock()
-        await pv_pool.propose_power(Power.from_watts(-90000.0))
+        await pv_pool.propose_power(Power.from_watts(-200000.0))
         await self._recv_reports_until(
             bounds_rx,
             lambda x: x.target_power is not None
-            and x.target_power.as_watts() == -90000.0,
+            and x.target_power.as_watts() == -100000.0,
         )
 
         self._assert_report(
-            await bounds_rx.receive(), power=-90000.0, lower=-100000.0, upper=0.0
+            await bounds_rx.receive(), power=-100000.0, lower=-100000.0, upper=0.0
         )
         await asyncio.sleep(0.0)
 
@@ -258,8 +318,20 @@ async def test_setting_power(  # pylint: disable=too-many-statements
             mocker.call(inv_ids[0], -10000.0),
             mocker.call(inv_ids[1], -20000.0),
             mocker.call(inv_ids[2], -30000.0),
-            mocker.call(inv_ids[3], -30000.0),
+            mocker.call(inv_ids[3], -40000.0),
         ]
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.from_watts(-100000.0)
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(18),
+            ComponentId(28),
+            ComponentId(38),
+        }
 
         # Setting 0 power should set all inverters to 0
         set_power.reset_mock()
@@ -281,6 +353,18 @@ async def test_setting_power(  # pylint: disable=too-many-statements
             mocker.call(inv_ids[2], 0.0),
             mocker.call(inv_ids[3], 0.0),
         ]
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.zero()
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(18),
+            ComponentId(28),
+            ComponentId(38),
+        }
 
         # Resetting the power should lead to default (full) power getting set for all
         # inverters.
@@ -301,3 +385,15 @@ async def test_setting_power(  # pylint: disable=too-many-statements
             mocker.call(inv_ids[2], -30_000.0),
             mocker.call(inv_ids[3], -40_000.0),
         ]
+        dist_results = await dist_results_rx.receive()
+        assert isinstance(
+            dist_results, _power_distributing.Success
+        ), f"Expected a success, got {dist_results}"
+        assert dist_results.succeeded_power == Power.from_watts(-100000.0)
+        assert dist_results.excess_power == Power.zero()
+        assert dist_results.succeeded_components == {
+            ComponentId(8),
+            ComponentId(18),
+            ComponentId(28),
+            ComponentId(38),
+        }