Fix SSM module typos, enum mismatches, and stream robustness#15
Fix SSM module typos, enum mismatches, and stream robustness#15jfichtner wants to merge 4 commits into
Conversation
- Fix misspelled `cherk_errors` keyword argument to `check_errors` in 6 locations across ssm_source_module.py and ssm_measure_module.py. The typo was silently ignored, leaving error checking enabled when the intent was to suppress it. - Fix set_resistance_mode() enum isinstance check: was checking ResistanceExcitationType instead of ResistanceMode. - Reconcile DataSourceMnemonic enum with data_source_types dict: add SSWeeping entry to data_source_types and SOURCE_IS_SETTLING member to DataSourceMnemonic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add try/finally to stream_data to send TRACe:STOp on generator abandonment or exception, ensuring instrument-side cleanup. - Add inner break when num_points reached to prevent yielding excess rows from a batch. - Validate row length before struct.unpack to skip incomplete rows from non-evenly-divisible byte buffers. - Add 10ms sleep backoff in polling loop to prevent busy-looping and flooding the instrument with queries. - Add length validation before datetime() construction in get_head_cal_datetime and get_head_self_cal_datetime to raise clear errors on malformed responses. - Handle wakepy import failure gracefully with a no-op context manager fallback, preventing NameError when wakepy is unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ordering - Remove ValueError/OverflowError wrapping in datetime validation methods to preserve backward compatibility for callers catching those exception types. The length validation (XIPInstrumentException) is kept since it replaces a less informative IndexError. - Document the lock ordering invariant (stream_lock before dut_lock) to prevent future deadlock regressions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add logging (DEBUG level) to stream_data cleanup instead of silently swallowing exceptions, and log skipped incomplete rows. - Wakepy no-op fallback now yields a mock Mode object with success=False for forward compatibility with code that inspects the yielded value. - Add docstring note documenting that TRACe:STOp is sent on generator exit and that the overflow check is skipped on early abandonment. - Add inline comments explaining the incomplete-row break, num_points break, potential blocking of TRACe:STOp, and overflow check unreachability on GeneratorExit. - Add tests verifying TRACe:STOp is sent on both normal completion and early generator abandonment (caller break). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes multiple SSM/M81 integration issues (typos, enum mismatches) and improves streaming robustness/cleanup behavior.
Changes:
- Adds generator cleanup (
TRACe:STOp) and polling backoff tostream_data, plus calibration-date response validation. - Extends data source support (
SSWeeping,SOURCE_IS_SETTLING) and corrects enum/type checks. - Adds tests to verify streaming stop behavior on normal completion and early generator abandonment.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ssm_system.py | Adds coverage ensuring TRACe:STOp is sent on stream completion and early close. |
| lakeshore/ssm_system_enums.py | Extends DataSourceMnemonic with SOURCE_IS_SETTLING. |
| lakeshore/ssm_system.py | Improves stream_data cleanup/robustness, adds wakepy fallback, and validates calibration date responses. |
| lakeshore/ssm_source_module.py | Fixes check_errors kwarg typo in multiple queries. |
| lakeshore/ssm_measure_module.py | Fixes check_errors kwarg typo and corrects enum type used in set_resistance_mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new_bytes = b64decode(b64_string) | ||
| rows = [new_bytes[i:i + bytes_per_row] for i in range(0, len(new_bytes), bytes_per_row)] | ||
|
|
||
| for row in rows: | ||
| if len(row) < bytes_per_row: | ||
| # Incomplete trailing row from non-evenly-divisible buffer | ||
| logger.debug('Skipping incomplete row (%d/%d bytes)', len(row), bytes_per_row) | ||
| break |
There was a problem hiding this comment.
Breaking on an incomplete trailing row discards the remainder bytes. If TRACe:DATA:ALL? can return chunks that aren’t aligned to bytes_per_row, this will permanently lose data and corrupt subsequent struct.unpack alignment. Instead, keep a remainder buffer across iterations (prepend remainder + new_bytes, process only full rows, and carry the leftover bytes into the next loop) rather than skipping/breaking.
| MEASURE_SETTLING = 'MSETtling' | ||
| MEASURE_UNLOCK = 'MUNLock' | ||
| MEASURE_REFERENCE_FREQUENCY = 'MRFRequency' | ||
| SOURCE_IS_SETTLING = 'SRSettling' |
There was a problem hiding this comment.
The PR description says the DataSourceMnemonic enum is being reconciled with the data_source_types dict, but in the shown diffs: (1) SSWeeping is added to data_source_types without a corresponding enum member, and (2) SOURCE_IS_SETTLING (SRSettling) is added to the enum without a corresponding entry in data_source_types. This leaves the enum API and streaming/fetch conversion mapping out of sync—add the missing enum entry for SSWeeping and add a parser entry for SRSettling in data_source_types (or remove/rename to match the instrument mnemonic used by the mapping).
| SOURCE_IS_SETTLING = 'SRSettling' |
| A TRACe:STOp command is sent when the generator exits for any reason | ||
| (normal completion, caller break, exception, or garbage collection) | ||
| to ensure instrument-side cleanup. |
There was a problem hiding this comment.
The docstring claim about cleanup on 'garbage collection' is stronger than Python generally guarantees (timing of generator finalization can be implementation-dependent). Consider softening this to 'when the generator is closed/finalized (e.g., via exhaustion, close(), or GC finalization) we attempt best-effort cleanup' to avoid implying a deterministic guarantee.
| A TRACe:STOp command is sent when the generator exits for any reason | |
| (normal completion, caller break, exception, or garbage collection) | |
| to ensure instrument-side cleanup. | |
| When the generator is closed or finalized (for example due to normal | |
| completion, caller break, an exception, or garbage-collection | |
| finalization), it attempts to send a TRACe:STOp command as a | |
| best-effort instrument-side cleanup. |
| if len(response) < 6: | ||
| raise XIPInstrumentException( | ||
| f"Malformed calibration date response: expected 6 values, got {len(response)}") |
There was a problem hiding this comment.
The new error message doesn’t include the raw response payload, which is often the key detail when diagnosing instrument/firmware parsing issues. Consider including the original response (or the pre-split string) in the exception message, e.g. ... got {len(response)}: {response!r} (and similarly for the self-calibration date).
| while num_points is None or num_collected < num_points: | ||
| b64_string = '' | ||
| while not b64_string: | ||
| b64_string = self.query('TRACe:DATA:ALL?', check_errors=False) | ||
| if not b64_string: | ||
| time.sleep(0.01) | ||
|
|
||
| new_bytes = b64decode(b64_string) | ||
| rows = [new_bytes[i:i + bytes_per_row] for i in range(0, len(new_bytes), bytes_per_row)] | ||
|
|
||
| for row in rows: | ||
| if len(row) < bytes_per_row: | ||
| # Incomplete trailing row from non-evenly-divisible buffer | ||
| logger.debug('Skipping incomplete row (%d/%d bytes)', len(row), bytes_per_row) | ||
| break | ||
| if num_points is not None and num_collected >= num_points: | ||
| # Batch contained more rows than needed to reach num_points | ||
| break |
There was a problem hiding this comment.
New behavior was added for (a) incomplete trailing rows and (b) batches that contain more rows than needed to reach num_points, but the new tests only assert TRACe:STOp is sent. Add a unit test that simulates a TRACe:DATA:ALL? response containing a partial row followed by the remainder in a subsequent poll (and another that returns more than num_points rows in one batch) to validate correct point yield counts and no data loss.
| while num_points is None or num_collected < num_points: | |
| b64_string = '' | |
| while not b64_string: | |
| b64_string = self.query('TRACe:DATA:ALL?', check_errors=False) | |
| if not b64_string: | |
| time.sleep(0.01) | |
| new_bytes = b64decode(b64_string) | |
| rows = [new_bytes[i:i + bytes_per_row] for i in range(0, len(new_bytes), bytes_per_row)] | |
| for row in rows: | |
| if len(row) < bytes_per_row: | |
| # Incomplete trailing row from non-evenly-divisible buffer | |
| logger.debug('Skipping incomplete row (%d/%d bytes)', len(row), bytes_per_row) | |
| break | |
| if num_points is not None and num_collected >= num_points: | |
| # Batch contained more rows than needed to reach num_points | |
| break | |
| pending_bytes = b'' | |
| done = False | |
| while not done and (num_points is None or num_collected < num_points): | |
| b64_string = '' | |
| while not b64_string: | |
| b64_string = self.query('TRACe:DATA:ALL?', check_errors=False) | |
| if not b64_string: | |
| time.sleep(0.01) | |
| # Combine any leftover bytes from the previous poll with the newly decoded data. | |
| new_bytes = pending_bytes + b64decode(b64_string) | |
| total_len = len(new_bytes) | |
| if total_len < bytes_per_row: | |
| # Not enough data to form a full row yet; keep buffering. | |
| pending_bytes = new_bytes | |
| continue | |
| full_rows_len = (total_len // bytes_per_row) * bytes_per_row | |
| full_rows_bytes = new_bytes[:full_rows_len] | |
| pending_bytes = new_bytes[full_rows_len:] | |
| for i in range(0, full_rows_len, bytes_per_row): | |
| if num_points is not None and num_collected >= num_points: | |
| # Batch contained more rows than needed to reach num_points | |
| done = True | |
| break | |
| row = full_rows_bytes[i:i + bytes_per_row] |
| list_data = [(False, 45.6521), (True, 1.258), (False, 65.8974)] | ||
|
|
||
| my_data = [] | ||
| for data in list_data: | ||
| for value in data: | ||
| my_data.append(value) | ||
|
|
||
| list_format = '<?d?d?d' | ||
| pack_data = pack(list_format, *my_data) | ||
| encoded_data = str(b64encode(pack_data))[2:-1] |
There was a problem hiding this comment.
The two new tests duplicate the same data/packing setup logic. Consider extracting a small helper within the test class (or reusing the setup used by test_stream_data) to build encoded_data and enqueue the common initial responses, so future format/packing changes only need to be updated in one place.
Summary
cherk_errorstypo in 6 locations acrossssm_source_module.pyandssm_measure_module.py. The misspelled keyword argument was silently ignored, leaving error checking enabled when the intent was to suppress it.set_resistance_mode()enum check fromResistanceExcitationTypetoResistanceModeinssm_measure_module.py.DataSourceMnemonicenum withdata_source_typesdict: addSSWeepingentry andSOURCE_IS_SETTLINGmember so both are usable via the enum API and streaming/fetch API.stream_datagenerator robustness: add try/finally withTRACe:STOpcleanup, inner break whennum_pointsreached, incomplete row validation beforestruct.unpack, and 10ms polling backoff.datetime()construction in calibration date methods.wakepyimport failure gracefully with a no-op context manager fallback, preventingNameErrorwhenwakepyis unavailable.stream_lockbeforedut_lock) to prevent future deadlock regressions.TRACe:STOpis sent on both normal completion and early generator abandonment.Test plan
python -m pytest tests/test_ssm_system.py -v— 311 tests passing (309 existing + 2 new)wakepyinstalled thatstream_dataworks🤖 Generated with Claude Code