From 5b19ac2ddd5749d7e9068cba9e966c1f71924e4f Mon Sep 17 00:00:00 2001 From: Khaled Eldoheiri Date: Fri, 10 Jul 2026 17:56:24 +0200 Subject: [PATCH 1/7] fix: Depricated way of installing in verify.sh --- verify.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/verify.sh b/verify.sh index 7ea6cca..086543f 100755 --- a/verify.sh +++ b/verify.sh @@ -4,7 +4,7 @@ set -e echo "Installing Dependencies..." python -m pip install --upgrade pip -python setup.py install +python -m pip install . pip install --upgrade flake8 pylint pytest pytest-cov pytest-asyncio pytest-httpserver black mypy echo "Running black..." From 7347e4b77708340d78168dccd1f85ff212f18d35 Mon Sep 17 00:00:00 2001 From: Khaled Eldoheiri Date: Fri, 10 Jul 2026 18:02:35 +0200 Subject: [PATCH 2/7] fix: black checks --- solax/inverter.py | 2 +- solax/response_parser.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/solax/inverter.py b/solax/inverter.py index 8abe642..8f24814 100644 --- a/solax/inverter.py +++ b/solax/inverter.py @@ -91,7 +91,7 @@ def sensor_map(cls) -> Dict[str, Tuple[int, Measurement]]: for name, mapping in cls.response_decoder().items(): unit = Measurement(Units.NONE) - (idx, unit_or_measurement, *_) = mapping + idx, unit_or_measurement, *_ = mapping if isinstance(unit_or_measurement, Units): unit = Measurement(unit_or_measurement) diff --git a/solax/response_parser.py b/solax/response_parser.py index cbccbdd..ec97755 100644 --- a/solax/response_parser.py +++ b/solax/response_parser.py @@ -95,7 +95,7 @@ def _postprocess_gen( Return map of functions to be applied to each sensor value """ for name, mapping in self.response_decoder.items(): - (_, _, *processors) = mapping + _, _, *processors = mapping for processor in processors: yield name, processor From 3acf75123565c25b0ae993a15c06283f6b9f9cd5 Mon Sep 17 00:00:00 2001 From: Khaled Eldoheiri Date: Fri, 10 Jul 2026 18:27:04 +0200 Subject: [PATCH 3/7] fix: pylint errors and test coverage --- solax/discovery.py | 6 +++--- solax/inverter_http_client.py | 6 +++--- solax/response_parser.py | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/solax/discovery.py b/solax/discovery.py index b9c5dda..2d553e3 100644 --- a/solax/discovery.py +++ b/solax/discovery.py @@ -13,12 +13,12 @@ if sys.version_info >= (3, 10): from importlib.metadata import entry_points else: - from importlib_metadata import entry_points + from importlib_metadata import entry_points # pragma: no cover if sys.version_info >= (3, 11): from typing import Unpack else: - from typing_extensions import Unpack + from typing_extensions import Unpack # pragma: no cover # registry of inverters REGISTRY: Set[Type[Inverter]] = { @@ -38,7 +38,7 @@ class DiscoveryKeywords(TypedDict, total=False): if sys.version_info >= (3, 9): _InverterTask = Task[Inverter] else: - _InverterTask = Task + _InverterTask = Task # pragma: no cover class _DiscoveryHttpClient: diff --git a/solax/inverter_http_client.py b/solax/inverter_http_client.py index bc6ed81..8a83e47 100644 --- a/solax/inverter_http_client.py +++ b/solax/inverter_http_client.py @@ -11,7 +11,7 @@ __all__ = ("InverterHttpClient", "Method") -if sys.version_info >= (3, 10): +if sys.version_info >= (3, 10): # pragma: no branch from dataclasses import KW_ONLY @@ -26,7 +26,7 @@ class Method(Enum): _kwargs: Dict[str, bool] = {} -if sys.version_info >= (3, 11): +if sys.version_info >= (3, 11): # pragma: no branch _kwargs["slots"] = True _kwargs["weakref_slot"] = True @@ -35,7 +35,7 @@ class Method(Enum): class InverterHttpClient: """Initialize the Http client.""" - if sys.version_info >= (3, 10): + if sys.version_info >= (3, 10): # pragma: no branch _: KW_ONLY url: str diff --git a/solax/response_parser.py b/solax/response_parser.py index ec97755..cd77c9d 100644 --- a/solax/response_parser.py +++ b/solax/response_parser.py @@ -16,7 +16,7 @@ if sys.version_info >= (3, 11): from typing import Unpack else: - from typing_extensions import Unpack + from typing_extensions import Unpack # pragma: no cover _LOGGER = logging.getLogger(__name__) _LOGGER.setLevel(logging.INFO) @@ -46,7 +46,7 @@ def serial_number(self): _KEY_TYPE = "type" -GenericResponseSchema = vol.All( +GENERIC_RESPONSE_SCHEMA = vol.All( vol.Schema({vol.Required(_KEY_SERIAL): str}, extra=vol.ALLOW_EXTRA), vol.Any( vol.Schema({vol.Required(_KEY_VERSION): str}, extra=vol.ALLOW_EXTRA), @@ -77,7 +77,7 @@ def __init__( dongle_serial_number_getter: Callable[[Dict[str, Any]], Optional[str]], inverter_serial_number_getter: Callable[[Dict[str, Any]], Optional[str]], ) -> None: - self.schema = vol.And(GenericResponseSchema, schema) + self.schema = vol.And(GENERIC_RESPONSE_SCHEMA, schema) self.response_decoder = decoder self.dongle_serial_number_getter = dongle_serial_number_getter self.inverter_serial_number_getter = inverter_serial_number_getter From bce8bc2d046630dfcba742e08663d433df716268 Mon Sep 17 00:00:00 2001 From: Khaled Eldoheiri Date: Fri, 10 Jul 2026 18:37:23 +0200 Subject: [PATCH 4/7] test: Schema collisions 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 --- tests/test_schema_ambiguity.py | 86 ++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 tests/test_schema_ambiguity.py diff --git a/tests/test_schema_ambiguity.py b/tests/test_schema_ambiguity.py new file mode 100644 index 0000000..4e0f282 --- /dev/null +++ b/tests/test_schema_ambiguity.py @@ -0,0 +1,86 @@ +""" +Checks that no two registered inverters accept the same response. + +Also runnable standalone to print inverter schemas ranked from most to +least permissive: + + python -m tests.test_schema_ambiguity +""" + +from collections import defaultdict + +import pytest +import voluptuous as vol +from voluptuous import Invalid, MultipleInvalid + +from solax.discovery import REGISTRY +from solax.response_parser import GENERIC_RESPONSE_SCHEMA +from tests import fixtures + + +def _matching_inverters(response): + normalized = {key.lower(): value for key, value in response.items()} + matches = set() + for inverter_class in REGISTRY: + combined_schema = vol.And(GENERIC_RESPONSE_SCHEMA, inverter_class.schema()) + try: + combined_schema(dict(normalized)) + except (Invalid, MultipleInvalid): + continue + matches.add(inverter_class) + return matches + + +def _permissiveness_scores(): + """ + Count, for each inverter class, how many fixture responses (belonging + to any inverter) its schema accepts. A schema that accepts more + unrelated responses is more permissive than one that accepts fewer. + """ + scores = defaultdict(int) + for case in fixtures.INVERTERS_UNDER_TEST: + for inverter_class in _matching_inverters(case.response): + scores[inverter_class] += 1 + return scores + + +_PERMISSIVENESS_SCORES = _permissiveness_scores() + + +def _most_permissive_first(inverter_classes): + return sorted( + inverter_classes, + key=lambda c: (-_PERMISSIVENESS_SCORES[c], c.__name__), + ) + + +@pytest.mark.parametrize( + "case", + fixtures.INVERTERS_UNDER_TEST, + ids=[ + f"{i}-{case.inverter.__name__}" + for i, case in enumerate(fixtures.INVERTERS_UNDER_TEST) + ], +) +def test_response_matches_exactly_one_inverter_schema(case): + matches = _matching_inverters(case.response) + extra = matches - {case.inverter} + assert matches == {case.inverter}, ( + f"expected only {case.inverter.__name__} to accept this response, " + "but it also validated against (most to least permissive): " + f"{[c.__name__ for c in _most_permissive_first(extra)]}" + ) + + +def _print_permissiveness_ranking(): + ambiguous = [c for c in REGISTRY if _PERMISSIVENESS_SCORES[c] > 1] + ordered = _most_permissive_first(ambiguous) + width = max(len(c.__name__) for c in ordered) + for rank, inverter_class in enumerate(ordered, start=1): + name = inverter_class.__name__ + matches = _PERMISSIVENESS_SCORES[inverter_class] + print(f"{rank:>2}. {name:<{width}} matches={matches}") + + +if __name__ == "__main__": + _print_permissiveness_ranking() From 5f1c2174c4d5d3c2635487b8df3e5f668e0dc06b Mon Sep 17 00:00:00 2001 From: Khaled Eldoheiri Date: Sat, 11 Jul 2026 12:11:50 +0200 Subject: [PATCH 5/7] fix: inverter discovery prefers least premissive schema 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). --- solax/discovery.py | 79 +++++++++++++++++++++++++++++----- tests/test_schema_ambiguity.py | 22 +++++++++- 2 files changed, 89 insertions(+), 12 deletions(-) diff --git a/solax/discovery.py b/solax/discovery.py index 2d553e3..8357188 100644 --- a/solax/discovery.py +++ b/solax/discovery.py @@ -3,7 +3,7 @@ import sys from asyncio import Future, Task from collections import defaultdict -from typing import Dict, Literal, Sequence, Set, Type, TypedDict, Union, cast +from typing import Dict, Literal, Sequence, Set, Tuple, Type, TypedDict, Union, cast from solax.inverter import Inverter from solax.inverter_http_client import InverterHttpClient @@ -20,13 +20,63 @@ else: from typing_extensions import Unpack # pragma: no cover -# registry of inverters -REGISTRY: Set[Type[Inverter]] = { - ep.load() - for ep in entry_points(group="solax.inverter") - if issubclass(ep.load(), Inverter) +# Ranks inverter schemas from most-specific (least permissive) to +# least-specific (most permissive). REGISTRY is sorted by this table so +# that discover()'s tie-break (lowest rank wins) prefers the more +# tightly matching inverter when several schemas validate the same +# response. +# +# This is NOT derived from entry_points.txt order: setuptools +# alphabetizes entry points on install , so that ordering can't be +# relied on. This table is maintained by hand; drift is caught by +# tests/test_schema_ambiguity.py::test_registry_order_matches_specificity, +# which recomputes the expected order from tests/fixtures.py. +# +# Classes not listed here (e.g. a newly added inverter) rank last -- a +# conservative default that never lets an unranked schema outrank a +# known-specific one. The guardrail test forces new inverters to be +# ranked explicitly rather than silently relying on this default. +_SCHEMA_SPECIFICITY_ORDER: Tuple[str, ...] = ( + "X3Ultra", + "X3MicProG2", + "X3EVC", + "X1Mini", + "QVOLTHYBG33P", + "XHybrid", + "X3HybridG4", + "X1Smart", + "X1HybridGen4", + "X1G4Series", + "X1", + "X3V34", + "X3", + "X1MiniV34", + "X1Boost", + "X1LiteLV", +) +_SCHEMA_SPECIFICITY_RANK: Dict[str, int] = { + name: rank for rank, name in enumerate(_SCHEMA_SPECIFICITY_ORDER) } + +def _specificity_rank(inverter_class: Type[Inverter]) -> int: + return _SCHEMA_SPECIFICITY_RANK.get( + inverter_class.__name__, len(_SCHEMA_SPECIFICITY_ORDER) + ) + + +# registry of inverters +REGISTRY: Tuple[Type[Inverter], ...] = tuple( + sorted( + dict.fromkeys( + loaded + for ep in entry_points(group="solax.inverter") + if issubclass(loaded := ep.load(), Inverter) + ), + key=_specificity_rank, + ) +) + logging.basicConfig(level=logging.INFO) @@ -73,6 +123,7 @@ async def _discovery_task(i) -> Inverter: async def discover( host, port, pwd="", **kwargs: Unpack[DiscoveryKeywords] ) -> Union[Inverter, Set[Inverter]]: + # pylint: disable=too-many-locals done: Set[_InverterTask] = set() pending: Set[_InverterTask] = set() failures = set() @@ -80,8 +131,13 @@ async def discover( asyncio.get_running_loop().create_future ) + # rank of the class each task was built from, in the order it was + # offered to discover(); used to break ties deterministically when + # several inverters' schemas match the same response + priority: Dict[_InverterTask, int] = {} + return_when = kwargs.get("return_when", asyncio.FIRST_COMPLETED) - for cls in kwargs.get("inverters", REGISTRY): + for rank, cls in enumerate(kwargs.get("inverters", REGISTRY)): for inverter in cls.build_all_variants(host, port, pwd): inverter.http_client = cast( InverterHttpClient, @@ -90,9 +146,9 @@ async def discover( ), ) - pending.add( - asyncio.create_task(_discovery_task(inverter), name=f"{inverter}") - ) + task = asyncio.create_task(_discovery_task(inverter), name=f"{inverter}") + priority[task] = rank + pending.add(task) if not pending: raise DiscoveryError("No inverters to try to discover") @@ -143,7 +199,8 @@ async def stagger() -> None: if done: logging.info("Discovered inverters: %s", {task.result() for task in done}) if return_when == asyncio.FIRST_COMPLETED: - return await next(iter(done)) + winner = min(done, key=priority.__getitem__) + return await winner return {task.result() for task in done} diff --git a/tests/test_schema_ambiguity.py b/tests/test_schema_ambiguity.py index 4e0f282..81566fe 100644 --- a/tests/test_schema_ambiguity.py +++ b/tests/test_schema_ambiguity.py @@ -35,7 +35,8 @@ def _permissiveness_scores(): """ Count, for each inverter class, how many fixture responses (belonging to any inverter) its schema accepts. A schema that accepts more - unrelated responses is more permissive than one that accepts fewer. + unrelated responses is less specific (more permissive) than one that + accepts fewer. """ scores = defaultdict(int) for case in fixtures.INVERTERS_UNDER_TEST: @@ -72,6 +73,25 @@ def test_response_matches_exactly_one_inverter_schema(case): ) +def test_registry_order_matches_specificity(): + """ + REGISTRY must be ordered least-to-most permissive so discover()'s + tie-break prefers the more specific/correct inverter. This is a + drift guard: if fixtures or schemas change such that the + fixture-computed ranking no longer matches REGISTRY's actual order, + update _SCHEMA_SPECIFICITY_ORDER in solax/discovery.py to match + (rerun `python -m tests.test_schema_ambiguity` and reverse it, or + use _most_permissive_first(REGISTRY) directly). + """ + expected = list(reversed(_most_permissive_first(REGISTRY))) + expected_literal = ",\n".join(f' "{c.__name__}"' for c in expected) + assert REGISTRY == tuple(expected), ( + "solax.discovery._SCHEMA_SPECIFICITY_ORDER has drifted from the " + "fixture-computed permissiveness ranking; update it to:\n" + f"{expected_literal}" + ) + + def _print_permissiveness_ranking(): ambiguous = [c for c in REGISTRY if _PERMISSIVENESS_SCORES[c] > 1] ordered = _most_permissive_first(ambiguous) From 8c5395b5f18fecf018da2eb7a3e1931a18e5cb5a Mon Sep 17 00:00:00 2001 From: Khaled Eldoheiri Date: Sat, 11 Jul 2026 12:22:14 +0200 Subject: [PATCH 6/7] fix: Make the schema ambiguity test allowed to fail I'm opting not to remove it, as a kind of reminder that schemas need more tighting. --- tests/test_schema_ambiguity.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_schema_ambiguity.py b/tests/test_schema_ambiguity.py index 81566fe..b5a263b 100644 --- a/tests/test_schema_ambiguity.py +++ b/tests/test_schema_ambiguity.py @@ -55,6 +55,11 @@ def _most_permissive_first(inverter_classes): ) +@pytest.mark.xfail( + reason="some inverter schemas are still too permissive; tightening them " + "is tracked separately and shouldn't block CI in the meantime", + strict=False, +) @pytest.mark.parametrize( "case", fixtures.INVERTERS_UNDER_TEST, From bb88f1299db908f54be33614dbe9fb374ae1e352 Mon Sep 17 00:00:00 2001 From: Khaled Eldoheiri Date: Sat, 11 Jul 2026 13:32:17 +0200 Subject: [PATCH 7/7] test: enforce pinning the type to a value in inverters schema 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. --- solax/discovery.py | 2 +- solax/inverters/x1_lite_lv.py | 2 +- solax/inverters/x_hybrid.py | 2 +- tests/test_schema_strictness.py | 60 +++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 tests/test_schema_strictness.py diff --git a/solax/discovery.py b/solax/discovery.py index 8357188..22fd5b8 100644 --- a/solax/discovery.py +++ b/solax/discovery.py @@ -45,6 +45,7 @@ "XHybrid", "X3HybridG4", "X1Smart", + "X1LiteLV", "X1HybridGen4", "X1G4Series", "X1", @@ -52,7 +53,6 @@ "X3", "X1MiniV34", "X1Boost", - "X1LiteLV", ) _SCHEMA_SPECIFICITY_RANK: Dict[str, int] = { name: rank for rank, name in enumerate(_SCHEMA_SPECIFICITY_ORDER) diff --git a/solax/inverters/x1_lite_lv.py b/solax/inverters/x1_lite_lv.py index 5bb0638..20a381d 100644 --- a/solax/inverters/x1_lite_lv.py +++ b/solax/inverters/x1_lite_lv.py @@ -11,7 +11,7 @@ class X1LiteLV(Inverter): # pylint: disable=duplicate-code _schema = vol.Schema( { - vol.Required("type"): int, + vol.Required("type"): vol.All(int, 103), vol.Required("sn"): str, vol.Required("ver"): str, vol.Required("data"): vol.Schema( diff --git a/solax/inverters/x_hybrid.py b/solax/inverters/x_hybrid.py index f2b3b55..35d913f 100644 --- a/solax/inverters/x_hybrid.py +++ b/solax/inverters/x_hybrid.py @@ -16,7 +16,7 @@ class XHybrid(Inverter): { vol.Required("method"): str, vol.Required("version"): str, - vol.Required("type"): str, + vol.Required("type"): vol.All(str, "AL_SE"), vol.Required("sn"): str, vol.Required("data"): vol.Schema( vol.All( diff --git a/tests/test_schema_strictness.py b/tests/test_schema_strictness.py new file mode 100644 index 0000000..43aff29 --- /dev/null +++ b/tests/test_schema_strictness.py @@ -0,0 +1,60 @@ +""" +Structural invariants every inverter schema must satisfy, regardless of +whether responses are currently ambiguous (see test_schema_ambiguity.py): + +* "type" must be pinned to a specific value/pattern, never a bare + int/str type check -- an unconstrained type field is the single + biggest source of accidental cross-inverter matches. +* every length constraint must be exact (min == max), possibly with + several exact alternatives via vol.Any -- never an open range, which + silently accepts lengths nobody has ever observed for that inverter. +""" + +import pytest +import voluptuous as vol + +from solax.discovery import REGISTRY + + +def _iter_length_validators(node): + if isinstance(node, vol.Length): + yield node + elif isinstance(node, vol.Schema): + yield from _iter_length_validators(node.schema) + elif isinstance(node, (vol.All, vol.Any)): + for validator in node.validators: + yield from _iter_length_validators(validator) + elif isinstance(node, list): + for validator in node: + yield from _iter_length_validators(validator) + + +@pytest.mark.parametrize("inverter_class", REGISTRY, ids=lambda c: c.__name__) +def test_type_field_is_pinned(inverter_class): + type_validator = inverter_class.schema().schema["type"] + assert not isinstance(type_validator, type), ( + f"{inverter_class.__name__}'s schema leaves 'type' unconstrained " + f"({type_validator!r}); pin it to the exact literal/pattern seen " + "in tests/samples/responses.py" + ) + + +@pytest.mark.xfail( + reason="some inverter schemas still use length ranges instead of exact " + "lengths; tightening them is tracked separately and shouldn't block CI " + "in the meantime", + strict=False, +) +@pytest.mark.parametrize("inverter_class", REGISTRY, ids=lambda c: c.__name__) +def test_length_constraints_are_exact(inverter_class): + schema_dict = inverter_class.schema().schema + for key in ("data", "information"): + if key not in schema_dict: + continue + for length in _iter_length_validators(schema_dict[key]): + assert length.min == length.max, ( + f"{inverter_class.__name__}'s '{key}' schema accepts a " + f"range of lengths ({length.min}-{length.max}); replace it " + "with vol.Any(vol.Length(x), vol.Length(y), ...) using the " + "exact lengths observed in tests/samples/responses.py" + )