Support fetching CHP data from the DataSourcingActor#1355
Support fetching CHP data from the DataSourcingActor#1355shsms merged 4 commits intofrequenz-floss:v1.x.xfrom
Conversation
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
To be able to keep the resampling actor from crashing or closing the stream for components that are not streaming, we have to keep their upstream channels open. This will be revamped soon when we start supporting stream lifetimes based on downstream needs. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
github-actions
bot
added
the
part:microgrid
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
| ) | ||
|
|
||
|
|
||
| @dataclass(kw_only=True) |
There was a problem hiding this comment.
I wonder why there is so much boilerplate code in this module, but given the module's name I assume this code will go away soon?
There was a problem hiding this comment.
yes, might be a few months, but this is a temporary layer to use old data sourcing and some other actors with the new client.
| Metric.AC_REACTIVE_POWER_PHASE_3: lambda msg: msg.reactive_power_per_phase[2], | ||
| } | ||
|
|
||
| _CHP_DATA_METHODS: dict[Metric | TransitionalMetric, Callable[[ChpData], float]] = { |
There was a problem hiding this comment.
Looks like there is quite some code duplication here, too.
There was a problem hiding this comment.
This is also old stuff, will go away.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for fetching telemetry data from Combined Heat and Power (CHP) components to the SDK's data pipeline. The implementation follows the established pattern used for other component types like inverters and EV chargers.
Changes:
- Added
ChpDataclass to represent CHP component telemetry data - Integrated CHP support into
MicrogridApiSourcefor data fetching and extraction - Updated release notes to document the new CHP telemetry support
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frequenz/sdk/microgrid/_old_component_data.py | Added ChpData class with fields, metrics, and conversion methods following the InverterData pattern |
| src/frequenz/sdk/microgrid/_data_sourcing/microgrid_api_source.py | Added CHP data extraction methods, request validation, and integrated CHP category into data sourcing logic; also includes unrelated removal of channel cleanup code |
| RELEASE_NOTES.md | Documented new CHP telemetry support feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -422,13 +482,6 @@ async def clean_tasks( | |||
| sending_tasks = await clean_tasks(sending_tasks) | |||
|
|
|||
| await asyncio.gather(*sending_tasks) | |||
There was a problem hiding this comment.
This code removal appears unrelated to adding CHP support. The removed lines were responsible for cleaning up and closing channels from the registry after data streaming ends. If this removal is intentional (perhaps as a bug fix or optimization), it should be documented in the PR description or release notes. If unintentional, these lines should be restored as they handle important cleanup logic.
github-project-automation
bot
moved this from To do
to Review approved
in Python SDK Roadmap
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
LATM (Looks Awful To Me), but LGTM. 😆
Even when I read the commit message, it is not entirely clear to me why "Keep data sourcing actor channels open for non-streaming components" is needed, but since this is throw-away code, I'm not very concerned. As long as it works for now, it is fine with me, so approving.
The resampling actor crashes when its input channel is gone. |
github-project-automation
bot
moved this from Review approved
to Done
in Python SDK Roadmap
3 participants
No description provided.