Bug fixes in the PowerManager and the BatteryPool (#898)

Author: shsms

RELEASE_NOTES.md

@@ -23,3 +23,7 @@
 ## Bug Fixes
 
 - A bug was fixed where the grid fuse was not created properly and would end up with a `max_current` with type `float` instead of `Current`.
+
+- `BatteryPool.propose_discharge` now converts power values to the passive-sign convention.  Earlier it was not doing this and that was causing it to charge instead of discharge.
+
+- Fix a bug that was causing the power managing actor to crash and restart when cleaning up old proposals.

src/frequenz/sdk/actor/_power_managing/_base_classes.py

@@ -231,6 +231,35 @@ def __lt__(self, other: Proposal) -> bool:
             self.priority == other.priority and self.source_id < other.source_id
         )
 
+    def __eq__(self, other: object) -> bool:
+        """Check if two proposals are equal.
+
+        Equality is determined by the priority and source ID of the proposals, so
+        two proposals are equal if they have the same priority and source ID, even
+        if they have different power values or creation times.
+
+        This is so that there is only one active proposal for each actor in the bucket,
+        and older proposals are replaced by newer ones.
+
+        Args:
+            other: The other proposal to compare to.
+
+        Returns:
+            Whether the two proposals are equal.
+        """
+        if not isinstance(other, Proposal):
+            return NotImplemented
+
+        return self.priority == other.priority and self.source_id == other.source_id
+
+    def __hash__(self) -> int:
+        """Get the hash of the proposal.
+
+        Returns:
+            The hash of the proposal.
+        """
+        return hash((self.priority, self.source_id))
+
 
 class Algorithm(enum.Enum):
     """The available algorithms for the power manager."""

src/frequenz/sdk/actor/_power_managing/_matryoshka.py

@@ -29,7 +29,6 @@
 from ...timeseries import Power
 from . import _bounds
 from ._base_classes import BaseAlgorithm, Proposal, _Report
-from ._sorted_set import SortedSet
 
 if typing.TYPE_CHECKING:
     from ...timeseries._base_types import SystemBounds
@@ -44,12 +43,12 @@ class Matryoshka(BaseAlgorithm):
     def __init__(self, max_proposal_age: timedelta) -> None:
         """Create a new instance of the matryoshka algorithm."""
         self._max_proposal_age_sec = max_proposal_age.total_seconds()
-        self._component_buckets: dict[frozenset[int], SortedSet[Proposal]] = {}
+        self._component_buckets: dict[frozenset[int], set[Proposal]] = {}
         self._target_power: dict[frozenset[int], Power] = {}
 
     def _calc_target_power(
         self,
-        proposals: SortedSet[Proposal],
+        proposals: set[Proposal],
         system_bounds: SystemBounds,
     ) -> Power:
         """Calculate the target power for the given components.
@@ -83,7 +82,7 @@ def _calc_target_power(
             exclusion_bounds = system_bounds.exclusion_bounds
 
         target_power = Power.zero()
-        for next_proposal in reversed(proposals):
+        for next_proposal in sorted(proposals, reverse=True):
             if upper_bound < lower_bound:
                 break
             if next_proposal.preferred_power:
@@ -183,9 +182,10 @@ def calculate_target_power(
             return None
 
         if proposal is not None:
-            self._component_buckets.setdefault(component_ids, SortedSet()).insert(
-                proposal
-            )
+            bucket = self._component_buckets.setdefault(component_ids, set())
+            if proposal in bucket:
+                bucket.remove(proposal)
+            bucket.add(proposal)
 
         # If there has not been any proposal for the given components, don't calculate a
         # target power and just return `None`.
@@ -243,7 +243,9 @@ def get_status(
         ):
             exclusion_bounds = system_bounds.exclusion_bounds
 
-        for next_proposal in reversed(self._component_buckets.get(component_ids, [])):
+        for next_proposal in sorted(
+            self._component_buckets.get(component_ids, []), reverse=True
+        ):
             if next_proposal.priority <= priority:
                 break
             proposal_lower = next_proposal.bounds.lower or lower_bound
@@ -286,4 +288,4 @@ def drop_old_proposals(self, loop_time: float) -> None:
                 if (loop_time - proposal.creation_time) > self._max_proposal_age_sec:
                     to_delete.append(proposal)
             for proposal in to_delete:
-                bucket.delete(proposal)
+                bucket.remove(proposal)

src/frequenz/sdk/timeseries/battery_pool/_battery_pool.py

@@ -215,8 +215,10 @@ async def propose_discharge(
         Raises:
             ValueError: If the given power is negative.
         """
-        if power and power < Power.zero():
-            raise ValueError("Discharge power must be positive.")
+        if power:
+            if power < Power.zero():
+                raise ValueError("Discharge power must be positive.")
+            power = -power
         await self._battery_pool._power_manager_requests_sender.send(
             _power_managing.Proposal(
                 source_id=self._source_id,