Fix cross-cutting issues: bool parsing, busy-wait polling, and misc by jfichtner · Pull Request #16 · lakeshorecryotronics/python-driver

jfichtner · 2026-02-18T00:34:31Z

Summary

Fix bool("0") always returning True at 5 locations across model_240, ssm_source_module, ssm_measure_module, and ssm_settings_profiles by wrapping with int() first. Also fix teslameter set_qualifier_latching_setting sending "True"/"False" instead of "1"/"0" and get_qualifier_latching_setting returning raw string instead of bool.
Add sleep and timeout to 8 busy-wait polling loops in fast_hall_controller.py that previously spun at max CPU with no sleep and blocked forever if a measurement hung. Each run_complete_* method now has time.sleep(0.1) and an optional timeout parameter. Also add sleep to the teslameter stream_buffered_data polling loop.
Guard _calculate_number_of_sweep_points against start == stop (caused math.log10(0) ValueError) and start_value == 0 in logarithmic mode (division by zero).
Add _validate_scpi_parameter helper to GenericInstrument for rejecting SCPI injection via semicolons/newlines, wired into Model 240 set_sensor_name and set_modname.
Fix Model 372 docstring copy-paste errors (set_setpoint_ohms/get_setpoint_ohms said "Kelvin" instead of "Ohms").
Make Model 240 get_celsius_reading, get_fahrenheit_reading, get_sensor_units_channel_reading return float instead of raw string, consistent with get_kelvin_reading.
Add command context to InstrumentException messages in EM power supply error handling.
24 new tests covering all changes including edge cases and error paths.

Test plan

python -m pytest tests/ -v — 890 passed, 26 warnings, 6 subtests passed

🤖 Generated with Claude Code

bool() on a non-empty string like "0" always returns True. Wrap with int() first so "0" correctly becomes False. Also fix set_qualifier_latching_setting sending "True"/"False" instead of "1"/"0", and get_qualifier_latching_setting returning a raw string instead of bool. Affected locations: - model_240.py: get_input_parameter (3 fields) - ssm_source_module.py: get_disable_on_compliance - ssm_measure_module.py: get_resistance_auto_range, get_resistance_optimization_state - ssm_settings_profiles.py: get_valid_for_restore - teslameter.py: get/set_qualifier_latching_setting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

All run_complete_* methods in fast_hall_controller.py spin at max CPU speed with no sleep, and block forever if a measurement hangs. Add time.sleep(0.1) to each loop and an optional timeout parameter that raises InstrumentException when exceeded. Also add sleep to the Teslameter stream_buffered_data polling loop when no data is available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Guard _calculate_number_of_sweep_points against start == stop (previously caused math.log10(0) ValueError) and start_value == 0 in logarithmic mode (division by zero) - Add _validate_scpi_parameter static method to GenericInstrument for rejecting semicolons and newlines in user-supplied parameter values - Fix Model 372 docstring copy-paste errors: set_setpoint_ohms and get_setpoint_ohms incorrectly said "Kelvin" instead of "Ohms" - Make Model 240 get_celsius_reading, get_fahrenheit_reading, and get_sensor_units_channel_reading return float instead of raw string, consistent with get_kelvin_reading - Add command context to InstrumentException messages in EM power supply Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Address all review findings with comprehensive test coverage: - Add test for bool "0" → False in Model 240 get_input_parameter - Add timeout tests for all 8 run_complete_* methods in FastHall - Add normal completion tests for run_complete_* methods - Add sweep edge case tests: start==stop, zero start in log mode - Add SCPI parameter validation tests and wire _validate_scpi_parameter into Model 240 set_sensor_name and set_modname - Add EM power supply error message context tests verifying command string appears in Command Error, Execution Error, and Query Error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR addresses three critical cross-cutting issues across the Lakeshore instrument driver codebase: incorrect boolean parsing from SCPI responses, CPU-intensive busy-wait polling loops, and several smaller bugs and documentation issues.

Changes:

Fixed bool("0") incorrectly evaluating to True by wrapping with int() at 5 locations across Model 240, SSM modules, and teslameter drivers
Added time.sleep(0.1) and optional timeout parameter to 8 polling loops in fast_hall_controller.py to prevent CPU-intensive busy-waiting and infinite hangs
Guarded sweep point calculation against division by zero edge cases and added SCPI injection prevention

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/test_ssm_system.py	Added 5 tests for sweep point calculation edge cases (start==stop, zero start in log mode)
tests/test_fast_hall.py	Added 10 tests for timeout behavior and normal completion of run_complete_* methods
tests/test_em_power_supply.py	Added 3 tests verifying error messages include query context
tests/test_240.py	Added 6 tests for bool parsing with zeros and SCPI parameter validation
lakeshore/teslameter.py	Fixed bool parsing in get/set_qualifier_latching_setting and added sleep to stream_buffered_data
lakeshore/ssm_source_module.py	Fixed bool parsing in get_disable_on_compliance and added edge case guards to _calculate_number_of_sweep_points
lakeshore/ssm_settings_profiles.py	Fixed bool parsing in get_valid_for_restore
lakeshore/ssm_measure_module.py	Fixed bool parsing in get_resistance_auto_range and get_resistance_optimization_state
lakeshore/model_372.py	Fixed docstring copy-paste errors (Kelvin→Ohms) in setpoint methods
lakeshore/model_240.py	Fixed 3 methods to return float instead of string, fixed bool parsing, added SCPI validation
lakeshore/generic_instrument.py	Added _validate_scpi_parameter helper to prevent SCPI injection via semicolons/newlines
lakeshore/fast_hall_controller.py	Added timeout parameter and sleep to 8 run_complete_* polling methods
lakeshore/em_power_supply.py	Enhanced error messages to include the query string that caused the error

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-18T14:06:52Z

@@ -1,4 +1,6 @@
+from unittest.mock import patch


The import 'from unittest.mock import patch' is added but never used in the test file. Consider removing this unused import to keep the code clean.

Suggested change

from unittest.mock import patch

Copilot · 2026-02-18T14:06:53Z

    def get_qualifier_latching_setting(self):
        """Returns whether the qualifier latches."""
-        return self.query("SENSE:QUALIFIER:LATCH?")
+        return bool(int(self.query("SENSE:QUALIFIER:LATCH?")))


Missing test coverage for get_qualifier_latching_setting. While set_qualifier_latching_setting is tested and was fixed to send int(latching) instead of str(latching), the corresponding getter get_qualifier_latching_setting was also modified to return bool(int(...)) but lacks test coverage. Consider adding a test to verify the getter correctly parses "0" and "1" responses as False and True respectively.

Copilot · 2026-02-18T14:06:53Z

+                raise InstrumentException(f"Command Error for '{query_string}': The instrument could not interpret "
+                                          "the command due to a syntax error, an unrecognized header, unrecognized "
+                                          "terminator, or an unsupported command.")
            if register.execution_error:
-                raise InstrumentException("Execution Error: The instrument was instructed to do something not within "
-                                          "its capabilities.")
+                raise InstrumentException(f"Execution Error for '{query_string}': The instrument was instructed to "
+                                          "do something not within its capabilities.")
            if register.query_error:
-                raise InstrumentException("Query Error: The output queue is full.")
+                raise InstrumentException(f"Query Error for '{query_string}': The output queue is full.")


The error messages include the modified query_string with "; *ESR?" appended, which may confuse users since they didn't send that exact query. Consider storing the original query_string before modification on line 220, and using the original in error messages at lines 231-238 for clarity.

jfichtner and others added 5 commits February 17, 2026 19:17

Move _validate_scpi_parameter to end of class to avoid merge conflict

e4e6a27

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Adam-- requested a review from Copilot February 18, 2026 14:00

Copilot started reviewing on behalf of Adam-- February 18, 2026 14:01 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix cross-cutting issues: bool parsing, busy-wait polling, and misc - #16

Fix cross-cutting issues: bool parsing, busy-wait polling, and misc#16
jfichtner wants to merge 5 commits into
mainfrom
fix/cross-cutting

jfichtner commented Feb 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 18, 2026

Uh oh!

Copilot AI Feb 18, 2026

Uh oh!

Copilot AI Feb 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jfichtner commented Feb 18, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants