Repository navigation
fix: bound three-channel captures by the physical buffer - #308
Open
YashAnand69 wants to merge 2 commits into
Open
YashAnand69 wants to merge 2 commits into
YashAnand69 wants to merge 2 commits into
Conversation
Reviewer's GuideThe PR fixes three-channel oscilloscope captures by validating against their actual four-channel firmware allocation, reducing their maximum to 2,500 samples per channel while preserving existing limits for other modes. Documentation and focused mock-based regression tests cover rejection timing, boundary capacities, command selection, internal MIC allocation, and returned-channel behavior. Sequence diagram for bounded three-channel oscilloscope capturesequenceDiagram
participant Caller
participant Oscilloscope
participant Firmware
Caller->>Oscilloscope: capture(channels, samples, timegap)
Oscilloscope->>Oscilloscope: _check_args(channels, samples, timegap)
alt channels == 3
Oscilloscope->>Oscilloscope: capture_channels = 4
alt samples > 2500
Oscilloscope-->>Caller: ValueError
else samples <= 2500
Oscilloscope->>Firmware: OSCILLOSCOPE_CaptureFour(samples, ...)
Firmware-->>Oscilloscope: CH1, CH2, CH3, MIC data
Oscilloscope-->>Caller: CH1, CH2, CH3 data
end
else channels == 1, 2, or 4
Oscilloscope->>Firmware: capture command for requested channels
Firmware-->>Oscilloscope: captured data
Oscilloscope-->>Caller: requested channel data
end
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 (4)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThree-channel oscilloscope captures now use the four-channel buffer-capacity limit. The documentation describes the 2,500-sample limit and internal MIC sampling. CLI sizing and hardware-free tests cover channel capacities and capture results. ChangesOscilloscope capture capacity
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The capture and CLI limits are aligned, and no merge-blocking issue remains from the reviewed evidence. This change is ready for normal merge checks. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change reduces oversized capture admission and rejects it before instrument-state changes or device writes. The reviewed paths introduce no material security risk or additional device authority. Physical hardware behavior was not validated. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pslab/instrument/oscilloscope.py" line_range="165" />
<code_context>
- max_samples = CP.MAX_SAMPLES // channels
+ # Three-channel captures use CAPTURE_FOUR and discard MIC.
+ capture_channels = 4 if channels == 3 else channels
+ max_samples = CP.MAX_SAMPLES // capture_channels
if not 0 < samples <= max_samples:
e1 = f"Cannot collect more than {max_samples} when sampling from "
</code_context>
<issue_to_address>
**Three-channel CLI captures abort**
When the CLI requests a three-channel batch larger than 2,500 samples, `cli.oscilloscope` sizes batches using `CP.MAX_SAMPLES // channels`, so it can pass up to 3,333 samples to `scope.capture`, which rejects batches above 2,500 with an uncaught `ValueError`. Collection stops instead of returning the requested data.
Make the CLI batch-size calculation use the same effective channel count as `capture` for three-channel requests.
</issue_to_address>
### Comment 2
<location path="pslab/instrument/oscilloscope.py" line_range="165" />
<code_context>
- max_samples = CP.MAX_SAMPLES // channels
+ # Three-channel captures use CAPTURE_FOUR and discard MIC.
+ capture_channels = 4 if channels == 3 else channels
+ max_samples = CP.MAX_SAMPLES // capture_channels
if not 0 < samples <= max_samples:
e1 = f"Cannot collect more than {max_samples} when sampling from "
</code_context>
<issue_to_address>
**Failed captures change trigger state**
When a three-channel request exceeds 2,500 samples and supplies a trigger setting, `capture` applies `trigger=False` or calls `configure_trigger` before `_check_args` rejects the sample count. The failed call can write changed trigger settings to the device, so later captures use an unexpected trigger setting.
Validate the sample count before applying or configuring the requested trigger setting.
</issue_to_address>
### Comment 3
<location path="pslab/instrument/oscilloscope.py" line_range="64-65" />
<code_context>
samples : int
Number of samples to fetch. Maximum 10000 divided by number of
- channels.
+ channels. Three-channel captures also sample MIC internally,
+ so their maximum is 2500 samples per channel.
timegap : float
Time gap between samples in microseconds. Will be rounded to the
</code_context>
<issue_to_address>
**Capture limit documentation conflicts**
The general “10000 divided by number of channels” statement gives a 3,333-sample limit for three channels, contradicting the following 2,500-sample limit and leaving readers unsure which limit applies.
Qualify the general formula to exclude three-channel captures, or state the per-channel limits explicitly.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: pslab/instrument/oscilloscope.py:165, pslab/instrument/oscilloscope.py:165
Author
|
Addressed the three findings in f871f42:
All 54 capacity/CLI/Pico tests pass; changed-file linters, package build and documentation build pass. The existing #298 optimization patch still applies cleanly. No physical hardware testing was performed. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three-channel captures use
CAPTURE_FOURand discard MIC when returning data. The currentMAX_SAMPLES // channelscheck nevertheless allows up to 3,333 samples per channel, exceeding the shared 10,000-sample buffer once the fourth channel is included.Validate against four captured channels for this case, limiting it to 2,500 samples per channel. Use the same limit for CLI batches, reject oversized requests before changing channel or trigger settings, and document the per-channel limits. Keep the one-, two- and four-channel limits unchanged. The Python command path and firmware's
OSCILLOSCOPE_CaptureFourboth confirm four-channel allocation.Validation:
Existing CLI optimization PR #298 applies cleanly alongside these changes; its separate concatenation change is not included.
No physical hardware or firmware integration was tested. No existing issue or overlapping PR found; historical #198 concerned the already-fixed time-gap lookup, not buffer capacity.
AI assistance: developed and tested with OpenAI Codex.
Summary by Sourcery
Enforce physical buffer limits for three-channel oscilloscope captures across the instrument and CLI paths.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit