Skip to content

Fix EM power supply correctness bugs and add safety validation #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jfichtner wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix EM power supply correctness bugs and add safety validation #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8d96232
Fix ERSTE argument order and TOCTOU race in EM power supply
jfichtner Feb 18, 2026
9343353
Add bounds validation to EM power supply safety-critical methods
jfichtner Feb 18, 2026
12a91a4
Harden EM power supply validation against NaN/Inf and add missing bounds
jfichtner Feb 18, 2026
b186f0a
Add validation to remaining safety-critical EM power supply methods
jfichtner Feb 18, 2026
2399fa7
Add comprehensive validation tests and strengthen ERSTE mask test
jfichtner Feb 18, 2026
1453ae5
Fix remaining validation gaps and extract magic numbers to constants
jfichtner Feb 18, 2026
8c02eb7
Complete test coverage for EM power supply validation
jfichtner Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 121 additions & 5 deletions lakeshore/em_power_supply.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,37 @@
"""Implements functionality unique to the Model 643 and 648 electromagnet power supplies."""

import math

import serial
from .generic_instrument import GenericInstrument, RegisterBase, _parse_response, InstrumentException


def _validate_number(value, name):
"""Validate that a value is a finite number (rejects bool, NaN, and inf).

Args:
value: The value to validate.
name (str): Parameter name for error messages.

Raises:
ValueError: If the value is not a finite number.
"""
if isinstance(value, bool):
raise ValueError(f"{name} must be a number, got bool")
if not isinstance(value, (int, float)):
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}")


class ElectromagnetPowerSupply(GenericInstrument):
"""Class object representing a Lake Shore Model 643 or 648 electromagnet power supply."""
vid_pid = [(0x1FB9, 0x0601), (0x1FB9, 0x0602)] # 643, 648

_MAX_CURRENT = 135.1 # Model 648 maximum; Model 643 is 70.1 A

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_MAX_CURRENT is hard-coded to the Model 648 limit (135.1 A) even though this class is used for MODEL643 as well (tests instantiate Model643). This contradicts the docstrings (MODEL643 max 70.1 A) and can allow unsafe currents/limits on a 643. Consider deriving the max current from the connected instrument model (e.g., based on self.model_number) or making it configurable per instance, and update validation accordingly.

Suggested change
_MAX_CURRENT = 135.1 # Model 648 maximum; Model 643 is 70.1 A
# Use the more restrictive of the two model limits (643: 70.1 A, 648: 135.1 A) for safety.
_MAX_CURRENT = 70.1 # A

Copilot uses AI. Check for mistakes.
_MIN_RAMP_RATE = 0.0001 # A/s
_MAX_RAMP_RATE = 50.0 # A/s

def __init__(self,
serial_number=None,
com_port=None,
Expand Down Expand Up @@ -251,7 +275,19 @@ def set_limits(self, max_current, max_ramp_rate):
A. The Model 648 bounds are 0.0000 - 135.1000 A.

max_ramp_rate (float): The maximum output current ramp rate setting allowed (0.0001 - 50.000 A/s).
"""

Raises:
ValueError: If max_current or max_ramp_rate is not a finite number or out of range.
"""
_validate_number(max_current, "Max current")
_validate_number(max_ramp_rate, "Max ramp rate")
if max_current < 0 or max_current > self._MAX_CURRENT:
raise ValueError(
f"Max current {max_current} A is outside valid range (0 - {self._MAX_CURRENT} A)")
if max_ramp_rate < self._MIN_RAMP_RATE or max_ramp_rate > self._MAX_RAMP_RATE:
raise ValueError(
f"Max ramp rate {max_ramp_rate} A/s is outside valid range "
f"({self._MIN_RAMP_RATE} - {self._MAX_RAMP_RATE} A/s)")
self.command(f"LIMIT {max_current}, {max_ramp_rate}")

def get_limits(self):
Expand All @@ -273,7 +309,15 @@ def set_ramp_rate(self, ramp_rate):
Args:
ramp_rate (float): The rate at which the current will ramp when a new output current setting is entered
(0.0001 - 50.000 A/s).

Raises:
ValueError: If ramp_rate is not a finite number or out of range.
"""
_validate_number(ramp_rate, "Ramp rate")
if ramp_rate < self._MIN_RAMP_RATE or ramp_rate > self._MAX_RAMP_RATE:
raise ValueError(
f"Ramp rate {ramp_rate} A/s is outside valid range "
f"({self._MIN_RAMP_RATE} - {self._MAX_RAMP_RATE} A/s)")
self.command(f"RATE {ramp_rate}")

def get_ramp_rate(self):
Expand All @@ -294,7 +338,20 @@ def set_ramp_segment(self, segment, current, ramp_rate):
current (float): Specifies the upper output current setting that will use this segment.
ramp_rate (float): Specifies the rate at which the current will ramp. (0.0001 - 50.000 A/s).

"""
Raises:
ValueError: If segment is not 1-5, or current/ramp_rate is not a finite number or out of range.
"""
if segment not in range(1, 6):
raise ValueError(f"Segment must be 1-5, got {segment}")
Comment on lines +344 to +345

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The segment validation allows bool values because True/False compare equal to 1/0 and will pass segment in range(1, 6). That would format into the command as RSEGS True,..., which the instrument will not accept. Add an explicit type check to require an int (and reject bool) before the range check.

Copilot uses AI. Check for mistakes.
_validate_number(current, "Current")
_validate_number(ramp_rate, "Ramp rate")
if abs(current) > self._MAX_CURRENT:
raise ValueError(
f"Current magnitude {abs(current)} A exceeds maximum safe value of {self._MAX_CURRENT} A")
if ramp_rate < self._MIN_RAMP_RATE or ramp_rate > self._MAX_RAMP_RATE:
raise ValueError(
f"Ramp rate {ramp_rate} A/s is outside valid range "
f"({self._MIN_RAMP_RATE} - {self._MAX_RAMP_RATE} A/s)")
self.command(f"RSEGS {segment}, {current}, {ramp_rate}")

def get_ramp_segment(self, segment):
Expand All @@ -315,7 +372,12 @@ def set_ramp_segments_enable(self, state):

Args:
state (bool): The state of the ramp segments enable. 0=Disabled and 1=Enabled.

Raises:
ValueError: If state is not a bool or 0/1.
"""
if state not in (True, False, 0, 1):

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state not in (True, False, 0, 1) will also accept 0.0/1.0 (and other numeric types equal to 0/1) due to equality semantics, even though the docstring says bool or 0/1. If you want strict input validation, check type explicitly (bool or int) and reject floats/other numerics.

Suggested change
if state not in (True, False, 0, 1):
# Enforce strict type checking: accept only bool or int 0/1, reject floats and other numerics.
if isinstance(state, bool):
valid = True
elif isinstance(state, int) and state in (0, 1):
valid = True
else:
valid = False
if not valid:

Copilot uses AI. Check for mistakes.
raise ValueError(f"Ramp segments enable must be True/False or 0/1, got {state}")
self.command(f"RSEG {int(state)}")

def get_ramp_segments_enable(self):
Expand All @@ -336,7 +398,14 @@ def set_current(self, current):
Args:
current (float): The output current value that the output will ramp to at the present ramp rate. The Model
643 bounds are 0.0000 - +/-70.1000 A. The Model 648 bounds are 0.0000 - +/-135.1000 A.

Raises:
ValueError: If current is not a finite number or exceeds safe bounds.
"""
_validate_number(current, "Current")
if abs(current) > self._MAX_CURRENT:
raise ValueError(
f"Current magnitude {abs(current)} A exceeds maximum safe value of {self._MAX_CURRENT} A")
self.command(f"SETI {current}")

def get_current(self):
Expand Down Expand Up @@ -376,7 +445,12 @@ def set_internal_water(self, mode):

Args:
mode (int): Internal water mode (0, 1, 2, or 3). 0 = Manual-Off, 1 = Manual-On, 2 = Auto, 3 = Disabled.

Raises:
ValueError: If mode is not 0, 1, 2, or 3.
"""
if mode not in (0, 1, 2, 3):
raise ValueError(f"Internal water mode must be 0, 1, 2, or 3, got {mode}")
self.command(f"INTWTR {mode}")
Comment on lines +452 to 454

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode not in (0, 1, 2, 3) will allow bools (True == 1 / False == 0). Passing True currently results in INTWTR True being sent, which is not a valid instrument command. Explicitly reject bool (and/or enforce isinstance(mode, int) and format with int(mode)).

Copilot uses AI. Check for mistakes.

def get_internal_water(self) :
Expand All @@ -393,7 +467,12 @@ def set_magnet_water(self, mode):

Args:
mode (int): Magnet water mode. (0, 1, 2, or 3). 0 = Manual-Off, 1 = Manual-On, 2 = Auto, 3 = Disabled.

Raises:
ValueError: If mode is not 0, 1, 2, or 3.
"""
if mode not in (0, 1, 2, 3):
raise ValueError(f"Magnet water mode must be 0, 1, 2, or 3, got {mode}")
self.command(f"MAGWTR {mode}")
Comment on lines +474 to 476

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode not in (0, 1, 2, 3) will allow bools (True == 1 / False == 0). Passing True will send MAGWTR True, which is not valid for the instrument. Add an explicit bool/type check and/or cast to int when formatting the command.

Copilot uses AI. Check for mistakes.

def get_magnet_water(self) :
Expand All @@ -409,7 +488,12 @@ def set_display_brightness(self, brightness_level):

Args:
brightness_level (int): The display brightness. 0=25%, 1=50%, 2=75%, 3=100%.

Raises:
ValueError: If brightness_level is not 0, 1, 2, or 3.
"""
if brightness_level not in (0, 1, 2, 3):
raise ValueError(f"Brightness level must be 0, 1, 2, or 3, got {brightness_level}")
self.command(f"DISP {brightness_level}")
Comment on lines +495 to 497

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brightness_level not in (0, 1, 2, 3) allows bools (True == 1 / False == 0), and f-string formatting would send DISP True/False. Add an explicit check to reject bool and require an int, and/or format the command with int(brightness_level).

Suggested change
if brightness_level not in (0, 1, 2, 3):
raise ValueError(f"Brightness level must be 0, 1, 2, or 3, got {brightness_level}")
self.command(f"DISP {brightness_level}")
if isinstance(brightness_level, bool):
raise ValueError("Brightness level must be an int between 0 and 3, got bool")
if not isinstance(brightness_level, int):
raise ValueError(
f"Brightness level must be an int between 0 and 3, got {type(brightness_level).__name__}"
)
if brightness_level not in (0, 1, 2, 3):
raise ValueError(f"Brightness level must be 0, 1, 2, or 3, got {brightness_level}")
self.command(f"DISP {int(brightness_level)}")

Copilot uses AI. Check for mistakes.

def get_display_brightness(self) :
Expand All @@ -426,7 +510,14 @@ def set_front_panel_lock(self, lock_state, code):
Args:
lock_state (int): The lock state to be set (0, 1, or 2). 0=unlock, 1=lock, and 2=lock limits.
code (int): Keypad lock code required to make changes to the lock state of the front panel.

Raises:
ValueError: If lock_state is not 0, 1, or 2, or code is not an integer.
"""
if lock_state not in (0, 1, 2):
raise ValueError(f"Lock state must be 0, 1, or 2, got {lock_state}")
if not isinstance(code, int) or isinstance(code, bool):
raise ValueError(f"Lock code must be an integer, got {type(code).__name__}")
self.command(f"LOCK {lock_state},{code}")
Comment on lines +517 to 521

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lock_state not in (0, 1, 2) allows bools (True == 1 / False == 0), which would result in LOCK True,<code> being sent. Add an explicit isinstance(lock_state, bool) rejection (or require int and cast with int(lock_state) when formatting).

Suggested change
if lock_state not in (0, 1, 2):
raise ValueError(f"Lock state must be 0, 1, or 2, got {lock_state}")
if not isinstance(code, int) or isinstance(code, bool):
raise ValueError(f"Lock code must be an integer, got {type(code).__name__}")
self.command(f"LOCK {lock_state},{code}")
if isinstance(lock_state, bool) or not isinstance(lock_state, int) or lock_state not in (0, 1, 2):
raise ValueError(f"Lock state must be 0, 1, or 2, got {lock_state}")
if not isinstance(code, int) or isinstance(code, bool):
raise ValueError(f"Lock code must be an integer, got {type(code).__name__}")
self.command(f"LOCK {int(lock_state)},{code}")

Copilot uses AI. Check for mistakes.

def get_front_panel_status(self):
Expand All @@ -450,7 +541,12 @@ def set_programming_mode(self, mode):

Args:
mode (int): Programming mode (0, 1, or 2). 0=Internal, 1=External, 2=Sum.

Raises:
ValueError: If mode is not 0, 1, or 2.
"""
if mode not in (0, 1, 2):
raise ValueError(f"Programming mode must be 0, 1, or 2, got {mode}")
self.command(f"XPGM {mode}")
Comment on lines +548 to 550

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode not in (0, 1, 2) allows bools, and f-string formatting would send XPGM True/False. Add an explicit bool/type check and/or cast to int when formatting the command.

Suggested change
if mode not in (0, 1, 2):
raise ValueError(f"Programming mode must be 0, 1, or 2, got {mode}")
self.command(f"XPGM {mode}")
if not isinstance(mode, int) or isinstance(mode, bool):
raise ValueError(f"Programming mode must be an integer, got {type(mode).__name__}")
if mode not in (0, 1, 2):
raise ValueError(f"Programming mode must be 0, 1, or 2, got {mode}")
self.command(f"XPGM {int(mode)}")

Copilot uses AI. Check for mistakes.

def get_programming_mode(self):
Expand All @@ -468,7 +564,16 @@ def set_ieee_488(self, terminator, eoi_enable, address):
terminator(int): the terminator. 0=<CR><LF>, 1=<LF><CR>, 2=<LF>, 3=no terminator (must have EOI enabled).
eoi_enable(int): Sets EOI (End of Interrupt) mode. 0=Enabled, 1=Disabled.
address (int): Specifies IEEE address. 1 - 30(0 and 31 are reserved).

Raises:
ValueError: If any parameter is out of its valid range.
"""
if terminator not in (0, 1, 2, 3):
raise ValueError(f"Terminator must be 0, 1, 2, or 3, got {terminator}")
if eoi_enable not in (0, 1):
raise ValueError(f"EOI enable must be 0 or 1, got {eoi_enable}")
if not isinstance(address, int) or isinstance(address, bool) or address < 1 or address > 30:
raise ValueError(f"IEEE address must be an integer from 1-30, got {address}")
self.command(f"IEEE {terminator},{eoi_enable},{address}")
Comment on lines +571 to 577

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

terminator and eoi_enable validation currently allows bools (True == 1 / False == 0). If a caller passes True, the command would become IEEE True,..., which is invalid. Consider explicitly rejecting bool and requiring ints, and/or formatting with int(...) after validating the allowed numeric range.

Suggested change
if terminator not in (0, 1, 2, 3):
raise ValueError(f"Terminator must be 0, 1, 2, or 3, got {terminator}")
if eoi_enable not in (0, 1):
raise ValueError(f"EOI enable must be 0 or 1, got {eoi_enable}")
if not isinstance(address, int) or isinstance(address, bool) or address < 1 or address > 30:
raise ValueError(f"IEEE address must be an integer from 1-30, got {address}")
self.command(f"IEEE {terminator},{eoi_enable},{address}")
if not isinstance(terminator, int) or isinstance(terminator, bool) or terminator < 0 or terminator > 3:
raise ValueError(f"Terminator must be an integer 0, 1, 2, or 3, got {terminator}")
if not isinstance(eoi_enable, int) or isinstance(eoi_enable, bool) or eoi_enable not in (0, 1):
raise ValueError(f"EOI enable must be an integer 0 or 1, got {eoi_enable}")
if not isinstance(address, int) or isinstance(address, bool) or address < 1 or address > 30:
raise ValueError(f"IEEE address must be an integer from 1-30, got {address}")
self.command(f"IEEE {int(terminator)},{int(eoi_enable)},{int(address)}")

Copilot uses AI. Check for mistakes.

def get_iee_488(self):
Expand All @@ -488,7 +593,12 @@ def set_ieee_interface_mode(self, mode):

Args:
mode (int): Interface mode. 0, 1 or 2. 0=local, 1=remote, and 2=remote with local lockout.

Raises:
ValueError: If mode is not 0, 1, or 2.
"""
if mode not in (0, 1, 2):
raise ValueError(f"Interface mode must be 0, 1, or 2, got {mode}")
self.command(f"MODE {mode}")
Comment on lines +600 to 602

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode not in (0, 1, 2) allows bools, and the command would be formatted as MODE True/False. Add an explicit bool/type check and/or cast to int when formatting the command.

Copilot uses AI. Check for mistakes.

def get_ieee_interface_mode(self):
Expand Down Expand Up @@ -662,6 +772,9 @@ def set_hardware_error_enable_mask(self, register_mask):
register_mask (ElectromagnetPowerSupply.EMPowerSupplyHardwareErrorsRegister): Register mask configuration
object.
"""
# Note: read-modify-write is not atomic. The lock is held per-call, so another thread could
# modify the register between the query and command. This is acceptable for typical use since
# concurrent register modification is unusual.
operational_mask = self.query("ERSTE?").split(',')[1]
self.command(f"ERSTE {register_mask.to_integer()},{operational_mask}")

Expand Down Expand Up @@ -710,11 +823,14 @@ def set_operational_error_enable_mask(self, register_mask):
Status Byte Register.

Args:
register_mask (ElectromagnetPowerSupply.EMPowerSupplyHardwareErrorsRegister): Register mask configuration
register_mask (ElectromagnetPowerSupply.EMPowerSupplyOperationalErrorsRegister): Register mask configuration
object.
"""
# Note: read-modify-write is not atomic. The lock is held per-call, so another thread could
# modify the register between the query and command. This is acceptable for typical use since
# concurrent register modification is unusual.
hardware_mask = self.query("ERSTE?").split(',')[0]
self.command(f"ERSTE {register_mask.to_integer()},{hardware_mask}")
self.command(f"ERSTE {hardware_mask},{register_mask.to_integer()}")

def get_operational_error_enable_mask(self):
"""Returns which operational error bits will set the summary bit in the Status Byte Register.
Expand All @@ -724,7 +840,7 @@ def get_operational_error_enable_mask(self):
Status Byte Register.

Returns:
ElectromagnetPowerSupply.EMPowerSupplyHardwareErrorsRegister: Register mask configuration object.
ElectromagnetPowerSupply.EMPowerSupplyOperationalErrorsRegister: Register mask configuration object.
"""
operational_mask = int(self.query("ERSTE?").split(',')[1])
return self.EMPowerSupplyOperationalErrorsRegister.from_integer(operational_mask)
Expand Down
Loading