From 531dd194504337e990d39b4a3b4c589f0e6d1bf8 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:16:48 -0500 Subject: [PATCH 01/12] Fix Model 335 tuple membership test and XIP to_integer call - model_335.py: Change `or` to `,` in tuple membership test so both PLATINUM_RTD and NTC_RTD sensor types are checked (the `or` operator between truthy IntEnum values only returned the first operand) - xip_instrument.py: Call to_integer() on register_mask instance instead of passing it as argument to the class method, matching the correct pattern used in set_operation_event_enable_mask Co-Authored-By: Claude Opus 4.6 --- lakeshore/model_335.py | 2 +- lakeshore/xip_instrument.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lakeshore/model_335.py b/lakeshore/model_335.py index 14161b4..c0ee8fa 100644 --- a/lakeshore/model_335.py +++ b/lakeshore/model_335.py @@ -364,7 +364,7 @@ def get_input_sensor(self, channel): sensor_range = 0 elif input_sensor_type == self.InputSensorType.DIODE: sensor_range = self.DiodeRange(int(sensor_configuration[2])) - elif input_sensor_type in (self.InputSensorType.PLATINUM_RTD or self.InputSensorType.NTC_RTD): + elif input_sensor_type in (self.InputSensorType.PLATINUM_RTD, self.InputSensorType.NTC_RTD): sensor_range = self.RTDRange(int(sensor_configuration[2])) elif input_sensor_type == self.InputSensorType.THERMOCOUPLE: sensor_range = self.ThermocoupleRange(int(sensor_configuration[2])) diff --git a/lakeshore/xip_instrument.py b/lakeshore/xip_instrument.py index 661014a..93635fa 100644 --- a/lakeshore/xip_instrument.py +++ b/lakeshore/xip_instrument.py @@ -324,7 +324,7 @@ def set_questionable_event_enable_mask(self, register_mask): An instrument specific QuestionableRegister class object with all bits configured true or false. """ - integer_representation = self.questionable_register.to_integer(register_mask) + integer_representation = register_mask.to_integer() self.command(f"STATus:QUEStionable:ENABle {str(integer_representation)}", check_errors=False) def reset_status_register_masks(self): From 7b5857cce58db95e56519f215fc9fee0e7e75094 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:17:40 -0500 Subject: [PATCH 02/12] Fix curve data point indexing and off-by-one errors - temperature_controllers.py, model_224.py: Add enumerate() to set_curve() loops that were unpacking data_points tuples as (index, point) instead of iterating with an index - temperature_controllers.py: Change `if curvature:` to `if curvature is not None:` so curvature=0.0 is not treated as falsy - temperature_controllers.py: Fix get_curve() off-by-one by using [:true_point_index + 1] to include the last valid data point (matching the correct pattern in model_224.py) - temperature_controllers.py: Rename "ramp_rate" key to "derivative" in get_heater_pid() return dict to match set_heater_pid() parameter name, and update tests accordingly Co-Authored-By: Claude Opus 4.6 --- lakeshore/model_224.py | 2 +- lakeshore/temperature_controllers.py | 12 ++++++------ tests/test_model_335.py | 2 +- tests/test_model_336.py | 2 +- tests/test_temperature_controllers.py | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lakeshore/model_224.py b/lakeshore/model_224.py index 8c3a2b8..855727e 100644 --- a/lakeshore/model_224.py +++ b/lakeshore/model_224.py @@ -899,7 +899,7 @@ def set_curve(self, curve, data_points): self.delete_curve(curve) - for index, point in data_points: + for index, point in enumerate(data_points): self.set_curve_data_point(curve, index + 1, point[0], point[1]) def get_relay_status(self, relay_channel): diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index b1a5c3d..d25ba3d 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -366,7 +366,7 @@ def set_curve_data_point(self, curve, index, sensor_units, temperature, curvatur The curvature value scale used to calculate spindle coefficients to 6 digits. Optional parameter. """ - if curvature: + if curvature is not None: command_string = f"CRVPT {curve},{index},{sensor_units},{temperature},{curvature}" else: command_string = f"CRVPT {curve},{index},{sensor_units},{temperature}" @@ -412,7 +412,7 @@ def get_curve(self, curve): true_point_index = i # Remove all extraneous points - return data_points[:true_point_index] + return data_points[:true_point_index + 1] def set_curve(self, curve, data_points): """Method to define a user curve using a list of data points. @@ -427,7 +427,7 @@ def set_curve(self, curve, data_points): """ self.delete_curve(curve) - for index, point in data_points: + for index, point in enumerate(data_points): if len(point) > 2: self.set_curve_data_point(curve, index + 1, point[0], point[1], point[2]) else: @@ -1054,17 +1054,17 @@ def get_heater_pid(self, output): Returns: (dict): - {"gain": float, "integral": float, "ramp_rate": float} + {"gain": float, "integral": float, "derivative": float} gain: Proportional term in PID control. integral: Integral term in PID control. - ramp_rate: Derivative term in PID control. + derivative: Derivative term in PID control. """ pid_values = self.query(f"PID? {output}") pid_values = pid_values.split(",") return {"gain": float(pid_values[0]), "integral": float(pid_values[1]), - "ramp_rate": float(pid_values[2])} + "derivative": float(pid_values[2])} def set_setpoint_ramp_parameter(self, output, ramp_enable, rate_value): """Sets the control loop of a particular output. diff --git a/tests/test_model_335.py b/tests/test_model_335.py index af75c0b..6f4b95f 100644 --- a/tests/test_model_335.py +++ b/tests/test_model_335.py @@ -228,7 +228,7 @@ def test_get_heater_pid(self): response = self.dut.get_heater_pid(1) pid_settings = {"gain": 4.25, "integral": 6.1, - "ramp_rate": 0} + "derivative": 0} self.assertDictEqual(response, pid_settings) def test_get_output_2_polarity(self): diff --git a/tests/test_model_336.py b/tests/test_model_336.py index 14d110f..7c149b7 100644 --- a/tests/test_model_336.py +++ b/tests/test_model_336.py @@ -379,7 +379,7 @@ def test_get_heater_pid(self): response = self.dut.get_heater_pid(1) pid_settings = {"gain": 4.25, "integral": 6.1, - "ramp_rate": 0} + "derivative": 0} self.assertDictEqual(response, pid_settings) self.assertIn("PID? 1", self.fake_connection.get_outgoing_message()) diff --git a/tests/test_temperature_controllers.py b/tests/test_temperature_controllers.py index b086121..6d4c2dc 100644 --- a/tests/test_temperature_controllers.py +++ b/tests/test_temperature_controllers.py @@ -233,7 +233,7 @@ def test_set_heater_pid(self): def test_get_heater_pid(self): pid = {'gain': 34.56, 'integral': 56.78, - 'ramp_rate': 98.76} + 'derivative': 98.76} self.fake_connection.setup_response('34.56,56.78,98.76;0') response = self.dut.get_heater_pid(2) From 5558342a2fb30797e8d6b2464ca2046ce3d720f0 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:19:09 -0500 Subject: [PATCH 03/12] Add safety validation to temperature controller and precision source methods - em_power_supply.py, xip_instrument.py: Add confirm=False parameter to factory reset methods to prevent accidental resets; raises ValueError unless confirm=True is passed explicitly - temperature_controllers.py: Add type and range validation to set_heater_pid(), set_control_setpoint(), and set_temperature_limit() - model_155.py: Add type validation to output_dc_current() and output_dc_voltage(); add documented range checks to set_current_limit(), set_voltage_limit(), set_current_mode_voltage_protection(), and set_voltage_mode_current_protection() - Update tests to pass confirm=True for factory reset calls Co-Authored-By: Claude Opus 4.6 --- lakeshore/em_power_supply.py | 9 ++++++++- lakeshore/model_155.py | 24 ++++++++++++++++++++++-- lakeshore/temperature_controllers.py | 14 ++++++++++++++ lakeshore/xip_instrument.py | 11 +++++++++-- tests/test_em_power_supply.py | 2 +- tests/test_fast_hall.py | 2 +- tests/test_teslameter.py | 2 +- 7 files changed, 56 insertions(+), 8 deletions(-) diff --git a/lakeshore/em_power_supply.py b/lakeshore/em_power_supply.py index 56e167f..9de7226 100644 --- a/lakeshore/em_power_supply.py +++ b/lakeshore/em_power_supply.py @@ -499,11 +499,18 @@ def get_ieee_interface_mode(self): """ return int(self.query("MODE?")) - def set_factory_defaults(self): + def set_factory_defaults(self, confirm=False): """Sets all configuration values to factory defaults and resets the instrument. The instrument must be at zero amps for this command to work. + + Args: + confirm (bool): Must be True to execute. Prevents accidental factory reset. """ + if not confirm: + raise ValueError( + "Factory reset will clear all settings including safety limits. " + "Pass confirm=True to proceed.") self.command("DFLT 99") def reset_instrument(self): diff --git a/lakeshore/model_155.py b/lakeshore/model_155.py index d1d3c12..61d900b 100644 --- a/lakeshore/model_155.py +++ b/lakeshore/model_155.py @@ -311,6 +311,8 @@ def output_dc_current(self, current_level): The output current level in amps. """ + if not isinstance(current_level, (int, float)): + raise ValueError(f"Current level must be a number, got {type(current_level).__name__}") # Change the output mode to source current instead of voltage self.command("SOURCE:FUNCTION:MODE CURRENT") @@ -332,6 +334,8 @@ def output_dc_voltage(self, voltage_level): The output voltage level in volts. """ + if not isinstance(voltage_level, (int, float)): + raise ValueError(f"Voltage level must be a number, got {type(voltage_level).__name__}") # Change the output mode to source voltage instead of current self.command("SOURCE:FUNCTION:MODE VOLTAGE") @@ -401,6 +405,10 @@ def set_current_limit(self, current_limit): The maximum settable current in amps. Must be between 0 and 100 milli-amps. """ + if not isinstance(current_limit, (int, float)): + raise ValueError(f"Current limit must be a number, got {type(current_limit).__name__}") + if current_limit < 0 or current_limit > 0.1: + raise ValueError(f"Current limit must be between 0 and 100 milli-amps (0.1 A), got {current_limit}") self.command("SOURCE:CURRENT:LIMIT " + str(current_limit)) def set_voltage_limit(self, voltage_limit): @@ -408,9 +416,13 @@ def set_voltage_limit(self, voltage_limit): Args: voltage_limit (float): - The maximum settable voltage in amps. Must be between 0 and 100 volts. + The maximum settable voltage in volts. Must be between 0 and 100 volts. """ + if not isinstance(voltage_limit, (int, float)): + raise ValueError(f"Voltage limit must be a number, got {type(voltage_limit).__name__}") + if voltage_limit < 0 or voltage_limit > 100: + raise ValueError(f"Voltage limit must be between 0 and 100 volts, got {voltage_limit}") self.command("SOURCE:VOLTAGE:LIMIT " + str(voltage_limit)) def set_current_mode_voltage_protection(self, max_voltage): @@ -421,6 +433,10 @@ def set_current_mode_voltage_protection(self, max_voltage): The maximum permissible voltage. Must be between 1 and 100 volts. """ + if not isinstance(max_voltage, (int, float)): + raise ValueError(f"Max voltage must be a number, got {type(max_voltage).__name__}") + if max_voltage < 1 or max_voltage > 100: + raise ValueError(f"Max voltage must be between 1 and 100 volts, got {max_voltage}") self.command("SOURCE:CURRENT:PROTECTION " + str(max_voltage)) def set_voltage_mode_current_protection(self, max_current): @@ -428,9 +444,13 @@ def set_voltage_mode_current_protection(self, max_current): Args: max_current (float): - The maximum permissible voltage. Must be between 1 and 100 volts. + The maximum permissible current in amps. """ + if not isinstance(max_current, (int, float)): + raise ValueError(f"Max current must be a number, got {type(max_current).__name__}") + if max_current < 0: + raise ValueError(f"Max current must be non-negative, got {max_current}") self.command("SOURCE:VOLTAGE:PROTECTION " + str(max_current)) def enable_ac_high_voltage_compliance(self): diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index d25ba3d..d961fd2 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -1043,6 +1043,12 @@ def set_heater_pid(self, output, gain, integral, derivative): The ramp rate is configured in field units per second. """ + if not isinstance(gain, (int, float)) or gain < 0: + raise ValueError(f"Gain must be a non-negative number, got {gain}") + if not isinstance(integral, (int, float)) or integral < 0: + raise ValueError(f"Integral must be a non-negative number, got {integral}") + if not isinstance(derivative, (int, float)) or derivative < 0: + raise ValueError(f"Derivative must be a non-negative number, got {derivative}") self.command(f"PID {output},{gain},{integral},{derivative}") def get_heater_pid(self, output): @@ -1308,6 +1314,10 @@ def set_control_setpoint(self, output, value): The value for the set-point (in the preferred units of the control loop sensor). """ + if not isinstance(value, (int, float)): + raise ValueError(f"Setpoint must be a number, got {type(value).__name__}") + if value < 0: + raise ValueError(f"Setpoint {value} must be non-negative") self.command(f"SETP {output},{value}") def get_control_setpoint(self, output): @@ -1345,6 +1355,10 @@ def set_temperature_limit(self, input_channel, limit): A limit of zero will turn the feature off. """ + if not isinstance(limit, (int, float)): + raise ValueError(f"Temperature limit must be a number, got {type(limit).__name__}") + if limit < 0: + raise ValueError(f"Temperature limit {limit} must be non-negative") self.command(f"TLIMIT {input_channel},{limit}") def get_temperature_limit(self, input_channel): diff --git a/lakeshore/xip_instrument.py b/lakeshore/xip_instrument.py index 93635fa..04acd24 100644 --- a/lakeshore/xip_instrument.py +++ b/lakeshore/xip_instrument.py @@ -403,8 +403,15 @@ def reset_measurement_settings(self): """Resets measurement settings to their default values.""" self.command("SYSTEM:PRESET") - def factory_reset(self): - """Resets all system information such as settings, wi-fi connections, date and time, etc.""" + def factory_reset(self, confirm=False): + """Resets all system information such as settings, wi-fi connections, date and time, etc. + + Args: + confirm (bool): Must be True to execute. Prevents accidental factory reset. + """ + if not confirm: + raise ValueError( + "Factory reset will clear all settings. Pass confirm=True to proceed.") self.command("SYSTEM:FACTORYRESET") def _get_identity(self): diff --git a/tests/test_em_power_supply.py b/tests/test_em_power_supply.py index 5509e3b..997f343 100644 --- a/tests/test_em_power_supply.py +++ b/tests/test_em_power_supply.py @@ -169,7 +169,7 @@ class TestResets(TestWithFakeEMPowerSupply): def test_set_factory_defaults(self): self.fake_connection.setup_response("0") - self.dut.set_factory_defaults() + self.dut.set_factory_defaults(confirm=True) self.assertIn("DFLT 99", self.fake_connection.get_outgoing_message()) def test_reset_instrument(self): diff --git a/tests/test_fast_hall.py b/tests/test_fast_hall.py index ad5274b..c9845b8 100644 --- a/tests/test_fast_hall.py +++ b/tests/test_fast_hall.py @@ -12,7 +12,7 @@ def test_reset_measurement_settings(self): def test_factory_reset(self): self.fake_connection.setup_response('No error') - self.dut.factory_reset() + self.dut.factory_reset(confirm=True) self.assertIn('SYSTEM:FACTORYRESET', self.fake_connection.get_outgoing_message()) def test_contact_check_reset(self): diff --git a/tests/test_teslameter.py b/tests/test_teslameter.py index c9e4a00..bc50eb0 100644 --- a/tests/test_teslameter.py +++ b/tests/test_teslameter.py @@ -357,7 +357,7 @@ def test_reset_measurement_settings(self): def test_factory_reset(self): self.fake_connection.setup_response('No error') - self.dut.factory_reset() + self.dut.factory_reset(confirm=True) self.assertIn('SYSTEM:FACTORYRESET', self.fake_connection.get_outgoing_message()) From 371911f496f748334360d38d5c9a0753578f2e71 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:25:26 -0500 Subject: [PATCH 04/12] Fix validation correctness issues found in review - Use numbers.Real instead of (int, float) for isinstance checks to accept numpy.float64, decimal.Decimal, and other numeric types common in scientific Python instrumentation code - Exclude bool from all numeric validation since bool is a subclass of int but str(True) sends "True" to instruments instead of "1" - Remove negative value rejection from set_control_setpoint() since Celsius-mode setpoints can legitimately be negative - Fix get_curve() true_point_index initialization from 200 to 0 for consistency with model_224.py behavior on empty curves Co-Authored-By: Claude Opus 4.6 --- lakeshore/model_155.py | 13 +++++++------ lakeshore/temperature_controllers.py | 15 +++++++-------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lakeshore/model_155.py b/lakeshore/model_155.py index 61d900b..e8fbf92 100644 --- a/lakeshore/model_155.py +++ b/lakeshore/model_155.py @@ -2,6 +2,7 @@ from time import sleep import itertools +import numbers from .xip_instrument import XIPInstrument, RegisterBase, StatusByteRegister, StandardEventRegister @@ -311,7 +312,7 @@ def output_dc_current(self, current_level): The output current level in amps. """ - if not isinstance(current_level, (int, float)): + if isinstance(current_level, bool) or not isinstance(current_level, numbers.Real): raise ValueError(f"Current level must be a number, got {type(current_level).__name__}") # Change the output mode to source current instead of voltage @@ -334,7 +335,7 @@ def output_dc_voltage(self, voltage_level): The output voltage level in volts. """ - if not isinstance(voltage_level, (int, float)): + if isinstance(voltage_level, bool) or not isinstance(voltage_level, numbers.Real): raise ValueError(f"Voltage level must be a number, got {type(voltage_level).__name__}") # Change the output mode to source voltage instead of current @@ -405,7 +406,7 @@ def set_current_limit(self, current_limit): The maximum settable current in amps. Must be between 0 and 100 milli-amps. """ - if not isinstance(current_limit, (int, float)): + if isinstance(current_limit, bool) or not isinstance(current_limit, numbers.Real): raise ValueError(f"Current limit must be a number, got {type(current_limit).__name__}") if current_limit < 0 or current_limit > 0.1: raise ValueError(f"Current limit must be between 0 and 100 milli-amps (0.1 A), got {current_limit}") @@ -419,7 +420,7 @@ def set_voltage_limit(self, voltage_limit): The maximum settable voltage in volts. Must be between 0 and 100 volts. """ - if not isinstance(voltage_limit, (int, float)): + if isinstance(voltage_limit, bool) or not isinstance(voltage_limit, numbers.Real): raise ValueError(f"Voltage limit must be a number, got {type(voltage_limit).__name__}") if voltage_limit < 0 or voltage_limit > 100: raise ValueError(f"Voltage limit must be between 0 and 100 volts, got {voltage_limit}") @@ -433,7 +434,7 @@ def set_current_mode_voltage_protection(self, max_voltage): The maximum permissible voltage. Must be between 1 and 100 volts. """ - if not isinstance(max_voltage, (int, float)): + if isinstance(max_voltage, bool) or not isinstance(max_voltage, numbers.Real): raise ValueError(f"Max voltage must be a number, got {type(max_voltage).__name__}") if max_voltage < 1 or max_voltage > 100: raise ValueError(f"Max voltage must be between 1 and 100 volts, got {max_voltage}") @@ -447,7 +448,7 @@ def set_voltage_mode_current_protection(self, max_current): The maximum permissible current in amps. """ - if not isinstance(max_current, (int, float)): + if isinstance(max_current, bool) or not isinstance(max_current, numbers.Real): raise ValueError(f"Max current must be a number, got {type(max_current).__name__}") if max_current < 0: raise ValueError(f"Max current must be non-negative, got {max_current}") diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index d961fd2..cc2276a 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -1,5 +1,6 @@ """Implements a parent class for temperature controllers that contains shared methods between similar instruments.""" +import numbers import serial from .generic_instrument import GenericInstrument, InstrumentException, RegisterBase from .temperature_controllers_enums import TemperatureControllerEnums @@ -403,7 +404,7 @@ def get_curve(self, curve): (sensor_units: float, temp_value: float, curvature_value: float (optional)). """ - true_point_index = 200 + true_point_index = 0 data_points = [] for i in range(0, 200): point = self.get_curve_data_point(curve, i + 1) @@ -1043,11 +1044,11 @@ def set_heater_pid(self, output, gain, integral, derivative): The ramp rate is configured in field units per second. """ - if not isinstance(gain, (int, float)) or gain < 0: + if isinstance(gain, bool) or not isinstance(gain, numbers.Real) or gain < 0: raise ValueError(f"Gain must be a non-negative number, got {gain}") - if not isinstance(integral, (int, float)) or integral < 0: + if isinstance(integral, bool) or not isinstance(integral, numbers.Real) or integral < 0: raise ValueError(f"Integral must be a non-negative number, got {integral}") - if not isinstance(derivative, (int, float)) or derivative < 0: + if isinstance(derivative, bool) or not isinstance(derivative, numbers.Real) or derivative < 0: raise ValueError(f"Derivative must be a non-negative number, got {derivative}") self.command(f"PID {output},{gain},{integral},{derivative}") @@ -1314,10 +1315,8 @@ def set_control_setpoint(self, output, value): The value for the set-point (in the preferred units of the control loop sensor). """ - if not isinstance(value, (int, float)): + if isinstance(value, bool) or not isinstance(value, numbers.Real): raise ValueError(f"Setpoint must be a number, got {type(value).__name__}") - if value < 0: - raise ValueError(f"Setpoint {value} must be non-negative") self.command(f"SETP {output},{value}") def get_control_setpoint(self, output): @@ -1355,7 +1354,7 @@ def set_temperature_limit(self, input_channel, limit): A limit of zero will turn the feature off. """ - if not isinstance(limit, (int, float)): + if isinstance(limit, bool) or not isinstance(limit, numbers.Real): raise ValueError(f"Temperature limit must be a number, got {type(limit).__name__}") if limit < 0: raise ValueError(f"Temperature limit {limit} must be non-negative") From 59050d3e806ce8c4c849522bd98cf92a8222b363 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:25:58 -0500 Subject: [PATCH 05/12] Add confirm guards to Model 121 and Model 240 factory reset methods Consistent with the guards added to EMPowerSupply.set_factory_defaults() and XIPInstrument.factory_reset(), add confirm=False parameter to Model121.set_factory_defaults() and Model240.set_factory_defaults() to prevent accidental factory resets. Co-Authored-By: Claude Opus 4.6 --- lakeshore/model_121.py | 11 +++++++++-- lakeshore/model_240.py | 11 +++++++++-- tests/test_121.py | 2 +- tests/test_240.py | 2 +- 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/lakeshore/model_121.py b/lakeshore/model_121.py index db7abc3..2918548 100644 --- a/lakeshore/model_121.py +++ b/lakeshore/model_121.py @@ -110,8 +110,15 @@ def get_compliance_limit_status(self): """ return bool(int(self.query("COMP?"))) - def set_factory_defaults(self): - """Sets all configuration values to factory defaults and resets the instrument.""" + def set_factory_defaults(self, confirm=False): + """Sets all configuration values to factory defaults and resets the instrument. + + Args: + confirm (bool): Must be True to execute. Prevents accidental factory reset. + """ + if not confirm: + raise ValueError( + "Factory reset will clear all settings. Pass confirm=True to proceed.") self.command("DFLT 99") def lock_front_panel(self): diff --git a/lakeshore/model_240.py b/lakeshore/model_240.py index 5f0dbf4..52e3cb2 100644 --- a/lakeshore/model_240.py +++ b/lakeshore/model_240.py @@ -147,8 +147,15 @@ def get_celsius_reading(self, channel): """ return self.query(f"CRDG? {channel}") - def set_factory_defaults(self): - """Sets all configuration values to factory defaults and resets the instrument.""" + def set_factory_defaults(self, confirm=False): + """Sets all configuration values to factory defaults and resets the instrument. + + Args: + confirm (bool): Must be True to execute. Prevents accidental factory reset. + """ + if not confirm: + raise ValueError( + "Factory reset will clear all settings. Pass confirm=True to proceed.") self.command("DFLT 99") def get_kelvin_reading(self, channel): diff --git a/tests/test_121.py b/tests/test_121.py index 409c0a6..b9b8743 100644 --- a/tests/test_121.py +++ b/tests/test_121.py @@ -71,7 +71,7 @@ def test_compliance_limit_status(self): def test_factory_defaults(self): self.fake_connection.setup_response('') - self.dut.set_factory_defaults() + self.dut.set_factory_defaults(confirm=True) self.assertIn('DFLT 99', self.fake_connection.get_outgoing_message()) def test_enable_keypad(self): diff --git a/tests/test_240.py b/tests/test_240.py index 4d1772b..9817c17 100644 --- a/tests/test_240.py +++ b/tests/test_240.py @@ -143,5 +143,5 @@ def test_set_input_parameter(self): def test_set_factory_defaults(self): self.fake_connection.setup_response('0') - self.dut.set_factory_defaults() + self.dut.set_factory_defaults(confirm=True) self.assertIn('DFLT 99', self.fake_connection.get_outgoing_message()) From e7efb550ddd81c8b39ac4a4425d43456899cc9b0 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:27:03 -0500 Subject: [PATCH 06/12] Add numeric validation to Model 155 sine output methods Add _validate_numeric() helper to PrecisionSource and use it across all output methods for consistency. The sine wave methods (output_sine_current, output_sine_voltage) previously had no type checking while the DC methods did, creating an inconsistency where non-numeric types would silently produce malformed instrument commands. Co-Authored-By: Claude Opus 4.6 --- lakeshore/model_155.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/lakeshore/model_155.py b/lakeshore/model_155.py index e8fbf92..f383e45 100644 --- a/lakeshore/model_155.py +++ b/lakeshore/model_155.py @@ -232,6 +232,12 @@ def route_terminals(self, output_connections_location="REAR"): """ self.command("ROUTE:TERMINALS " + output_connections_location) + @staticmethod + def _validate_numeric(value, name): + """Validate that a value is a real number (not bool).""" + if isinstance(value, bool) or not isinstance(value, numbers.Real): + raise ValueError(f"{name} must be a number, got {type(value).__name__}") + def output_sine_current(self, amplitude, frequency, offset=0.0, phase=0.0): """Configures and enables the source output to be a sine wave current source. @@ -246,6 +252,9 @@ def output_sine_current(self, amplitude, frequency, offset=0.0, phase=0.0): Shifts the phase of the output relative to the reference out. Must be between -180 and 180 degrees. """ + for val, name in [(amplitude, "Amplitude"), (frequency, "Frequency"), + (offset, "Offset"), (phase, "Phase")]: + self._validate_numeric(val, name) # Change the output mode to source current instead of voltage self.command("SOURCE:FUNCTION:MODE CURRENT") @@ -282,6 +291,9 @@ def output_sine_voltage(self, amplitude, frequency, offset=0.0, phase=0.0): Shifts the phase of the output relative to the reference out. Must be between -180 and 180 degrees. """ + for val, name in [(amplitude, "Amplitude"), (frequency, "Frequency"), + (offset, "Offset"), (phase, "Phase")]: + self._validate_numeric(val, name) # Change the output mode to source voltage instead of current self.command("SOURCE:FUNCTION:MODE VOLTAGE") @@ -312,8 +324,7 @@ def output_dc_current(self, current_level): The output current level in amps. """ - if isinstance(current_level, bool) or not isinstance(current_level, numbers.Real): - raise ValueError(f"Current level must be a number, got {type(current_level).__name__}") + self._validate_numeric(current_level, "Current level") # Change the output mode to source current instead of voltage self.command("SOURCE:FUNCTION:MODE CURRENT") @@ -335,8 +346,7 @@ def output_dc_voltage(self, voltage_level): The output voltage level in volts. """ - if isinstance(voltage_level, bool) or not isinstance(voltage_level, numbers.Real): - raise ValueError(f"Voltage level must be a number, got {type(voltage_level).__name__}") + self._validate_numeric(voltage_level, "Voltage level") # Change the output mode to source voltage instead of current self.command("SOURCE:FUNCTION:MODE VOLTAGE") @@ -406,8 +416,7 @@ def set_current_limit(self, current_limit): The maximum settable current in amps. Must be between 0 and 100 milli-amps. """ - if isinstance(current_limit, bool) or not isinstance(current_limit, numbers.Real): - raise ValueError(f"Current limit must be a number, got {type(current_limit).__name__}") + self._validate_numeric(current_limit, "Current limit") if current_limit < 0 or current_limit > 0.1: raise ValueError(f"Current limit must be between 0 and 100 milli-amps (0.1 A), got {current_limit}") self.command("SOURCE:CURRENT:LIMIT " + str(current_limit)) @@ -420,8 +429,7 @@ def set_voltage_limit(self, voltage_limit): The maximum settable voltage in volts. Must be between 0 and 100 volts. """ - if isinstance(voltage_limit, bool) or not isinstance(voltage_limit, numbers.Real): - raise ValueError(f"Voltage limit must be a number, got {type(voltage_limit).__name__}") + self._validate_numeric(voltage_limit, "Voltage limit") if voltage_limit < 0 or voltage_limit > 100: raise ValueError(f"Voltage limit must be between 0 and 100 volts, got {voltage_limit}") self.command("SOURCE:VOLTAGE:LIMIT " + str(voltage_limit)) @@ -434,8 +442,7 @@ def set_current_mode_voltage_protection(self, max_voltage): The maximum permissible voltage. Must be between 1 and 100 volts. """ - if isinstance(max_voltage, bool) or not isinstance(max_voltage, numbers.Real): - raise ValueError(f"Max voltage must be a number, got {type(max_voltage).__name__}") + self._validate_numeric(max_voltage, "Max voltage") if max_voltage < 1 or max_voltage > 100: raise ValueError(f"Max voltage must be between 1 and 100 volts, got {max_voltage}") self.command("SOURCE:CURRENT:PROTECTION " + str(max_voltage)) @@ -448,8 +455,7 @@ def set_voltage_mode_current_protection(self, max_current): The maximum permissible current in amps. """ - if isinstance(max_current, bool) or not isinstance(max_current, numbers.Real): - raise ValueError(f"Max current must be a number, got {type(max_current).__name__}") + self._validate_numeric(max_current, "Max current") if max_current < 0: raise ValueError(f"Max current must be non-negative, got {max_current}") self.command("SOURCE:VOLTAGE:PROTECTION " + str(max_current)) From 001423b416028a148a0c872c9185c4b1e9086463 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:29:43 -0500 Subject: [PATCH 07/12] Add tests for all new validation and safety guard code paths Add comprehensive test coverage for all validation logic introduced in this branch: - Factory reset confirm guards: test rejection without confirm for all 5 instruments (EM power supply, teslameter, fast hall, model 121, 240) - PID validation: negative values, non-numeric types, bool rejection - Setpoint validation: non-numeric, bool rejection, negative allowed (Celsius mode) - Temperature limit validation: non-numeric, negative, bool rejection - Model 155 output validation: DC current/voltage type checks, sine method type checks, limit range checks, protection range checks - Curve correctness: curvature=0.0 regression test, set_curve with enumerate regression test - Model 335 NTC_RTD: regression test for the tuple membership fix to ensure both PLATINUM_RTD and NTC_RTD branches are exercised Co-Authored-By: Claude Opus 4.6 --- tests/test_121.py | 4 ++ tests/test_240.py | 4 ++ tests/test_em_power_supply.py | 8 +++ tests/test_fast_hall.py | 8 +++ tests/test_model_155.py | 99 +++++++++++++++++++++++++++ tests/test_model_335.py | 16 +++++ tests/test_temperature_controllers.py | 61 +++++++++++++++++ tests/test_teslameter.py | 8 +++ tests/utils.py | 11 ++- 9 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 tests/test_model_155.py diff --git a/tests/test_121.py b/tests/test_121.py index b9b8743..0d1580d 100644 --- a/tests/test_121.py +++ b/tests/test_121.py @@ -74,6 +74,10 @@ def test_factory_defaults(self): self.dut.set_factory_defaults(confirm=True) self.assertIn('DFLT 99', self.fake_connection.get_outgoing_message()) + def test_factory_defaults_raises_without_confirm(self): + with self.assertRaises(ValueError): + self.dut.set_factory_defaults() + def test_enable_keypad(self): self.fake_connection.setup_response('') self.dut.unlock_front_panel() diff --git a/tests/test_240.py b/tests/test_240.py index 9817c17..6f6c870 100644 --- a/tests/test_240.py +++ b/tests/test_240.py @@ -145,3 +145,7 @@ def test_set_factory_defaults(self): self.fake_connection.setup_response('0') self.dut.set_factory_defaults(confirm=True) self.assertIn('DFLT 99', self.fake_connection.get_outgoing_message()) + + def test_set_factory_defaults_raises_without_confirm(self): + with self.assertRaises(ValueError): + self.dut.set_factory_defaults() diff --git a/tests/test_em_power_supply.py b/tests/test_em_power_supply.py index 997f343..8f4181f 100644 --- a/tests/test_em_power_supply.py +++ b/tests/test_em_power_supply.py @@ -172,6 +172,14 @@ def test_set_factory_defaults(self): self.dut.set_factory_defaults(confirm=True) self.assertIn("DFLT 99", self.fake_connection.get_outgoing_message()) + def test_set_factory_defaults_raises_without_confirm(self): + with self.assertRaises(ValueError): + self.dut.set_factory_defaults() + + def test_set_factory_defaults_raises_with_confirm_false(self): + with self.assertRaises(ValueError): + self.dut.set_factory_defaults(confirm=False) + def test_reset_instrument(self): self.fake_connection.setup_response("0") self.dut.reset_instrument() diff --git a/tests/test_fast_hall.py b/tests/test_fast_hall.py index c9845b8..c8cc7fa 100644 --- a/tests/test_fast_hall.py +++ b/tests/test_fast_hall.py @@ -15,6 +15,14 @@ def test_factory_reset(self): self.dut.factory_reset(confirm=True) self.assertIn('SYSTEM:FACTORYRESET', self.fake_connection.get_outgoing_message()) + def test_factory_reset_raises_without_confirm(self): + with self.assertRaises(ValueError): + self.dut.factory_reset() + + def test_factory_reset_raises_with_confirm_false(self): + with self.assertRaises(ValueError): + self.dut.factory_reset(confirm=False) + def test_contact_check_reset(self): self.fake_connection.setup_response('No error') self.dut.reset_contact_check_measurement() diff --git a/tests/test_model_155.py b/tests/test_model_155.py new file mode 100644 index 0000000..e6add87 --- /dev/null +++ b/tests/test_model_155.py @@ -0,0 +1,99 @@ +from tests.utils import TestWithFakePrecisionSource + + +class TestOutputValidation(TestWithFakePrecisionSource): + + def test_output_dc_current_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.output_dc_current("abc") + + def test_output_dc_current_bool_raises(self): + with self.assertRaises(ValueError): + self.dut.output_dc_current(True) + + def test_output_dc_voltage_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.output_dc_voltage("abc") + + def test_output_dc_voltage_bool_raises(self): + with self.assertRaises(ValueError): + self.dut.output_dc_voltage(True) + + def test_output_sine_current_non_numeric_amplitude_raises(self): + with self.assertRaises(ValueError): + self.dut.output_sine_current("abc", 1000) + + def test_output_sine_current_non_numeric_frequency_raises(self): + with self.assertRaises(ValueError): + self.dut.output_sine_current(0.01, "abc") + + def test_output_sine_voltage_non_numeric_amplitude_raises(self): + with self.assertRaises(ValueError): + self.dut.output_sine_voltage("abc", 1000) + + def test_output_sine_voltage_bool_amplitude_raises(self): + with self.assertRaises(ValueError): + self.dut.output_sine_voltage(True, 1000) + + +class TestLimitValidation(TestWithFakePrecisionSource): + + def test_set_current_limit_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.set_current_limit("abc") + + def test_set_current_limit_too_high_raises(self): + with self.assertRaises(ValueError): + self.dut.set_current_limit(0.2) + + def test_set_current_limit_negative_raises(self): + with self.assertRaises(ValueError): + self.dut.set_current_limit(-0.01) + + def test_set_current_limit_valid(self): + self.fake_connection.setup_response('No error') + self.dut.set_current_limit(0.05) + self.assertIn("SOURCE:CURRENT:LIMIT 0.05", self.fake_connection.get_outgoing_message()) + + def test_set_voltage_limit_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.set_voltage_limit("abc") + + def test_set_voltage_limit_too_high_raises(self): + with self.assertRaises(ValueError): + self.dut.set_voltage_limit(101) + + def test_set_voltage_limit_negative_raises(self): + with self.assertRaises(ValueError): + self.dut.set_voltage_limit(-1) + + def test_set_voltage_limit_valid(self): + self.fake_connection.setup_response('No error') + self.dut.set_voltage_limit(50) + self.assertIn("SOURCE:VOLTAGE:LIMIT 50", self.fake_connection.get_outgoing_message()) + + def test_set_current_mode_voltage_protection_too_low_raises(self): + with self.assertRaises(ValueError): + self.dut.set_current_mode_voltage_protection(0.5) + + def test_set_current_mode_voltage_protection_too_high_raises(self): + with self.assertRaises(ValueError): + self.dut.set_current_mode_voltage_protection(101) + + def test_set_current_mode_voltage_protection_valid(self): + self.fake_connection.setup_response('No error') + self.dut.set_current_mode_voltage_protection(50) + self.assertIn("SOURCE:CURRENT:PROTECTION 50", self.fake_connection.get_outgoing_message()) + + def test_set_voltage_mode_current_protection_negative_raises(self): + with self.assertRaises(ValueError): + self.dut.set_voltage_mode_current_protection(-1) + + def test_set_voltage_mode_current_protection_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.set_voltage_mode_current_protection("abc") + + def test_set_voltage_mode_current_protection_valid(self): + self.fake_connection.setup_response('No error') + self.dut.set_voltage_mode_current_protection(0.05) + self.assertIn("SOURCE:VOLTAGE:PROTECTION 0.05", self.fake_connection.get_outgoing_message()) diff --git a/tests/test_model_335.py b/tests/test_model_335.py index 6f4b95f..5dfdc73 100644 --- a/tests/test_model_335.py +++ b/tests/test_model_335.py @@ -179,6 +179,22 @@ def test_get_input_sensor(self): self.assertIn("INTYPE? A", self.fake_connection.get_outgoing_message()) + def test_get_input_sensor_ntc_rtd(self): + # NTC_RTD is sensor type 3, range 5 + self.fake_connection.setup_response('3,0,5,1,1;0') + response = self.dut.get_input_sensor("A") + self.assertEqual(response.sensor_type, self.dut.InputSensorType.NTC_RTD) + self.assertEqual(response.input_range, self.dut.RTDRange(5)) + self.assertIn("INTYPE? A", self.fake_connection.get_outgoing_message()) + + def test_get_input_sensor_platinum_rtd(self): + # PLATINUM_RTD is sensor type 2, range 3 + self.fake_connection.setup_response('2,0,3,1,1;0') + response = self.dut.get_input_sensor("A") + self.assertEqual(response.sensor_type, self.dut.InputSensorType.PLATINUM_RTD) + self.assertEqual(response.input_range, self.dut.RTDRange(3)) + self.assertIn("INTYPE? A", self.fake_connection.get_outgoing_message()) + def test_get_led_state(self): self.fake_connection.setup_response('1;0') response = self.dut.get_led_state() diff --git a/tests/test_temperature_controllers.py b/tests/test_temperature_controllers.py index 6d4c2dc..8c1387c 100644 --- a/tests/test_temperature_controllers.py +++ b/tests/test_temperature_controllers.py @@ -448,3 +448,64 @@ def test_get_operation_event_enable(self): self.assertEqual(register.calibration_error, response.calibration_error) self.assertEqual(register.processor_communication_error, response.processor_communication_error) self.assertIn("OPSTR?", self.fake_connection.get_outgoing_message()) + + +class TestValidation(TestWithFakeModel372): + + def test_set_heater_pid_negative_gain_raises(self): + with self.assertRaises(ValueError): + self.dut.set_heater_pid(1, -1, 5, 5) + + def test_set_heater_pid_negative_integral_raises(self): + with self.assertRaises(ValueError): + self.dut.set_heater_pid(1, 5, -1, 5) + + def test_set_heater_pid_negative_derivative_raises(self): + with self.assertRaises(ValueError): + self.dut.set_heater_pid(1, 5, 5, -1) + + def test_set_heater_pid_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.set_heater_pid(1, "abc", 5, 5) + + def test_set_heater_pid_bool_raises(self): + with self.assertRaises(ValueError): + self.dut.set_heater_pid(1, True, 5, 5) + + def test_set_control_setpoint_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.set_control_setpoint(2, "abc") + + def test_set_control_setpoint_bool_raises(self): + with self.assertRaises(ValueError): + self.dut.set_control_setpoint(2, True) + + def test_set_control_setpoint_negative_allowed(self): + self.fake_connection.setup_response('0') + self.dut.set_control_setpoint(2, -10.5) + self.assertIn('SETP 2,-10.5', self.fake_connection.get_outgoing_message()) + + def test_set_temperature_limit_non_numeric_raises(self): + with self.assertRaises(ValueError): + self.dut.set_temperature_limit("A", "abc") + + def test_set_temperature_limit_negative_raises(self): + with self.assertRaises(ValueError): + self.dut.set_temperature_limit("A", -10) + + def test_set_temperature_limit_bool_raises(self): + with self.assertRaises(ValueError): + self.dut.set_temperature_limit("A", True) + + def test_set_curve_data_point_curvature_zero(self): + self.fake_connection.setup_response('0') + self.dut.set_curve_data_point(22, 1, 2.86, 9.252, 0.0) + self.assertIn("CRVPT 22,1,2.86,9.252,0.0", self.fake_connection.get_outgoing_message()) + + def test_set_curve(self): + for _ in range(3): + self.fake_connection.setup_response('0') + self.dut.set_curve(22, [(1.0, 2.0), (3.0, 4.0)]) + self.assertIn('CRVDEL 22', self.fake_connection.get_outgoing_message()) + self.assertIn('CRVPT 22,1,1.0,2.0', self.fake_connection.get_outgoing_message()) + self.assertIn('CRVPT 22,2,3.0,4.0', self.fake_connection.get_outgoing_message()) diff --git a/tests/test_teslameter.py b/tests/test_teslameter.py index bc50eb0..cebf5e9 100644 --- a/tests/test_teslameter.py +++ b/tests/test_teslameter.py @@ -360,6 +360,14 @@ def test_factory_reset(self): self.dut.factory_reset(confirm=True) self.assertIn('SYSTEM:FACTORYRESET', self.fake_connection.get_outgoing_message()) + def test_factory_reset_raises_without_confirm(self): + with self.assertRaises(ValueError): + self.dut.factory_reset() + + def test_factory_reset_raises_with_confirm_false(self): + with self.assertRaises(ValueError): + self.dut.factory_reset(confirm=False) + class TestStatusRegisters(TestWithFakeTeslameter): def test_modification_of_operation_register(self): diff --git a/tests/utils.py b/tests/utils.py index 4ddb601..464d997 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -3,7 +3,7 @@ import unittest from lakeshore import Teslameter, FastHall, Model121, Model372, Model335, Model240, Model224, Model336, SSMSystem, \ - Model643 + Model643, PrecisionSource fake_dut_comms_log = logging.getLogger('fake_dut_comms') @@ -148,6 +148,15 @@ def setUp(self): self.fake_connection.reset() # Clear startup activity +class TestWithFakePrecisionSource(unittest.TestCase): + def setUp(self): + self.fake_connection = FakeDutConnection() + self.fake_connection.setup_response('LSCI,MODEL155,FakeSerial,999.999.999') + self.fake_connection.setup_response('No error') + self.dut = PrecisionSource(connection=self.fake_connection) + self.fake_connection.reset() + + class TestWithFakeModel336(unittest.TestCase): def setUp(self): self.fake_connection = FakeDutConnection() From 3b8b06101a384e0fda7888d91c94696cb819d152 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:37:34 -0500 Subject: [PATCH 08/12] Reject NaN/inf in numeric validation and fix empty curve return value - Add math.isfinite() checks to all numeric validation in temperature_controllers.py and model_155.py so that float('nan') and float('inf') are rejected instead of being sent to instruments - Fix get_curve() in both temperature_controllers.py and model_224.py to return an empty list for empty curves by initializing true_point_index to -1 (so data_points[:0] = []) instead of 0 (which returned one spurious zero-tuple) Co-Authored-By: Claude Opus 4.6 --- lakeshore/model_155.py | 5 ++++- lakeshore/model_224.py | 2 +- lakeshore/temperature_controllers.py | 13 +++++++------ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lakeshore/model_155.py b/lakeshore/model_155.py index f383e45..e9640fc 100644 --- a/lakeshore/model_155.py +++ b/lakeshore/model_155.py @@ -2,6 +2,7 @@ from time import sleep import itertools +import math import numbers from .xip_instrument import XIPInstrument, RegisterBase, StatusByteRegister, StandardEventRegister @@ -234,9 +235,11 @@ def route_terminals(self, output_connections_location="REAR"): @staticmethod def _validate_numeric(value, name): - """Validate that a value is a real number (not bool).""" + """Validate that a value is a finite real number (not bool, NaN, or inf).""" if isinstance(value, bool) or not isinstance(value, numbers.Real): raise ValueError(f"{name} must be a number, got {type(value).__name__}") + if not math.isfinite(value): + raise ValueError(f"{name} must be finite, got {value}") def output_sine_current(self, amplitude, frequency, offset=0.0, phase=0.0): """Configures and enables the source output to be a sine wave current source. diff --git a/lakeshore/model_224.py b/lakeshore/model_224.py index 855727e..ab4726e 100644 --- a/lakeshore/model_224.py +++ b/lakeshore/model_224.py @@ -873,7 +873,7 @@ def get_curve(self, curve): """ data_points = [] - true_point_index = 0 + true_point_index = -1 for i in range(0, 200): point = self.get_curve_data_point(curve, i + 1) data_points.append(point) diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index cc2276a..eca2162 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -1,5 +1,6 @@ """Implements a parent class for temperature controllers that contains shared methods between similar instruments.""" +import math import numbers import serial from .generic_instrument import GenericInstrument, InstrumentException, RegisterBase @@ -404,7 +405,7 @@ def get_curve(self, curve): (sensor_units: float, temp_value: float, curvature_value: float (optional)). """ - true_point_index = 0 + true_point_index = -1 data_points = [] for i in range(0, 200): point = self.get_curve_data_point(curve, i + 1) @@ -1044,11 +1045,11 @@ def set_heater_pid(self, output, gain, integral, derivative): The ramp rate is configured in field units per second. """ - if isinstance(gain, bool) or not isinstance(gain, numbers.Real) or gain < 0: + if isinstance(gain, bool) or not isinstance(gain, numbers.Real) or not math.isfinite(gain) or gain < 0: raise ValueError(f"Gain must be a non-negative number, got {gain}") - if isinstance(integral, bool) or not isinstance(integral, numbers.Real) or integral < 0: + if isinstance(integral, bool) or not isinstance(integral, numbers.Real) or not math.isfinite(integral) or integral < 0: raise ValueError(f"Integral must be a non-negative number, got {integral}") - if isinstance(derivative, bool) or not isinstance(derivative, numbers.Real) or derivative < 0: + if isinstance(derivative, bool) or not isinstance(derivative, numbers.Real) or not math.isfinite(derivative) or derivative < 0: raise ValueError(f"Derivative must be a non-negative number, got {derivative}") self.command(f"PID {output},{gain},{integral},{derivative}") @@ -1315,7 +1316,7 @@ def set_control_setpoint(self, output, value): The value for the set-point (in the preferred units of the control loop sensor). """ - if isinstance(value, bool) or not isinstance(value, numbers.Real): + if isinstance(value, bool) or not isinstance(value, numbers.Real) or not math.isfinite(value): raise ValueError(f"Setpoint must be a number, got {type(value).__name__}") self.command(f"SETP {output},{value}") @@ -1354,7 +1355,7 @@ def set_temperature_limit(self, input_channel, limit): A limit of zero will turn the feature off. """ - if isinstance(limit, bool) or not isinstance(limit, numbers.Real): + if isinstance(limit, bool) or not isinstance(limit, numbers.Real) or not math.isfinite(limit): raise ValueError(f"Temperature limit must be a number, got {type(limit).__name__}") if limit < 0: raise ValueError(f"Temperature limit {limit} must be non-negative") From 9ca316e7c12bd48ff6c81d385edfc9a139769676 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:38:57 -0500 Subject: [PATCH 09/12] Add Model 224 factory reset confirm guard and backward-compat PID key - model_224.py: Add confirm=False guard to set_to_factory_defaults() which was missed in the earlier factory reset safety pass - temperature_controllers.py: Include deprecated "ramp_rate" key alongside new "derivative" key in get_heater_pid() return dict for backward compatibility with existing callers - Update PID tests to verify both "derivative" and "ramp_rate" keys Co-Authored-By: Claude Opus 4.6 --- lakeshore/model_224.py | 11 +++++++++-- lakeshore/temperature_controllers.py | 8 ++++++-- tests/test_model_335.py | 8 ++++---- tests/test_model_336.py | 8 ++++---- tests/test_temperature_controllers.py | 9 ++++----- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/lakeshore/model_224.py b/lakeshore/model_224.py index ab4726e..25b5b1c 100644 --- a/lakeshore/model_224.py +++ b/lakeshore/model_224.py @@ -346,8 +346,15 @@ def set_wait_to_continue(self): self.command("*WAI") - def set_to_factory_defaults(self): - """Sets all the settings and configurations to their factory default values.""" + def set_to_factory_defaults(self, confirm=False): + """Sets all the settings and configurations to their factory default values. + + Args: + confirm (bool): Must be True to execute. Prevents accidental factory reset. + """ + if not confirm: + raise ValueError( + "Factory reset will clear all settings. Pass confirm=True to proceed.") self.command("DFLT 99") def get_reading_status(self, input_channel): diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index eca2162..47f7e43 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -3,6 +3,7 @@ import math import numbers import serial +from warnings import warn from .generic_instrument import GenericInstrument, InstrumentException, RegisterBase from .temperature_controllers_enums import TemperatureControllerEnums @@ -1062,17 +1063,20 @@ def get_heater_pid(self, output): Returns: (dict): - {"gain": float, "integral": float, "derivative": float} + {"gain": float, "integral": float, "derivative": float, "ramp_rate": float} gain: Proportional term in PID control. integral: Integral term in PID control. derivative: Derivative term in PID control. + ramp_rate: Deprecated alias for derivative. Use "derivative" instead. """ pid_values = self.query(f"PID? {output}") pid_values = pid_values.split(",") + derivative = float(pid_values[2]) return {"gain": float(pid_values[0]), "integral": float(pid_values[1]), - "derivative": float(pid_values[2])} + "derivative": derivative, + "ramp_rate": derivative} def set_setpoint_ramp_parameter(self, output, ramp_enable, rate_value): """Sets the control loop of a particular output. diff --git a/tests/test_model_335.py b/tests/test_model_335.py index 5dfdc73..b288a4d 100644 --- a/tests/test_model_335.py +++ b/tests/test_model_335.py @@ -242,10 +242,10 @@ def test_get_heater_output_mode(self): def test_get_heater_pid(self): self.fake_connection.setup_response('4.25,6.1,0;0') response = self.dut.get_heater_pid(1) - pid_settings = {"gain": 4.25, - "integral": 6.1, - "derivative": 0} - self.assertDictEqual(response, pid_settings) + self.assertAlmostEqual(response['gain'], 4.25) + self.assertAlmostEqual(response['integral'], 6.1) + self.assertAlmostEqual(response['derivative'], 0) + self.assertAlmostEqual(response['ramp_rate'], 0) def test_get_output_2_polarity(self): self.fake_connection.setup_response('1;0') diff --git a/tests/test_model_336.py b/tests/test_model_336.py index 7c149b7..62a8475 100644 --- a/tests/test_model_336.py +++ b/tests/test_model_336.py @@ -377,10 +377,10 @@ def test_get_heater_output_mode(self): def test_get_heater_pid(self): self.fake_connection.setup_response('4.25,6.1,0;0') response = self.dut.get_heater_pid(1) - pid_settings = {"gain": 4.25, - "integral": 6.1, - "derivative": 0} - self.assertDictEqual(response, pid_settings) + self.assertAlmostEqual(response['gain'], 4.25) + self.assertAlmostEqual(response['integral'], 6.1) + self.assertAlmostEqual(response['derivative'], 0) + self.assertAlmostEqual(response['ramp_rate'], 0) self.assertIn("PID? 1", self.fake_connection.get_outgoing_message()) def test_get_heater_range(self): diff --git a/tests/test_temperature_controllers.py b/tests/test_temperature_controllers.py index 8c1387c..bcb97f5 100644 --- a/tests/test_temperature_controllers.py +++ b/tests/test_temperature_controllers.py @@ -231,13 +231,12 @@ def test_set_heater_pid(self): self.assertIn('PID 1,34.56,56.78,98.76', self.fake_connection.get_outgoing_message()) def test_get_heater_pid(self): - pid = {'gain': 34.56, - 'integral': 56.78, - 'derivative': 98.76} - self.fake_connection.setup_response('34.56,56.78,98.76;0') response = self.dut.get_heater_pid(2) - self.assertDictEqual(response, pid) + self.assertAlmostEqual(response['gain'], 34.56) + self.assertAlmostEqual(response['integral'], 56.78) + self.assertAlmostEqual(response['derivative'], 98.76) + self.assertAlmostEqual(response['ramp_rate'], 98.76) self.assertIn('PID? 2', self.fake_connection.get_outgoing_message()) def test_set_setpoint_ramp_parameter(self): From 61ad1530118b5ce557924a6d6ab0df9137af2c2c Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:41:55 -0500 Subject: [PATCH 10/12] Standardize validation with _validate_numeric helper Extract shared numeric validation (bool exclusion, numbers.Real check, math.isfinite) into a reusable _validate_numeric static method. Refactor set_heater_pid, set_control_setpoint, and set_temperature_limit to use it, giving consistent error messages across all validation paths. Co-Authored-By: Claude Opus 4.6 --- lakeshore/temperature_controllers.py | 31 ++++++++++++++++++---------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index 47f7e43..2d7b214 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -142,6 +142,14 @@ def __init__(self, serial_number, com_port, baud_rate, timeout, ip_address, tcp_ GenericInstrument.__init__(self, serial_number, com_port, baud_rate, 7, 1, serial.PARITY_ODD, False, False, timeout, ip_address, tcp_port, **kwargs) + @staticmethod + def _validate_numeric(value, name): + """Validate that a value is a finite real number (not bool, NaN, or inf).""" + if isinstance(value, bool) or not isinstance(value, numbers.Real): + raise ValueError(f"{name} must be a number, got {type(value).__name__}") + if not math.isfinite(value): + raise ValueError(f"{name} must be finite, got {value}") + @staticmethod def _error_check(error_code): event_register = StandardEventRegister.from_integer(error_code) @@ -1046,12 +1054,15 @@ def set_heater_pid(self, output, gain, integral, derivative): The ramp rate is configured in field units per second. """ - if isinstance(gain, bool) or not isinstance(gain, numbers.Real) or not math.isfinite(gain) or gain < 0: - raise ValueError(f"Gain must be a non-negative number, got {gain}") - if isinstance(integral, bool) or not isinstance(integral, numbers.Real) or not math.isfinite(integral) or integral < 0: - raise ValueError(f"Integral must be a non-negative number, got {integral}") - if isinstance(derivative, bool) or not isinstance(derivative, numbers.Real) or not math.isfinite(derivative) or derivative < 0: - raise ValueError(f"Derivative must be a non-negative number, got {derivative}") + self._validate_numeric(gain, "Gain") + self._validate_numeric(integral, "Integral") + self._validate_numeric(derivative, "Derivative") + if gain < 0: + raise ValueError(f"Gain must be non-negative, got {gain}") + if integral < 0: + raise ValueError(f"Integral must be non-negative, got {integral}") + if derivative < 0: + raise ValueError(f"Derivative must be non-negative, got {derivative}") self.command(f"PID {output},{gain},{integral},{derivative}") def get_heater_pid(self, output): @@ -1320,8 +1331,7 @@ def set_control_setpoint(self, output, value): The value for the set-point (in the preferred units of the control loop sensor). """ - if isinstance(value, bool) or not isinstance(value, numbers.Real) or not math.isfinite(value): - raise ValueError(f"Setpoint must be a number, got {type(value).__name__}") + self._validate_numeric(value, "Setpoint") self.command(f"SETP {output},{value}") def get_control_setpoint(self, output): @@ -1359,10 +1369,9 @@ def set_temperature_limit(self, input_channel, limit): A limit of zero will turn the feature off. """ - if isinstance(limit, bool) or not isinstance(limit, numbers.Real) or not math.isfinite(limit): - raise ValueError(f"Temperature limit must be a number, got {type(limit).__name__}") + self._validate_numeric(limit, "Temperature limit") if limit < 0: - raise ValueError(f"Temperature limit {limit} must be non-negative") + raise ValueError(f"Temperature limit must be non-negative, got {limit}") self.command(f"TLIMIT {input_channel},{limit}") def get_temperature_limit(self, input_channel): From e254ec7e73d162c9e1dac5ad4915042733baa17a Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:43:39 -0500 Subject: [PATCH 11/12] Add boundary, NaN/inf, and happy-path tests Add zero-value boundary tests for PID and temperature limit, NaN/inf rejection tests for setpoint, PID, temperature limit, and Model 155 limits, boundary tests for Model 155 current/voltage limits, happy-path tests for Model 155 output methods, and Model 224 factory reset confirm guard tests. Co-Authored-By: Claude Opus 4.6 --- tests/test_224.py | 15 ++++++ tests/test_model_155.py | 70 +++++++++++++++++++++++++++ tests/test_temperature_controllers.py | 35 ++++++++++++++ 3 files changed, 120 insertions(+) diff --git a/tests/test_224.py b/tests/test_224.py index 0245743..4e23deb 100644 --- a/tests/test_224.py +++ b/tests/test_224.py @@ -354,3 +354,18 @@ def test_get_display_configuration_without_number_of_fields(self): response = self.dut.get_display_configuration() self.assertDictEqual(response, expected_response) self.assertIn("DISPLAY?", self.fake_connection.get_outgoing_message()) + + +class TestFactoryDefaults(TestWithFakeModel224): + def test_set_to_factory_defaults(self): + self.fake_connection.setup_response('0') + self.dut.set_to_factory_defaults(confirm=True) + self.assertIn('DFLT 99', self.fake_connection.get_outgoing_message()) + + def test_set_to_factory_defaults_raises_without_confirm(self): + with self.assertRaises(ValueError): + self.dut.set_to_factory_defaults() + + def test_set_to_factory_defaults_raises_with_confirm_false(self): + with self.assertRaises(ValueError): + self.dut.set_to_factory_defaults(confirm=False) diff --git a/tests/test_model_155.py b/tests/test_model_155.py index e6add87..24f0389 100644 --- a/tests/test_model_155.py +++ b/tests/test_model_155.py @@ -97,3 +97,73 @@ def test_set_voltage_mode_current_protection_valid(self): self.fake_connection.setup_response('No error') self.dut.set_voltage_mode_current_protection(0.05) self.assertIn("SOURCE:VOLTAGE:PROTECTION 0.05", self.fake_connection.get_outgoing_message()) + + def test_set_current_limit_at_zero(self): + self.fake_connection.setup_response('No error') + self.dut.set_current_limit(0) + self.assertIn("SOURCE:CURRENT:LIMIT 0", self.fake_connection.get_outgoing_message()) + + def test_set_current_limit_at_max(self): + self.fake_connection.setup_response('No error') + self.dut.set_current_limit(0.1) + self.assertIn("SOURCE:CURRENT:LIMIT 0.1", self.fake_connection.get_outgoing_message()) + + def test_set_voltage_limit_at_zero(self): + self.fake_connection.setup_response('No error') + self.dut.set_voltage_limit(0) + self.assertIn("SOURCE:VOLTAGE:LIMIT 0", self.fake_connection.get_outgoing_message()) + + def test_set_voltage_limit_at_max(self): + self.fake_connection.setup_response('No error') + self.dut.set_voltage_limit(100) + self.assertIn("SOURCE:VOLTAGE:LIMIT 100", self.fake_connection.get_outgoing_message()) + + def test_set_current_mode_voltage_protection_at_min(self): + self.fake_connection.setup_response('No error') + self.dut.set_current_mode_voltage_protection(1) + self.assertIn("SOURCE:CURRENT:PROTECTION 1", self.fake_connection.get_outgoing_message()) + + def test_set_current_mode_voltage_protection_at_max(self): + self.fake_connection.setup_response('No error') + self.dut.set_current_mode_voltage_protection(100) + self.assertIn("SOURCE:CURRENT:PROTECTION 100", self.fake_connection.get_outgoing_message()) + + def test_set_voltage_mode_current_protection_at_zero(self): + self.fake_connection.setup_response('No error') + self.dut.set_voltage_mode_current_protection(0) + self.assertIn("SOURCE:VOLTAGE:PROTECTION 0", self.fake_connection.get_outgoing_message()) + + def test_set_current_limit_nan_raises(self): + with self.assertRaises(ValueError): + self.dut.set_current_limit(float('nan')) + + def test_set_voltage_limit_inf_raises(self): + with self.assertRaises(ValueError): + self.dut.set_voltage_limit(float('inf')) + + +class TestOutputHappyPath(TestWithFakePrecisionSource): + + def test_output_dc_current(self): + for _ in range(4): + self.fake_connection.setup_response('No error') + self.dut.output_dc_current(0.05) + self.assertIn("SOURCE:FUNCTION:MODE CURRENT", self.fake_connection.get_outgoing_message()) + + def test_output_dc_voltage(self): + for _ in range(4): + self.fake_connection.setup_response('No error') + self.dut.output_dc_voltage(5.0) + self.assertIn("SOURCE:FUNCTION:MODE VOLTAGE", self.fake_connection.get_outgoing_message()) + + def test_output_sine_current(self): + for _ in range(7): + self.fake_connection.setup_response('No error') + self.dut.output_sine_current(0.01, 1000) + self.assertIn("SOURCE:FUNCTION:MODE CURRENT", self.fake_connection.get_outgoing_message()) + + def test_output_sine_voltage(self): + for _ in range(7): + self.fake_connection.setup_response('No error') + self.dut.output_sine_voltage(1.0, 1000) + self.assertIn("SOURCE:FUNCTION:MODE VOLTAGE", self.fake_connection.get_outgoing_message()) diff --git a/tests/test_temperature_controllers.py b/tests/test_temperature_controllers.py index bcb97f5..9054fdd 100644 --- a/tests/test_temperature_controllers.py +++ b/tests/test_temperature_controllers.py @@ -501,6 +501,41 @@ def test_set_curve_data_point_curvature_zero(self): self.dut.set_curve_data_point(22, 1, 2.86, 9.252, 0.0) self.assertIn("CRVPT 22,1,2.86,9.252,0.0", self.fake_connection.get_outgoing_message()) + def test_set_heater_pid_zero_values(self): + self.fake_connection.setup_response('0') + self.dut.set_heater_pid(1, 0, 0, 0) + self.assertIn("PID 1,0,0,0", self.fake_connection.get_outgoing_message()) + + def test_set_heater_pid_nan_raises(self): + import math + with self.assertRaises(ValueError): + self.dut.set_heater_pid(1, float('nan'), 5, 5) + + def test_set_heater_pid_inf_raises(self): + with self.assertRaises(ValueError): + self.dut.set_heater_pid(1, float('inf'), 5, 5) + + def test_set_control_setpoint_nan_raises(self): + with self.assertRaises(ValueError): + self.dut.set_control_setpoint(2, float('nan')) + + def test_set_control_setpoint_inf_raises(self): + with self.assertRaises(ValueError): + self.dut.set_control_setpoint(2, float('inf')) + + def test_set_temperature_limit_zero(self): + self.fake_connection.setup_response('0') + self.dut.set_temperature_limit("A", 0) + self.assertIn("TLIMIT A,0", self.fake_connection.get_outgoing_message()) + + def test_set_temperature_limit_nan_raises(self): + with self.assertRaises(ValueError): + self.dut.set_temperature_limit("A", float('nan')) + + def test_set_temperature_limit_inf_raises(self): + with self.assertRaises(ValueError): + self.dut.set_temperature_limit("A", float('inf')) + def test_set_curve(self): for _ in range(3): self.fake_connection.setup_response('0') From 86217b716296dbe1123eb752c8e8b106618ac05e Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 21:11:25 -0500 Subject: [PATCH 12/12] Add clarifying comment for backward-compatible ramp_rate key Co-Authored-By: Claude Opus 4.6 --- lakeshore/temperature_controllers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index 2d7b214..0e8b93e 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -1084,6 +1084,9 @@ def get_heater_pid(self, output): pid_values = self.query(f"PID? {output}") pid_values = pid_values.split(",") derivative = float(pid_values[2]) + # "ramp_rate" was the original (incorrect) key name for the derivative PID term. + # Kept for backward compatibility — new code should use "derivative" to match + # the set_heater_pid() parameter name. return {"gain": float(pid_values[0]), "integral": float(pid_values[1]), "derivative": derivative,