Skip to content

Support returning max allowed current at grid connection point #38

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
tiyash-basu-frequenz merged 2 commits into from
Apr 20, 2023
Merged

Support returning max allowed current at grid connection point #38

tiyash-basu-frequenz merged 2 commits into from
Apr 20, 2023

Conversation

@tiyash-basu-frequenz
Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz commented Apr 12, 2023

This PR

  • adds component-category-specific metadata, and
  • introduces metadata for the grid component category, and
  • adds the max allowed current to the grid metadata

@tiyash-basu-frequenz tiyash-basu-frequenz added this to the v0.12.0 milestone Apr 12, 2023
@tiyash-basu-frequenz tiyash-basu-frequenz self-assigned this Apr 12, 2023
@tiyash-basu-frequenz tiyash-basu-frequenz requested a review from a team as a code owner April 12, 2023 12:19
@github-actions github-actions bot added part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files labels Apr 12, 2023
@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 12, 2023

An alternative would have been to just add a max_current_magnitude item to the Component message, but that would have introduced two different methods of setting bounds on inverters and batteries (the other one being dynamic bounds). Would be nice to hear your thoughts on this idea as well, besides the PR.

thomas-nicolai-frequenz
thomas-nicolai-frequenz reviewed Apr 12, 2023
View reviewed changes
@thomas-nicolai-frequenz thomas-nicolai-frequenz mentioned this pull request Apr 12, 2023
Closed
@tiyash-basu-frequenz tiyash-basu-frequenz force-pushed the grid_max_current branch 5 times, most recently from 9f7790b to 7c6417e Compare April 13, 2023 11:06
leandro-lucarella-frequenz
leandro-lucarella-frequenz reviewed Apr 13, 2023
View reviewed changes
Copy link
Contributor

@leandro-lucarella-frequenz leandro-lucarella-frequenz left a comment

Choose a reason for hiding this comment

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

Other than the few comments, LGTM.

leandro-lucarella-frequenz
leandro-lucarella-frequenz reviewed Apr 13, 2023
View reviewed changes
@tiyash-basu-frequenz tiyash-basu-frequenz force-pushed the grid_max_current branch 2 times, most recently from 38e3f07 to fdb41b4 Compare April 13, 2023 12:25
@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 13, 2023

Updated the docs to specify that the limit applies individually to all the 3 phases.

leandro-lucarella-frequenz
leandro-lucarella-frequenz previously approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@leandro-lucarella-frequenz leandro-lucarella-frequenz left a comment

Choose a reason for hiding this comment

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

As said in the previous comment, I would add a comment about the gap in the code for extra clarity, but not a blocker, so approving.

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 14, 2023

Following our discussion here, I have updated the PR to introduce a Fuse message with limits for individual phases, and added it to the grid.Metadata message

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 19, 2023

I reverted the last change according to the latest discussions and inputs.

Now we just have the following:

// The grid connection point metadata.
message Metadata {
  // The rating of the fuse at the grid connection point.
  // This rating specifies the maximum amount of current, measured in amperes,
  // that can flow in or out of each of the 3 phases individually.
  // The current _i_ A at the grid connection point must comply with the
  // following constraint: : `-max_fuse_current <= i <= max_fuse_current`
  uint32 max_fuse_current = 1;
}

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 19, 2023

max_fuse_current

Shall we call it fuse_current_limit instead? the term max feels a little ambiguous.

@leandro-lucarella-frequenz
Copy link
Contributor

leandro-lucarella-frequenz commented Apr 19, 2023

fuse_limit_current? So we can change the suffix if there are new limits.

It is quite unfortunate that no matter what it is still ambiguous and could be misunderstood as the limit rigth now instead of the limit on the electrons flow.

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 19, 2023

How about rated_fuse_current? The inclusion of the term fuse itself implies that the current should be limited here.

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 19, 2023

It is quite unfortunate that no matter what it is still ambiguous and could be misunderstood as the limit rigth now instead of the limit on the electrons flow.

true.

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 19, 2023

How about rated_fuse_current? The inclusion of the term fuse itself implies that the current should be limited here.

Done. Ready for another look.

leandro-lucarella-frequenz
leandro-lucarella-frequenz reviewed Apr 20, 2023
View reviewed changes
Copy link
Contributor

@leandro-lucarella-frequenz leandro-lucarella-frequenz left a comment

Choose a reason for hiding this comment

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

Other than a few comments about outdated docs, it looks like we have a winner!

@tiyash-basu-frequenz
Add metadata specific to component category
4f9b840 
This metadata is returned in response to `ListComponents` calls,
in a new message variable `Component.metadata`.

This is a more general way of representing category-specific metadata,
like type, and removes `Component.type`.

Signed-off-by: Tiyash Basu <tiyash.basu@frequenz.com>
@tiyash-basu-frequenz
Add grid-connection-point metadata
d501ba1 
This commit introduces a `grid.Metadata` message, which contains the item
`rated_fuse_current`. This is the rating of the fuse at the grid connection
point.
This rating specifies the maximum amount of current, measured in amperes,
that can flow in or out of each of the 3 phases individually.
The current _i_ A at the grid connection point must comply with the
following constraint: : `-rated_fuse_current <= i <= rated_fuse_current`

Signed-off-by: Tiyash Basu <tiyash.basu@frequenz.com>
leandro-lucarella-frequenz
leandro-lucarella-frequenz approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@leandro-lucarella-frequenz leandro-lucarella-frequenz left a comment

Choose a reason for hiding this comment

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

Unless there is any pending topic in the SDK discussion that need to be considered here, this is, as far as I am concerned, ready to go! :shipit:

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 20, 2023

@thomas-nicolai-frequenz Do you have any further input on this? If not, we can get this merged.

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 20, 2023

I think I have created enough trouble :-) Lets merge it!

@tiyash-basu-frequenz tiyash-basu-frequenz merged commit ade07ae into frequenz-floss:v0.x.x Apr 20, 2023
@tiyash-basu-frequenz tiyash-basu-frequenz deleted the grid_max_current branch April 20, 2023 08:43
@tiyash-basu-frequenz tiyash-basu-frequenz linked an issue Apr 21, 2023 that may be closed by this pull request
Report grid connection point's current limit #19
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomas-nicolai-frequenz thomas-nicolai-frequenz thomas-nicolai-frequenz left review comments

@leandro-lucarella-frequenz leandro-lucarella-frequenz leandro-lucarella-frequenz approved these changes

@sahas-subramanian-frequenz sahas-subramanian-frequenz Awaiting requested review from sahas-subramanian-frequenz sahas-subramanian-frequenz was automatically assigned from frequenz-floss/api-team

Assignees

@tiyash-basu-frequenz tiyash-basu-frequenz

Labels

part:docs Affects the documentation part:protobuf Affects the protocol buffer definition files

Projects

None yet

Milestone

v0.12.0

Development

Successfully merging this pull request may close these issues.

Report grid connection point's current limit

4 participants

@tiyash-basu-frequenz @thomas-nicolai-frequenz @leandro-lucarella-frequenz @sahas-subramanian-frequenz