Skip to content

fix(inverter): retry a service call the HA layer rejected instead of deduplicating it away - #4878

Open
chalfontchubby wants to merge 1 commit into
mainfrom
fix/service-call-retry-on-failure
Open

fix(inverter): retry a service call the HA layer rejected instead of deduplicating it away#4878
chalfontchubby wants to merge 1 commit into
mainfrom
fix/service-call-retry-on-failure

Conversation

@chalfontchubby

Copy link
Copy Markdown
Collaborator

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:

else:
    # Record the last service called
    self.base.last_service_hash[hash_index] = this_service_hash   # recorded up front
    service_repeat = False
...
    self.base.call_service_wrapper(service_name, **service_data)  # result discarded

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

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: True on a service template already achieves manually (documented in docs/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_limit path 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:

if not self.call_service_template("charge_freeze_service", ...):
    self.call_service_template("charge_stop_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_wrapper returns None on an explicit websocket failure or a two-minute timeout, and the standalone loopback branch returns False when the service is not in EVENT_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 no write_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_retry covering: 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:

ERROR: a rejected call was recorded as done - the next cycle will skip it
ERROR: the call was not retried after being rejected - this is the #4876 hang
ERROR: the hash from the previous accepted call survived a rejection

./run_all --quick and ./run_all --test debug_cases both pass.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

charge_limit/program_point writes via SK driver show same "verification discarded" pattern as #4845

1 participant