Send BTP acks more frequently; large memory reduction in responder - #554
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The respond change relies on subtle unsafe Pin invariants for async futures that warrant a human sign-off despite appearing sound.
Pull request overview
This PR bundles two independent fixes for rs-matter. First, it corrects the BTP (Bluetooth Transport Protocol) stand-alone ACK cadence: the responder was scheduling ACKs using the 15s receive-side ACK deadline (BTP_CONN_IDLE_TIMEOUT_SECS / 2), which risks the peer tearing down the session during long-running responses (e.g. ConnectNetwork). It introduces a dedicated, smaller BTP_ACK_SEND_TIMEOUT_SECS (2s) for the send side. Second, it reduces the memory footprint of the respond module by pinning the handlers vector in place instead of via pin!, avoiding a duplicate copy of the (large) handlers array in the generated future.
Changes:
- Add
BTP_ACK_SEND_TIMEOUT_SECS = 2and use it as the BTP send-side ACK timeout everywhereBTP_ACK_TIMEOUT_SECSwas previously used (the old constant is retained for documentation with#[allow(dead_code)]). - Replace
pin!(handlers)+map_unchecked_mutwithPin::new_unchecked(handlers.as_mut_slice())to avoid storing the handlers twice in the future. - Document both fixes in
CHANGELOG.md.
File summaries
| File | Description |
|---|---|
rs-matter/src/transport/network/btp/session.rs |
Adds the new BTP_ACK_SEND_TIMEOUT_SECS constant and explanatory docs; keeps the old constant for reference. |
rs-matter/src/transport/network/btp.rs |
Switches the default/reset ACK timeout and import to the new send-side constant. |
rs-matter/src/respond.rs |
Pins the handlers slice in place to halve the future's memory usage; imports Pin. |
CHANGELOG.md |
Documents the BTP ACK and respond memory fixes under a new [Unreleased] section. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
PR #554: Size comparison from b6a708f to 9176001 Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
Subject says it all. Really two unrelated fixes, but since the memory fix in the
respondmodule was a one-liner, I combined it with the BTP timeout fix.EDIT 1: Also back-ported an mDNS fix from this: sysgrok/edge-net#104
EDIT 2: The memory fix in
respond(a port of sysgrok/edge-net#105) might have far-reaching consequences, like completely getting rid of the bump allocator in downstream crates.