Switch Thorlabs Stepper Motor#222
Conversation
…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 integrates Thorlabs Stepper Motors into the SweepMe! framework by introducing a new Python device driver. The driver leverages the Kinesis .NET SDK to manage motor connections, movements, and parameter settings. It offers a comprehensive set of features for precise control, including configurable velocity, acceleration, and homing, alongside a simplified version for straightforward applications. An accompanying integration test ensures the core functionality works as expected. 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. 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
|
There was a problem hiding this comment.
Code Review
The pull request introduces a new driver for Thorlabs Stepper Motors, along with a minimal version and integration tests. The changes primarily involve setting up communication with Kinesis DLLs, handling device connection/disconnection, and implementing basic motor control functionalities like moving to a position and homing. Several areas for improvement have been identified, including robust error handling, consistent API usage, proper logging instead of print statements, and addressing issues in the integration tests for better reliability and clarity.
| # self.stepper.Disconnect(True) # attribute error | ||
| self.stepper_motor.Disconnect(True) |
There was a problem hiding this comment.
The commented-out line # self.stepper.Disconnect(True) and the subsequent self.stepper_motor.Disconnect(True) indicate potential confusion or an unresolved issue regarding which object should be disconnected. Furthermore, self.stepper_motor is only initialized in initialize(). If disconnect() is called before initialize() (e.g., if connect() fails), self.stepper_motor might not exist, leading to an AttributeError. It's crucial to ensure self.stepper_motor is available or handle its absence gracefully.
if hasattr(self, 'stepper_motor') and self.stepper_motor is not None:
self.stepper_motor.Disconnect(True)|
|
||
| # Try to reconnect to verify that the rack can be reconnected after disconnect | ||
| time.sleep(5) | ||
| rack.Connect(serial_number) # --> this raises an exception |
There was a problem hiding this comment.
| print("Could not check if rack is initialized:", e) | ||
|
|
There was a problem hiding this comment.
Catching a bare Exception and merely printing it can hide critical errors and make debugging difficult. It's better to catch more specific exceptions or re-raise the exception after logging it, to ensure proper error handling.
except Exception as e:
# logging.getLogger(__name__).error(f"Could not check if rack is initialized: {e}")
raise # Re-raise if it's a critical error, or handle appropriately| 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 kinesis_path is hardcoded to C:\Program Files\Thorlabs\Kinesis. The commented-out part suggests that the path might differ based on system bitness (Program Files (x86) for 32-bit systems). Hardcoding this path can lead to issues on systems where Kinesis is installed in a different location or on 32-bit systems. It's better to dynamically determine the correct path based on bitness.
| 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" |
| print("Settings failed to initialize:", e) | ||
|
|
There was a problem hiding this comment.
Catching a bare Exception and merely printing it can hide critical errors and make debugging difficult. It's better to catch more specific exceptions or re-raise the exception after logging it, to ensure proper error handling.
except Exception as e:
# logging.getLogger(__name__).error(f"Settings failed to initialize: {e}")
raise # Re-raise if it's a critical error, or handle appropriately| del parameters | ||
| return { | ||
| "SweepMode": ["Position"], | ||
| "Bay": "1", # maybe just use line edit |
| #self.stepper_motor.Home(60000) | ||
| #print("Device Homed") | ||
|
|
||
| #print(self.stepper, type(self.stepper), self.stepper_motor, type(self.stepper_motor)) |
|
|
||
| def apply(self) -> None: | ||
| """'apply' is used to set the new setvalue that is always available as 'self.value'.""" | ||
| print(self.stepper_motor.MoveTo.Overloads) |
There was a problem hiding this comment.
Using print statements for debugging or informational messages in a driver is generally not ideal for production code. It's better to use a proper logging mechanism (e.g., Python's logging module) which allows for configurable log levels and output destinations.
# import logging
# logging.getLogger(__name__).debug(f"MoveTo Overloads: {self.stepper_motor.MoveTo.Overloads}")| #print(f"Moving Device to {position}") | ||
| # Move the stepper motor to the specified position |
| #print(f"Device Moved to {self.stepper_motor.Position}") | ||
|
|
No description provided.