[modbus] Continuous polling for reads - #12612
Conversation
|
To use the changes from this PR as an external component, add the following to your ESPHome configuration YAML file: external_components:
- source: github://pr#12612
components: [modbus]
refresh: 1h(Added by the PR bot) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #12612 +/- ##
=======================================
Coverage 86.59% 86.60%
=======================================
Files 60 60
Lines 14214 14217 +3
Branches 2146 2147 +1
=======================================
+ Hits 12309 12312 +3
Misses 1594 1594
Partials 311 311 🚀 New features to boost your workflow:
|
465c8b4 to
3bfd20b
Compare
Memory Impact AnalysisComponents:
📊 Component Memory Breakdown
🔍 Symbol-Level Changes (click to expand)Changed Symbols
New Symbols (top 15)
Removed Symbols (top 15)
This analysis runs automatically when components change. Memory usage is measured from a representative test configuration. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Still relevant. Waiting on earlier PRs to merge. |
An anonymous send (device == nullptr, the YAML-lambda path) matching a queued identical frame is now always dropped, never promoted: with no callback there is no lifecycle to absorb the request into, and no owner to route the READ_AGAIN re-run to - previously the promotion happened but the re-run never did, silently swallowing the request. The waiting-slot scan keeps its nullptr exclusion, now documented: a null device IN FLIGHT means a detached shell, not an anonymous send. Both drop branches (anonymous and unabsorbable owned duplicates) now log at DEBUG with the address and function code, so a discarded write leaves evidence. maybe_requeue_completed_() documents its divergence from notify_no_response_() around mid-callback clears (closed by the continuous-polling follow-up), and on_not_sent()'s doc points at the lifecycle contract instead of contradicting it. New tests: AnonymousDuplicateDroppedNotPromoted, ReadAgainOutranksFreshReads (the previously untested middle rung of the priority enum, against a non-empty queue), and FullQueueCompletedRequeueRefusalDeliversNotSent (the completed re-queue's refusal branch, PDU included). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # esphome/components/modbus/modbus.cpp
WritesSendBeforeQueuedReads reads frames through the pdu() accessor instead of hard-coded layout offsets, and the queue-filling comments say why the frames are distinct - on this branch the dedup is real and the distinct start addresses are load-bearing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The three cosmetic fixes from the esphome#17888 review round were pushed as the PR merged and did not make the squash: packed_bit_bytes() moves above the PackedBits doc block so that documentation stays adjacent to its class, mask_trailing_pad_bits() states its whole-PDU precondition (the coil data is always the PDU's tail), and the predicates keep named start_address/quantity locals so the PDU offsets stay self-documenting at the call sites. No behavior change; content already verified by review on esphome#17888. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The last hand-rolled bits-to-bytes ceiling, deferred from esphome#17888 because this file was under churn here - it lands where the churn is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A READ_AGAIN command stands for exactly two accepted requests; the transmit-failure path now delivers on_not_sent() twice, keeping the same books as the clear sweep - previously the absorbed request lost its terminal. TransmitFailureReadAgainDeliversBothNotSent pins it via the FlakyBlockHub gate flip. Comment fixes from the same round: insert_by_priority_() no longer claims a sorted queue (in-place promotion keeps the entry's position; only the insert paths honor priority order), and the queue cap records its pathological worst case (~26 kB of transient heap at 128 distinct max-size frames) for whoever tunes it on ESP8266. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commit resolved both READ_AGAIN lifecycles with two on_not_sent() deliveries, over-generalizing from the clear sweep. A transmit failure is transient, not a cancellation: the failed attempt resolves one request, and the absorbed request still owes a run - the frame re-queues demoted to READ_ONCE via requeue_waiting_frame_(), keeping the same books as the timeout path (including the full-queue refusal that delivers the second on_not_sent). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the device requests a retry, on_no_response() resolved nothing - so a refused re-queue of a READ_AGAIN entry must deliver both pending requests' terminals, not one. Without device_retry the single delivery stays correct: the on_no_response() was the first request's terminal. Composes with the transmit-failure path, which re-queues via device_retry=false. Anonymous duplicate drops split by idempotency class: reads stay at DEBUG, writes and custom function codes warn - the caller registered no device, so the log line is the only place the loss is observable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MASK_WRITE_REGISTER and READ_WRITE_MULTIPLE_REGISTERS mutate registers, so they now take WRITE transmit priority (they were already non-requeueable via the read allow-list). The transmit-failure path re-queues the demoted READ_AGAIN frame BEFORE delivering the failed attempt's on_not_sent(): the popped command is a stack local a mid-callback clear could never detach, but queued first the frame is swept - and resolved - by such a clear like any other queued frame. The dedup comment no longer claims frames are never queued twice outright: the internal re-queue paths insert directly, and an identical handler re-send can momentarily coexist - wire order only, the per-entry request accounting is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Confirmed resolved, all four. Promotion now targets Two residual gaps, both non-blocking:
Also: |
PR Review — [modbus] Continuous polling for readsBoth prior findings are genuinely fixed and the continuous-polling semantics now hold up; five non-blocking follow-ups, none of which gate merge. What's solid:
What to look at (all non-blocking):
One bookkeeping note: the type-of-change boxes mark this "code quality / tests", but it widens the public device API ( ✅ Resolved since last review (5)Previously-flagged issues verified fixed
🟢 Suggestions
1. Converting a READ_AGAIN entry to READ_CONTINUOUS drops the absorbed request's second resolution
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
# Conflicts: # esphome/components/modbus/modbus.cpp
…mantics A send from inside on_response()/on_error() can match the command whose callbacks are running; it now resolves against that command like any other duplicate, so the post-callback re-queue serves it instead of coexisting with a fresh twin. Exception-flagged function codes are never silently re-sendable: is_function_code_read() masks the exception bit, so the requeueable check excludes them explicitly and their duplicates take the drop path. Documented rather than changed, per review discussion: converting a queued entry to continuous SUPERSEDES any request it had absorbed - the caller opted into streaming semantics and the poll's responses are the accounting from then on. The lifecycle contract also lists the mid-completion detach among the silent clear cases, and the stale pre-fix KNOWN LIMITATION comment above the cancel test now describes what the test actually proves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A sweep handler re-sending the in-flight frame was absorbed into a waiting slot the very same clear then detached silently, leaving that request with zero callbacks. Detached first, the dedup's existing nullptr exclusion makes the re-send queue fresh - and, unmarked, it survives the clear like any other reentrant re-send. New coverage from the same round: maybe_requeue_completed_() re-runs a READ_AGAIN after an error response, and 0x16 ranks as WRITE for transmit ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The state descriptor exists only for the log lines, so it takes the same flash-resident LogString form clear_rx_buffer_() uses instead of a RAM-resident const char* literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # esphome/components/modbus/modbus.cpp # tests/components/modbus/modbus_client_hub_test.cpp
|
Round addressed at 5c31e0e (this branch now also carries #17887's current tip, so the resolve_duplicate region here is final-form):
|
Brings in the final 17887 review round (exception-mask requeueability exclusion, cap-comment math, front-insert heap probe, doc nits); the duplicated ExceptionFlaggedDuplicateDroppedNotPromoted test from the earlier backport is collapsed to one copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This PR is part of a series of fixes for modbus and related components.
The features in this branch were fully implemented in #17922 so this PR was closed.
Core modbus architecture:
Overhaul of modbus_controller
New features for server mode
New features for client mode
There are associated tests
What does this implement/fix?
Stacked on #17886 and #17887 — the first commits in the diff belong to those PRs; this PR's own delta is continuous polling (split proposal).
Continuous polling for reads
A continuous read re-queues itself at the lowest priority (
READ_CONTINUOUS) after every successful response, filling idle bus time with polling; writes always outrank it and fresh one-shot reads run first.Failures are not re-queued — the caller's normal update path recovers — and
continuousis ignored for writes.The dedup rules protect the poll: only a
READ_ONCEentry is ever promoted, so a duplicate send never demotes a continuous entry into one that stops after two more runs, and an incoming{.continuous = true}request that matches a queued one-shot turns that entry into the continuous poll.A retried continuous read stays continuous.
Cancelling from inside the response callback
The command under completion is exposed to
clear_tx_queue_for_device()/clear_tx_queue_for_address()for the duration of itson_response()/on_error()callbacks (thecompleting_slot), so "stop polling now" from inside the response callback actually cancels the pending re-queue — previously a documented limitation.Every continuous lifecycle still balances: one terminal callback per accepted command, with each successful re-queue counting as a new command (
ContinuousLifecyclesBalance).This is the base for the upcoming
continuous:option inmodbus_controller(#17677).Types of changes
Related issue or feature (if applicable):
Pull request in esphome.io with documentation (if applicable):
continuous:option arrives with [modbus_controller] Span response path; keep sensor addresses as configured #17677)Test Environment
Example entry for
config.yaml:# No configuration changes.Checklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: