frequenz-floss / frequenz-api-microgrid

gRPC+protobuf specification and Python bindings for the Frequenz Microgrid API
https://frequenz-floss.github.io/frequenz-api-microgrid/
MIT License
6 stars 6 forks source link

Allow clients to specify validity durations for bounds #231

Closed tiyash-basu-frequenz closed 7 months ago

tiyash-basu-frequenz commented 7 months ago

This commit adds a field named validity_duration to the AddComponentBoundsRequest message, which can be used by clients to specify a custom default duration to bounds while adding them. If not specified, a default validity duration of 5 seconds is applied.

tiyash-basu-frequenz commented 7 months ago

The context being that we do not want clients to set bounds for arbitrarily long durations: An alternative approach to the one in this PR would be to allow the client to ask for an arbitrary time, but limit it internally to 15 minutes (and of course, document it). I did not go for that approach because that approach requires users to go through the docs, whereas this approach enforces such limits directly in the API.

Would be interested in hearing your opinions.

llucax commented 7 months ago

An alternative approach to the one in this PR would be to allow the client to ask for an arbitrary time, but limit it internally to 15 minutes (and of course, document it). I did not go for that approach because that approach requires users to go through the docs, whereas this approach enforces such limits directly in the API.

I agree it is better to fail if the requested time can't be fulfilled, otherwise it could lead to unexpected behaviour. But any reason not to use an arbitrary number, document a maximum and minimum allowed value and fail the request if the value is out of the valid range? For me it looks a bit weird to have these times as an enum.

tiyash-basu-frequenz commented 7 months ago

But any reason not to use an arbitrary number, document a maximum and minimum allowed value and fail the request if the value is out of the valid range? For me it looks a bit weird to have these times as an enum.

If it is an enum, users will get a compilation error if they choose an invalid timestamp. If itis a number, then they will get a runtime error, which could mean more mistakes and more error-handling (so more code). Hence my first choice was an enum.

But TBH, I do not have a strong opinion either way. I figured I could just create the PR and discuss it here.

llucax commented 7 months ago

I'm not completely against the enum either, which I guess it is a bad thing, as it is harder to decide :P

I just find it a bit weird, but I never used it either, so maybe I'm just concerned that it might be a bit inflexible but it is an unjustified concern and it might be more than enough flexibility. So probably it is better to get someone that really use bounds to talk. Maybe @shsms @daniel-zullo-frequenz @Marenz ?

shsms commented 7 months ago

The SDK won't be sending bounds once we upgrade to this version, just setting power. For power, the interval is not customizable right? If it is customizable, we'd have to get an option from the actors, so that the SDK doesn't have to make generalized decisions.

Also, I'm curious about the motivation for asking the users to decide. Why/when would I as a user choose 15 minutes over the default 5 seconds, and repeat commands every 3 seconds? Are there any specific usecases/requirements that's driving this?

tiyash-basu-frequenz commented 7 months ago

The SDK won't be sending bounds once we upgrade to this version, just setting power. For power, the interval is not customizable right? If it is customizable, we'd have to get an option from the actors, so that the SDK doesn't have to make generalized decisions.

The idea is to make it customizable.

Also, I'm curious about the motivation for asking the users to decide. Why/when would I as a user choose 15 minutes over the default 5 seconds, and repeat commands every 3 seconds? Are there any specific usecases/requirements that's driving this?

Good point, I forgot to link the issue. It is linked now, and has the use-case mentioned there.

tiyash-basu-frequenz commented 7 months ago

ping