Skip to content
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

Report grid connection point's current limit #19

Closed
tiyash-basu-frequenz opened this issue Oct 4, 2022 · 22 comments · Fixed by #38
Closed

Report grid connection point's current limit #19

tiyash-basu-frequenz opened this issue Oct 4, 2022 · 22 comments · Fixed by #38
Assignees
Labels
part:protobuf Affects the protocol buffer definition files priority:high Address this as soon as possible type:enhancement New feature or enhancement visitble to users

Comments

@tiyash-basu-frequenz
Copy link
Contributor

What's needed?

Every microgrid has a limit on how much current is allowed to flow through the grid connection point. This is generally a static value, dictated by the fuses/circuit breakers placed in the microgrid.

The microgrid API should report this value, so that clients can incorporate it into their decision making mechanisms.

Proposed solution

No response

Use cases

This value can be used by clients to preemptively check if a command does not allow the current at a grid connection point to exceed its fuse's limits.

Alternatives and workarounds

No response

Additional context

No response

@tiyash-basu-frequenz tiyash-basu-frequenz added priority:❓ We need to figure out how soon this should be addressed type:enhancement New feature or enhancement visitble to users part:protobuf Affects the protocol buffer definition files labels Oct 4, 2022
@thomas-nicolai-frequenz thomas-nicolai-frequenz added priority:high Address this as soon as possible and removed priority:❓ We need to figure out how soon this should be addressed labels Mar 30, 2023
@thomas-nicolai-frequenz

The microgrid API needs to be enhanced to expose the grid connection component as well as the grid fuse limit property. This is needed for the PowerManager and other grid fuse protection layers to work properly. Right now this is needed for preventing the FCR test with multiple batteries to exceed the grid fuse limit.

@tiyash-basu-frequenz
Copy link
Contributor Author

The API already exposes the grid connection component. It does not however report the fuse limit, as you indicated. This is precisely what this issue will address.

@thomas-nicolai-frequenz
Copy link

@sahas-subramanian-frequenz and @leandro-lucarella-frequenz we should extend the SDK so that it already exposed a grid fuse limit for the grid connection even the API doesn't yet expose it as part of the PowerManager. Of course by default it would be NULL as in not available but that would help to have a more stable interface right away.

@leandro-lucarella-frequenz

Do you see this as metadata at the same level as the location of the microgrid? If so I guess it would make sense to have it as frequenz.sdk.microgrid.grid_max_current (or whatever name we think it's best.

So, this value could be None (unknown) for real in an installation? This is important info for the type system, to know if really None is a possible value or if we should refuse to run without knowing this limit because it is too dangerous.

@thomas-nicolai-frequenz
Copy link

Do you see this as metadata at the same level as the location of the microgrid

No the grid fuse limit is a property of the grid connection component. that has nothing to do with the metadata. Its just that the property is not yet exposed but will be soon. But in order to move fast we could have the SDK already provide that property even its not set yet.

@thomas-nicolai-frequenz
Copy link

know if really None is a possible value or if we should refuse to run without knowing this limit because it is too dangerous.

if there is no value given None should be fine. Its a good question if the system should run without. At least it should trigger a warning if its not set.

@tiyash-basu-frequenz tiyash-basu-frequenz self-assigned this Apr 6, 2023
@leandro-lucarella-frequenz
Copy link

leandro-lucarella-frequenz commented Apr 6, 2023

No the grid fuse limit is a property of the grid connection component.

OK, I'm trying to understand this, but at the microgrid API level there is no explicit "grid connection component", that would be just the root component (meter) in the component graph. Am I correct?

So if this is so, should all meters have now this max_current property?

I think it would be a good idea to also add it to the microgrid API (this repo), specially if it could be null, the microgrid service could just not set it for now and all should be fine and just add it to the API specs should be pretty trivial (once the design is decided).

Then it will much easier to know exactly how to plug it into the SDK, because we would have to think about it depending on where the data comes from (like if it will be just some other meter metric or what).

@tiyash-basu-frequenz
Copy link
Contributor Author

OK, I'm trying to understand this, but at the microgrid API level there is no explicit "grid connection component", that would be just the root component (meter) in the component graph. Am I correct?

No, there is a grid connection point category. The max current limit will be provided as a metadata of this category.

@leandro-lucarella-frequenz

No, there is a grid connection point category. The max current limit will be provided as a metadata of this category.

I can see the COMPONENT_CATEGORY_GRID, I guess is that one, but I can't find any Grid component. Can you point me to where is it defined? Or it is not defined yet?

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 6, 2023

So if this is so, should all meters have now this max_current property?

Probably. This issue is just related to adding the max current grid connection point. There can be further fuses in sections of the microgrid, and that would cover the point you raised. This topic is already under our radar. More here: #32.
As far as only meters are concerned, one solution could be setting a current limit bound.

@tiyash-basu-frequenz
Copy link
Contributor Author

I can't find any Grid component. Can you point me to where is it defined? Or it is not defined yet?

It does not exist yet. The support for this feature will be added as a part of this issue.

@leandro-lucarella-frequenz

OK, I think before adding anything to the SDK we should have a clear design about where this will exist exactly, otherwise is very hard to put it anywhere, it will be useless at first (always None) and if we start using it, it will probably be just a pointless breaking change when we actually start receiving this from the microgrid API if it comes from somewhere else.

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 6, 2023

OK, I think before adding anything to the SDK we should have a clear design about where this will exist exactly, otherwise is very hard to put it anywhere

OK, this is already included as a component category. Only thing thats missing is the fuse limit property.

COMPONENT_CATEGORY_GRID = 1;

@thomas-nicolai-frequenz
Copy link

Given, that there is already a component category which will have one property fuse limit I see no reason to not move forward on the SDK side by exposing that to actors. Where is the problem of doing so?

@leandro-lucarella-frequenz

Are suggesting that we can create a new component in the SDK called Grid that for now only has a max_current property that can be None and for now will only have None?

If that is correct, then as I said before I see no point in doing this. What is the point on having an object that all it does is having a property that it is always assigned to None?

I just think is putting energy in the wrong place at the moment, unless you have some use case in mind where this would be useful to have even if it is hardcoded to None for now.

There is also the risk that if when implementing this in the microgrid service we figure this should actually be a property of all meters or somewhere else, then if anybody is using it even if it always returns None, then it will be a breaking change.

So I just see downsides in adding this now in SDK and no upsides. But maybe I am missing something, if you see any upsides, please let me know.

@thomas-nicolai-frequenz

Are suggesting that we can create a new component in the SDK called Grid

Indeed as its already existing:

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 11, 2023

There is also the risk that if when implementing this in the microgrid service we figure this should actually be a property of all meters or somewhere else, then if anybody is using it even if it always returns None, then it will be a breaking change.

Well the actors can already start using the property and correctly implement it. Otherwise we keep waiting and waiting and waiting. This is rather an important feature otherwise I wouldn't flag it as such. I also don't understand why we are creating an artificial dependency by saying we only implement it in the SDK once the Microgrid API exposes something else than NONE. Why can't we work on those two things in parallel? There is also an updated Microgrid protobuf definition being release by tomorrow it will contain all the required changes anyhow.

@leandro-lucarella-frequenz

I also don't understand why we are creating an artificial dependency

That wasn't my intention, it is just that for me the requirements weren't clear enough to create an issue in the SDK.

If you say it is enough to create a Grid component in the SDK with just one property max_current hardcoded to None and we already have places where this should be used, then sure we can already add it to the SDK.

If you confirm this is enough, I'll create the issue in the SDK.

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 11, 2023

If you say it is enough to create a Grid component in the SDK with just one property max_current hardcoded to None and we already have places where this should be used, then sure we can already add it to the SDK.

Once this issue is being worked on the max_current property can be retrieved from the new Microgrid API protobuf definition thats in the making as we speak. There will be no need anymore to set it to NONE but of course NONE (or 0) will be the default returned from the Microgrid API as it may not be set to some value.

@leandro-lucarella-frequenz

OK, I was thinking about this, an really to me the dependency is not really having it defined in the microgrid API, it is to have a definition, I feel I am missing a specification to know exactly how to implement this in the SDK. It was just that once there is a definition in the microgrid API, then the specification is suddenly almost done, as I can infer it from it :)

But I think if we want to decouple both, then we should discuss this in the context of the SDK, as I have a few questions if we want to implement it now. I created a discussion in the SDK so we don't keep spamming this issue with SDK stuff :)

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 12, 2023

I feel I am missing a specification to know exactly how to implement this in the SDK

see down below for more details.

#38

@tiyash-basu-frequenz
Copy link
Contributor Author

Closing this issue, since this was implemented in PR #38.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:protobuf Affects the protocol buffer definition files priority:high Address this as soon as possible type:enhancement New feature or enhancement visitble to users
Projects
None yet
3 participants