Skip to content

add Thorlabs DotNetAPI#386

Open
DCEM wants to merge 35 commits into
QCoDeS:mainfrom
Geowissenschaften:add-Thorlabs-DotNetAPI
Open

add Thorlabs DotNetAPI#386
DCEM wants to merge 35 commits into
QCoDeS:mainfrom
Geowissenschaften:add-Thorlabs-DotNetAPI

Conversation

@DCEM

@DCEM DCEM commented Nov 27, 2024

Copy link
Copy Markdown

replacement for: #267 discussed in #262

This pull request introduces drivers for the QCoDeS framework, enabling control of Thorlabs devices via the Thorlabs .NET API. The integration aims to provide a structured and extendable approach to instrument control within QCoDeS.

What's Included:

  • Integration Manual: A guide for integrating Thorlabs devices with QCoDeS via .NET API
  • API Structure Classes: Classes that provide a framework mirroring the Thorlabs .NET API.
  • Drivers for BSCxxx and KDC101: Initial implementations for specific Thorlabs devices.

Current Status:
The API-structured classes are foundational but not complete. They can be expanded with more features and support for additional devices.

Further Information:
Detailed implementation notes can be found in the README at ...Thorlabs/private/DotNetAPI/README.md.

@DCEM DCEM mentioned this pull request Nov 27, 2024
DCEM and others added 28 commits November 27, 2024 17:23
… decimal-to-float conversions

- Introduce a new constructor argument `decimal_as_float` to toggle .NET Decimal conversion to either Python Decimal or float.
- Refactor getter/setter methods to distinguish between DIRECT and ACCESSOR API interface usage with reflection.
- Migrate from string-based .NET Enum handling to integer-based handling (verifying Int32 underpinnings).
- Add `_get_thorlabs_enum_dict` to generate a QCoDeS-friendly dictionary for enum names and values.
- Use `CultureInfo.InvariantCulture` for .NET Decimal to string conversion to ensure consistent locale handling.
- Improve validation and error handling throughout the mixin to guard against incorrect usage and data type mismatches.
Untested with sceleton classes for: IntegratedStepperMotor and CageRotor, I do not have the hardware to test.
@jenshnielsen jenshnielsen reopened this Jul 22, 2025
@jenshnielsen jenshnielsen requested a review from Copilot July 22, 2025 13:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a comprehensive driver framework for the QCoDeS framework that enables control of Thorlabs devices via the Thorlabs .NET API. The integration provides a structured and extendable approach to instrument control, mirroring the Thorlabs .NET API inheritance structure.

  • Integration Framework: Implements API structure classes that mirror the Thorlabs .NET API with ThorlabsMixin, ThorlabsQcodesInstrument, and various interface classes
  • Device Drivers: Provides initial implementations for BSCxxx, KDC101, K10CR1, MFF10x, KLSnnn, and TDC001 devices
  • Documentation and Templates: Includes comprehensive README documentation and template files for adding new device support

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
qcodes_thorlabs_integration.py Core integration framework with mixins and base classes for .NET API interaction
init.py Import handling for pythonnet dependency
README.md Comprehensive documentation for implementation approach and examples
Device driver files Individual drivers for specific Thorlabs devices (BSCxxx, KDC101, etc.)
API structure files Classes mirroring .NET API inheritance (DeviceManagerCLI, GenericMotorCLI, etc.)
Template file Template for creating new device drivers
Comments suppressed due to low confidence (3)

src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py:67

  • The parameter 'setter_name' should be 'setter_name' instead of 'getter_name' in the set_cmd. The current code incorrectly uses 'getter_name' which would cause the setter operation to fail.
            decimal_as_float (bool): If True, automatically converts .NET Decimal attributes to Python floats.

src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py:422

  • The method call should access 'VelocityMaximum' instead of 'AccelerationMaximum' for the velocity_max parameter, as indicated by the docstring.
                except Exception as e:

self.log.error(f"Failed to import DLLs for Thorlabs device with serial number {self._serial_number}")
raise

# Enable siumulation if self._simulation is True.

Copilot AI Jul 22, 2025

Copy link

Choose a reason for hiding this comment

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

Typo in comment: 'siumulation' should be 'simulation'.

Suggested change
# Enable siumulation if self._simulation is True.
# Enable simulation if self._simulation is True.

Copilot uses AI. Check for mistakes.
self.add_parameter(
'firmware_version',
get_cmd=lambda: device_info.FirmwareVersion.ToString(),
docstring='Serial model of the device, read-only.')

Copilot AI Jul 22, 2025

Copy link

Choose a reason for hiding this comment

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

The docstring should describe 'firmware version' not 'serial model' since this parameter is for firmware_version.

Suggested change
docstring='Serial model of the device, read-only.')
docstring='Firmware version of the device, read-only.')

Copilot uses AI. Check for mistakes.
Comment on lines +89 to +90
while self.state() != 'Idle':
if countdown.passed():

Copilot AI Jul 22, 2025

Copy link

Choose a reason for hiding this comment

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

The variable 'countdown' is referenced but never defined. This will cause a NameError when the method is called.

Suggested change
while self.state() != 'Idle':
if countdown.passed():
start_time = time.time()
while self.state() != 'Idle':
if timeout_s is not None and (time.time() - start_time) > timeout_s:

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +24
docstring='Gets the actual position.'
'This parameter is motor dependent so its range is variable.'
'The position or -1 if the filter is in motion.'
'TODO: The operation will use the current value of "movement_timeout" for the move operation.'

Copilot AI Jul 22, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The docstring spans multiple lines but is formatted as a single string. It should either be properly formatted as a multi-line docstring or combined into a single line.

Suggested change
docstring='Gets the actual position.'
'This parameter is motor dependent so its range is variable.'
'The position or -1 if the filter is in motion.'
'TODO: The operation will use the current value of "movement_timeout" for the move operation.'
docstring="""Gets the actual position.
This parameter is motor dependent so its range is variable.
The position or -1 if the filter is in motion.
TODO: The operation will use the current value of "movement_timeout" for the move operation."""

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +39
if position < 0:
raise ValueError("position must be non-negative.")

Copilot AI Jul 22, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The position validation should be done by the validator in the parameter definition rather than in the setter method to ensure consistency.

Suggested change
if position < 0:
raise ValueError("position must be non-negative.")

Copilot uses AI. Check for mistakes.
get_cmd=lambda: self._get_thorlabs_decimal('StepSize'),
set_cmd=lambda x: self._set_thorlabs_decimal(x, 'StepSize'),
unit=length_unit,
docstring="Stop Mode",

Copilot AI Jul 22, 2025

Copy link

Choose a reason for hiding this comment

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

The docstring should describe 'Step Size' not 'Stop Mode' since this parameter is for step_size.

Suggested change
docstring="Stop Mode",
docstring="Step Size",

Copilot uses AI. Check for mistakes.
Raises:
ValueError: If the provided mode is not one of the accepted modes.
"""
if not hasattr(self, '_set_startup_mode_value'):

Copilot AI Jul 22, 2025

Copy link

Choose a reason for hiding this comment

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

The condition checks for '_set_startup_mode_value' method but should check for '_startup_mode_value' attribute instead.

Suggested change
if not hasattr(self, '_set_startup_mode_value'):
if not hasattr(self, '_startup_mode_value'):

Copilot uses AI. Check for mistakes.
@jenshnielsen

Copy link
Copy Markdown
Collaborator

@DCEM I was experiencing with code review from copilot but I am not sure of the quality of the input. Please feel free do discard its suggestions where they don't apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants