-
Notifications
You must be signed in to change notification settings - Fork 14
Add a new variant Meter to ElectricalComponentCategorySpecificInfo
#414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a new variant Meter to ElectricalComponentCategorySpecificInfo
#414
Conversation
Signed-off-by: Tiyash Basu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for meters in the microgrid electrical components API by introducing a new Meter message type and extending the existing component categorization system.
- Introduces a new
Metermessage with polarity reversal configuration - Extends
ElectricalComponentCategorySpecificInfoenum to include the newMetervariant - Updates release notes to document the new meter functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| electrical_components.proto | Adds new Meter message type and extends enum with meter variant |
| RELEASE_NOTES.md | Documents the new meter features in the release notes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
proto/frequenz/api/common/v1alpha8/microgrid/electrical_components/electrical_components.proto
Outdated
Show resolved
Hide resolved
This commit introduces a new message `Meter` to the `v1alpha8` package, which encapsulates static information about meters in a microgrid. Additionally, the `ElectricalComponentCategorySpecificInfo` enum has been extended to include the `Meter` variant. This enhancement enables the representation of meter-specific information within a microgrid. Signed-off-by: Tiyash Basu <[email protected]>
4efa6c0 to
ccda72c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only comment is it looks a bit like a negative saying "polarity reversed". I wonder if it would make more sense/be more extensible, if we have maybe a enum instead, like SignConvention with PASSIVE and ACTIVE.
When bouncing with Gemini, it agreed the enum is more robust but took it further and suggested PASSIVE, REVERSED_PASSIVE and ACTIVE, saying even if reversed passive and active are the same intent matter. I'm not so sure, but mentioning in case you can think of cases where differentiating both can make a difference.
In any case, this is just food for thought, I know in practice probably the enum works as well. But I thought throwing it out there in case anyone can think of more reasons to really prefer an enum in the long term.
FYI @shsms
|
When challenging Gemini about the need for both it said (among other things):
So I guess for the UI/people on the other side of the SDK it could be valuable information, if there is really a conceptual difference between both. |
|
@llucax thanks for the review. We are thinking about other ways to do this, which do not involve other ways to add this information to this API. I will update the PR according to what we eventually agree upon. |
|
We have successfully agreed upon an alternate solution. This PR s not needed anymore. |
This commit introduces a new message
Meterto thev1alpha8package, which encapsulates static information about meters in a microgrid. Additionally, theElectricalComponentCategorySpecificInfoenum has been extended to include theMetervariant. This enhancement enables the representation of meter-specific information within a microgrid.