Repository navigation
fix(gateway): bound the reserve target to what the inverter accepts - #5170
Merged
Merged
Conversation
Some GivEnergy inverters refuse a battery reserve of 100% (Gen2, Gen3 and AIO are the documented ones — our own inverter-setup docs advise inverter_reserve_max: 98). Nothing on the gateway route knew that: the firmware reported no ceiling and GATEWAY_ATTRIBUTE_TABLE hardcodes "max": 100, so reserve_device_bounds() had nothing to clamp with. Every hold path writes min(soc_percent + 1, 100), so a battery sitting at 100% asks for a reserve of 100. The write is accepted on the wire, the register keeps its old value, and write_and_poll_value reads the mismatch back as a failed write — ten retries, 2s apart, every engine cycle, then record_status(..., had_errors=True). A customer saw "Warn: Inverter 0 write to reserve failed" from before 14:21 until about 17:40 on 17 Sep 2026, with "Trying to write 100 to reserve didn't complete got 4.0" in the log. The battery held correctly throughout — the gateway's own plan entry does the holding — so this was a false error that cost a support ticket and an escalation. Gateway firmware now reports the ceiling per inverter (predbat-gateway#346, PR #349). This publishes it as the reserve entity's "max" attribute in place of the hardcoded 100, which adjust_reserve() already honours through reserve_device_bounds() (GH#4826). A full-battery hold on GivEnergy therefore targets 98 and the write lands first time. - ControlStatus.reserve_soc_max (field 11), mirroring the firmware copy of gateway_status.proto; the two must stay in sync for wire compatibility. - 0 means "not reported" — older gateway firmware — and is read as 100, as is anything outside 1-100. The table entry is copied per inverter, so one inverter's ceiling cannot become another's. Appended field, so both directions stay compatible — verified by round-trip across the old and new generated bindings: new firmware -> old PredBat: parses cleanly, unknown field ignored old firmware -> new PredBat: parses, reserve_soc_max defaults to 0 Regenerated with grpcio-tools and the repo's black hook, keeping the 7.34.1 gencode header the file ships so the protobuf runtime floor does not move: against the unchanged schema the two generators produce a byte-identical body, so the result is what protoc 34.1 emits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The schema, runtime handling, fallback behavior, and per-inverter isolation are consistently implemented and covered by tests.
Review effort: Lite
Findings: None
What changed in this PR
Updates the gateway protobuf and entity publication so PredBat respects each inverter’s reported reserve ceiling.
Changes:
- Adds
reserve_soc_maxto the gateway schema and generated bindings. - Publishes a validated per-inverter reserve
maxattribute. - Adds regression tests for valid, missing, invalid, and isolated bounds.
| File | Description |
|---|---|
apps/predbat/gateway.py |
Publishes per-inverter reserve limits. |
apps/predbat/gateway_status.proto |
Defines the new protobuf field. |
apps/predbat/gateway_status_pb2.py |
Regenerated protobuf bindings. |
apps/predbat/tests/test_gateway.py |
Tests reserve limit publication and fallback behavior. |
Files not reviewed (1)
- apps/predbat/gateway_status_pb2.py: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Pairs with predbat-gateway #349, which fixes predbat-gateway#346. Neither half works alone.
The bug
Some GivEnergy inverters refuse a battery reserve of 100% — Gen2, Gen3 and AIO are the ones our own
docs/inverter-setup.mddocuments, advisinginverter_reserve_max: 98. On the gateway route nothing knew that: the firmware reported no ceiling, andGATEWAY_ATTRIBUTE_TABLEhardcodes"max": 100, soreserve_device_bounds()had nothing to clamp with.Every hold path writes
min(soc_percent + 1, 100), so a battery sitting at 100% asks for a reserve of 100. The write is accepted on the wire, the register keeps its old value, andwrite_and_poll_valuereads the mismatch back as a failed write — ten retries, 2 s apart, every engine cycle, thenrecord_status(..., had_errors=True).A customer saw "Warn: Inverter 0 write to reserve failed" from before 14:21 until about 17:40 on 17 Sep 2026, with
Trying to write 100 to reserve didn't complete got 4.0in the log and the register reading back 4%. The battery held correctly the whole time — the gateway's own plan entry does the holding — so this was a false error that cost a support ticket and an escalation, and it cleared only when the hold ended.The fix
The gateway firmware now reports the ceiling per inverter (98 on the GivEnergy register path, 100 elsewhere). This PR publishes it as the reserve entity's
maxattribute in place of the hardcoded 100:adjust_reserve()already honours that attribute throughreserve_device_bounds()(GH#4826), andset_arg("reserve", reserve_entities)points it at exactly these entities — so a full-battery hold on GivEnergy now targets 98 and the write lands first time. No new config, and nothing changes for an inverter that accepts 100.0means "not reported" — gateway firmware predating the field — and is read as 100, as is anything outside 1-100. The table entry is copied per inverter, so one inverter's ceiling cannot leak into another's.Holding a full battery at 98 rather than 100 changes nothing in practice: the gateway's plan entry is what holds the battery, and the reserve is a floor beneath it.
Schema
ControlStatus.reserve_soc_max(field 11), mirroring the firmware copy ofgateway_status.proto— the two must stay in sync for wire compatibility. Appended, so both directions stay compatible, verified by round-trip across the old and new generated bindings:reserve_soc_maxdefaults to 0gateway_status_pb2.pywas regenerated with grpcio-tools and the repo's black hook, keeping the7.34.1gencode header the file ships so the protobuf runtime floor doesn't move for users. That is safe to assert rather than assume: regenerating the unchanged schema with this toolchain reproduces the checked-in file byte-for-byte apart from that header, so the body here is what protoc 34.1 emits. The diff is the serialized descriptor plus the shifted_serialized_start/_endoffsets — same shape as #4495.Tests
Four new cases in
TestInjectEntities(apps/predbat/tests/test_gateway.py):test_reserve_soc_max_published_as_the_entity_maxmax, and the rest of the table entry survives — fails without this changetest_reserve_soc_max_zero_falls_back_to_100test_reserve_soc_max_out_of_range_falls_back_to_100test_reserve_soc_max_does_not_mutate_the_shared_tablerun_gateway_tests()passes in full (280 cases).black,ruff(the hook's rule set) andcspellare clean on the changed files;interrogatesits where it already did (78.3% -> 78.5% on these two files — this change adds docstrings, it doesn't remove any).🤖 Generated with Claude Code