frequenz-floss / frequenz-api-common

Shared protobuf definitions and Python bindings for Frequenz APIs
https://frequenz-floss.github.io/frequenz-api-common/
MIT License
1 stars 12 forks source link

Address fuse limits redundancy in `GridConnectionPoint` and `Fuse` #246

Closed tiyash-basu-frequenz closed 2 months ago

tiyash-basu-frequenz commented 2 months ago

What's needed?

The GridConnectionPoint message has a field rated_fuse_current to store the fuse limit at the grid connection point.

https://github.com/frequenz-floss/frequenz-api-common/blob/c75dba3a24be767b24c929f6c418535e48bfdc83/proto/frequenz/api/common/v1/microgrid/components/grid.proto#L34-L46

However, we also have a dedicated message for fuses, called Fuse.

https://github.com/frequenz-floss/frequenz-api-common/blob/c75dba3a24be767b24c929f6c418535e48bfdc83/proto/frequenz/api/common/v1/microgrid/components/grid.proto#L34-L46

This means that there are two different ways of specifying the fuse limit at the grid connection point -

Proposed solution

Proposal 1

We keep things as they are. That way we statically ensure that a grid connection point always has specified fuse limit.

If a Fuse entity is the only successor to a GridConnectionPoint entity, then the effective fuse limit is the minimum of the two.

Proposal 2

We remove rated_fuse_current from GridConnectionPoint, and always add a following Fuse entity. This then needs to be checked by clients at runtime.

Use cases

No response

Alternatives and workarounds

No response

Additional context

No response

llucax commented 2 months ago

I would say this is related to https://github.com/frequenz-floss/frequenz-api-common/issues/242. It is also having metadata vs a component-graph based representation of some aspect of the microgrid.

I think we should be ideally consistent and if we go with metadata, then allow it for GridConnectionPoint, but if we lean towards having a more precise component graph, we should probably remove the rated_fuse_current from GridConnectionPoint and required people creating a Fuse in the component graph instead.

I think the UI could help here, if we can add some rules based on the component, we might add a way to automatically always showing both a GridConnectionPoint and Fuse in the creation form when a user wants to create a new GridConnectionPoint, so it is harder to make mistakes and forget about the fuse.

tiyash-basu-frequenz commented 2 months ago

I think it is slightly different in concept to #242.

242 is specifically about representing a separate branch in the circuit. It is not about a property of a meter.

This one is about our model for PCC (which we call GridConnectionPoint for better UX), and its properties. So far we have used the approach of clubbing data fields related to the PCC in GridConnectionPoint, and treating those as properties of GridConnectionPoint entities. This happens to include fuse limits as of now. This could contain more data fields in the future, e.g., max consumption and feed-in power limits.

In short, #242 is about representing electrical connections, whereas this issue is about the properties of GridConnectionPoint, and checking if splitting them up makes sense.

Note that we cannot represent every property via different components (e.g., max feed-in power). So the current approach is still a valid one.

I think the UI could help here, if we can add some rules based on the component, we might add a way to automatically always showing both a GridConnectionPoint and Fuse in the creation form when a user wants to create a new GridConnectionPoint, so it is harder to make mistakes and forget about the fuse.

That is independent of the API definition.

llucax commented 2 months ago

I think it is slightly different in concept to #242.

Yes, of course, this is why I said related :)

This one is about our model for PCC (which we call GridConnectionPoint for better UX), and its properties. So far we have used the approach of clubbing data fields related to the PCC in GridConnectionPoint, and treating those as properties of GridConnectionPoint entities. This happens to include fuse limits as of now. This could contain more data fields in the future, e.g., max consumption and feed-in power limits.

In short, https://github.com/frequenz-floss/frequenz-api-common/issues/242 is about representing electrical connections, whereas this issue is about the properties of GridConnectionPoint, and checking if splitting them up makes sense.

Note that we cannot represent every property via different components (e.g., max feed-in power). So the current approach is still a valid one.

Yeah makes sense, of course if there are properties that are part of the grid connection point they should live as metadata/info of that component. If I understand correctly, this is sort of a virtual component, there is no grid connection point as a device, is just sort of 2 cabled being plug together, but there are regulations or contracts or requirements that put some restriction on how electricity can flow through that coupling right? For those, I think grid connection point metadata makes sense. If there is real physical device, like a fuse, just before the coupling, then from a "representing reality" perspective, it sounds like it makes more sense to have a fuse component in the component graph before the grid connection point, as we already have a fuse component.

That is independent of the API definition.

Yeah, but making it harder for users to make mistakes while configuring the microgrid was brought up as a reason for going with metadata instead of a separate component, so I just brought it up because usability issues might be addressed somewhere else.

tiyash-basu-frequenz commented 2 months ago

We discussed this in a meeting. The conclusion was to keep the fuse limit field inside the GridConnectionPoint message now.

In addition to that, we also concluded that the Fuse component category will not be used internally, but will remain in the API for now.

This issue can be closed.