feat(solax-integration): Persist discovery result and lets the user be the tie breaker instead of asyncio scheduler - #176275
Conversation
|
Hey there @squishykid, @Darsstar, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Adds optional manual inverter model selection to SolaX setup while retaining automatic detection.
Changes:
- Adds an inverter-model selector.
- Applies the selected model during validation and startup.
- Parameterizes config-flow tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/solax/__init__.py |
Applies the saved model during setup. |
homeassistant/components/solax/config_flow.py |
Adds model selection and validation. |
homeassistant/components/solax/const.py |
Defines the inverter entry-point group. |
homeassistant/components/solax/strings.json |
Adds selector text and guidance. |
tests/components/solax/test_config_flow.py |
Covers automatic and manual inputs. |
linked to this PR home-assistant/core#176275
|
@squishykid can you please review? latest release is broken for most of users (including me) due to malfunctioning auto-discovery. Please don't make us wait for long :) |
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Why can't we automatically select the model and just store that?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
The automatic selection (auto discovery) is heuristic by nature. One inverter can end up being auto discovered as another one, which will "technically" work (no errors, warnings, etc...), but it will expose garbage values to a non-existing (in reality), yet declared, sensors. for more information take a look at this issue in the underlying lib squishykid/solax#212 (another issue for assurance squishykid/solax#185) The only deterministic way of correctly picking the right inverter, is the user selecting what they already see on the physical device on their wall. |
The current auto-discovery behaviour often results in multiple "valid" inveters, and it just picks one, effectively, at random. This patch allows the user to hand pick the model. If they did so, the auto-discovery is disabled and the hand picked model is chosen. If left empty, the current auto-discovery behaviour is picked up. making this backward compatible (UX wise).
Instead of blocking everytime we need to access the list.
instead of loading them all at import time.
Do the auto-discovery first, which will return "compatible" inverters. Using whose to populate the model selection dropdown. This avoids frustration when the user choses a non-compatible inverter, thinking that this would work, when it won't.
The semantics behind the var is that it holds potentially correct types for the single inverter discovered, not a list of more than one inverter discovered.
avoid repetitive boilerplat. Less error pron later when/if changes happen.
Moves tests to more relevant test file.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
eee58f0 to
df9c674
Compare
|
@joostlek can you please re-review? |
|
Any update on this PR? the latest commits look good, and the requested change has been implemented. |
|
what's the problem with this PR? can someone approve it? |
|
THANK YOU!!! @joostlek |
Proposed change
In Solax integration, the current auto-discovery behavior often results in multiple "valid" inverters, and it just picks one, effectively, at random. Which means one inverter could be detected as a different one, rendering the integration effectively exporting the wrong sensors (correct for the detected, but non-existing for the actual real one).
This patch allows the user to hand pick the model. If they did so, the auto-discovery is disabled and the hand picked model is chosen.
If left empty, the current auto-discovery behavior is picked up. making this backward compatible (UX wise).
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: