Conversation
* Change text when entity is not available * Add checks of the values for `Installation.set_limit_current`.
|
Would you mind labeling all of the current PRs as either 0.8 or after-0.8, just so that we have a clear understanding of what gets added to the release? |
|
We don't have a release branch atm, so everything that's merged between now and the actual v0.8.0 release gets included into it. Which is the same as that if we want to merge them after v0.8.0 we must wait until after it has been released. So labelling them is a good idea to indicate where it should go. |
|
I'm not sure what release flows you are familiar with. We can create a master-next if we want to proceed with these merges now. If for some reason we need a |
|
I think this creating-a-release-branch-only-when-needed description is quite close to what I'm used to, except our release cycles were so long it didn't really make sense to try to fix bugs on trunk and cherrypick. Instead we typically fixed bugs on the release branch and merged that branch back to master/develop/whatever-your-main-branch-is-called after each patch release (without deleting the release branch). I'm currently travelling for work and don't have access to test on my system (or much leftover brainpower to spend on coding/reviewing), so to me it makes most sense to just wait until 0.8.0 is out. |
|
Np. I'm in no hurry to get things in, so it's ok that these PRs sit. The number of PRs I'm making is because I have the opportunity to spend time on it now, not that I'm overly eager. I know that I won't be able to keep this tempo up when I'm back to work over the vacation. |
custom_components/zaptec/api.py
Outdated
| if not (0 <= v <= 32): | ||
| raise ValueError(f"{k} must be between 0 and 32 amps") |
There was a problem hiding this comment.
Shouldn't the max be determined by the max_current of the installation? While I would expect 32 to be the most common, both less and more is possible.
There was a problem hiding this comment.
There was a problem hiding this comment.
This number dial does not explain how to use it, so the user must be aware of the significance of setting the value to 0, 1-31 and 32.
I'm not sure there is any good way to handle the GUI for this in HA without creating a lot of logic. In Zaptec Portal, this is a simple number input, so having a number dial is no different from that.
There was a problem hiding this comment.
But this isn't the switching function, this is setting the available current. As far as I can see, the linked documentation is only referring to phaseswitching behaviour.
There was a problem hiding this comment.
Oh, I'm sorry, I mixed this up with the other PR.
Yeah, we could compare it against the reported max current for that installation for the upper limit, yes.
There was a problem hiding this comment.
Have no idea. Need to ask Zaptec that. Zaptec Portal lets me set "2" and are happy with that, so they don't enforce any limits.
How would you calculate the minimum permittable value? min(charger["charger_min_current"] for charger in self.chargers)?
There was a problem hiding this comment.
I'm really not sure. After having set the value to 4 or 5 by accident once and needing to unplug and replug my car to get it out of whatever error mode the charging was in, I've never touched available_current directly while the car is plugged in, only through automations.
There was a problem hiding this comment.
In my charger the charger_min_current is 0, so I have no direct indication that the minimum is 6A. When both charger_min_current and charger_max_current is 0, which is the case when the charging is paused due to no available current in the house, then the field allocated_charge_current is 6A. So maybe this gives a hint.
TL;DR: I'm not sure how to reliably use this to set a lower current limit.
There was a problem hiding this comment.
Ok, thanks for investigating, I guess it's better to just leave it at that.
There was a problem hiding this comment.
I have learned that PRO can have max currents up to 63A, so hardcoding 32 A is not a good idea. I did as you proposed and use max_current as basis for the max value in the function. Thanks.
|
@steinmn How does this look now? |
steinmn
left a comment
steinmn
left a comment
There was a problem hiding this comment.
Just a small question, otherwise LGTM
| max_current = float(self.get("max_current", 32.0)) | ||
| except (TypeError, ValueError): | ||
| max_current = 32.0 |
There was a problem hiding this comment.
I'm counting 3 different ways this can default to 32, should we maybe include a _LOGGER.debug in the except case, or is there nothing to gain from that info?
There was a problem hiding this comment.
Neeh, not really. It's the case if the value is not present or is null/None, where we don't have a value to go by. I don't think its terribly important to inform, as this is only used for range checking.
2 participants
While working on other issues, these two were discovered and improved.
Installation.set_limit_current.