Fix register bit_names mismatches and FastHall JSON parsing#17
Fix register bit_names mismatches and FastHall JSON parsing#17jfichtner wants to merge 2 commits into
Conversation
- Fix FastHallOperationRegister.bit_names to match __init__ params - Fix FastHallQuestionableRegister.bit_names to match __init__ params - Add missing commas in Model224 ServiceRequest/StatusByte bit_names (Python was silently concatenating adjacent strings) - Remove underscore prefix from Model335/Model372 register attributes so TemperatureController methods can access them correctly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add _parse_json_response helper wrapping json.loads with error
handling that raises InstrumentException on JSONDecodeError
- Replace all json.loads() calls with _parse_json_response()
- Add default None to all dict.pop() calls to prevent KeyError
- Guard chained .get().get() patterns with intermediate variables
to prevent AttributeError when first .get() returns None
- Fix broken multi-line string keys that produced wrong dict lookups
('Excitation\n...MeasurementRange' and 'NumberOfVoltage...Average')
- Fix wrong JSON key for max_samples (was 'MeasurementRange',
now 'MaximumNumberOfSamples')
- Store FourWireParameters.excitation_reversal as bool instead of
str(int(bool)) to fix round-trip type mismatch with JSON parsing
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple runtime issues in the Lake Shore instrument drivers by correcting status register bit mappings, improving FastHall JSON response parsing robustness, and aligning Model335/Model372 register attribute naming with TemperatureController expectations.
Changes:
- Correct register
bit_namesdefinitions (including missing commas / mismatches) to preventRegisterBase.from_integer()constructor errors. - Make Model335/Model372 expose
status_byte_registerandservice_request_enablesoTemperatureControllerbase methods can access them. - Harden FastHall result parsing by adding
_parse_json_response()and fixing several JSON key/lookup issues; also fix boolean round-trip for reversal parameters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lakeshore/model_372.py | Exposes status_byte_register / service_request_enable for TemperatureController compatibility. |
| lakeshore/model_335.py | Exposes status_byte_register / service_request_enable for TemperatureController compatibility. |
| lakeshore/model_224.py | Fixes bit_names list literal concatenation issues by adding missing commas. |
| lakeshore/fast_hall_controller.py | Fixes FastHall register bit_names, improves JSON parsing safety/error handling, corrects JSON key usage, and fixes reversal bool handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _parse_json_response(self, response, context=""): | ||
| """Parse a JSON response from the instrument with error handling.""" | ||
| try: | ||
| return json.loads(response) | ||
| except json.JSONDecodeError as ex: | ||
| raise InstrumentException( | ||
| f"Failed to parse JSON response{' for ' + context if context else ''}: {ex}" | ||
| ) from ex |
There was a problem hiding this comment.
The new JSON parsing/error-wrapping behavior isn’t covered by unit tests. Please add tests that (a) exercise one of the get_*_setup_results/get_*_measurement_results methods with a representative JSON payload, and (b) verify malformed JSON raises InstrumentException via _parse_json_response() (to prevent regressions in the parsing/key fixes).
| width=setup_results.get('SampleWidthInMeters'), | ||
| separation=setup_results.get('SampleArmSeparationInMeters'), | ||
| max_samples=setup_results.get('MaxNumberOfSamples'), | ||
| max_samples=setup_results.get('MaxNumberOfSamples'), # TODO: verify key from M91 manual (other methods use 'MaximumNumberOfSamples') |
There was a problem hiding this comment.
get_resistivity_setup_results() hard-codes MaxNumberOfSamples, but other setup parsers in this file use MaximumNumberOfSamples. To avoid returning None depending on firmware/manual differences, consider supporting both keys (e.g., try one then fall back to the other) instead of relying on the TODO.
| max_samples=setup_results.get('MaxNumberOfSamples'), # TODO: verify key from M91 manual (other methods use 'MaximumNumberOfSamples') | |
| max_samples=setup_results.get('MaxNumberOfSamples', setup_results.get('MaximumNumberOfSamples')), |
| _status_byte_register = Model335StatusByteRegister | ||
| _service_request_enable = Model335ServiceRequestEnable | ||
| status_byte_register = Model335StatusByteRegister | ||
| service_request_enable = Model335ServiceRequestEnable |
There was a problem hiding this comment.
This change fixes register wiring for the TemperatureController base class, but it doesn’t appear to be covered by tests. Consider adding a small unit test that instantiates Model335 with a fake connection and calls get_status_byte()/get_service_request() to ensure these attributes are used and no AttributeError/type issues occur.
| service_request_enable = Model335ServiceRequestEnable | |
| service_request_enable_register = Model335ServiceRequestEnable |
| # Initialize registers | ||
| _status_byte_register = Model372StatusByteRegister | ||
| _service_request_enable = Model372ServiceRequestEnable | ||
| status_byte_register = Model372StatusByteRegister | ||
| service_request_enable = Model372ServiceRequestEnable |
There was a problem hiding this comment.
This change fixes register wiring for the TemperatureController base class, but it doesn’t appear to be covered by tests. Consider adding a small unit test that instantiates Model372 with a fake connection and calls get_status_byte()/get_service_request() to ensure these attributes are used and no AttributeError/type issues occur.
Summary
bit_namesmismatches that causedTypeErrorcrashes when callingfrom_integer()onFastHallOperationRegister,FastHallQuestionableRegister, andModel224StatusByteRegisterModel224ServiceRequestRegisterandModel224StatusByteRegisterbit_nameslists where Python was silently concatenating adjacent string literalsModel335/Model372status_byte_registerandservice_request_enableclass attributes soTemperatureControllerbase class methods can access them (matchingModel336)_parse_json_response()helper that wrapsjson.loads()withInstrumentExceptionmax_samples, broken multi-line string keys, unguarded chained.get()calls, unsafedict.pop()without defaultsFourWireParameters.excitation_reversalandDCHallParameters.with_field_reversalround-trip type mismatch (store raw bool, convert at SCPI command time)"HBARmissing closing quote, wrong class/method names in docstrings)Test plan
from_integer()round-trip on actual M91 hardwareMaxNumberOfSamplesin resistivity setup against M91 manual (see TODO comment — other methods useMaximumNumberOfSamples)🤖 Generated with Claude Code