Add sensors listing and data streaming #146
Conversation
Signed-off-by: Leandro Lucarella <[email protected]>
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR adds sensor support to the microgrid client by introducing new RPCs for listing sensors and streaming sensor data, while updating related data structures and tests. Key changes include:
- New sensor-related classes and methods (e.g. Sensor, SensorMetricSample) in the library.
- Updates to the client to support list_sensors() and stream_sensor_data(), along with corresponding test cases.
- Adjustments to ID types and release notes to include sensor functionality.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sensor.py | Added tests for sensor creation, equality, and hashing. |
| tests/test_metrics.py | Added tests for metric sample creation and aggregation. |
| tests/test_lifetime.py | Updated tests for lifetime validation and activity. |
| tests/test_id.py | Updated ID tests to include SensorId. |
| tests/test_client.py | Added tests for list_sensors(), stream_sensor_data(), and grpc error handling. |
| src/frequenz/client/microgrid/sensor.py | New sensor-related classes and utilities. |
| src/frequenz/client/microgrid/metrics.py | New definitions for metric aggregation. |
| src/frequenz/client/microgrid/_util.py | Utility function for enum conversion. |
| src/frequenz/client/microgrid/_sensor_proto.py | Converters from protobuf messages to sensor objects. |
| src/frequenz/client/microgrid/_lifetime.py | Lifetime validation and activity checking. |
| src/frequenz/client/microgrid/_id.py | Updated ID definitions to include sensors. |
| src/frequenz/client/microgrid/_client.py | Extended client with sensor listing and streaming functionality. |
| src/frequenz/client/microgrid/init.py | Updated exports to include sensor types. |
| RELEASE_NOTES.md | Updated release notes to describe new sensor features. |
|
@shsms I should have addressed all comments in #144. @tiyash-basu-frequenz maybe it would be good if you have a look to see if there is nothing that goes against future directions. |
|
OK, it looks like we are going to unify the |
|
BTW, now I tested this in a real microgrid and it seems to be working fine. |
|
@shsms I hope you are happier with the commit split now too 🙏 |
|
Will rename |
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]>
llucax
force-pushed
the
sensors2
branch
2 times, most recently
from
a4494c3 to
d957b8f
Compare
| UNSPECIFIED = 0 | ||
| """Default value (this should not be normally used and usually indicates an issue).""" | ||
|
|
||
| TEMPERATURE = 1 |
There was a problem hiding this comment.
Time to switch to pb2 names instead of numbers now?
There was a problem hiding this comment.
I'm still not convinced. This is a pure-python class/interface, all the dealing with protobuf conversion is done in the _from_proto() functions. I just happen to pick the same numeric IDs for convenience, to make conversion more easy.
What arguments do you have in favor of using the PB values here?
There was a problem hiding this comment.
This is a pure-python class/interface
We are trying to build a client for the microgrid API. Not a stand-alone thing. This is just increasing the chances for bugs, especially for esoteric component types, which we might add support for only later. To me, this is just overcomplicating things for no apparent benefit.
What arguments do you have in favor of using the PB values here?
Single source of truth for enums without any manual work.
and just a few days back, I saw some API PRs changing the numbers for enums.
There was a problem hiding this comment.
I understand your point about this being very dependant on the defined protobuf stuff, but all wrappers are protobuf free. I know it is a very theoretical thing but I like it. :D
It is a very minor thing, but the alternative to me is also a very minor thing. The single point of truth is pointless once this is stable, is a write once, never ever touch it again, ever. So I'm not concerned at about about it.
That said, since it is controversial, and the API is currently NOT stable, I can see it might be problematic until we get there, and if we have some issue because of this, I don't want you in my face telling me I told you so, so I'll change it and leave the discussion for a distant future :P
There was a problem hiding this comment.
I told you so
Oh, I really enjoy doing it, too bad. But there will be other opportunities. :D
|
Added some new and some fixup! commits to address the comments. One minor advantage of using |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for sensor management in the microgrid client by introducing sensor listing and sensor data streaming functionality. Key changes include:
- Adding tests for sensor-related functionality in test_client.py.
- Introducing new sensor types, data structures, and conversion utilities in the sensor modules.
- Updating client code to support sensor streaming and sensor ID handling.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_lifetime.py | Adds tests for asset lifetimes with no sensor-specific changes. |
| tests/test_id.py | Updates ID tests to reflect new import and parameters changes. |
| tests/test_connection.py | Adjusts imports for connection tests. |
| tests/test_component_data.py | Updates ComponentData imports with new ID module. |
| tests/test_component.py | Imports updated for component tests. |
| tests/test_client.py | Introduces tests for list_sensors() and stream_sensor_data() methods. |
| src/frequenz/client/microgrid/sensor.py | Adds sensor classes and enums for sensor metrics and states. |
| src/frequenz/client/microgrid/metrics.py | Introduces aggregated metric value logic. |
| src/frequenz/client/microgrid/id.py | Implements strongly typed IDs including SensorId. |
| src/frequenz/client/microgrid/_sensor_proto.py | Provides conversion functions for sensor-related protobuf messages. |
| src/frequenz/client/microgrid/_client.py | Adds sensor listing and data streaming methods to the client. |
| Others | Miscellaneous import updates and deprecation removals. |
|
Will check in the morning. |
| # Copyright © 2025 Frequenz Energy-as-a-Service GmbH | ||
|
|
||
| """Strongly typed IDs for microgrids, components and sensors.""" | ||
| r'''Provides strongly-typed unique identifiers for entities. |
There was a problem hiding this comment.
This is very nice, also useful outside the microgrid like for the GridpoolID, etc. and also the reporting client. Maybe this /and/ the subclasses should go to the base client.
There was a problem hiding this comment.
Yes, next step is move lots of stuff in here to client-common and client-base.
Signed-off-by: Leandro Lucarella <[email protected]>
This class will be used to list and retrieve sensors information from the microgrid. The class 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. It is also implemented having in mind that sensor types will be removed in future releases (and it is actually not being set properly in the 0.15 service), so sensor types are being ignored. 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 RPC is also 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. It uses the `ListComponents` RPC filtering to retrieve only sensors. Signed-off-by: Leandro Lucarella <[email protected]>
These wrappers are also not present in 0.15, but are added so users can already start writing code that supports aggregates to ease the migration to 0.17. Because of this, no protobuf parsing code is added yet. Signed-off-by: Leandro Lucarella <[email protected]>
Again, these wrappers are done with 0.17 in mind, and trying to be forward-compatible, although sensor data in 0.15 is already more similar to 0.17 (compared to component data). Signed-off-by: Leandro Lucarella <[email protected]>
This RPC is also 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. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
This module will be moved to `client-common` in the future, and there will be more additions, so it makes sense that it is separated from the microgrid module. In an upcoming commit we'll also add a `BaseId` class to make defining new IDs easier. Signed-off-by: Leandro Lucarella <[email protected]>
Defining new IDs via the new base class is much simpler and concise now, and avoids a lot of code duplication. Please see the module documentation for more details. Signed-off-by: Leandro Lucarella <[email protected]>
We need to remove some `SensorErrorCode`s because they are not defined in 0.15, they were included as a future-compatibility with 0.17. Signed-off-by: Leandro Lucarella <[email protected]>
2 participants
This pull request introduces support for sensors to the microgrid client.
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 have been designed based on the v0.17 specifications. This approach aims to simplify the future migration process for users.Fixes #136.