[modbus_client] Add component for ad-hoc modbus request/response - #17676
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#17676
components: [modbus, modbus_client]
refresh: 1h(Added by the PR bot) |
|
👋 Hi there! This PR modifies 24 file(s) with codeowners. @leeuwte, @sourabhjaiswal, @ssieb, @martgras, @stegm, @jesserockz, @polyfaces - As codeowner(s) of the affected files, your review would be appreciated! 🙏 Note: Automatic review request may have failed, but you're still welcome to review. |
There was a problem hiding this comment.
📦 Pull Request Size
Hey @exciton, thanks for the contribution! Just a heads up, this PR is on the large side (1114 line changes excluding tests), which makes it harder for maintainers to review.
Smaller, focused PRs tend to be reviewed much faster since they fit into the short gaps between other maintainer work; large ones often have to wait for a rare long uninterrupted block of time. If you can break this up into smaller pieces that can be reviewed independently, it will almost certainly land faster overall.
Before putting more time in, it's also worth popping into #devs on Discord so we can help you scope things and flag anything already in flight.
For more details (including how to split the work up), see: https://developers.esphome.io/c 8000 ontributing/submitting-your-work/#how-to-approach-large-submissions
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #17676 +/- ##
==========================================
+ Coverage 87.13% 87.28% +0.15%
==========================================
Files 64 64
Lines 14697 14697
Branches 2217 2217
==========================================
+ Hits 12806 12829 +23
+ Misses 1578 1558 -20
+ Partials 313 310 -3 🚀 New features to boost your workflow:
|
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 merged configuration with 2 components. |
ba73248 to
784163f
Compare
|
|
There was a problem hiding this comment.
Warning
Important issues found.
- Empty PDU still exits play() with no outcome trigger
… structure An empty lambda PDU now resolves via on_not_sent instead of vanishing (the hub already refuses it with a warning); pdu lists must be 1-253 bytes at validation, with the limit shared from the modbus package. The on_error override moves into ClientActionBase beside the trigger register_client_action() wires for every action, and the retry function drops its redundant optional wrapper. The test retry lambdas now bound their retries with a counter.
|
|
There was a problem hiding this comment.
Warning
Important issues found.
- Deferring actions inside the reply handlers make the PDU spans dangle (use-after-free read)
…etry counter The request/response spans are only valid while a handler runs, and DelayAction captures trigger args by value for later replay, so delay/wait_until/script.wait inside any handler now fail validation (same guard the api component uses). The combined-retry example resets its counter on success so the cap applies per transaction.
| /// Re-firing an action while its identical frame is still pending follows the hub's dedup rules: a | ||
| /// duplicate read is absorbed into the pending transaction (one reply serves both), a duplicate write is | ||
| /// dropped and resolves via on_not_sent. | ||
| template<typename... Ts> class ClientActionBase : public Action<Ts...>, public modbus::ModbusClientDevice { |
There was a problem hiding this comment.
esphbot: be sure to check for simplification opportunities
https://developers.esphome.io/architecture/components/automations/
There was a problem hiding this comment.
Read the automations doc. Checked both options here.
build_callback_automation targets components exposing an add_on_*_callback template over a callback manager. These outcomes arrive as virtual overrides on modbus::ModbusClientDevice (on_sent/on_response/on_error/on_no_response/on_not_sent), not callbacks. No manager to register into.
Cost also favours triggers. Trigger holds one automation_parent_ pointer — 4 bytes (automation.h:482). LazyCallbackManager also 4; CallbackManager 12. So a forwarder saves nothing and adds a registration path. Five triggers = 20 bytes per send action.
One real simplification exists: play_complex (line 68) overrides only to stamp target_address_. Fold that into ModbusClientSendAction::play() and drop a vtable slot. Stated rationale — subclasses cannot forget — pays off only once sibling actions land in #17467 / #17676.
Separate simplification worth taking now: _synchronous_handler sits inside cv.Any on on_no_response, which swallows its message. Hoisting it outside removes a nested cv.All. Detail in the inline finding.
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
- Hoist the deferring-actions guard outside the on_no_response cv.Any: inside it, the lambda branch's error always won and the real message never showed. - Note that a lambda-built PDU over MAX_PDU_SIZE is silently truncated. - Drop the incorrect "no reply" claim from the target_address comment; a broadcast still resolves through on_no_response. - Reset the retry counter in the first common.yaml example so its cap is per transaction rather than per device lifetime. - Type-hint register_client_action(). - Add config-validation tests pinning the guard to every handler slot, including deferring actions nested in if:/repeat: blocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Awrm2eoWiMsJzsDUdiyLik
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
Fixed 1&2 |
The retry counter was reset in on_sent, which fires again on every retry, so the cap was never reached and a dead device was retried forever. Reset it before the send instead, and say in the header comment why on_sent is the one place it must not go. Also drop the class comment's duplicate-read claim: a duplicate read is not served by one reply. It increments the pending count and the sweep re-queues the frame, so each pending request gets its own on_response. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Awrm2eoWiMsJzsDUdiyLik
| // Converting constructor from a smaller StaticVector of the same element type | ||
| template<size_t M> StaticVector(const StaticVector<T, M> &other) : StaticVector(other.begin(), other.end()) { | ||
| static_assert(M <= N, "Source StaticVector cannot be larger than the destination"); | ||
| } | ||
|
|
There was a problem hiding this comment.
should get a separate PR since it changes core helpers
Confirmed 1 and 2 fixed. 3: agreed, core scope. Dropped, not re-raising. 4: agreed, deferred. Recorded non-blocking. One new item, same family as 1: |
PR Review — [modbus_client] Add component for ad-hoc modbus request/responseMerge-ready. Everything that mattered from the previous rounds is fixed; what remains is nits, one of them in the example YAML. Specific things done well this round. The I also re-verified the plumbing end to end rather than trusting the diff: One correction to an earlier round of mine: the "failures are invisible without opt-in YAML" concern does not hold. The hub already logs timeouts (
✅ Resolved since last review (2)Previously-flagged issues verified fixed
🟢 Suggestions
1. `combined_retries` is reset only in `on_response`, so a never-answering device permanently exhausts its retry budget
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
This PR is part of a series of fixes for modbus and related components.
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?
Adds a new
modbus_clientaction-only component: ad-hoc modbus request/response (or fire-and-forget) from YAML or a lambda, without hand-rolling amodbus::ModbusClientDevicesubclass in C++.The actions
Nothing is declared: the actions only need a
modbushub withrole: client(the default).Each action instance is its own
modbus::ModbusClientDeviceon the hub, so the hub routes the reply (or its absence) back to the exact action that sent it, by device pointer, with no request-matching table.modbus_client.sendfires an ad-hoc PDU (function code + data) at a templatableaddress; the hub prepends the address and appends the CRC.The
pduis templatable too: a lambda returns a stack-allocatedmodbus::helpers::PduBuffer, either hand-assembled bytes or the result of amodbus::helpers::create_*_pdu()builder.Per-send handlers
Every handler belongs to the send that fired it.
on_sentfires when the frame reaches the wire; then exactly one ofon_response/on_error/on_no_response/on_not_sentdelivers the outcome.on_errorexposes(request, exception_code)— an exception response is fixed-format, so the exception code, not a response PDU, is the useful payload.on_no_response(a timeout) can grant a retry: a returning lambda — standalone, or nested asretry:under athen:automation — returns true to have the hub re-queue the frame.on_not_sent(the frame never reached the wire: a refused duplicate write, a cleared queue, a full tx buffer) is distinguished fromon_no_response, and sends the hub refuses at the door resolve through it as well, so no outcome is silently swallowed.Because replies are matched by action identity rather than by address or request bytes, a templated address cannot mis-route an earlier reply, and overlapping sends do not cross wires.
Handlers must run to completion: deferring actions (
delay,wait_until,script.wait, ...) are rejected at validation, because the request/response spans are only valid while the handler runs.An action stores only its triggers and two pointer-sized templated fields; there is no handler table and nothing allocates after
setup().Supporting changes
esphome/core/helpers.hgains aStaticVectorconverting constructor from a smallerStaticVectorof the same element type (with unit tests), so the fixed-sizecreate_*_pdu()builder results convert toPduBuffer.The
modbushubAUTO_LOADsmodbus_client, so the actions are available whenever a client hub exists; they are registry entries only, and no code is generated unless a config uses one.Covered by an integration test driving a mock UART pair (inline response decode, the timeout path, and the refused-duplicate
on_not_sentpath) plus component compile tests for esp32-idf / esp8266-ard / rp2040-ard.Types of changes
Related issue or feature (if applicable):
Pull request in esphome.io with documentation (if applicable): esphome/esphome.io#7106
Test Environment
Example entry for
config.yaml:Checklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: