Repository navigation
fix: read SPI response words in bytes - #309
YashAnand69 wants to merge 1 commit into
Conversation
Reviewer's GuideFixes SPI response framing by reading exactly the packed response word size before consuming the ACK, preventing oversized reads from consuming acknowledgement bytes. Adds transport-level regressions covering 8-/16-bit scalar and bulk operations, command framing, and sequential transfers without changing the public API. Sequence diagram for SPI response framingsequenceDiagram
participant SPI as SPISlave
participant Device as ConnectionHandler
participant Firmware
SPI->>Device: send_byte(CP.SPI_HEADER)
SPI->>Device: send_byte(command)
SPI->>Device: write(interger_type.pack(data))
Device->>Firmware: transmit command and packed data
Firmware-->>Device: response bytes
Device-->>SPI: read(interger_type.size)
Device-->>SPI: get_ack()
SPI-->>SPI: interger_type.unpack(response)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughSPI transfers now read response words using the selected integer type’s byte size. Tests cover 8-bit and 16-bit operations, framing, and response exhaustion. ChangesSPI response framing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to SPI response reads now follow the selected value’s byte width, while acknowledgements remain separate. The supplied test coverage checks framing and response exhaustion across the supported widths; no actionable merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
_transferpasses an 8- or 16-bit width toConnectionHandler.read, whose argument is a byte count. Firmware returns one or two response bytes followed by an ACK. The oversized read consumes the ACK and fails unpacking instead of completing the transfer.Read the packed integer's byte size, then consume the ACK separately. This fixes scalar and bulk read/write/transfer operations without changing commands or public APIs. It is separate from #275's firmware-v3 command migration; that patch retains this read length.
Validation:
SPISlaveregressions fail on unchanged main. A byte-oriented response stream checks exact response lengths, ACK boundaries, sequential replies and command framing.No overlapping response-length fix found in open issues/PRs. Developed and tested with OpenAI Codex assistance.
Summary by Sourcery
Fix SPI transfers to preserve response words and acknowledgements as separate protocol frames.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests