refactor(export): replace the packed export-limit float with a (mode, target, power) tuple

[chalfontchubby]

Claude wrote this PR (code and description) working with @chalfontchubby.

Why

export_limits_best[n] was a single packed float carrying three orthogonal signals: target SoC in the integer part, export power as 1 - fraction in the decimal part, and mode via two reserved sentinel values (99.0 = freeze, 100.0 = idle). Every consumer re-derived intent by comparing against those sentinels, inconsistently - some == 99, some < 99, some >= 99. That encoding caused real bugs, notably GH#4914: a target of 99% at reduced power lands in the reserved range and reads as "freeze" instead of "export to 99%", leaving the window doing nothing.

This replaces the packed float with a (mode, target, power) tuple, decoded everywhere through named accessors, with the C++ kernel updated to match.

What changed, commit by commit

1. refactor(export): route all export-limit reads through named accessors - adds export_mode_of / export_target_of / export_power_of / export_limit_exports_no_battery / export_limit_is_full_discharge / export_limit_sort_key / pack_export_limit in utils.py, and converts every read site across plan.py, prediction.py, execute.py, output.py and gateway.py to use them. The packed float is still the live representation underneath - this commit only changes how it's read. A pre-commit hook (check_export_limit_encoding.py) rejects new code that compares an export limit against the reserved sentinels or bare 99/100, so the old idiom can't quietly come back.
2. fix(plan): keep the export power out of the SoC a target exports to - a real bug this sweep surfaced: clip_export_slots was reading the raw packed float as the target SoC, so a reduced-power export (e.g. 47.3 = target 47% at 70% power) clipped against 47.3% instead of 47%. Fixed in both the Python and C++ paths, with a regression test.
3. refactor(export): make an export limit a (mode, target, power) tuple - swaps the internal representation. Because every read site already went through an accessor in commit 1, this is a small, mechanical change: the accessors grow an isinstance(tuple) fast path, write sites build tuples via pack_export_limit, and the kernel marshalling wraps values through export_limit_sort_key for ordering.
4. feat(export): persist export limits as a self-describing mapping - the persisted plan (cache and debug dumps) now stores {mode: "target", target: 47, power: 0.7} instead of a bare float. A permanent compatibility path (export_limit_from_stored) reads the old float form, the new mapping, or a bare 3-element list (what a tuple becomes after a YAML/JSON round trip) - so an old cached plan or an old bug-report debug dump keeps loading.
5. refactor(kernel): pass export limits as three field arrays, not one packed double (ABI 6) - the C++ kernel takes export_modes/export_targets/export_powers as three parallel arrays instead of one packed double array, so the reserved-value ambiguity GH#4914 depended on can't exist at the kernel boundary either.
6. perf(kernel): marshal export limits as one array-of-structs, packed with struct (ABI 7) - three parallel arrays meant three separate marshalling passes and three pointers per call; this packs {mode, target, power} into one struct.Struct("@iid")-packed buffer, from_buffer_copy'd into a C array-of-structs, cutting marshalling overhead.
7. perf(kernel): cache the marshalled export-limit buffer by contents - the same export_limits_best list is often marshalled many times across a plan search; this caches the packed buffer keyed by its contents (bounded at 512 entries) so repeated scenarios skip re-marshalling.

Testing

• Full unit suite (./run_all, not just --quick) and ./run_pre_commit pass standalone at every one of the 7 commits, verified with a script that checks out each commit in turn and runs both - not just at the tip.
• run_random (20 fixed scenarios) is bit-identical to main at every commit except commit 2, which is a deliberate behaviour fix (see above).
• Kernel parity and model_kernel tests green throughout; KERNEL_PARITY_REVISION and KERNEL_ABI_VERSION bumped appropriately per commit, all 6 cross-compiled .so targets rebuilt via zig at each ABI change.
• New regression tests added in the commit that fixes each bug: the SoC-clip bug (commit 2), three tuple-shaped export scenarios in test_window.py (commit 3), and round-trip/old-float/plain-YAML persistence tests (commit 4).
• A real bug found deploying to my own running addon: scenario_summary_state() (the verbose debug "STATE:" log line) compared an export-limit tuple directly against a float with >=, a leftover bare-number comparison the accessor sweep in commit 1 missed. It never surfaced in the test suite because that call site only runs behind debug_enable, and only when an active freeze/target export window is below the current SoC - which is exactly what a live plan produced within a few hours of deploying. Fixed and folded back into commit 1 (where the sweep belongs), with a new direct regression test (test_plan_scenario_summary.py) that reproduces it and fails without the fix.
• Replayed my own live debug dumps (--debug_file) against the branch: a fresh dump (already saved in the new mapping format, proving the upgrade path works) replays cleanly; an old dump from before this branch existed (packed-float format) also replays cleanly through the legacy compatibility path, with no exceptions either way.
• Cross-checked the same old dump against v9.0.1 (the version actually running in production) to separate "replay drift" from "branch regression": both versions compute the identical fresh metric (-141.7968) from the same input, confirming the refactor is behaviour-neutral on real data as well as synthetic scenarios.

Speed

This needed more digging than expected, and the honest answer is: no measured regression, but also no confidently measured improvement in production.

• A Mac-local interleaved A/B (run_random-based) measured struct-packing (commit 6) alone at ~+2.9% slower than main, recovered to ~+1.2% net once the buffer cache (commit 7) was added.
• Deployed to my own running Predbat addon (a Pi 4) and compared Plan calculation took log timings, p10 (the uncontended single-pass figure) over an equivalent overnight window: branch measured ~+11% slower than the prior night's v9.0.1 baseline (p10 2.91s vs 2.63s), consistent across every percentile - too large and too consistent to be noise.
• To isolate the cause, redeployed at commit 5 (ABI 6 - three parallel arrays, no struct packing, no buffer cache) rather than the tip. Overnight plan_stats.sh comparison for that version is still accumulating samples as of this PR.
• Separately, replaying an old debug dump through both the branch and v9.0.1 interleaved (6 rounds each, alternating) showed no measurable difference - branch and v9.0.1 medians were within noise of each other (~0.27-0.28s).

So: real production plans on the full 7-commit branch showed a slowdown the synthetic Mac benchmark didn't predict; a single-scenario interleaved replay shows nothing. That's not fully reconciled yet. Flagging it rather than claiming a number I can't stand behind - happy to keep investigating post-merge, or to hold commits 6/7 back if reviewers would rather land the correctness fix (commits 1-5) first and chase the last bit of kernel perf separately.

Known follow-ups

• Reconcile the Mac-vs-Pi kernel performance discrepancy (see Speed above) - which of ABI 6, ABI 7's struct packing, or the buffer cache is actually responsible, under real traffic rather than the synthetic run_random corpus.
• self.export_limits_best is not defensively coerced on load if a hot-reload or partial restart leaves stale data in memory (as opposed to the on-disk formats, which are all handled) - a clean process restart always fixes this via load_saved_plan, so it's a narrow window, but worth hardening if it recurs.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

[Copilot]

🟡 Changes recommended

Unresolved critical persistence and deserialization findings remain, along with additional correctness fixes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR replaces packed export-limit floats with explicit (mode, target, power) tuples across planning, persistence, execution, and Python/C++ kernel paths.

Changes:

• Adds tuple accessors, persistence compatibility, and static encoding checks.
• Updates kernel ABI 7 marshalling and buffer caching.
• Adds regression coverage for planning, persistence, execution, and replay.

File summaries

File	Reviewed changes / final review note
tools/test_check_export_limit_encoding.py	Tests export-limit encoding validation.
tools/check_export_limit_encoding.py	Adds static validation; moderate (3 votes): cover reversed bare-number comparisons.
apps/predbat/utils.py	Provides tuple accessors and persistence helpers; moderate (2 votes): validate malformed mappings and fall back to idle.
apps/predbat/userinterface.py	Handles debug YAML serialization; critical (1 vote): normalize nested preclip export limits on load.
apps/predbat/unit_test.py	Updates test registration and infrastructure.
apps/predbat/tests/test_window.py	Adds tuple-shaped window scenarios.
apps/predbat/tests/test_trim_export.py	Updates export trimming tests.
apps/predbat/tests/test_single_debug.py	Updates debug replay coverage.
apps/predbat/tests/test_prune_dead_slots.py	Updates dead-slot pruning tests.
apps/predbat/tests/test_prediction_batch.py	Covers batched tuple marshalling.
apps/predbat/tests/test_plan_scenario_summary.py	Covers tuple scenario summaries.
apps/predbat/tests/test_plan_persistence.py	Tests saved-plan restoration and compatibility.
apps/predbat/tests/test_optimise_swap_export.py	Tests export-window swapping.
apps/predbat/tests/test_optimise_solar.py	Updates solar optimization tests.
apps/predbat/tests/test_optimise_levels.py	Updates optimization-level tests.
apps/predbat/tests/test_manual_overrides.py	Updates tuple-based override tests.
apps/predbat/tests/test_kernel_static_cache.py	Tests kernel buffer caching.
apps/predbat/tests/test_kernel_parity.py	Tests Python/kernel parity.
apps/predbat/tests/test_infra.py	Updates test fixtures and helpers.
apps/predbat/tests/test_hit_charge_cache.py	Updates cache scenarios.
apps/predbat/tests/test_export_encoding.py	Tests tuple and legacy-format compatibility.
apps/predbat/tests/test_export_commitment.py	Tests export commitments.
apps/predbat/tests/test_execute.py	Updates execution behavior tests.
apps/predbat/tests/test_clip_export_slots.py	Tests target clipping.
apps/predbat/tests/test_calculate_yesterday.py	Covers historical-plan reconstruction.
apps/predbat/prediction.py	Uses tuple accessors during simulation.
apps/predbat/prediction_kernel.py	Marshals and caches structured limits; nit (1 vote): update the stale three-array comment.
apps/predbat/prediction_kernel.cpp	Implements ABI 7 structs; nit (1 vote): correct the stale ABI-history note.
apps/predbat/predbat.py	Persists and restores plans; critical (2 votes): normalize nested preclip lists during restoration.
apps/predbat/plan.py	Migrates planning and clipping; moderate (1 vote): use the semantic target accessor instead of the packed sort key.
apps/predbat/output.py	Publishes and persists tuple limits; critical (2 votes): normalize nested saved targets; moderate (1 vote): use the target field for chart series.
apps/predbat/inverter.py	Uses tuple-form export defaults.
apps/predbat/gateway.py	Maps tuple limits to gateway plans.
apps/predbat/execute.py	Consumes tuple accessors for execution decisions.
apps/predbat/enphase.py	Documents deliberate legacy comparisons.
apps/predbat/const.py	Defines export modes and legacy sentinels.
.pre-commit-config.yaml	Registers encoding checks.
.cspell/custom-dictionary-workspace.txt	Adds terminology for the new tooling and tests.

Review details

Suppressed comments (4)

apps/predbat/output.py:2320

• The chart series is supposed to contain the requested SoC percentage, but export_limit_sort_key() includes 1 - power for target limits. A reduced-power 50% export is therefore published as 50.3/50.7% and its kW series is scaled to that inflated target. Read the target field for target mode; retain the legacy sort key only for freeze's sentinel display.

                soc_perc = float(export_limit_sort_key(export_limits[window_n]))

apps/predbat/plan.py:3094

• export_limit_sort_key() deliberately recreates the legacy packed value, including the reduced-power fraction. Storing it as the window's semantic target makes a 50% target at 30% power appear as 50.7% to the plan/status consumers (and to the execute fallback), even though the target field is documented here as a plain percentage. Use export_target_of(limit) for target mode and reserve the sort key for mode-only display sentinels.

            window["target"] = float(export_limit_sort_key(limit))

apps/predbat/prediction_kernel.cpp:50

• This ABI note says the export limits use three parallel arrays, but the current ABI is 7 and PkExportLimit is an array-of-structs below. The stale description makes the ABI history and Python/C++ marshalling contract contradictory; document ABI 6 as the parallel-array transition and ABI 7 as the struct packing change.

// ABI 6: export limits travel as three parallel arrays (mode, target, power) rather than one packed
// double per window. The packed form put the target in the integer part, the export power in the
// fraction and the mode in two reserved whole values, so the kernel had to unpack it by arithmetic -
// and reconstructing the power as 1 - frac round-tripped through binary floating point on the hot
// path. Fields carry the same three signals with no arithmetic and no reserved range, which also
// removes the (99.0, 100.0) interval that read as neither a freeze nor a target (GH#4914).

apps/predbat/prediction_kernel.py:899

• This comment still describes the export limits as three memoised arrays, but limit_arrays() now caches one PkExportLimit buffer. Update it to describe the single marshalled buffer so future maintenance does not assume three arrays or three marshalling passes.

    # The export limits are shared across a fan-out for exactly the same reason the window lists are,
    # so their three arrays are memoised the same way and on the same terms - keyed on identity, with
    # the list retained so an id() cannot be recycled mid-batch.

• Files reviewed: 38/44 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved critical persistence and deserialization findings remain, along with additional correctness fixes....

Automated reply from Copilot agent: addressed in a79a69cc. I applied the remaining review-thread fixes, including target normalization in output.py, semantic target use in plan/chart paths, checker coverage for reversed numeric comparisons, and the stale ABI/marshalling comment updates.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in tuple validation, kernel compatibility, execution, sorting, rendering, and encoding checks.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (11)

Previously missed (1) — in code that hasn't changed since the last review.

apps/predbat/utils.py:1633

• export_mode_of(99.5) deliberately returns TARGET and export_power_of(99.5) returns 0.5, but this legacy compatibility branch still classifies the same value as no-battery because it uses >= 99. The trim pass can therefore treat a legacy 99%-at-reduced-power export as freeze/neutral; use the decoded mode membership test here for both tuples and legacy numbers.

apps/predbat/execute.py:501

• If update_target_values() or a debug replay has populated window["target"] with the tuple instruction, .get("target") returns that tuple and int(target) raises TypeError; the fallback only handles a missing key. Decode whichever value is present with export_target_of(...), as the HTML path does, before converting it.

                        target = self.export_window_best[0].get("target")
                        if target is None:
                            target = export_target_of(self.export_limits_best[0]) or 0
                        self.isExporting_Target = int(target)

apps/predbat/execute.py:531

• The freeze branch has the same tuple-valued target failure as the force-export branch: a present tuple bypasses the None fallback and is passed to int(). Normalize the stored target through export_target_of(...) regardless of whether the window field is present.

                            target = self.export_window_best[0].get("target")
                            if target is None:
                                target = export_target_of(self.export_limits_best[0]) or 0
                            self.isExporting_Target = int(target)

apps/predbat/plan.py:982

• export_limit_sort_key() deliberately preserves the legacy power fraction, so (target=50, power=0.3) sorts as 50.7. Comparing that value with soc_percent_max can label a target that has actually been reached as HldExp (for example, SoC max 50.5%). Compare export_target_of(export_target) for target mode; keep the freeze-mode branch separate.

                if export_limit_sort_key(export_target) >= soc_percent_max:

apps/predbat/tests/test_plan_scenario_summary.py:17

• This new regression function is never imported by unit_test.py or added to TEST_REGISTRY, so ./run_all will not execute the tuple-vs-float debug-path test despite the PR relying on it for coverage. Register the function in the shared runner (as was done for run_export_encoding_tests) so the regression actually runs.
  apps/predbat/utils.py:1748
• A new tuple such as (EXPORT_MODE_TARGET, 99, 0.7) is serialized by export_limit_to_stored() with target: 99, but this validation treats it as malformed and reloads it as idle. That makes the advertised 99% reduced-power export disappear after a plan/debug round trip; allow target 99 and update the corresponding malformed-input test while still rejecting values above the valid target range.
  apps/predbat/utils.py:1744
• A truncated mapping such as {"mode": "target"} is treated as a target-0% export because the missing target defaults to zero. That is a destructive command, not a safe malformed-input fallback, and contradicts the docstring's promise that malformed limits become idle; require the target field for target mode or return idle when it is absent.
  apps/predbat/utils.py:1753
• The legacy-float fallback does not catch OverflowError. A YAML/JSON value such as -inf converts to float('-inf'), reaches int(packed) in unpack_export_limit(), and raises instead of becoming idle, contradicting the stated malformed-input behavior and aborting a debug replay/load.
  apps/predbat/utils.py:1661
• The sort key still places freeze at 99.0, but a valid tuple target of 99% at reduced power sorts to 99.3 (or higher). Thus freeze is considered a deeper limit than that target, contradicting the docstring's requirement that no-battery modes sort above every target and causing the trim pass's n_best_soc >= current guard to reject a reduction to freeze. The ordering needs an explicit mode rank or a key above the entire target range.
  tools/check_export_limit_encoding.py:31
• The checker skips the entire prediction_kernel.cpp file even though ABI 7 no longer contains a legitimate packed-float decoder there. Any future C++ consumer that reintroduces a comparison against the old 99/100 encoding will therefore bypass the hook's stated protection; remove this broad exemption or make the exemption line-scoped to only intentional compatibility code.
  tools/check_export_limit_encoding.py:84
• The checker searches only one physical line at a time (stripped), so a parenthesized comparison split across lines can evade both regexes, e.g. export_limits[n] >= followed by 99. That leaves the promised guard bypassable; scan logical/tokenized comparisons and add a multiline regression case.

• Files reviewed: 38/44 changed files
• Comments generated: 3
• Review effort level: Lite

[chalfontchubby]

Claude here. Rewrote history to fold the review fixes into the commits that introduced each bug, rather than stacking them on top - so the branch stays bisectable and every commit still passes the full suite standalone. The earlier automated replies (55b3e2d6, c05b0e79, 1be5cde6, a79a69cc) point to commits that no longer exist after the rewrite, but the fixes themselves are all still present.

One of a79a69cc's own fixes (output.py's stored_target handling) turned out to be wrong when folded back in - it broke the ordinary case while fixing the legacy one, caught by run_random before push. Fixed properly and documented in the amended commit message.

Also fixed two more gaps the same review flagged that hadn't been addressed yet: the kernel marshaller trusting every element was already a tuple (prediction_kernel.py), and the 3-element sequence branch of export_limit_from_stored being unvalidated (utils.py).

[Copilot]

🟡 Changes recommended

Valid 99% targets are lost during persistence, and remaining packed-key comparisons still misclassify reduced-power targets.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

apps/predbat/utils.py:1745

• A 99% target is the regression case this PR is meant to make representable, but this validation converts every persisted {mode: "target", target: 99, ...} back to idle. Thus a valid 99%-at-reduced-power plan works only until it is saved/reloaded (and the round-trip test excludes 99). Allow target 99 and update the malformed-input/round-trip cases accordingly.

• Files reviewed: 38/44 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Malformed persisted values can produce unsafe export instructions, and Enphase still conflates a valid 99% target with freeze mode.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

apps/predbat/utils.py:1874

• A target mapping with either field missing is treated as a valid 0%/full-power export. For example, a truncated {"mode": "target"} cache entry reloads as a command to discharge fully, contradicting the documented idle fallback for malformed data. Require both target-mode fields before decoding so incomplete persisted data fails safe.
  apps/predbat/utils.py:1850
• This range check accepts NaN power because both comparisons with NaN are false. A hand-edited/corrupt stored value such as power: .nan therefore reaches prediction and execution as a NaN discharge multiplier instead of taking the promised idle fallback. Express the range as a positive bounded predicate so non-finite NaN is rejected.

• Files reviewed: 38/44 changed files
• Comments generated: 2
• Review effort level: Balanced