Skip to content

fix: preserve requested analog frequency lists - #302

Open
YashAnand69 wants to merge 1 commit into
fossasia:mainfrom
YashAnand69:fix/waveform-frequency-input
Open

YashAnand69 wants to merge 1 commit into
fossasia:mainfrom
YashAnand69:fix/waveform-frequency-input

Conversation

@YashAnand69

@YashAnand69 YashAnand69 commented Oct 6, 2026 •

Copy link
Copy Markdown

Fixes #301.

Analog waveform generation currently overwrites the caller’s requested-frequency list with the rounded device frequencies. That can change saved experiment configuration unexpectedly, and a rejected two-channel request can leave the first entry changed even though no command was sent.

Copy the normalized frequency list before calculating actual values. Return the actual values as before, preserve the original request, and document the input-list behavior. The list has at most two elements; frequency rounding and firmware commands are unchanged.

Validation on macOS/Python 3.12:

  • Unmodified main: four input-preservation regressions fail; one valid scalar/list command-equivalence control passes.
  • All five new tests pass: single-channel SI1/SI2, two different frequencies, an out-of-range second frequency, and identical command calls for scalar versus list requests.
  • New tests plus existing CLI/MQ135/Pico suites: 38 passed.
  • Black, Flake8, Bandit source, pydocstyle source/tests and diff checks pass. Wheel/sdist builds succeed. Sphinx HTML builds with seven existing warnings.

Tests use the real generator with a mocked connection. Physical-device integration and output-signal measurements were not performed. This changes input-list aliasing; callers should use the returned list to read actual frequencies rather than relying on undocumented input mutation. No dependency, signature or protocol change. AI tools assisted implementation and testing; I reviewed the full diff and compatibility cases.

Summary by Sourcery

Preserve requested analog frequency inputs while continuing to return the actual generated frequencies.

Bug Fixes:

  • Preserve caller-provided analog frequency lists during waveform generation, including when validation rejects a request.

Enhancements:

  • Document that requested frequency lists remain unchanged while generated frequencies are returned separately.

Tests:

  • Add regression coverage for single- and dual-channel input preservation, failed generation without partial mutation, and scalar/list command equivalence.

Summary by CodeRabbit

  • Bug Fixes
    • Waveform generation no longer changes the frequency list supplied in a request. The requested values remain intact, while the generated frequencies are still reported separately.
    • Invalid frequency requests leave the request and device commands unchanged. Equivalent scalar and list requests produce consistent results.

@sourcery-ai

sourcery-ai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

The PR prevents analog waveform generation from mutating caller-provided frequency lists by copying normalized inputs before calculation, while preserving existing rounding, return values, and firmware commands. Documentation and focused tests cover successful requests, rejected requests, and scalar/list compatibility.

Sequence diagram for preserving requested waveform frequencies

sequenceDiagram
    participant Caller
    participant Generator as waveform_generator.generate
    participant Connection as MockedConnection

    Caller->>Generator: generate(channel, frequency)
    Generator->>Generator: _listify(channels, 2, frequency)
    Generator->>Generator: frequency.copy()
    Generator->>Generator: Calculate rounded actual frequencies
    alt valid request
        Generator->>Connection: Send unchanged firmware command
        Generator-->>Caller: Return actual frequencies
    else rejected request
        Generator-->>Caller: Reject request
    end
    Note over Caller,Generator: Caller frequency list remains unchanged
Loading

File-Level Changes

Change Details Files
Preserve caller-owned frequency lists while continuing to return device-rounded frequencies.
  • Copy the normalized frequency list before per-channel rounding and validation.
  • Keep generated-frequency return values, device programming, and scalar/list command behavior unchanged.
  • Document that input lists are not modified and returned values represent actual frequencies.
pslab/instrument/waveform_generator.py
Add regression coverage for input preservation, failure atomicity, and compatibility.
  • Verify single- and dual-channel requests remain unchanged while returned values reflect generated frequencies.
  • Verify rejected dual-channel requests neither mutate input nor send device commands.
  • Verify equivalent scalar and identical-frequency list requests produce the same commands and results.
tests/test_waveform_frequency_input.py

Assessment against linked issues

Issue Objective Addressed Explanation
#301 Preserve the caller's requested-frequency list without overwriting or partially mutating it, including when generation validation fails. ✅
#301 Continue returning the rounded actual generated frequencies separately while preserving the existing frequency-rounding behavior and valid firmware command sequence. ✅
#301 Document the input-list preservation behavior and add regression coverage for single-channel, two-channel, failed, and scalar/list-equivalent requests. ✅

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ddd7a8f9-6231-44fc-80ea-2b51f1c0c78f
📥 Commits

Reviewing files that changed from the base of the PR and between 08de0d0 and 9278f24.

📒 Files selected for processing (2)
  • pslab/instrument/waveform_generator.py
  • tests/test_waveform_frequency_input.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

WaveformGenerator.generate now preserves caller-provided frequency lists while returning the generated frequencies separately. Tests cover single- and two-channel requests, invalid frequencies, and equivalent scalar and list inputs.

Changes

Waveform frequency input

Layer / File(s) Summary
Preserve requests and verify generation
pslab/instrument/waveform_generator.py, tests/test_waveform_frequency_input.py
generate copies the frequency list before updating it with generated frequencies. Tests check input preservation, validation failure without device calls, and equivalent scalar and list requests.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 9278f

The requested frequency-preservation behavior appears covered, with no identified issue requiring a fix before merge.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 9278f

The change preserves caller-owned requests without changing device commands, validation, or device access. Callers that relied on input-list mutation must use the returned frequencies instead. No material security risk was found in the inspected change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For the supported numeric-input contract, the changed behavior affects ownership of the caller's frequency list. It does not expand the selected device or output-channel scope, introduce another entrypoint, or grant additional device authority.

Trust Boundaries and Controls

  • observed — The same supplied or automatically connected device receives commands. Dimension checks, frequency validation, timing calculations and acknowledgements are unchanged; the new list copy does not bypass these controls.

Resilience and Maintainability Implications

  • observed — Generation remains nontransactional: internal channel state can change before another frequency is rejected or device acknowledgement fails, and shared device access remains unsynchronized. These limitations predate the PR. The copy improves failure containment for caller-owned requests without adding rollback or concurrency guarantees.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: preserving caller-provided analog frequency lists during waveform generation.
Linked Issues check ✅ Passed [ #301 ] WaveformGenerator.generate() copies the normalized frequency list before replacing entries with actual frequencies. It returns actual frequencies separately and documents that the input lis…
Out of Scope Changes check ✅ Passed The reported changes are limited to the analog waveform input handling, its documentation, and regression tests for issue #301. Each change supports the linked issue. No unrelated changes are identifi…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!

Sourcery assessment

Approved.


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analog waveform generation overwrites the requested-frequency list

1 participant