Skip to content

Add protobuf definition for communication components #292

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 1, 2025
Merged

Add protobuf definition for communication components #292

merged 6 commits into from
Apr 1, 2025

Conversation

@tar-viturawong-frequenz
Copy link
Contributor

@tar-viturawong-frequenz tar-viturawong-frequenz commented Mar 27, 2025

No description provided.

@tar-viturawong-frequenz tar-viturawong-frequenz requested a review from a team as a code owner March 27, 2025 15:10
@github-actions github-actions bot added part:protobuf Affects the protocol buffer definition files part:tests Affects the unit, integration and performance (benchmarks) tests labels Mar 27, 2025
llucax
llucax reviewed Mar 28, 2025
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this was discussed anywhere, but I find it confusing that communication components are called components but have nothing to do with the existing Component. Can we name them differently to avoid confusion?

Does this mean the microgrid API will get a new RPC for listing and getting these new type of devices as we have for components and sensors? A bit more context of where this is coming from would be useful.

@llucax
Copy link
Contributor

llucax commented Mar 28, 2025

Oh, I see #291 now.

@tiyash-basu-frequenz tiyash-basu-frequenz added this to the v0.7.0 milestone Mar 31, 2025
@tiyash-basu-frequenz
Copy link
Contributor

tiyash-basu-frequenz commented Mar 31, 2025

Thanks @tar-viturawong-frequenz. Could you please add an entry to the RELEASE_NOTES file as well?

@tar-viturawong-frequenz
Copy link
Contributor Author

tar-viturawong-frequenz commented Mar 31, 2025 •
edited
Loading

but I find it confusing that communication components are called components but have nothing to do with the existing Component. Can we name them differently to avoid confusion?

@llucax Unfortunately this is not a decision that I can change on my own. The term communication component was decided by the team some time ago, even if informally, and been incorporated into the UI as such. Renaming decision would have to come from upstream. When/If that happens, I'm happy to submit another change.

@llucax
Copy link
Contributor

llucax commented Mar 31, 2025

@llucax Unfortunately this is not a decision that I can change on my own. The term communication component was decided by the team some time ago, even if informally, and been incorporated into the UI as such. Renaming decision would have to come from upstream. When/If that happens, I'm happy to submit another change.

Yeah, all good, I didn't see that we were renaming component -> electrical_component when I wrote that, and wasn't aware this was coming up, this is why I was a bit surprised. But with the rename, it make sense.

@github-actions github-actions bot added the part:docs Affects the documentation label Mar 31, 2025
@tar-viturawong-frequenz
Update RELEASE_NOTES.md
5829065 
Signed-off-by: Tar Viturawong <[email protected]>
@tar-viturawong-frequenz tar-viturawong-frequenz force-pushed the feat/communication-components branch from b73db37 to e30300c Compare March 31, 2025 12:48
llucax
llucax reviewed Mar 31, 2025
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but leaving the approval to @tiyash-basu-frequenz

tiyash-basu-frequenz
tiyash-basu-frequenz reviewed Mar 31, 2025
View reviewed changes
Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I just have a few nits and one date related comment.

@tar-viturawong-frequenz tar-viturawong-frequenz force-pushed the feat/communication-components branch from 34b3f9c to 41002f0 Compare March 31, 2025 14:27
llucax
llucax reviewed Apr 1, 2025
View reviewed changes
pytests/test_common.py
def test_module_import_microgrid_communication_components() -> None:
"""Test that the modules can be imported."""
# pylint: disable=import-outside-toplevel
from frequenz.api.common.v1.microgrid import communication_components
Copy link
Contributor

@llucax llucax Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be communication_components_pb2.

Copy link
Contributor Author

@tar-viturawong-frequenz tar-viturawong-frequenz Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here 167052f

@github-actions github-actions bot added the part:python Affects the Python bindings label Apr 1, 2025
@tar-viturawong-frequenz tar-viturawong-frequenz force-pushed the feat/communication-components branch from 79d236a to 9ee3a11 Compare April 1, 2025 11:56
@tar-viturawong-frequenz tar-viturawong-frequenz added this pull request to the merge queue Apr 1, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 6a8452e Apr 1, 2025
11 checks passed
@tar-viturawong-frequenz tar-viturawong-frequenz deleted the feat/communication-components branch April 1, 2025 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@llucax llucax llucax left review comments

@tiyash-basu-frequenz tiyash-basu-frequenz tiyash-basu-frequenz approved these changes

@mhrrmk mhrrmk Awaiting requested review from mhrrmk mhrrmk is a code owner automatically assigned from frequenz-floss/api-team

Assignees

No one assigned

Labels

part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files part:python Affects the Python bindings part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Milestone

v0.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@tar-viturawong-frequenz @llucax @tiyash-basu-frequenz