Skip to content

feat(awg): add burst to awg - #367

Draft
LizzyC-115 wants to merge 2 commits into
mainfrom
lizzychanpaibool/add-burst
Draft

feat(awg): add burst to awg#367
LizzyC-115 wants to merge 2 commits into
mainfrom
lizzychanpaibool/add-burst

Conversation

@LizzyC-115

@LizzyC-115 LizzyC-115 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Add burst funtionality into the Rigol DG1022Z AWG driver. Using the same structure as modulation, we use set/get pairs for enable, delay, period, trigger, type, polarity, and ncycles.

Type of change

  • Bug fix (fix)
  • New feature (feat)
  • Breaking change (feat! / fix!)
  • Refactor (refactor)
  • Documentation (docs)
  • Chore / tooling (chore)

Verification

Screenshot 2026-08-06 at 5 37 59 PM Screenshot 2026-08-06 at 5 38 15 PM

Tests

  • Unit tests added or updated
  • Existing tests cover this change
  • No tests — explain why:

Checklist

  • PR title follows Conventional Commits (e.g. feat(driver): add support for Keysight E36300)
  • I have read CONTRIBUTING.md
  • Documentation updated if user-facing behavior changed
  • Code follows the style/conventions of the surrounding code

@mintlify

mintlify Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
instro 🟢 Ready View Preview Aug 6, 2026, 9:39 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @LizzyC-115's task in 4m 26s —— View job


Review complete

  • Gather context (PR diff, changed files, AGENTS.md conventions)
  • Review architecture/ergonomics of the burst API surface
  • Post inline findings

Overview: Solid addition — burst mirrors the existing modulation feature's shape closely (optional AWGDriverBase methods with NotImplementedError defaults, @publish_command/_execute_command/_execute_measurement used consistently with the same rules as existing setters, driver validation ordering matches precedent, docs/method-reference table kept in sync). Two things worth a look:

  1. Missing test coverage at the HAL contract level. tests/unstable/awg/test_awg.py maintains parametrized lists that exercise every optional AWGDriverBase method for (a) NotImplementedError when unimplemented (test_02, lines 84-94) and (b) channel-bounds validation (test_05, lines 197-215) — modulation methods are in both lists. None of the new burst methods (set_burst, burst_enable, set_burst_trigger, set_burst_delay, set_gate_polarity, set_ncycles, set_burst_period, or their getters) were added to either list, so this generic contract coverage doesn't extend to burst even though the Rigol-specific driver tests are thorough. (Couldn't leave this inline — the file isn't touched by this PR so GitHub won't anchor a line comment to it.)
  2. Naming asymmetry: set_gate_polarityget_burst_polarity and set_ncyclesget_burst_ncycles break the setter/getter name symmetry used everywhere else (set_offset/get_offset, set_burst_delay/get_burst_delay, set_burst_period/get_burst_period). Minor, but worth a pass before this API surface stabilizes.

Inline comment posted on rigol_dg1022z.py re: no readback for burst trigger source, despite the PR description stating set/get pairs were added for "trigger."

_check_channel(channel)
return self._visa.query(f":SOUR{channel}:BURS:STAT?").strip() == "ON"

def set_burst_trigger(self, channel: int, source: BurstTriggerSource) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Every other burst property has a set/get pair (enable/state, delay, type, polarity, ncycles, period) — matching the PR description's stated goal of "set/get pairs for ... trigger". There's no get_burst_trigger/readback for trigger source here or in awg.py. If :BURS:TRIG:SOUR? is queryable on this instrument, worth adding for symmetry; if not queryable, worth a one-line note explaining the omission.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the naming conventions to match the other functions with set_gate_/get_gate_


def burst_enable(self, channel: int, enable: bool) -> None:
_check_channel(channel)
self._write_checked(f":SOUR{channel}:BURS:STAT {'ON' if enable else 'OFF'}")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that commands with _write_checked use a helper function that called _check_errors()

@LizzyC-115
LizzyC-115 marked this pull request as ready for review August 10, 2026 21:04
@greptile-apps

greptile-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown

Greptile Summary

Adds burst-mode support to the Rigol DG1022Z AWG driver and exposes it through the public AWG abstraction.

  • Introduces burst type, trigger source, and gate polarity enums.
  • Adds configuration and readback APIs for burst mode, state, triggering, delay, polarity, cycle count, and period.
  • Adds software and hardware coverage and documents the new command and telemetry channels.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issues identified.

The new public APIs follow existing AWG packaging and locking patterns, the Rigol implementation checks device errors and mode restrictions, and the added tests cover the principal command, parsing, validation, and hardware paths.

Important Files Changed

Filename Overview
packages/instro-unstable/instro/unstable/awg/awg.py Extends the base-driver contract and public instrument facade with consistently packaged burst commands and measurements.
packages/instro-unstable/instro/unstable/awg/drivers/rigol_dg1022z.py Implements DG1022Z burst configuration and readback using checked SCPI operations and mode-specific validation.
packages/instro-unstable/instro/unstable/awg/types.py Adds enums representing supported burst modes, trigger sources, and gate polarities.
tests/unstable/awg/rigol/test_rigol_dg1022z_software.py Adds software-level coverage for command serialization, response parsing, and invalid burst inputs.
tests/unstable/awg/rigol/test_rigol_dg1022z_hardware.py Adds hardware coverage for supported carriers, burst modes, state transitions, and trigger restrictions.
docs/guides/instrumentation/awg.mdx Documents the new burst API methods and their command and telemetry descriptors.

Sequence Diagram

sequenceDiagram
    participant User
    participant AWG as InstroAWG
    participant Driver as RigolDG1022Z
    participant Device as DG1022Z
    User->>AWG: Configure burst parameters
    AWG->>Driver: set_burst / set_ncycles / set_burst_period
    Driver->>Device: SCPI burst commands
    Device-->>Driver: Status and readback values
    Driver-->>AWG: Parsed enums, booleans, and numbers
    AWG-->>User: Command or Measurement
Loading

Reviews (1): Last reviewed commit: "update getter tests" | Re-trigger Greptile

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @LizzyC-115's task in 3m 18s —— View job


Code review complete

  • Gather context (diff, AGENTS.md conventions, prior review comments)
  • Review driver conventions / API surface
  • Check test coverage
  • Check docs sync
  • Post findings

Overview: The remaining gaps from the earlier review pass have been addressed well — tests/unstable/awg/test_awg.py now covers all burst methods in both the NotImplementedError contract test and the channel-bounds validation test, and set_gate_polarity/get_gate_polarity naming is now symmetric. Driver-level tests (test_rigol_dg1022z_software.py, +hardware.py) are thorough, including good regression-guard tests for the GATED/INFINITE trigger-source rejections. Docs (awg.mdx) are kept in sync across all three tables. The set_burst guard against bursting a StaticValue carrier is a nice bit of defensive validation consistent with the existing _validate_carrier pattern for modulation.

Two items, both minor:

  • 👍'd the still-open prior inline comment — no get_burst_trigger readback exists yet, despite the PR description stating set/get pairs were added for "trigger." The reply on that thread addressed the naming-symmetry finding but not this one.
  • New inline comment: set_ncycles/get_burst_ncycles (and its awg.py counterparts) still break setter/getter name symmetry — same class of issue as set_gate_polarity/get_gate_polarity, which was already fixed elsewhere in this PR, but this pair wasn't.

No other correctness, convention, or scope issues found. Base-class contract, _write_checked/_check_errors duplication, and transport composition all follow repo conventions.
(branch)

@nominal-io nominal-io deleted a comment from claude Bot Aug 10, 2026
self._check_errors()
return result

def set_ncycles(self, channel: int, n_cycles: int) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Naming asymmetry remains here: set_ncyclesget_burst_ncycles. The set_gate_polarity/get_gate_polarity pair was already fixed for symmetry per the earlier review thread, but this pair (and its AWGDriverBase/InstroAWG counterparts in awg.py) wasn't — worth the same treatment (get_ncycles or set_burst_ncycles) before this API stabilizes.

@LizzyC-115
LizzyC-115 marked this pull request as draft August 10, 2026 21:31
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.

1 participant