Fix: repeating group length inference must include nested subgroup points - #120
Open
LinusCrugnola wants to merge 2 commits into
Open
LinusCrugnola wants to merge 2 commits into
LinusCrugnola wants to merge 2 commits into
Conversation
hilman2
added a commit
to hilman2/ha-sunspec2
that referenced
this pull request
Sep 1, 2026
…s included (#72) When a repeating group's count point reads 0, pysunspec2 infers the count from the model length: repeating registers divided by the length of one group instance. It divided by points_len, which counts only the group's own points and ignores any repeating subgroup nested inside. Model 705, the volt-var curves, is exactly that shape: each Crv holds its own ten registers plus NPt curve points of two registers each. With two points per curve the real instance is 14 registers, points_len said 10, and the scan raised "Repeating group count not consistent with model length". A device that exposes 705 lost every model behind it in the chain. g.len is the resolved length of one instance, subgroups included, and equals points_len for a group without them, so the change is a no-op for every other model. This is sunspec/pysunspec2#120, unmerged upstream since August; the regression test is theirs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #119
Group._init_repeating_group's legacy count-inference path (used when a repeating group's count can't be read directly from its count point, so the count is derived from total model length instead) divided by g.points_len, which only counts a group's own direct points and ignores any nested repeating subgroup.
This breaks models where a repeating group nests another repeating group, e.g. model 705's Crv group nesting a Pt group for curve points. With 2 curves of 2 points each, each Crv instance is actually 14 registers (10 own + 2×2 from Pt), but points_len reports only 10, so device.scan() raised ModelError: Repeating group count not consistent with model length for model 705, model repeating len = 28, model repeating group len = 10.
g.len is the fully-resolved length of one group instance including nested subgroups, and is the correct divisor. For groups with no nested subgroups g.len == g.points_len, so this is a no-op for the common case.
Added a regression test (test__init_repeating_group_legacy_count_nested_group) that reproduces the exact reported error against the old logic and passes against the fix.