Add sensors listing and data streaming #144

llucax · 2025-05-14T11:27:56Z

This pull request introduces support for sensors to the microgrid client and includes several bug fixes.

A key focus of this update is forward-compatibility with the upcoming microgrid API v0.17. The new sensor-related RPCs (list_sensors(), stream_sensor_data()) and their associated data structures (AggregatedMetricValue, Lifetime) have been designed based on the v0.17 specifications. This approach aims to simplify the future migration process for users.

The list_sensors() functionality introduces a new sensor class hierarchy, offering a more Pythonic and organized way to interact with different sensor types.

The main changes are:

Sensor Support:
- Addition of list_sensors() RPC to retrieve available sensors.
- Addition of stream_sensor_data() RPC to stream data from sensors.
- Introduction of data structures like SensorId, Lifetime, and AggregatedMetricValue to support current and future sensor functionalities.
Bug Fixes:
- Resolved an issue where microgrid metadata would incorrectly default to a (0,0) location if the actual location was missing.
- Ensured that broadcasters are properly stopped and cleaned up during client disconnection or exit, preventing potential resource leaks.
Testing:
- Added tests for microgrid metadata retrieval.

Fixes #136.

We also use a fixture to always clean the client used for test appropriately. Signed-off-by: Leandro Lucarella <[email protected]>

Signed-off-by: Leandro Lucarella <[email protected]>

Copilot

Pull Request Overview

This PR enhances the microgrid client with support for sensor interactions by introducing new RPCs and a sensor class hierarchy while also addressing bugs related to metadata location defaults and resource cleanup.

Added sensor listing via list_sensors() and sensor data streaming via stream_sensor_data().
Introduced dedicated sensor classes and associated data structures (e.g., Pyranometer, SensorId).
Fixed issues with default metadata location and disconnection cleanup.

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/frequenz/client/microgrid/sensor/_pyranometer.py	Added a new sensor class for pyranometer functionality.
src/frequenz/client/microgrid/sensor/_proto.py	Implements sensor construction from protobuf messages.
src/frequenz/client/microgrid/sensor/_problematic.py	Adds classes to handle problematic sensor cases.
src/frequenz/client/microgrid/sensor/_data_proto.py	Provides conversion of sensor data samples from protobuf.
src/frequenz/client/microgrid/_client.py	Adds list_sensors() and stream_sensor_data() with cleanup fixes.
Others	Introduce sensor types, metrics, lifetime, and utility functions to support the new sensor functionality.

src/frequenz/client/microgrid/_util.py

src/frequenz/client/microgrid/_lifetime.py

llucax · 2025-05-14T11:30:00Z

I still need to do some proper testing against a real microgrid API, but I'm pretty confident things shouldn't change much after the tests, so it should be fairly safe to review.

This PR aligns as much as possible with:

Initial upgrade to the API v0.17.x #94

The lifetime will be introduced in the next microgrid API version, but we add it now so we can add a rpc call to get sensors information in a way that is compatible with future microgrid API versions. Signed-off-by: Leandro Lucarella <[email protected]>

Signed-off-by: Leandro Lucarella <[email protected]>

This RPC is based on version 0.17 of the microgrid API instead of the current 0.15 version, so users can already use the new interface and make migrating to 0.17 more straight forward. Instead of having only one sensor class as done in the current client, we introduce a Sensor hierarchy, with a specific type for each sensor type. This allows grouping sensors more easily (like problematic sensors), and also provides a more Pythonic interface. The same approach will be done for component in future versions of this client. To be compatible with 0.17, sensors also have a "fake" lifetime that is not really retrieved from the microgrid (it is filled with a "eternal" lifetime, so sensors are present always). Signed-off-by: Leandro Lucarella <[email protected]>

This class will not really be used in this microgrid API version, but it is added in preparation of the upcoming v0.17 update, which includes aggregated metric values as possible sample values, so clients can already start writing code that contemplates the possibility of receiving these type of values. Signed-off-by: Leandro Lucarella <[email protected]>

This RPC is based on version 0.17 of the microgrid API instead of the current 0.15 version, so users can already use the new interface and make migrating to 0.17 more straight forward. Because of this the data structures used to encapsulate the data samples is based on the data structures in 0.17. Signed-off-by: Leandro Lucarella <[email protected]>

Signed-off-by: Leandro Lucarella <[email protected]>

shsms

Taking a break in the middle of the big commit.

shsms · 2025-05-14T11:37:16Z

src/frequenz/client/microgrid/_client.py


        location: Location | None = None
-        if microgrid_metadata.location:
+        if microgrid_metadata.HasField("location"):


What happened to all our deployments at https://en.wikipedia.org/wiki/Null_Island? 😱

We are fine, now microgrids in Null Island need to provide a explicit (0, 0) location 😆

shsms · 2025-05-14T11:51:52Z

src/frequenz/client/microgrid/_lifetime.py

+            return False
+        if self.end is not None:
+            return self.end >= timestamp
+        # Both are None, so it is always active


Only self.end is known to be None with the above checks. self.start could be None or earlier than the given timestamp.

shsms · 2025-05-14T11:58:34Z

tests/test_lifetime.py

+            start_type=_Time.NOW,
+            end_type=_Time.NOW,
+            expected_active=True,
+        ),


missed case: start=None, end=PAST? Probably need a _Time.PLUSQUAM

😑 Added.

shsms · 2025-05-14T12:04:25Z

src/frequenz/client/microgrid/sensor/_barometer.py

+class Barometer(Sensor):
+    """A barometer sensor.
+
+    Measures pressure.


Suggested change

Measures pressure.

Measures atmospheric pressure.

Most text here is copied from the API, so maybe you can create an issue in the API specs to suggests improvements there too.

shsms · 2025-05-14T12:18:39Z

src/frequenz/client/microgrid/sensor/_base.py

+
+
+@dataclasses.dataclass(frozen=True, kw_only=True)
+class Sensor:


Come on @llucax, make more commits!

I don't see the point on splitting commits that are only adding files, you can just mark files as viewed and that's it. I guess it is very related to your review workflow. But if it makes reviewing easier for you, I can split them, sure, but only if you ask nicely 😄

shsms · 2025-05-14T12:20:42Z

src/frequenz/client/microgrid/sensor/_base.py

+        It is only provided for using with a newer version of the API where the
+        client doesn't know about a new category yet (i.e. for use with
+        [`UnrecognizedSensor`][frequenz.client.microgrid.sensor.UnrecognizedSensor])
+        and in case some low level code needs to know the category of a sensor.


This is why it can't be an ABC? you're blocking __new__ below anyway.

I did this a long time ago in #94, and to be honest I'm not sure if I tried to make it a ABC or not and if I had issues if I did. I would have to refresh my mind a bit. But in any case, I will remove all the class hierarchy for sensors, as we are removing the sensor type anyways, so we can leave that for #94.

shsms · 2025-05-14T12:23:25Z

src/frequenz/client/microgrid/sensor/_base.py

+    @cached_property
+    def active(self) -> bool:
+        """Whether this sensor is currently active."""
+        return self.active_at(datetime.now(timezone.utc))


Not sure you can make this a cached_property. datetime.now is not a constant value

Yeah, good point, it doesn't make sense for active. 👍 🤦

I decided to remove these active properties methods, if we need to add them is a non-breaking change, and they make no sense right now, as sensors will always be active in this sense.

shsms · 2025-05-14T12:24:14Z

src/frequenz/client/microgrid/_lifetime.py

+    @cached_property
+    def active(self) -> bool:
+        """Whether this lifetime is currently active."""
+        return self.active_at(datetime.now(timezone.utc))


This shouldn't be a cached_property, because then you can do this with less assumptions. This property is not expected to be in any hot path, so this is too much optimization anyway.

shsms · 2025-05-14T12:29:43Z

src/frequenz/client/microgrid/sensor/_category.py

+    UNSPECIFIED = 0
+    """Unspecified sensor category."""
+
+    THERMOMETER = 1


These numbers are copied from proto? If you don't want to import the generated code here and assign from that, maybe you should add a link to the proto as a comment.

Yeah, we also talked about this in #94. I'm not a 100% convinced about this, but since these numbers should NEVER EVER change, and we are writing the wrappers manually, I thought I could just skip the generated wrappers, there no value in using them, and if we use them it means all the protobuf definition will be imported when using this simple Python enum (it will probably be imported anyway indirectly at some point, at least if parsing from protobuf is involved, so this is probably more a philosophical issue and a practical one, I know :D).

But the code also looked really ugly or I had to add exceptions for long lines when using stuff like sensor_pb2.SENSOR_CATEGORY_THERMOMETER (with sensors is also not that bad, but for components there are enum names that are really long, and will get worse when they are named ElectricalComponentCategory.ELECTRICAL_COMPONENT_CATEGORY_INVERTER).

Maybe we can have a brief meeting to discuss this, I was hoping to open the discussion when I move this to api-common, whichever we pick it is a backwards compatible change.

llucax · 2025-05-15T07:43:38Z

Sorry @shsms, of course I was wrong and after doing some tests with the real API I found out the specs for sensors are very old (even in common v0.x.x) and will be changed quite a bit in the future (for example the type will be gone, as we have weather stations that provide many metrics, so they don't fall into one specific type). Also the SensorMetric might be gone (and we might use the general Metric, shared with components), but this is still to be decided, so for now I will keep it.

So this PR will need some big update.

I will split it though, as I accumulated enough unrelated stuff to be worth of a separate PR.

llucax · 2025-05-15T08:10:34Z

Split out the bugfixes to:

Fix cleanup and missing location metadata #145

I will probably open a new PR for the sensor stuff, so I leave this PR/branch as is, as the sensors specs seem to be changing, I want to have this PR under my sleeve in case case we decide to go back to sensor types somehow.

llucax · 2025-05-15T15:57:43Z

Superseded by:

Add sensors listing and data streaming #146

llucax added 4 commits May 14, 2025 11:32

Stop and cleanup broadcasters on disconnect/aexit

e5af5ca

We also use a fixture to always clean the client used for test appropriately. Signed-off-by: Leandro Lucarella <[email protected]>

Fix setting of wrong (0, 0) location if the location is missing

258939f

Signed-off-by: Leandro Lucarella <[email protected]>

Add tests for microgrid metadata retrieval

aaf52ad

Signed-off-by: Leandro Lucarella <[email protected]>

Add a sensor ID wrapper

025b072

Signed-off-by: Leandro Lucarella <[email protected]>

Copilot AI review requested due to automatic review settings May 14, 2025 11:27

llucax requested review from a team as code owners May 14, 2025 11:27

llucax requested review from Marenz and thea-leake May 14, 2025 11:27

github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:client Affects the client code labels May 14, 2025

llucax requested review from matthias-wende-frequenz and shsms May 14, 2025 11:28

llucax self-assigned this May 14, 2025

llucax added this to the v0.8.0 milestone May 14, 2025

Copilot AI reviewed May 14, 2025

View reviewed changes

src/frequenz/client/microgrid/_util.py Outdated Show resolved Hide resolved

src/frequenz/client/microgrid/_lifetime.py Outdated Show resolved Hide resolved

llucax force-pushed the sensors branch 2 times, most recently from ea8f3b6 to ada656a Compare May 14, 2025 12:28

llucax added 6 commits May 14, 2025 14:29

Add a Lifetime wrapper

17e05e8

The lifetime will be introduced in the next microgrid API version, but we add it now so we can add a rpc call to get sensors information in a way that is compatible with future microgrid API versions. Signed-off-by: Leandro Lucarella <[email protected]>

Add a utility to convert protobuf enums to Python enums

0aca677

Signed-off-by: Leandro Lucarella <[email protected]>

Update release notes

218ce82

Signed-off-by: Leandro Lucarella <[email protected]>

llucax force-pushed the sensors branch from ada656a to 218ce82 Compare May 14, 2025 12:29

shsms reviewed May 14, 2025

View reviewed changes

llucax closed this May 15, 2025

This was referenced May 15, 2025

Add sensors listing and data streaming #146

Merged

Initial upgrade to the API v0.17.x #94

Merged



		@dataclasses.dataclass(frozen=True, kw_only=True)
		class Sensor:

Add sensors listing and data streaming #144

Add sensors listing and data streaming #144

Uh oh!

Conversation

llucax commented May 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

llucax commented May 14, 2025

Uh oh!

shsms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llucax May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llucax commented May 15, 2025

Uh oh!

llucax commented May 15, 2025

Uh oh!

llucax commented May 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

llucax May 15, 2025 •

edited

Loading