fix(inverter): retry a service call the HA layer rejected instead of deduplicating it away - #4878
Open
chalfontchubby wants to merge 1 commit into
Open
fix(inverter): retry a service call the HA layer rejected instead of deduplicating it away#4878chalfontchubby wants to merge 1 commit into
chalfontchubby wants to merge 1 commit into
Conversation
call_service_template() recorded its deduplication hash before making any of the calls, and discarded each call's return value. A service that was rejected was therefore remembered as done, and every later cycle carrying the same target logged "Skipped service ... as it was previously called" - so Predbat believed it had set a control it had not, with nothing to correct it until the payload itself changed, which for a held target can be hours. Reported twice now on unrelated hardware. #2903 left a Libbi un-charged for a 3.5 hour cheap-rate window after a failed switch turn-on. #4876 left a Deye sitting in its idle program - confirmed on the physical display - while predbat.status read Charging, until the reporter re-sent the identical select_option by hand and the battery moved within seconds. Record the hash only once every call has been accepted, and drop any earlier record when one is rejected, so the next cycle reissues it naturally. This is what "repeat: True" on a service template already achieves by hand; neither reporter had it set, and needing it is the thing being removed. Deliberately NOT read-back verification. The integrations involved cache writes asynchronously, so re-reading an entity cannot settle whether a value reached the inverter - the same report's charge_limit path shows a readback that trails the write by a whole cycle. This checks only that the service call itself was accepted, which is a fact the HA layer already returns and this code threw away. The return value is left alone at True. Callers use it to choose a fallback service ("if not charge_freeze_service: charge_stop_service"), so reporting a transient failure there would silently downgrade a freeze into a stop instead of retrying it. Related but separate: #4845 covers the same act-on-an-unverified-write class at the ten adjust_* notify/MQTT sites, which announce changes that did not happen; this path has no write_and_poll_value() at all and its failure mode is the opposite - silence, and no retry. Fixes #4876 Co-Authored-By: Claude Opus 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.
Written by Claude, posted on behalf of @chalfontchubby.
Fixes #4876. Originally raised in discussion #4866.
call_service_template()recorded its deduplication hash before making any of the calls, and discarded each call's return value:A rejected call was therefore remembered as done, and every later cycle carrying the same target logged
Skipped service ... as it was previously called. Predbat believed it had set a control it had not, with nothing to correct it until the payload itself changed — which for a held charge target can be hours.Reported twice, on unrelated hardware
switch.myenergi_libbi_charge_from_gridturn-on. Plan was correct throughout (Charging target 7%-100%); the switch call simply never landed and was never retried.predbat.statusreadCharging. The reporter re-sent the identicalselect_optionby hand from Developer Tools and the battery moved from ~-30W to +850W within seconds.What this changes
Record the hash only once every call has been accepted, and drop any earlier record when one is rejected, so the next cycle reissues it naturally.
That is exactly what
repeat: Trueon a service template already achieves manually (documented indocs/inverter-setup.md). Neither reporter had it set, and needing it is the thing being removed.What this deliberately does not do
It is not read-back verification, which is what #4866 originally asked for. As noted in that discussion, the integrations involved cache writes asynchronously, so re-reading an entity cannot settle whether a value actually reached the inverter — the same report's
charge_limitpath shows a readback trailing the write by a full cycle, which is a separate false-failure problem rather than this one.This checks only that the service call itself was accepted. That is a fact the HA layer already returns and this code was throwing away.
The return value is left at
True. Callers use it to choose a fallback service:Reporting a transient failure there would silently downgrade a freeze into a stop instead of retrying it, so the returned value keeps its existing "was a service configured and dispatched" meaning.
Caveat worth a reviewer's eye
call_service_wrapperreturnsNoneon an explicit websocket failure or a two-minute timeout, and the standalone loopback branch returnsFalsewhen the service is not inEVENT_LISTEN_LIST.trigger_callback's docstring states that shared True/False contract is intentional, and Predbat.com does not drive inverters through HA services — but a vendor-specific service called in loopback mode would now look permanently rejected and be reissued each cycle rather than skipped.Relationship to #4845
Adjacent, not overlapping. #4845 covers the same act-on-an-unverified-write class at the ten
adjust_*notify/MQTT sites, whose failure mode is announcing changes that did not happen. This path has nowrite_and_poll_value()at all and fails the opposite way — silently, with no retry. #4876's log shows both halves within one cycle of each other.Testing
New
test_call_service_template_retrycovering: a rejected call is retried next cycle; an accepted call is still deduplicated as before; and a later rejection drops the earlier record rather than leaving it stale. Confirmed it fails without the fix:./run_all --quickand./run_all --test debug_casesboth pass.