feat(awg): add modulation to AWG - #335
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
PR author is not in the allowed authors list. |
|
Claude finished @LizzyC-115's task in 2m 40s —— View job Code review
Overview: Clean, convention-following addition. Findings:
|
|
My only major concern is that the modulation command as currently implemented is not stateful. In other words, a user can send a command to modulate a waveform, but they can't turn it off or turn if back on again after the fact. I think the correct shape here is to have an internally tracked struct for each channel that records the modulation type and its characteristics and to have methods for updating the modulation signal and for turning on/off modulation for a given channel |
|
Added |
|
Decoupled modulate() into enable_modulation() and set_modulation() |
|
@claude review |
|
Claude finished @LizzyC-115's task in 2m 43s —— View job Code review
Overview: Good iteration since the last pass — the previously-flagged mocked-transport gap (ASK, carrier-rejection path) is now fully covered in Findings:
👍'd the still-relevant duplicate- |
| def get_modulation_state(self, channel: int) -> tuple[ModulationType, bool]: | ||
| """Get the modulation type and enabled state currently active on channel, read from the instrument.""" | ||
| _check_channel(channel) | ||
| with self._visa.lock(): | ||
| mod_type = ModulationType(self._visa.query(f":SOUR{channel}:MOD:TYP?").strip()) | ||
| enabled = self._visa.query(f":SOUR{channel}:MOD:STAT?").strip() == "ON" | ||
| return mod_type, enabled | ||
|
|
There was a problem hiding this comment.
Choosing to defer to hardware to query state instead of having a ModulationState class. This was to avoid inconsistencies with software state and hardware state. In the previous iteration, we only used type and enabled fields, so that is what we query for here.
| self._visa.write(f":SOUR{channel}:PSK:PHAS {magnitude}") | ||
| else: | ||
| raise AssertionError(f"unhandled ModulationType {mod_type}") | ||
| self._visa.write(f":SOUR{channel}:MOD:TYP {mod_type.value}") |
There was a problem hiding this comment.
This automatically disables the previous modulation type and enables the new modulation type
There was a problem hiding this comment.
Avoids having a check to disable before setting modulation and allows modulation_enable to be idempotent
| def get_modulation_state(self, channel: int) -> tuple[ModulationType, bool]: | ||
| """Get the modulation type and enabled state currently active on channel, read directly from the instrument.""" | ||
| raise NotImplementedError(f"get_modulation_state is not implemented for {type(self).__name__}") |
There was a problem hiding this comment.
Any reason this shouldn't be split into two separate methods, get_modulation_type and get_modulation_state? Follows the pattern already set by get_waveform and get_output_state, is a cleaner, simpler API, and makes the measurement publishing more straight forward
2 participants
Summary
Adding modulation feature to AWG drivers.
Type of change
fix)feat)feat!/fix!)refactor)docs)chore)Verification
Connection to Tektronix Scope:
Hardware Tests:
Software Tests:
Tests
Checklist
feat(driver): add support for Keysight E36300)Notes for Reviewers
For this implementation, we are decoupling setting modulation features (mod_type, shape, and magnitude) from enabling modulation on a specific channel. This is to avoid having the user reset the channel's modulation in procedural tasks. Therefore, instead of solely modulate(), we use enable_modulation(), set_modulation(), and disable_modulation().
This pattern-matches well with Keysight 33500B Waveform Generator as well. For the Keysight, you can enable/disable modulation with the command
[mod_type]:STAT:ONor[mod_type]:STAT:OFF.