Repository navigation
fix(inverter): also prefer Control.Discharge_Target_SOC_1 when deciding whether to write - #4518
Merged
Merged
Conversation
…ng whether to write adjust_force_export() still decided whether a discharge target write was needed by reading raw.invertor.discharge_target_soc_1 alone - the same slow, self_run-poll-refreshed field #4492 moved away from as the primary signal inside rest_setDischargeTarget() itself. On hardware where that field never catches up, the write looked permanently "needed" and fired every cycle even when nothing had changed (#4517). rest_readDischargeTarget() centralises the same Control-first/raw-fallback read used for write verification, so the caller now correctly skips the write once it's actually landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the inverter REST “should we write?” logic for the discharge target SoC to prefer GivTCP’s synchronous Control.Discharge_Target_SOC_1 signal (with raw.invertor.discharge_target_soc_1 as a fallback), preventing redundant discharge-target writes on cycles where the applied value is already correct.
Changes:
- Add
rest_readDischargeTarget()helper that reads discharge target with a Control-first, raw-fallback preference order. - Update
adjust_force_export()to userest_readDischargeTarget()when deciding whether to writerest_setDischargeTarget(). - Add a regression test covering Control-present, Control-missing/raw-fallback, and neither-present read scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/predbat/inverter.py | Introduces rest_readDischargeTarget() and switches adjust_force_export() to use it before writing discharge target. |
| apps/predbat/tests/test_inverter.py | Adds regression test for the new read-preference helper and wires it into the inverter test suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This was referenced Aug 15, 2026
springfall2008
pushed a commit
that referenced
this pull request
Aug 16, 2026
…nverters (#4517) (#4543) AC Coupled inverters don't have a working Discharge_Target_SOC_1 register - GivTCP still reports each write as successful, but it never persists between cycles, so adjust_force_export() saw a permanent mismatch and rewrote it every single cycle indefinitely. Confirmed against GivEnergy's own firmware archive (github.com/DJBenson/giv-firmware): "AC Coupled" is a single product line with exactly two firmware releases ever published, not an early/late generational split the way the Hybrid line has Gen1/2/3 - so there's no newer AC-coupled variant this would need to keep working for. Skip the write outright when raw.invertor.model reports "Ac", rather than attempting one already known to be doomed. Supersedes the earlier enable-before-write approach on this same issue - that theory relied on a correlation with a second reporter's data that turned out to be from before #4492/#4518 existed and was confounded by the exact race condition those fixes address, so it was never solid evidence that AC Coupled's register works anywhere. Dropped rather than kept alongside this fix, since it has no remaining supporting evidence. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Summary
adjust_force_export()decided whether a write was even needed by readingraw.invertor.discharge_target_soc_1alone - the same slow,self_run-poll-refreshed field fix(inverter): settle before verifying a GivTCP discharge target write #4492 moved away from as the primary signal insiderest_setDischargeTarget()'s own write verification. That earlier fix (Reoccurence of #4404 - Warn: Inverter 0 REST failed to setExportTarget when exporting #4421/fix(inverter): settle before verifying a GivTCP discharge target write #4492) only touched the verification insiderest_setDischargeTarget(), not this caller.rest_readDischargeTarget(), a small shared helper mirroring the same Control-first/raw.invertor-fallback preference order, and switches the caller to use it.rest_setDischargeTarget()'s own internals are left untouched to avoid any risk to its existing fix(inverter): settle before verifying a GivTCP discharge target write #4492 regression tests.Scope - what this does and doesn't fix
This fixes the redundant-write problem for the population #4492 was built for: setups where GivTCP populates
Control.Discharge_Target_SOC_1synchronously at write time (confirmed against GivTCP's own source,write.py'supdateControlCache()). For those, the caller now correctly recognises "already set" once the write has landed, instead of re-writing every cycle.It does not fix every case reported on #4517. @mpartington's own REST response dump shows
Control.Discharge_Target_SOC_1is entirely absent on his (Gen 1 / AC) hardware, so this fix falls straight through to the sameraw.invertor.discharge_target_soc_1fallback the original code used - no change in behaviour for him (also no regression - that fallback path is covered by the new test). Hispredbat.logshows that field genuinely flapping cycle to cycle, which is the original #4421 staleness problem on hardware with no synchronous signal to prefer at all. That needs a separate fix (likely a hardware capability gate - firmware/model based, per his own suggestion) and is being tracked as a follow-up on #4517, not blocked on this PR.Related: #4421 (original spurious-failure report), #4492 (the merged fix for it), #4517 (this follow-up regression, partially addressed here).
Test plan
test_discharge_target_read_prefers_control(Control present, Control missing/raw fallback, neither present)./run_all --test inverter- all tests pass, including the two existing Reoccurence of #4404 - Warn: Inverter 0 REST failed to setExportTarget when exporting #4421/fix(inverter): settle before verifying a GivTCP discharge target write #4492 regression testsrun_pre_commit- ruff, black, cspell, markdownlint all pass🤖 Generated with Claude Code