Fix temperature controller, XIP, and Model 155 safety issues - #19
Open
jfichtner wants to merge 12 commits into
Open
Fix temperature controller, XIP, and Model 155 safety issues#19jfichtner wants to merge 12 commits into
jfichtner wants to merge 12 commits into
Conversation
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes critical bugs and adds safety validations to temperature controller and precision source methods.
Changes:
- Fixed Model 335 tuple membership bug that prevented correct sensor type detection for PLATINUM_RTD and NTC_RTD sensors
- Fixed XIP instrument
to_integer()call site bug and curve indexing off-by-one errors - Added numeric validation with NaN/inf/bool rejection to temperature controller and Model 155 methods
- Added
confirm=Truesafety guards to all factory reset methods across 7 instrument types - Preserved backward compatibility in
get_heater_pidby returning bothderivativeand deprecatedramp_ratekeys
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/utils.py | Added TestWithFakePrecisionSource test fixture for Model 155 tests |
| tests/test_teslameter.py | Added factory reset confirm guard tests |
| tests/test_temperature_controllers.py | Added validation tests for PID, setpoint, and temperature limit methods; updated get_heater_pid test for backward compatibility |
| tests/test_model_336.py | Updated get_heater_pid test to check both derivative and ramp_rate keys |
| tests/test_model_335.py | Added tests for NTC_RTD and PLATINUM_RTD sensor types; updated get_heater_pid test |
| tests/test_model_155.py | New file with comprehensive validation tests for DC/sine output and limit methods |
| tests/test_fast_hall.py | Added factory reset confirm guard tests |
| tests/test_em_power_supply.py | Added factory reset confirm guard tests |
| tests/test_240.py | Added factory reset confirm guard test |
| tests/test_224.py | Added factory reset confirm guard tests |
| tests/test_121.py | Added factory reset confirm guard test |
| lakeshore/xip_instrument.py | Fixed to_integer() call from class method to instance method; added confirm parameter to factory_reset |
| lakeshore/temperature_controllers.py | Added numeric validation to set_heater_pid, set_control_setpoint, and set_temperature_limit; fixed curve indexing bugs; added derivative key to get_heater_pid return value |
| lakeshore/model_335.py | Fixed tuple membership bug (or → ,) for RTD sensor type checking |
| lakeshore/model_240.py | Added confirm parameter to set_factory_defaults |
| lakeshore/model_224.py | Added confirm parameter to set_to_factory_defaults; fixed set_curve enumerate bug |
| lakeshore/model_155.py | Added numeric validation to output and limit methods; fixed documentation typos |
| lakeshore/model_121.py | Added confirm parameter to set_factory_defaults |
| lakeshore/em_power_supply.py | Added confirm parameter to set_factory_defaults with enhanced warning message |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import math | ||
| import numbers | ||
| import serial | ||
| from warnings import warn |
There was a problem hiding this comment.
The warn import is added but never used in this file. Consider removing this unused import.
Suggested change
| from warnings import warn |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
or→,) and XIPto_integer()call siteenumerate()toset_curve, fixget_curveoff-by-one, fix curvature falsy check, handle empty curvesset_heater_pid,set_control_setpoint,set_temperature_limit) and Model 155 precision source methods usingnumbers.Realwithboolexclusion andmath.isfinite()checksconfirm=Truesafety guards to all factory reset methods (XIP, EM power supply, Model 121, Model 224, Model 240) preventing accidental resetsget_heater_pid(returns bothderivativeand deprecatedramp_ratekeys)Test plan
confirm=True🤖 Generated with Claude Code