Repository navigation
fix: separate ADC source offsets from buffer destinations - #304
YashAnand69 wants to merge 1 commit into
Conversation
Reviewer's GuideFixes ADC buffer writes by tracking payload offsets independently from destination addresses, ensuring nonzero-destination and multi-chunk writes transmit all input samples in order; comprehensive mocked command-sequence tests cover offsets, boundaries, immutability, and empty writes. Sequence diagram for corrected ADC buffer writessequenceDiagram
participant Caller
participant BufferMixin
participant Device
Caller->>BufferMixin: fill_buffer(data, starting_position)
loop Each chunk up to 128 samples
BufferMixin->>Device: send_byte(COMMON)
BufferMixin->>Device: send_byte(FILL_BUFFER)
BufferMixin->>Device: send_int(starting_position + offset)
BufferMixin->>Device: send_int(samps)
loop Each sample in data[offset:offset + samps]
BufferMixin->>Device: send_int(value)
end
Device-->>BufferMixin: get_ack()
BufferMixin->>BufferMixin: offset += samps
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
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. 📝 WalkthroughWalkthrough
ChangesADC buffer writes
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change fixes the reported nonzero-offset truncation by writing input values in order to the requested destinations. No actionable merge-blocking risk is established; it appears ready to merge after normal checks. 🚥 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 |
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If the offset calculation is wrong, samples could be written to the wrong positions in the device buffer and later reads could use those incorrect values. Reverting stops further incorrect writes, and the bounded buffer contents can be repaired by rewriting them.
Fixes #303.
fill_buffer(data, starting_position)currently uses the device destination as an index into the input list. For example, writing[7, 8, 9]at position 10 declares three samples but sends none because it slicesdata[10:13]. Multi-chunk writes also skip input values and can declare more payload words than they send.Track the input offset from zero and add the destination only when sending the buffer address. Every chunk then contains its declared number of samples, in order. The firmware’s BUFFER_Fill handler reads exactly that declared count; no firmware or protocol change is needed.
Validation on macOS/Python 3.12:
Physical-device integration was not performed. This restores the documented nonzero-destination behavior; existing zero-destination and empty writes are preserved. No dependency or signature change. AI tools assisted implementation and testing; I reviewed the full diff and checked the firmware’s payload-consumption loop.
Summary by Sourcery
Correct ADC buffer filling to keep source data offsets independent from device destinations.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests