Add BackupMode to Fronius integration. #126973
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @schinckel
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
bot
added
cla-needed
integration: fronius
new-feature
small-pr
|
Hey there @farmio, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
bot
added
Quality Scale: platinum
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
|
There is no documentation link, because the documentation already mentions "Backup Mode", even though this does not appear to be shown. |
schinckel
force-pushed
the
fronius/backup-mode
branch
from
6da2d56
to
bba6bcc
Compare
|
Hi 👋!
Yes, I think it should add a |
Ah, right. I thought that the This turns out to be somewhat problematic: the Still working through how to handle that - suggestions welcome! |
|
Maybe change core/homeassistant/components/fronius/coordinator.py Lines 52 to 54 in b996bd3 dict[SolarNetId, dict[Platform, list[FroniusEntityDescription]]and check each platform individually. If you can find a better system over all that's fine too - the current seems a bit overly complex 😬 |
|
Afaic there are only 2 datapoints in the currently used api endpoints that return Boolean values: So these would make only 2 entities which are only usable for battery systems. Non-battery systems seem to just omit those keys https://github.com/home-assistant/core/blob/dev/tests/components/fronius/fixtures/symo/GetPowerFlowRealtimeData.json As a workaround to avoid complexity, it may be an option to provide a sensor entity with an Enum state - having 2 possible values 🤔 Nicer option would probably be binary sensor though. |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
I've made some progress (took me ages to realise that HA magically selects the platform based upon which module is currently being run, and you need to enumerate the platforms the integration uses). I'm stuck at the moment because the binary_sensor that is created does not seem to have the correct device_id. Still working through it, but using an enumeration for the backup mode (and battery standby) is looking pretty appealing. |
|
Ah, so the incorrect entity_id was because that was HA loading it from the entity registry. |
|
@schinckel if you have any specific questions, feel free to ask here or find me on HA Discord |
|
Wow, that took some figuring out. I'm not totally happy with the duplication, but it was easier to do that than get mypy to be satisfied with my subclassing. If you really want I'll try to rationalise the Fronius*Entity classes since they share a bunch of code. 
Let me know if you want handled, or I'll just squash the commits. |
schinckel
force-pushed
the
fronius/backup-mode
branch
from
03c1758
to
ae5ae3b
Compare
schinckel
force-pushed
the
fronius/backup-mode
branch
2 times, most recently
from
dbbdd17
to
211e622
Compare
This binary sensor shows if the Fronius Gen24 inverter is running in "Backup Mode", which means the grid power is not available, and the device is running from battery (or using the PVPoint).
This also adds the "Battery Standby" binary sensor.
schinckel
force-pushed
the
fronius/backup-mode
branch
from
6736ca9
to
243da05
Compare
| @@ -40,7 +42,7 @@ class FroniusCoordinatorBase( | |||
|
|
|||
| default_interval: timedelta | |||
| error_interval: timedelta | |||
| valid_descriptions: list[FroniusSensorEntityDescription] | |||
| valid_descriptions: dict[Platform, list[FroniusEntityDescription]] | |||
There was a problem hiding this comment.
| valid_descriptions: dict[Platform, list[FroniusEntityDescription]] | |
| valid_descriptions: Mapping[Platform, list[FroniusEntityDescription]] |
maybe use Mapping as type annotation to mark that read-only / non-mutable.
There was a problem hiding this comment.
Hmm. This causes issues because mypy won't let you call .copy() on a Mapping, I think.
There was a problem hiding this comment.
from collections.abc import Mapping
from copy import copy
x: Mapping[str, int]
y = copy(x)seems to work fine according to https://mypy-play.net/?mypy=latest&python=3.12&flags=strict
There was a problem hiding this comment.
Oh, it works, just mypy complains about it.
- hook id: mypy
- exit code: 1
homeassistant/components/fronius/coordinator.py:84: error: Argument 1 to "deepcopy" has incompatible type "Mapping[Platform, list[FroniusEntityDescription]]"; expected "dict[Platform, list[FroniusEntityDescription]]" [arg-type]
Found 1 error in 1 file (checked 1 source file)
There was a problem hiding this comment.
(But yeah, mypy-play.net says that is fine. Neat tool, I've never seen that before).
There was a problem hiding this comment.
Oh 🤔 that's unfortunate that it behaves differently locally.
Are you sure you are running latest mypy locally since I can't seem to be able to reproduce that. Also the stub for deepcopy looks like
_T = TypeVar("_T")
def deepcopy(x: _T, memo: dict[int, Any] | None = None, _nil: Any = []) -> _T: ...which doesn't seem to care at all about the type of the first argument.
A workaround would be to use dict() which creates a shallow copy itself, so deepcopy(dict(valid_descriptions)) which isn't beautiful, but would work 🙃
Or a dict comprehension like {platform: descriptors.copy() for platform, descriptors in valid_descriptions.items()} (untested sent form mobile 😬)
|
It would be great if some basic unit-tests for the new binary sensors would be added. We already have API-response fixtures that include the keys and also some that don't. |
Yeah, I was working on some unit tests last night, but for some reason the two binary_sensors are coming back with "unknown" in the test case. Ran out of time to figure out why. |
|
Hmm. It seems that when running the code the binary sensors are lagging behind the sensors: that is, when my test system boots up, the sensors are ready immediately, but the binary sensors seem to be 10 seconds behind...I wonder if this is related to the polling frequency, and if the first poll misses the binary sensors. (I think this may be the cause of the failing tests, actually). |
| self.entity_description = description | ||
| self.response_key = description.response_key or description.key | ||
| self.solar_net_id = solar_net_id | ||
| self._attr_native_value = self._get_entity_value() |
There was a problem hiding this comment.
I didn't notice when I refactored the class that in the __init__ it was doing the value set in a way that only works for sensors. This was the same problem that was preventing binary sensors from working at all initially, it needs to store a boolean value in a different instance variable.
|
Usually each platform has its own test file (eg. Could you also add a test with a fixture that doesn't have those keys - asserting the entities aren't created? |
Proposed change
This adds a binary sensor that shows if the Fronius Gen24 inverter is running in "Backup Mode", which means the grid power is not available, and the device is running from battery (or using the PVPoint).
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: