Battery status with channel communication #171

ela-kotulska-frequenz · 2023-01-23T09:38:37Z

No description provided.

ela-kotulska-frequenz · 2023-01-26T13:41:01Z

Unit tests are waiting for: frequenz-floss/frequenz-channels-python#68

shsms

I've checked the first 4 commits later (and one thing with the big commit).

I'll continue tomorrow.

src/frequenz/sdk/actor/power_distributing/_batteries_status.py

src/frequenz/sdk/microgrid/_battery/_status.py

shsms · 2023-01-26T16:00:01Z

src/frequenz/sdk/microgrid/_battery/_status.py

 class _ComponentData(Generic[T]):
    component_id: int
-    receiver: Peekable[T]
+    last_msg_timestamp: datetime = datetime.now(tz=timezone.utc)
+    last_msg_correct: bool = False


I guess you can now rename this class to _ComponentStreamStatus? Also it is not generic anymore.

src/frequenz/sdk/actor/power_distributing/_batteries_status.py

shsms · 2023-01-27T14:03:55Z

src/frequenz/sdk/microgrid/_battery/_status.py

@@ -45,10 +40,25 @@ class BatteryStatus(Enum):
    """Component is working"""


+@dataclass
+class RequestResult:


Does this have to go to microgrid/_battery? I think this is specific to the power distributor.

can we also rename this to SetPowerResult or something? RequestResult sounds too confusing.

I moved whole file to frequez.sdk.actor.power_distributing.
Renamed RequestResult to SetPowerResult.

shsms · 2023-01-27T14:10:46Z

src/frequenz/sdk/microgrid/_battery/_status.py

@@ -113,12 +123,10 @@ def is_blocked(self) -> bool:
        return self.blocked_until > datetime.now(tz=timezone.utc)


-class BatteryStatusTracker(AsyncConstructible):
+class BatteryStatusTracker:


This too, I think should sit outside the microgrid package. The microgrid package is supposed to be a thin wrapper providing access to data from the microgrid and for sending commands to the microgrid.

But this class is really stateful tracking logic, which should ideally be part of a higher level package, possibly the actor/power_distributing. I see that this code was already merged, so I can move out later also, if you prefer.

Moved to actor/power_distributing

shsms · 2023-01-27T14:11:49Z

src/frequenz/sdk/microgrid/_battery/_status.py

-    To create an instance of this class you should use `async_new` class method.
-    Standard constructor (__init__) is not supported and using it will raise
-    `NotSyncConstructible` error.
+    Status in send only when it change.


Suggested change

Status in send only when it change.

Status updates are sent out only when there is a status change.

shsms · 2023-01-27T14:22:37Z

src/frequenz/sdk/actor/power_distributing/_batteries_status.py

+        self.sender = channel.new_sender()
+
+
+class BatteriesStatus:


I guess you're going to rename this to AllBatteryStatus as discussed, and the file accordingly.

I renamed to BatteryPoolStatus once again.
Because it has battery_ids as argument, so it can be created for pool, too.
Also BatteryPoolStatus sounds better after all.

shsms · 2023-01-27T14:26:35Z

src/frequenz/sdk/actor/power_distributing/_batteries_status.py

+
+        self._batteries: Dict[str, BatteryStatusTracker] = {}
+
+        # Receivers are needed to create Select object


Suggested change

# Receivers are needed to create Select object

# Receivers for individual battery statuses are needed to create a `MergeNamed` object

shsms · 2023-01-27T14:31:27Z

src/frequenz/sdk/actor/power_distributing/_batteries_status.py

+            try:
+                await self._update_status(self._battery_status_channel)
+            except Exception as err:  # pylint: disable=broad-except
+                _logger.error("BatteriesStatus failed with error: %s. Restart.", err)


Suggested change

_logger.error("BatteriesStatus failed with error: %s. Restart.", err)

_logger.error("BatteriesStatus failed with error: %s. Restarting.", err)

shsms · 2023-01-27T14:35:56Z

src/frequenz/sdk/actor/power_distributing/_batteries_status.py


-        Raises:
-            KeyError: If any battery in the given batteries is not in the pool.
+    async def _update_status(self, status_channel: MergeNamed[BatteryStatus]) -> None:


maybe more readable to decompose to a Receiver, like below, but it is ok to keep it as it is also.

Suggested change

async def _update_status(self, status_channel: MergeNamed[BatteryStatus]) -> None:

async def _update_status(self, status_channel: Receiver[tuple[str, BatteryStatus]]) -> None:

I would prefer to leave it MergeNamed.
It is clear and shorter.

btw, why not just use self._battery_status_channel and not pass anything at all?

shsms · 2023-01-27T14:51:45Z

src/frequenz/sdk/microgrid/_battery/_status.py

-        """Return status of the battery.
+    async def stop(self) -> None:
+        """Stop tracking battery status."""
+        if self._select is not None:


you should probably also cancel_and_await(self._task) here, before killing the Select.

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

And simplify BatteriesStatus code. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

In the future commit blocking will be only in BatteryStatusTracker. All blocking related public functions will be removed. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Move getting status functionality to separate method. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

BatteryStatusTracker receives messages from components, parse it. If status change BatteryStatusTracker sends new status using channel. BatteriesStatus is now only class that listen for change of status from any batteries and updates set of working batteries. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

... and stop MergeNamed object in BatteriesStatus. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Move from frequenz/sdk/microgrid/_battery/_status.py to frequenz/sdk/actor/power_distributing/_battery_status.py Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Rename from batteries_status.py to battery_pool_status.py Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

... battery_status.py and battery_pool_status.py Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

BatteryStatus will be used outside, by users. Status is just an internal class. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

ela-kotulska-frequenz added part:data-pipeline Affects the data pipeline part:power-management Affects the management of battery power and distribution labels Jan 23, 2023

ela-kotulska-frequenz added this to the v0.17.0 milestone Jan 23, 2023

ela-kotulska-frequenz self-assigned this Jan 23, 2023

ela-kotulska-frequenz requested a review from a team as a code owner January 23, 2023 09:38

ela-kotulska-frequenz requested a review from christianparpart January 23, 2023 09:38

ela-kotulska-frequenz modified the milestones: v0.17.0, v0.18.0 Jan 23, 2023

ela-kotulska-frequenz force-pushed the battery_status_impl branch 4 times, most recently from 6bd3146 to 3adb945 Compare January 26, 2023 13:34

shsms reviewed Jan 26, 2023

View reviewed changes

shsms reviewed Jan 27, 2023

View reviewed changes

ela-kotulska-frequenz added 7 commits January 30, 2023 11:24

Rename BatteryPoolStatus to BatteriesStatus

c76c273

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Rename _batteries_pool_status.py to _batteries_status.py

3788ceb

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Rename StatusTracker to BatteryStatusTracker

3057baf

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Remove circular dependency in BatteriesStatus

34c7f02

And simplify BatteriesStatus code. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Clean BatteryStatusTracker code and improve documentation

291f3b2

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Move blocking feature of BatteryStatusTracker into separate class

61a4f66

In the future commit blocking will be only in BatteryStatusTracker. All blocking related public functions will be removed. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Clean BatteriesStatusTracker code

5b99f97

Move getting status functionality to separate method. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

ela-kotulska-frequenz force-pushed the battery_status_impl branch from 3adb945 to b5a1219 Compare January 30, 2023 10:30

ela-kotulska-frequenz added 2 commits January 30, 2023 13:43

Update frequenz-channel-python to v0.13.0

1821682

... and stop MergeNamed object in BatteriesStatus. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

ela-kotulska-frequenz force-pushed the battery_status_impl branch from b5a1219 to 1821682 Compare January 30, 2023 12:55

github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Jan 30, 2023

ela-kotulska-frequenz added 5 commits January 30, 2023 14:15

Move BatteryStatusTracker file

fbc9f8e

Move from frequenz/sdk/microgrid/_battery/_status.py to frequenz/sdk/actor/power_distributing/_battery_status.py Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Rename BatteriesStatus file

055404b

Rename from batteries_status.py to battery_pool_status.py Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Rename BatteriesStatus to BatteryPoolStatus

fd38ff2

Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Rename some internal classes

50f89d4

... battery_status.py and battery_pool_status.py Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

Switch class names in battery_pool_status and battery_status

6a865d4

BatteryStatus will be used outside, by users. Status is just an internal class. Signed-off-by: ela-kotulska-frequenz <elzbieta.kotulska@frequenz.com>

ela-kotulska-frequenz force-pushed the battery_status_impl branch from 7ce92ff to 6a865d4 Compare January 30, 2023 14:25

sahas-subramanian-frequenz approved these changes Jan 30, 2023

View reviewed changes

ela-kotulska-frequenz merged commit bf36ba6 into frequenz-floss:v0.x.x Jan 31, 2023

shsms mentioned this pull request Feb 21, 2023

Change battery status when component stopped sending data #207

Merged

ela-kotulska-frequenz deleted the battery_status_impl branch March 7, 2023 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Battery status with channel communication #171

Battery status with channel communication #171

ela-kotulska-frequenz commented Jan 23, 2023

ela-kotulska-frequenz commented Jan 26, 2023

shsms left a comment

shsms Jan 26, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 27, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

shsms Jan 30, 2023

shsms Jan 27, 2023

ela-kotulska-frequenz Jan 30, 2023

	Status in send only when it change.
	Status updates are sent out only when there is a status change.


		self._batteries: Dict[str, BatteryStatusTracker] = {}

		# Receivers are needed to create Select object

	# Receivers are needed to create Select object
	# Receivers for individual battery statuses are needed to create a `MergeNamed` object

	_logger.error("BatteriesStatus failed with error: %s. Restart.", err)
	_logger.error("BatteriesStatus failed with error: %s. Restarting.", err)

	async def _update_status(self, status_channel: MergeNamed[BatteryStatus]) -> None:
	async def _update_status(self, status_channel: Receiver[tuple[str, BatteryStatus]]) -> None:

Battery status with channel communication #171

Battery status with channel communication #171

Conversation

ela-kotulska-frequenz commented Jan 23, 2023

ela-kotulska-frequenz commented Jan 26, 2023

shsms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment