Re-enable wantresponse and other fields in simradio when on the decoded path#10934
Conversation
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (27)
Build artifacts expire on 2026-08-06. Updated for |
github-actions
Bot
added
needs-review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change modifies ChangesSimRadio Decoded Packet Handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/platform/portduino/SimRadio.cpp (1)
255-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLogic looks correct; comment blocks exceed the repo comment-length rule.
The conditional clearing is sound: on the
!carryEncryptedpathperhapsDecode()has already produced a validData, and preservingwant_response/reply_id/bitfield/… lets them survive the sim loopback (downstreamPacketCache::cacheandunpackAndReceive()only replacepayload/portnum), which matches the PR intent. Union aliasing is also respected sincecis a stack local and allp->encryptedreads complete beforepb_encode_to_byteswritesp->decoded.payload.bytes.The two explanatory blocks (Lines 256-259 and 262-265) are 4-line comments. The tradeoff is real here—the union-aliasing rationale is genuinely non-obvious—but consider tightening each to one or two lines to conform.
As per coding guidelines: "Keep code comments minimal: use at most one or two lines, only when the reason is not obvious; do not add multi-paragraph explanatory comments or restate what the next line does."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/portduino/SimRadio.cpp` around lines 255 - 265, The logic in SimRadio::... is fine, but the inline comments around the carryEncrypted branch are too verbose and violate the repo’s comment-length rule. Tighten the two explanatory blocks near p->decoded = meshtastic_Data_init_zero to one or two lines each, keeping only the essential reason about preserving decoded fields on the !carryEncrypted path and clearing the struct on the encrypted path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/platform/portduino/SimRadio.cpp`:
- Around line 255-265: The logic in SimRadio::... is fine, but the inline
comments around the carryEncrypted branch are too verbose and violate the repo’s
comment-length rule. Tighten the two explanatory blocks near p->decoded =
meshtastic_Data_init_zero to one or two lines each, keeping only the essential
reason about preserving decoded fields on the !carryEncrypted path and clearing
the struct on the encrypted path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: be943c82-5502-4aa2-8ca3-f2f622315356
📒 Files selected for processing (1)
src/platform/portduino/SimRadio.cpp
Firmware Size Report23 targets | vs
Show 18 more target(s)
Size budgets
Budgets live in Updated for ff7a707 |
|
I rather suspect that flash usage report isn't accurate given this PR is adding an |
It's totally off. Tom is investigating. Every PR shouldn't be growing 25k 😅 |
2 participants
This was regressed by Copilot due to c92e69d#r3420608957
My machine's assessment suggests that this change was & is correct on the
carryEncryptedbranch added by that PR (#10730) generally, but on the existing side it clears out all the fields on the SIMULATOR_APP packet that are needed -- want_response is the one that mattered for my testing (running traceroutes over a simulated network) but other fields are also cleared by it.Let me know if the machine has been too verbose in the commenting, heh.
Found due to work on meshtastic/python#946
🤝 Attestations
Summary by CodeRabbit