Pass through 0W requests to the microgrid API independent of exclusion bounds #801
Conversation
shsms
added
the
part:power-management
github-actions
bot
added
part:tests
Instead of adjusting them to the exclusion bounds, because the API accepts zero power values even when there are exclusion bounds. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
shsms
force-pushed
the
0-pass-through
branch
from
14c2aeb to
c801aa7
Compare
There was a problem hiding this comment.
The PR looks good to me in general code-wise, but I feel uneasy about this special casing.
If "It now forwards 0 power requests directly to the microgrid API, which can decide how to adjust for exclusion bounds, if necessary.", why are we taking care of bounds in the SDK at all?
Again, I have the feeling that we need an API call to release a device (unsetting power so to speak) instead of using 0 as a special case. I think we discussed this before, but don't remember where and what was the conclusion.
| # If the given value is within the exclusion bounds and the exclusion bounds are | ||
| # within the given bounds, clamp the given value to the closest exclusion bound. | ||
| if exclusion_bounds is not None: | ||
| if exclusion_bounds is not None and not value.isclose(Power.zero()): |
There was a problem hiding this comment.
If the power is close to zero, shouldn't we use Power.zero() instead of value? What happens if the API receives something that is close to zero but not exactly zero?
I understand in general we need to compare using is close comparison because of floats work, but if we treat zero specially we might run into problems too.
There was a problem hiding this comment.
The adjusting to zero is done in the PowerDistributor. In the PowerManager, we do this check just to decide if the value needs should be clamped to the exclusion bounds or not.
| if is_close_to_zero(power): | ||
| return DistributionResult( | ||
| distribution={ | ||
| inverter.component_id: 0.0 | ||
| for _, inverters in components | ||
| for inverter in inverters | ||
| }, | ||
| remaining_power=0.0, | ||
| ) |
There was a problem hiding this comment.
In this case, if I understand correctly, you are setting it to zero if it is close to zero, right?
There was a problem hiding this comment.
yup, this is the value that gets sent to the microgrid API.
I have lately been hearing that the SDK now has its own bounds. Don't know why though.
|
|
Ok, that makes sense, if 0 is explicitly allowed I guess we can keep zero as a special case. The question about float precision still remains though, what happens if the API receives a set power with 0.00000001? Does it consider it 0 and apply it or it ignores it? Maybe we can allow calling set power with no power to unset power? |
|
One last thing that comes to mind, I guess my issue with 0 as a special value is that, if 0 is really not in bounds (like we need DC heating to the battery will be either charging or discharging even if you set power to 0), it will be misleading, and people might think that they stopped a battery from (dis)charging and that is not really true. |
|
Another option would be to keep 0 out of bounds and make the SDK decide what to do if 0 is not in bounds, and pick the lower or upper bound closer to 0 explicitly. |
This is a little bit of a weak argument. This would be like saying setting power to 0 does indeed stop any discharging of a battery but there are things like self-discharge etc. I would be very careful with absolute assumptions. Another example is when the BMS on itself starts balancing a battery. So I don't feel this speaks for or against 0 as a special value. |
|
We discussed this only for the battery pool interface @llucax, and there it is simplified and users don't have to worry about exclusion bounds at all. While communicating with the API, this information needs to be shared in whatever way possible now, but I like the approach @tiyash-basu-frequenz proposes for the next version, of adding And I think we can consider close-to-zero values in the micro-watt or lower ranges as zero. |
In which case would the SDK want to set something to 0? If it can't why would it need set anything at all? Of course it always can set the value to the lower bound. Lets assume the lower bound is 5kW and so we would have the same discussion as the value can't be set below 5kW. I don't feel this case is simply related to 0. |
This was already there. But that's getting in the way of the DC heater, for no reason. Now we have this behaviour for non-zero powers within the exclusion bounds, but zero is no longer always adjusted to (-300, 150, 150) in case of 3 batteries. |
This PR doesn't affect the interface for the users. They don't have to deal with exclusion bounds, etc. They propose power to the power manager and they get what they get. This PR is only about the SDK's communication with the microgrid API. |
|
OK, I'm OK with keeping 0 as a special value if the API explicitly allows it in the bounds, and if it is defined behavior that "small enough" values are considered 0. And yeah @thomas-nicolai-frequenz, I know what you ask for is not always what you get (this also happens with resolution, if you ask for 10W but the device has a resolution of 15W you could get 15W), so the user should never trust the power is what they set, but still I wanted to make the point for the sake of discussion and make sure we are aligned, because besides that, the intention here to set the power to as little as possible, not necessarily zero. But yeah, my main concern was about the inconsistency of sending 0 when we knew it was out of bounds only because we know the microgrid service is weird and will accept it anyway instead of reject it. If we have the bounds (0,0) always included, that inconsistency goes away. @shsms in the SDK I would then make sure to always send exactly |
This PR is taking care of that already. |
That part I missed and thats strange. What does sending bounds (0,0) do when the DC heating is going on and has set the bounds to (5,0). Why would the API accept (0,0). Now I'm also confused. |
When there's no client request (or 0W request), the DC heater sets -30kW for 30 secs, then switches to +30kW, etc. to minimize the impact on the soc, etc. In a location with 1 battery, the power distributor would just set to -30kW always, until the exclusion bounds go away. That might not be desirable, especially when the user asked for 0 and wasn't expecting to see soc drop. Although now that I think about it, in a location with 3 batteries for example, the PowerDistributor would do a better job with reducing the impact on the grid consumption. For example, if each battery has exclusion bounds -30k..+30k, without this patch, the PowerDistributor would distribute a 0W request as (-60k, 30k, 30k). (If -60k is not possible on any of the batteries but -50k is, I guess we'll see a 10k consumption from the grid) And it would switch up the order based on soc changes between the batteries (I think, needs to be confirmed). But again, most actors don't repeat 0 powers and we don't repeat power values either in the SDK, but have plans to change that in the long term. I guess there are clearly more options than always offloading the work to the DC heater, at least in some configurations. |
|
I will take this in as it is now, because this is already correct, and keep any further optimizations for later. |
4 participants
The PowerManager and PowerDistributor were adjusting 0 power requests to exclusion bounds, which it shouldn't do.
It now forwards 0 power requests directly to the microgrid API, which can decide how to adjust for exclusion bounds, if necessary.
Closes #794