among matched inverters, favour the least permissive schema - #213
among matched inverters, favour the least permissive schema#213kdehairy wants to merge 7 commits into
Conversation
Schemas are ambiguious. This leads to potential wrong discovery, even if "technically" it works. This commit adds a test for that to just surface the issue. Hopefully fixes will follow
6d66b04 to
bce8bc2
Compare
|
This is a list of the colliding schemas with the most permissive first: |
REGISTRY was effectively an alphabitically ordered list of inverters. Since inverter's schemas here are ambiguious, leading to having multiple inverters being a match, it is effectively a luck game. A schema such as the X1LiteLV is so permissive that it acts as a catch all kinda match. And given it's alphabitical position at the top, it hide other, potentially, legit candidates. This commit is not changing the luck factor, unfortunately, but at least put a higher tax on permissive schemas, by putting them at the tail of the discovery list (and the least to be picked among matches).
I'm opting not to remove it, as a kind of reminder that schemas need more tighting.
pinning the type is the most effective disambiguation factor when finding a match. All known inverters has known type values returned. It makes sense to enforce pinning that value in the schemas. The second best is the length constraints on the data and information arrays. but since I, personally, am not 100% sure if it is unnecessary to have "ranges", hence enforce pinned single value(s), I opted to make it as allow-to-fail test. A reminder that it is worth checking by the authors/maintainers of offending inverters.
f515d0c to
bb88f12
Compare
|
now the list is: |
|
Hello @squishykid, Home Assistant has upgraded the dependency on your package to 3.2.4 home-assistant/core#173787. This PR with a small change (or the alternative #211) fixes an important issue in your package since version 3.2.4 (present since September last year) and is now breaking thousands of Home Assistant inverters. Can you therefore please merge either this PR or PR #211 and release it as a tag? It's not a big fix or much work. This would be immensely helpful, thanks so much. |
|
You are not able to plan or test anything right for your super "Autodiscover" shit algorithm. I am busy... you can contribute... and then you fucking ignore the PR's. Just ask Claude to make the fucking dropdown menu to select the right inverter manually bunch of lazy monkeys. If you can not test (I understand there is a lot of variants) then relax your expectations put in place a manual mechanism to no lock people out of their own inverters. Your fucking EGO is killing this Extension. I hope shit developers like you loose their jobs. Root cause of the problem, too many variants, to ambitious feature, poor testing, poor PR support, human EGO. |
|
I don't think this is helpful at all @japeral. Nobody is obligated anything and I am only trying to respectfully explain the situation that it's not a large bugfix that costs much time to review it and it is impacting a lot of people at the moment. |
|
@japeral I understand the anger here, really. But ... I worked with @squishykid before on this repo in a much much bigger PR before, and I certainly did not see any EGO, and he was super responsive and cooperative. That leads me to conclude that something in his life currently requires more attention than this, which happens in life! May be the improvement/advise I'd give is to elect a co-author(s) to remove the bus factor, since this project has a serious user base now. |
|
And by the way, the dropdown suggestion of yours @japeral is indeed the way to go. But it belongs to home assistant repo itself, and there is an open PR for it their home-assistant/core#176275 |
This is an attempt to fix schema ambiguities that leads to issue #212
Details:
favoring least permissive schemas while discovering
REGISTRY was effectively an alphabetically ordered list of inverters.
Since inverter's schemas here are ambiguous, leading to having multiple
inverters being a match, it is effectively a luck game.
A schema such as the X1LiteLV is so permissive that it acts as a catch
all kinda match. And given it's alphabetical position at the top, it
hide other, potentially, legit candidates.
This patch is not changing the luck factor, unfortunately, but at least
put a higher tax on permissive schemas, by putting them at the tail of
the discovery list (and the least to be picked among matches).
Requiring a pinned type value in schema
This turns out to be the most effective disambiguation factor among
inverters' schemas.