Add missing state and error wrappers #54
Conversation
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
If we can't find the received state between the ones we know, we should set it to `UNKNOWN`, as there is actually a state, it is **specified**, but we just don't know about it. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
github-actions
bot
added
part:docs
|
I will release v0.4.0 after this is merged. |
llucax
added
scope:breaking-change
|
Enabled auto-merge. |
| @@ -94,6 +94,6 @@ def from_pb(cls, evc_state: ev_charger.ComponentState) -> Self: | |||
| Enum value corresponding to the protobuf message. | |||
| """ | |||
| if not any(t.value == evc_state for t in EVChargerComponentState): | |||
| return cls(cls.UNSPECIFIED) | |||
| return cls(cls.UNKNOWN) | |||
There was a problem hiding this comment.
I guess we can't escape having to repurpose something. Now we are giving extra meaning to UNKNOWN instead, saying that either the modbus drivers couldn't recognize the state (and it gets set by the api service), or the client couldn't recognize the state.
There was a problem hiding this comment.
We can have UNKNOWN_TO_CLIENT = -1 and because protobuf enums can't have negative numbers, it will always be unique, but might be overkill, and hard to name.
There was a problem hiding this comment.
There are some enums that don't have a UNKNOWN, and for those, we're still returning UNSPECIFIED when a code is unknown to the client. So maybe it is worth making them all consistent.
There was a problem hiding this comment.
Yeah, not sure. If you look at the gRPC errors, some codes can be produced by the server or client, like INTERNAL or RESOURCE_EXHAUSTED (see https://grpc.github.io/grpc/core/md_doc_statuscodes.html), I think we can see this similarly and say that the estate is unknown, I'm not sure if it is that relevant to the user of the client to know if it is unknown to the server or client, I can only see the case where the client is outdated compared to the API specs and the users somehow re-generate its own bindings and use that instead to see if the code is actually known by the server.
IMHO UNKNOWN is definitely more accurate here, and if some estate don't have this in the spec, I think that should be a specs bug. @tiyash-basu-frequenz ?
There was a problem hiding this comment.
(ah, Tiyash will probably not answer soon, so we shouldn't wait for his input.
There was a problem hiding this comment.
IMHO UNKNOWN is definitely more accurate here, and if some estate don't have this in the spec, I think that should be a specs bug.
I think UNKNOWN was introduced for component states where we can have a bug in parsing the raw data. That's why the the descriptions for all UNKNOWNs say "Component has state, but we understand it". For other enums where the data doesn't come from hardware, the API doesn't have an UNKNOWN option.
But we are trying to re-purpose UNKNOWN to suggest server-client communication failure, which could happen to all enums, and that's why I think we are trying to give two meanings.
One more option is to say UNKNOWN = -1 in all python definitions of enums that don't have unknown.
We can keep for later if you prefer, but until then, we'll have UNKNOWN and UNSPECIFIED both being repurposed for something they were not originally meant to do.
There was a problem hiding this comment.
Yeah, I think this goes beyond the microgrid API, maybe it would be a good topic for the API meeting.
|
To speed up things I accepted the suggestion to improve the wording for the UNKNOWN estates and added a |
|
Actually I'm leaving the auto-merge enabled. Since the fixup commit is not signed-of-by, the automerge won't happen until I squash the commits, so if I get an approval I can squash, force-push and the PR will be queued if the resulting diff doesn't change, so it won't require a second approval after the squashing. |
|
Fixes look good to me, can approve once squashed. |
We add some missing wrappers for battery and inverter errors. To do that we also need to add new enums for battery and inverter error codes and the common error levels. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Add some missing wrappers for battery and inverter states and battery relay states. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
We adjust the documentation and enum lookup to match the newly added wrappers. The new enum conversion also avoid a double linear search when creating enums. We can just try to create it and if it fails create an `UNSPECIFIED` / `UNKNOWN` item, so we do only one search. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Adds a summary for the new release. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The original wording comes from the API specs docs (for example https://github.com/frequenz-floss/frequenz-api-microgrid/blob/2332734e86dd8c1d3bf4ee601b93d52525654a6f/proto/frequenz/api/microgrid/inverter.proto#L70-L72), but we change the wording here to make it more clear. Co-authored-by: Sahas Subramanian <sahas.subramanian@proton.me> Signed-off-by: Leandro Lucarella <luca@llucax.com>
|
You can have another look if you want, but the only that could be messed up is the split of the commits, as long as the resulting diff is the same, the approval still stands. |
UNKNOWNEV charger component stateUNKNOWNinstead ofUNSPECIFIEDfor unknown states