Conversation
franz-sweepMe
commented
Feb 20, 2026
franz-sweepMe
commented
- add non-blocking move
- add homing
- add relative move
…dd home timeout parameter
Summary of ChangesHello @franz-sweepMe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request expands the capabilities for controlling Thorlabs motion devices within the SweepMe! framework. It refines the existing Thorlabs LTS driver by adding advanced movement options and introduces robust support for Thorlabs Stepper Motors through new, dedicated drivers. These changes aim to provide more precise and versatile control over experimental setups involving Thorlabs stages. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Thorlabs_LTS driver and adds two new drivers for Thorlabs_StepperMotor. The changes to the LTS driver add useful features like non-blocking moves and homing. My review includes a few critical and high-severity findings regarding correctness, particularly with timeout units and homing velocity.
For the new Thorlabs_StepperMotor drivers, it appears two versions have been added (main.py and main.py in StepperMotorMinimal). They seem to be at different stages of development, with the "Minimal" version containing more debug code and lacking some robustness. It would be better to consolidate these into a single, polished driver. My review for these files points out several areas for cleanup, including removing debug code, clarifying confusing comments, and improving robustness.
I also noticed a file mwe_stepper.txt which seems to be a copy of a Python script. This might be a mistake and should probably be removed or renamed.
| def calculate_timeout(self, new_position: float) -> int: | ||
| """Calculate the timeout in ms for the MoveTo command based on the distance to travel and the velocity. | ||
|
|
||
| # todo reach. Currently, the command is blocking until the position is reached. | ||
| self.stage.MoveTo(new_position, timeout_s * 1000) # timeout in ms | ||
| The timeout is set to twice the expected travel time, with a minimum of 60s. | ||
| """ | ||
| current_position = self.get_position_mm() | ||
| distance = abs(new_position - current_position) | ||
|
|
||
| def call(self) -> float: | ||
| """Return the measurement results. Must return as many values as defined in self.variables.""" | ||
| return self.get_position_mm() | ||
| expected_travel_time = distance / float(self.velocity) | ||
| return max(60, int(expected_travel_time * 2)) * 1000 |
There was a problem hiding this comment.
There's a unit mismatch in timeout handling. This function returns the timeout in milliseconds, but it's used in the reach method as if it were seconds. This will cause premature timeouts. The function should return the timeout in seconds, and the docstring should be updated accordingly.
| def calculate_timeout(self, new_position: float) -> int: | |
| """Calculate the timeout in ms for the MoveTo command based on the distance to travel and the velocity. | |
| # todo reach. Currently, the command is blocking until the position is reached. | |
| self.stage.MoveTo(new_position, timeout_s * 1000) # timeout in ms | |
| The timeout is set to twice the expected travel time, with a minimum of 60s. | |
| """ | |
| current_position = self.get_position_mm() | |
| distance = abs(new_position - current_position) | |
| def call(self) -> float: | |
| """Return the measurement results. Must return as many values as defined in self.variables.""" | |
| return self.get_position_mm() | |
| expected_travel_time = distance / float(self.velocity) | |
| return max(60, int(expected_travel_time * 2)) * 1000 | |
| def calculate_timeout(self, new_position: float) -> int: | |
| """Calculate the timeout in s for the MoveTo command based on the distance to travel and the velocity. | |
| The timeout is set to twice the expected travel time, with a minimum of 60s. | |
| """ | |
| current_position = self.get_position_mm() | |
| distance = abs(new_position - current_position) | |
| expected_travel_time = distance / float(self.velocity) | |
| return max(60, int(expected_travel_time * 2)) |
| if self.home_at_start: | ||
| # Set homing velocity | ||
| motor_device_settings = self.stage.MotorDeviceSettings | ||
| motor_device_settings.Home.set_HomeVel(Decimal(float(self.velocity))) |
There was a problem hiding this comment.
The homing velocity is being set using self.velocity instead of self.home_velocity. The home_velocity is configured via the GUI parameter "Home velocity in mm/s", but it's not being used here. This will cause the homing to use the general movement velocity, not the specific homing velocity.
| motor_device_settings.Home.set_HomeVel(Decimal(float(self.velocity))) | |
| motor_device_settings.Home.set_HomeVel(Decimal(float(self.home_velocity))) |
| self.stepper.StopPolling() | ||
| self.rack.Disconnect(True) | ||
| # self.stepper.Disconnect(True) # attribute error | ||
| self.stepper_motor.Disconnect(True) |
There was a problem hiding this comment.
The stepper_motor attribute is initialized in the initialize method, but it is used here in disconnect. If initialize were to fail, this would raise an AttributeError. It's safer to initialize self.stepper_motor = None in __init__ and add a check here before using it.
| self.stepper_motor.Disconnect(True) | |
| if self.stepper_motor: | |
| self.stepper_motor.Disconnect(True) |
| time.sleep(0.5) # Wait for device to be enabled | ||
|
|
||
| print("Device is initialized, polling, and enabled.") | ||
| self.stepper_motor = self.rack.GetStepperChannel(int(self.channel)) |
There was a problem hiding this comment.
The stepper_motor attribute is initialized here in the initialize method. However, it is used in other methods like apply and call. If initialize were to fail before this line, those methods would raise an AttributeError. It's safer to initialize self.stepper_motor = None in __init__ and add checks where it's used if necessary.
| def call(self) -> str: | ||
| """Return the measurement results. Must return as many values as defined in self.variables.""" | ||
| #return 1 | ||
| return float(str(self.stepper_motor.Position)) |
There was a problem hiding this comment.
This conversion from the device's position to a float is not robust against different system locales. If the system uses a comma as a decimal separator, str(self.stepper_motor.Position) might produce a string like "1,23", and float() will fail. You should replace the comma with a period before converting to float, as done in the other driver.
return float(str(self.stepper_motor.Position).replace(",", "."))| def initialize(self) -> None: | ||
| """Initialize the device. This function is called only once at the start of the measurement.""" | ||
| # pass if the device is already initialized | ||
| if not self.stepper.IsSettingsInitialized(): | ||
| with contextlib.suppress(Exception): | ||
| self.stepper.WaitForSettingsInitialized(5000) | ||
|
|
||
| # The polling loop requests regular status requests to the motor to ensure the program keeps track of the device. | ||
| self.stepper.StartPolling(250) | ||
| time.sleep(0.5) | ||
|
|
||
| # Enable the channel otherwise any move is ignored | ||
| self.stepper.EnableDevice() | ||
| time.sleep(0.5) | ||
|
|
||
| print(self.rack.BayDeviceType(int(self.channel))) | ||
|
|
||
| # continue with stepper_motor object - unclear why | ||
| self.stepper_motor = self.rack.GetStepperChannel(int(self.channel)) | ||
| # Why? | ||
| motorConfiguration = self.stepper_motor.LoadMotorConfiguration(self.stepper.DeviceID) | ||
| currentDeviceSettings = self.stepper_motor.MotorDeviceSettings | ||
|
|
||
| if self.acceleration or self.max_velocity: | ||
| velocity_parameters = self.stepper_motor.GetVelocityParams() | ||
| if self.acceleration: | ||
| velocity_parameters.Acceleration = Decimal(float(self.acceleration)) | ||
| if self.max_velocity: | ||
| velocity_parameters.MaxVelocity = Decimal(float(self.max_velocity)) | ||
| self.stepper_motor.SetVelocityParams(velocity_parameters) | ||
|
|
||
| # homing leads to timeout errors if the device is too far from home, leave it for now | ||
| # TODO: add increased homing speed | ||
| if self.home_at_start: | ||
| print("Homing at start") | ||
| self.set_homing_velocity(float(self.home_velocity)) | ||
| self.stepper_motor.Home(self.timeout_ms) # why no Int32()? | ||
|
|
There was a problem hiding this comment.
This initialize method contains several items that should be cleaned up before merging:
- A
printstatement on line 229. - Comments indicating confusion (e.g.,
# unclear why,# Why?) on lines 231 and 233. - Unused local variables
motorConfigurationandcurrentDeviceSettingson lines 234-235. - A TODO comment on line 246.
- A confusing comment
# why no Int32()?on line 250.
Please clean up this method by removing debug statements, resolving points of confusion, and removing dead code.
| self.stepper_motor.MoveRelative(direction, Decimal(self.value), Int32(self.timeout_ms)) | ||
|
|
||
| elif self.sweep_mode == "Position": | ||
| self.stepper_motor.MoveTo(position, Int32(self.timeout_ms)) # no need for Int32 |
There was a problem hiding this comment.
The comment # no need for Int32 is confusing because Int32() is used in the line. If Int32() is necessary for the .NET interop, the comment is incorrect and should be removed. If it's not necessary, the call should be changed. Given the similar call in MoveRelative, it's likely required.
| self.stepper_motor.MoveTo(position, Int32(self.timeout_ms)) # no need for Int32 | |
| self.stepper_motor.MoveTo(position, Int32(self.timeout_ms)) |
| kinesis_imported = False | ||
|
|
||
| bitness = 64 if sys.maxsize > 2**32 else 32 | ||
| kinesis_path = "C:\\Program Files\\Thorlabs\\Kinesis" # if bitness == 64 else "C:\\Program Files (x86)\\Thorlabs\\Kinesis" |
There was a problem hiding this comment.
The logic to determine the Kinesis path for 32-bit systems is commented out. If this driver is intended to support 32-bit Python, this logic should be enabled to ensure the correct path is used.
| kinesis_path = "C:\\Program Files\\Thorlabs\\Kinesis" # if bitness == 64 else "C:\\Program Files (x86)\\Thorlabs\\Kinesis" | |
| kinesis_path = "C:\\Program Files\\Thorlabs\\Kinesis" if bitness == 64 else "C:\\Program Files (x86)\\Thorlabs\\Kinesis" |
| def update_gui_parameters(self, parameters: dict[str, Any]) -> dict[str, Any]: | ||
| """Determine the new GUI parameters of the driver depending on the current parameters.""" | ||
| del parameters | ||
| return { | ||
| "SweepMode": ["Position"], | ||
| "Bay": "1", # maybe just use line edit | ||
| "Serial Number": "52870913", | ||
| } |
There was a problem hiding this comment.
Using del parameters is unnecessary and considered bad practice. If the parameters argument is not used, it can be ignored.
def update_gui_parameters(self, parameters: dict[str, Any]) -> dict[str, Any]:
"""Determine the new GUI parameters of the driver depending on the current parameters."""
return {
"SweepMode": ["Position"],
"Bay": "1", # maybe just use line edit
"Serial Number": "52870913",
}| msg = "Failed to connect to the device within the timeout period." | ||
| raise TimeoutError(msg) | ||
|
|
||
| print(f"Connected to device {self.serial_number}. Connection status: {self.rack.IsConnected}") |
There was a problem hiding this comment.
This print statement seems to be for debugging. It should be removed from the production code. Consider using a logging framework if you need to output information during runtime. Similar debug prints exist on lines 135 and 180.
1 participant
