Skip to content
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.

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

Thorlabs kinesis drivers #262

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

Conversation

thangleiter
Copy link
Contributor

@thangleiter thangleiter commented Oct 18, 2023

This is another backlog driver project that started before the recent (#244) Thorlabs merge. Unfortunately, this and #244 diverge in the design of the underlying glue code and also have finite overlap in the devices that are implemented using the Kinesis API.

Since Thorlabs deprecated the APT interface (which still work though) I opted for supporting both APT and Kinesis drivers in parallel.

Additionally, there is a new driver for the PM100 powermeter, which, in the newest version of the software, also uses a dll instead of VISA.

It might be worth discussing how to proceed (ping @julienbarrier) to provide a common base architecture for Thorlabs drivers in qcodes.

Thorlabs APT is marked as "legacy":
(https://www.thorlabs.com/newgrouppage9.cfm?objectgroup_id=10285)

To Do:
 - add actual functionality to the drivers.
 - add documentation
A lot of dll functions of different devices have the same signature but
just a different prefix.
Where two different drivers are available, there is now a file for each
named after the driver type (tlpm, visa, kinesis, apt) which contains
the instrument class.
Doesn't work for a K10CR1 on my end (yet). The LoadSettings function
returns an unspecified error and so do setting/getting the position.
@astafan8
Copy link
Contributor

@thangleiter since the other PR got merged, there are merge conflicts now - could you resovle those?

It might be worth discussing how to proceed (ping @julienbarrier) to provide a common base architecture for Thorlabs drivers in qcodes.

that's a good point. However, if you're not sure about it yet, it could be OK to merge different solutions now, provided that later a unification will arrive and the impact on the user-facing API of the driver is minimal (to keep compatibility where possible and reasonable)

@thangleiter
Copy link
Contributor Author

@thangleiter since the other PR got merged, there are merge conflicts now - could you resovle those?

It might be worth discussing how to proceed (ping @julienbarrier) to provide a common base architecture for Thorlabs drivers in qcodes.

that's a good point. However, if you're not sure about it yet, it could be OK to merge different solutions now, provided that later a unification will arrive and the impact on the user-facing API of the driver is minimal (to keep compatibility where possible and reasonable)

The problem is that both versions go about the common infrastructure differently, so I don't think it makes sense to resolve merge conflicts before we agreed on what that should look like. Unfortunatley, (almost) every Thorlabs Kinesis device has their own DLL, yet many of them share common functions (which each have names differing by a device-specific prefix...).

Solving conflicts now would introduce a lot of functionally duplicitous code (everything in private basically). The only exception is the PM100D, where the Visa and the TLPM driver can co-exist since they aren't part of the Kinesis framework. I could split that off this PR.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 768 lines in your changes missing coverage. Please review.

Project coverage is 10.56%. Comparing base (608a88d) to head (553f207).
Report is 16 commits behind head on main.

Current head 553f207 differs from pull request most recent head 1cbc4e4

Please upload reports for the commit 1cbc4e4 to get more accurate results.

Files Patch % Lines
...b_drivers/drivers/Thorlabs/private/kinesis/core.py 0.00% 451 Missing ⚠️
..._drivers/drivers/Thorlabs/private/kinesis/enums.py 0.00% 110 Missing ⚠️
...b_drivers/drivers/Thorlabs/Thorlabs_PM100D/tlpm.py 0.00% 65 Missing ⚠️
...rivers/drivers/Thorlabs/private/kinesis/structs.py 0.00% 33 Missing ⚠️
...rib_drivers/drivers/Thorlabs/private/kinesis/cc.py 0.00% 23 Missing ⚠️
...ib_drivers/drivers/Thorlabs/private/kinesis/isc.py 0.00% 23 Missing ⚠️
...ib_drivers/drivers/Thorlabs/Thorlabs_MFF10x/apt.py 0.00% 20 Missing ⚠️
...rivers/drivers/Thorlabs/Thorlabs_MFF10x/kinesis.py 0.00% 18 Missing ⚠️
...ib_drivers/drivers/Thorlabs/Thorlabs_K10CR1/apt.py 0.00% 10 Missing ⚠️
...ib_drivers/drivers/Thorlabs/Thorlabs_PRM1Z8/apt.py 0.00% 7 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
- Coverage   10.97%   10.56%   -0.42%     
==========================================
  Files         132      140       +8     
  Lines       17540    18224     +684     
==========================================
  Hits         1925     1925              
- Misses      15615    16299     +684     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thangleiter thangleiter changed the title Draft: Thorlabs kinesis drivers Thorlabs kinesis drivers May 15, 2024
@thangleiter
Copy link
Contributor Author

Thanks for approving this @einsmein. Before merging, it should still be discussed with @julienbarrier and @DCEM how to proceed with Kinesis drivers though, as there are now three different approaches; main, this PR, and #267.

@DCEM
Copy link

DCEM commented May 17, 2024

I might not get everything correct in the following comment - it has been a while since I have been working on this.

When I started working on implementing the Thorlabs_BSCxxx_DotNet implementation I was only aware of @julienbarrier approach.

I ran into issues with the C# implementation. For example I think i was unable to get the instrument model name (for me: BSC203).
Maybe I overlooked it in the documentation, or it is something that is only documented/implemented in the .Net API.
Also the C# API seemed very cumbersome, there are a lot of Prefixes and you have to explicitly implement everything.
In contrast, with the .Net API I did not have to struggle with this. No prefixes, and it felt more like importing a python module and something like "dir" would let me know what is available, without having to explicitly implement it.

I am not sure how the C# API will behave, but for the .Net one I liked that you can change the motor parameters in the Thorlabs Kinesis GUI and than load it via the .Net API.

Because the Inheritance structure mimics the Thorlabs one, if you add functionality to a base class, all instruments inheriting from it will have it right away. I don't know if this true (or even possible) for the C# one with all the prefixes.
More information on my approach: readme.md

In my opinion it can be acceptable for the .Net and the C# to live beside each other and that is why I took some care to allow for it. I also wrote some documentation on how to add instruments/functionality.

I do not claim/know that my approach is better than the others provided and am not sure how to proceed.
Im happy to participate in discussing, but I don't think it will make much sense for me to comment on how to merge the two C# implementations.

I'm not dogmatic, and if it is decided that the .Net API way (or my implementation of it) is undesirable I won't feel offended.

@thangleiter
Copy link
Contributor Author

I might not get everything correct in the following comment - it has been a while since I have been working on this.

Same here, it's been a while.

When I started working on implementing the Thorlabs_BSCxxx_DotNet implementation I was only aware of @julienbarrier approach.

I ran into issues with the C# implementation. For example I think i was unable to get the instrument model name (for me: BSC203). Maybe I overlooked it in the documentation, or it is something that is only documented/implemented in the .Net API.

I never had this problem with our devices since they have dedicated classes in the C API. I would expect it's either in TLI_DeviceInfo or TLI_HardwareInformation.

Also the C# API seemed very cumbersome, there are a lot of Prefixes and you have to explicitly implement everything. In contrast, with the .Net API I did not have to struggle with this. No prefixes, and it felt more like importing a python module and something like "dir" would let me know what is available, without having to explicitly implement it.

I implemented this by decorating base class methods with the prefix (see, #262 (comment)).

I am not sure how the C# API will behave, but for the .Net one I liked that you can change the motor parameters in the Thorlabs Kinesis GUI and than load it via the .Net API.

Can you connect to the same instrument concurrently? Otherwise what do you mean by loading the settings? Are they persistent?

Because the Inheritance structure mimics the Thorlabs one, if you add functionality to a base class, all instruments inheriting from it will have it right away. I don't know if this true (or even possible) for the C# one with all the prefixes. More information on my approach: readme.md

See above.

In my opinion it can be acceptable for the .Net and the C# to live beside each other and that is why I took some care to allow for it. I also wrote some documentation on how to add instruments/functionality.

I do not claim/know that my approach is better than the others provided and am not sure how to proceed. Im happy to participate in discussing, but I don't think it will make much sense for me to comment on how to merge the two C# implementations.

I'm not dogmatic, and if it is decided that the .Net API way (or my implementation of it) is undesirable I won't feel offended.

Agreed. For me it would also be fine if the drivers live side-by-side (also with the old APT drivers).

Lastly, an idea: if you have the time, we could each try to implement one of the instrument drivers the other wrote in our version of the driver infrastructure. It could help make one or another decision depending on how easy or difficult it is. That's where I see the most benefit of choosing one over the other; ease of implementing new drivers.

@DCEM
Copy link

DCEM commented May 20, 2024

I might not get everything correct in the following comment - it has been a while since I have been working on this.

Same here, it's been a while.

When I started working on implementing the Thorlabs_BSCxxx_DotNet implementation I was only aware of @julienbarrier approach.
I ran into issues with the C# implementation. For example I think i was unable to get the instrument model name (for me: BSC203). Maybe I overlooked it in the documentation, or it is something that is only documented/implemented in the .Net API.

I never had this problem with our devices since they have dedicated classes in the C API. I would expect it's either in TLI_DeviceInfo or TLI_HardwareInformation.

If I remember correctly I tried that, but it returned something different (someting like "Benchtop Stepper Device").
I do get it from the .Net API via GetDeviceInfo.
It might be because of the "Core Device" and "Channel Device" implementation by Thorlabs that will utilize mixin for single channel devices, but composition for multi channel devices.
When I was working on the C# API I was not aware of this, I am not even sure If it is the same way there.
I guess it might also be related to the BSC203 - that was not an easy instrument to start with. It also wants to have the DeviceID instead of the serial number for LoadMotorConfiguration of the channels. I'm not sure how much it deviates from the default structure in total.

Also the C# API seemed very cumbersome, there are a lot of Prefixes and you have to explicitly implement everything. In contrast, with the .Net API I did not have to struggle with this. No prefixes, and it felt more like importing a python module and something like "dir" would let me know what is available, without having to explicitly implement it.

I implemented this by decorating base class methods with the prefix (see, [#262 (comment)](#262 (comment))).

I am not sure how the C# API will behave, but for the .Net one I liked that you can change the motor parameters in the Thorlabs Kinesis GUI and than load it via the .Net API.

Can you connect to the same instrument concurrently? Otherwise what do you mean by loading the settings? Are they persistent?

No, I don't think it is possible to connect to an instrument concurrently.
What I ment was you can change something like Home Settings, Jog Settings or Limit Settings in the Thorlabs GUI and then load these via the API. I presume this is also possible via the C# API. I changed to the .Net API early on, so I cannot say for sure, because I was missing that understanding at the time.

Because the Inheritance structure mimics the Thorlabs one, if you add functionality to a base class, all instruments inheriting from it will have it right away. I don't know if this true (or even possible) for the C# one with all the prefixes. More information on my approach: [readme.md](https://github.com/QCoDeS/Qcodes_contrib_drivers/blob/9b67d6db4efc212d7d956a4e648f932646b9f6df/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/README.md)

See above.

In my opinion it can be acceptable for the .Net and the C# to live beside each other and that is why I took some care to allow for it. I also wrote some documentation on how to add instruments/functionality.
I do not claim/know that my approach is better than the others provided and am not sure how to proceed. Im happy to participate in discussing, but I don't think it will make much sense for me to comment on how to merge the two C# implementations.
I'm not dogmatic, and if it is decided that the .Net API way (or my implementation of it) is undesirable I won't feel offended.

Agreed. For me it would also be fine if the drivers live side-by-side (also with the old APT drivers).

If I saw it correctly you chose to have a folder for each instrument and then a filename by implementation type?
I chose to add a _DotNet at the end on the instruments.
The [Naming Instrument](https://microsoft.github.io/Qcodes/examples/writing_drivers/Creating-Instrument-Drivers.html#Naming-Instruments) section states the filename should be qcodes\instrument_drivers\{Vendor}\{Vendor}_{Model}.py thats why I added the Thorlabs in front, but it seems to be omitted a lot or that part of the manual is outdated.
Naming the files kinesis.py might be confusing if indeed both implementations live next to each other - maybe calling the .Net ones ...\{Vendor}\{Model}\kinesis_DotNet.py could be acceptable to get consistency there.
I'm somewhat inexperienced - so I'm not sure which naming scheme is desirable here.

Lastly, an idea: if you have the time, we could each try to implement one of the instrument drivers the other wrote in our version of the driver infrastructure. It could help make one or another decision depending on how easy or difficult it is. That's where I see the most benefit of choosing one over the other; ease of implementing new drivers.

I do agree, that ease of driver implementation is a good metric.

I would love to try that and I will try to find the time, but I unfortunately cannot make strong commitment to it at the moment. My schedule is a bit crazy right now. If you want to have a look there is a Practical Implementation Example in the: [readme.md](https://github.com/QCoDeS/Qcodes_contrib_drivers/blob/9b67d6db4efc212d7d956a4e648f932646b9f6df/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/README.md)

How easy it is will depend a lot on what features are missing in the base classes of the .Net implementation.

@thangleiter
Copy link
Contributor Author

If I saw it correctly you chose to have a folder for each instrument and then a filename by implementation type? I chose to add a _DotNet at the end on the instruments. The Naming Instrument section states the filename should be qcodes\instrument_drivers\{Vendor}\{Vendor}_{Model}.py thats why I added the Thorlabs in front, but it seems to be omitted a lot or that part of the manual is outdated. Naming the files kinesis.py might be confusing if indeed both implementations live next to each other - maybe calling the .Net ones ...\{Vendor}\{Model}\kinesis_[DotNet.py](http://DotNet.py) could be acceptable to get consistency there. I'm somewhat inexperienced - so I'm not sure which naming scheme is desirable here.

Since I would argue for retaining the APT versions of the drivers for backwards compatibility, the ...\{Vendor}\{Model}\{apt,kinesis,visa,tlpm}_{dll,DotNet}.py scheme makes the most sense to me (For the PM100D there are also the tlpm and the visa drivers.)

This is something where input from the maintainers would be welcome @astafan8. What's the policy on multiple driver interfaces? Do you have a stance on the naming scheme?

I would love to try that and I will try to find the time, but I unfortunately cannot make strong commitment to it at the moment. My schedule is a bit crazy right now. If you want to have a look there is a Practical Implementation Example in the: [readme.md](https://github.com/QCoDeS/Qcodes_contrib_drivers/blob/9b67d6db4efc212d7d956a4e648f932646b9f6df/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/README.md)

How easy it is will depend a lot on what features are missing in the base classes of the .Net implementation.

Agreed. I will try my best to put something together in the next two weeks but also cannot promise anything.

On another note, does this PR supercede @julienbarrier's version currently in main?

@jenshnielsen
Copy link
Collaborator

Since I would argue for retaining the APT versions of the drivers for backwards compatibility, the ...{Vendor}{Model}{apt,kinesis,visa,tlpm}_{dll,DotNet}.py scheme makes the most sense to me (For the PM100D there are also the tlpm and the visa drivers.)

In qcodes we have indeed tried to follow this schema and not have the drivers nested too deeply. However, if there is a good reason to have multiple implementation I think the current schema is acceptable.

@jenshnielsen
Copy link
Collaborator

@thangleiter @DCEM Trying to catch up on the status of this pr and #267

We would be happy to merge improvements if we can agree what is the most suitable to merge but I have lost track the various versions of the drivers.

@DCEM
Copy link

DCEM commented Aug 23, 2024

@jenshnielsen Thank you for taking the time to catch up on the status. Here’s my understanding of the current situation:

We currently have two C# implementations and my .Net implementation. The key difference between the C# ones is that @thangleiter implementation uses decorators, which should simplify the process of implementing new instruments compared to @julienbarrier approach.

Please note that I’m still somewhat new to QCoDeS and don’t consider myself a programming expert, so my preference for the .Net implementation might be partly due to my familiarity with it as the one I worked on.

Advantages of the .Net Implementation:

  • No Naming Conflicts: Unlike C#, the .Net implementation doesn’t suffer from potential issues where functions with the same name might exist in different DLLs (at least one such case has been noted in C#).
  • Simplified Function Implementation: With .Net, there's no need to manually add decorators to shared functions when implementing new instruments.
  • Automatic Enum Handling: Enums and other similar elements do not need to be manually added in the .Net implementation, simplifying the process further.
  • Functionality Inheritance: The .Net implementation mirrors the Thorlabs API inheritance structure. This means that any functionality added to a base class is immediately available to all instruments that inherit from it.

Drawbacks:

  • Additional Dependency: The primary downside of the .Net implementation is the additional dependency on pythonnet.

Common Challenge:

In both C# and .Net implementations, there’s the issue of handling the Decimal type, which is suggested by the Thorlabs API. Even when converting this to Python’s Decimal type, it often leads to practical issues. A potential solution could be an automatic type conversion feature that can be toggled off via a parameter if necessary.

Key Questions:

  1. Should we maintain multiple implementations side by side, or should some be merged?
  2. Which implementations, if any, should be consolidated?
  3. What should the naming scheme look like if we maintain multiple variants?

These are the initial questions we need to address to move forward.

Looking forward to your thoughts.

Best regards
David

@thangleiter
Copy link
Contributor Author

Hi both, apologies for the radio silence on my end. I'm afraid that I do not have the time to migrate the drivers from my branch to @DCEM's at the moment. That means if we chose the .NET version over this PR, I'd stay on our fork so that I can continue my experiments (fine by me though).

In general and in an ideal world, I think maintaining both the .NET and the C-API versions in parallel is a bad idea. However, if neither @DCEM nor me have time to migrate the already implemented drivers, not doing so means fewer supported devices. Hence, at least for now we could go the parallel route. If at any point other people implement additional devices, they can choose from the two APIs and the issue can go the evolutionary way...

If we go down that road, the naming scheme I'd go for is ...\Thorlabs\{Model}\kinesis_{C,DotNet}.py.

@DCEM DCEM mentioned this pull request Nov 27, 2024
@DCEM
Copy link

DCEM commented Nov 27, 2024

I'm sorry I had to recreate #267 under: #386

@DCEM
Copy link

DCEM commented Nov 27, 2024

@thangleiter I created a basic driver for the KDC101 for you to look at or test if you can find time to do so:
https://github.com/Geowissenschaften/Qcodes_contrib_drivers/tree/add_KDC101

@jenshnielsen
which naming schema do you prefer, do you want a subfolder per device as suggested by @thangleiter (...\Thorlabs{Model}\kinesis_{C,DotNet}.py) or should it be multiple files something like i used - similar to (...\Thorlabs\Thorlabs_{Model}{C,DotNet}.py) or if needed (...\Thorlabs\Thorlabs{Model}kinesis{C,DotNet}.py)

I do not mind either way. I think taking into account which outcome is prefered when/if we manage to settle on one approach would be nice.

@julienbarrier do you have any input here?

@thangleiter
Copy link
Contributor Author

@DCEM Didn't have a lot of time to debug, but this is as far as I get:

rotator = K10CR1('rotator', 55001014)
2024-12-06 10:55:25,086 ¦ qcodes_contrib_drivers.drivers.Thorlabs.private.DotNetAPI.qcodes_thorlabs_integration ¦ ERROR ¦ qcodes_thorlabs_integration ¦ _import_dll_class ¦ 451 ¦ An unexpected error occurred while importing the DLL class.
Traceback (most recent call last):
  File "~\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\qcodes_thorlabs_integration.py", line 442, in _import_dll_class
    module = importlib.import_module(namespace)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~\miniforge3\envs\lab_main\Lib\importlib\__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1310, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1310, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1324, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'ThorLabs'
2024-12-06 10:55:25,088 ¦ qcodes.instrument.instrument_base ¦ ERROR ¦ qcodes_thorlabs_integration ¦ __init__ ¦ 508 ¦ [rotator(K10CR1)] Failed to import DLLs for Thorlabs device with serial number 55001014
Traceback (most recent call last):

  Cell In[10], line 1
    rotator = K10CR1('rotator', 55001014)

  File ~\Documents\Qcodes\src\qcodes\instrument\instrument_meta.py:36 in __call__
    new_inst = super().__call__(*args, **kwargs)

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\Thorlabs_K10CR1_DotNet.py:48 in __init__
    super().__init__(

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\IntegratedStepperMotorsCLI_CageRotator.py:24 in __init__
    super().__init__(*args, **kwargs)

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\IntegratedStepperMotorsCLI_IntegratedStepperMotor.py:13 in __init__
    super().__init__(*args, **kwargs)

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\GenericMotorCLI_AdvancedMotor.py:40 in __init__
    super().__init__(*args, **kwargs)

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\GenericMotorCLI.py:35 in __init__
    super().__init__(*args, **kwargs)

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\DeviceManagerCLI.py:124 in __init__
    super().__init__(*args, **kwargs)

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\qcodes_thorlabs_integration.py:506 in __init__
    self._import_device_dll()

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\Thorlabs_K10CR1_DotNet.py:66 in _import_device_dll
    self._import_dll_class('ThorLabs.MotionControl.IntegratedStepperMotorsCLI', 'CageRotator')

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\qcodes_thorlabs_integration.py:452 in _import_dll_class
    raise e

  File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\qcodes_thorlabs_integration.py:442 in _import_dll_class
    module = importlib.import_module(namespace)

  File ~\miniforge3\envs\lab_main\Lib\importlib\__init__.py:90 in import_module
    return _bootstrap._gcd_import(name[level:], package, level)

  File <frozen importlib._bootstrap>:1387 in _gcd_import

  File <frozen importlib._bootstrap>:1360 in _find_and_load

  File <frozen importlib._bootstrap>:1310 in _find_and_load_unlocked

  File <frozen importlib._bootstrap>:488 in _call_with_frames_removed

  File <frozen importlib._bootstrap>:1387 in _gcd_import

  File <frozen importlib._bootstrap>:1360 in _find_and_load

  File <frozen importlib._bootstrap>:1310 in _find_and_load_unlocked

  File <frozen importlib._bootstrap>:488 in _call_with_frames_removed

  File <frozen importlib._bootstrap>:1387 in _gcd_import

  File <frozen importlib._bootstrap>:1360 in _find_and_load

  File <frozen importlib._bootstrap>:1324 in _find_and_load_unlocked

ModuleNotFoundError: No module named 'ThorLabs'

I applied the following patches to get this far:

diff --git a/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py b/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
index fe851f42..195e19ef 100644
--- a/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
+++ b/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
@@ -2,12 +2,12 @@ import logging
 from typing import Optional
 # from time import sleep
 
-from .private.DotNetAPI.IntegratedStepperMotorsCLI_CageRotor import CageRotor
+from .private.DotNetAPI.IntegratedStepperMotorsCLI_CageRotator import CageRotator
 from .private.DotNetAPI.qcodes_thorlabs_integration import ThorlabsQcodesInstrument
 
 log = logging.getLogger(__name__)
 
-class K10CR1(CageRotor, ThorlabsQcodesInstrument):
+class K10CR1(CageRotator, ThorlabsQcodesInstrument):
     """
     Driver for interfacing with the Thorlabs K10CR1 Motorised Rotation Mount
     via the QCoDeS framework and the .NET API.
@@ -21,7 +21,7 @@ class K10CR1(CageRotor, ThorlabsQcodesInstrument):
     Args:
         name (str): Name of the instrument.
         serial_number (str): The serial number of the Thorlabs device.
-        startup_mode_value: .Net Enum value to be stored in '_startup_mode' and used as 
+        startup_mode_value: .Net Enum value to be stored in '_startup_mode' and used as
                             'startupSettingsMode' in 'LoadMotorConfiguration'
             Valid startup modes:
                 UseDeviceSettings: Use settings from device
@@ -63,7 +63,7 @@ class K10CR1(CageRotor, ThorlabsQcodesInstrument):
         """Import the device-specific DLLs and classes from the .NET API."""
         self._add_dll('Thorlabs.MotionControl.GenericMotorCLI.dll')
         self._add_dll('ThorLabs.MotionControl.IntegratedStepperMotorsCLI.dll')
-        self._import_dll_class('ThorLabs.MotionControl.IntegratedStepperMotorsCLI', 'CageRotor')
+        self._import_dll_class('ThorLabs.MotionControl.IntegratedStepperMotorsCLI', 'CageRotator')
 
     def _get_api_interface_from_dll(self, serial_number: str):
         """Retrieve the API interface for the Thorlabs device using its serial number."""
@@ -79,7 +79,7 @@ class K10CR1(CageRotor, ThorlabsQcodesInstrument):
             #'K10CR1 (Simulated)'
         ]
         if self.model() not in knownmodels:
-            raise ValueError(f"'{model}' is an unknown model.")
+            raise ValueError(f"'{self.model()}' is an unknown model.")
 
     def _post_enable(self):
         """
diff --git a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/IntegratedStepperMotorsCLI_CageRotator.py b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/IntegratedStepperMotorsCLI_CageRotator.py
index 35f9ce64..69c89197 100644
--- a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/IntegratedStepperMotorsCLI_CageRotator.py
+++ b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/IntegratedStepperMotorsCLI_CageRotator.py
@@ -5,7 +5,7 @@ from .IntegratedStepperMotorsCLI_IntegratedStepperMotor import IntegratedStepper
 log = logging.getLogger(__name__)
 
 
-class CageRotor(IntegratedStepperMotor):
+class CageRotator(IntegratedStepperMotor):
     """
     Represents a generic interface for Thorlabs Cage Rotator motor controllers.
     

@DCEM
Copy link

DCEM commented Dec 6, 2024

Thank you for taking the time @thangleiter, i think the uppercase L in the Namespace might be the issue here, sorry for that:

diff --git forkSrcPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py forkDstPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
index 195e19ef755a962093f1f5da629c06c53382d1dd..ec9fbdc6987089eecbb51f251a260061f5f02db6 100644
--- forkSrcPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
+++ forkDstPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
@@ -62,8 +62,8 @@ class K10CR1(CageRotator, ThorlabsQcodesInstrument):
     def _import_device_dll(self):
         """Import the device-specific DLLs and classes from the .NET API."""
         self._add_dll('Thorlabs.MotionControl.GenericMotorCLI.dll')
-        self._add_dll('ThorLabs.MotionControl.IntegratedStepperMotorsCLI.dll')
-        self._import_dll_class('ThorLabs.MotionControl.IntegratedStepperMotorsCLI', 'CageRotator')
+        self._add_dll('Thorlabs.MotionControl.IntegratedStepperMotorsCLI.dll')
+        self._import_dll_class('Thorlabs.MotionControl.IntegratedStepperMotorsCLI', 'CageRotator')
 
     def _get_api_interface_from_dll(self, serial_number: str):
         """Retrieve the API interface for the Thorlabs device using its serial number."""

Also note that I did not impliment any of the features that are specifict to the motor, i am willing the help with that if you want - just let me know.

@thangleiter
Copy link
Contributor Author

The serial still needs to be converted to strings -- maybe also in other places:

diff --git a/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py b/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
index ec9fbdc6..655887c0 100644
--- a/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
+++ b/src/qcodes_contrib_drivers/drivers/Thorlabs/Thorlabs_K10CR1_DotNet.py
@@ -67,7 +67,7 @@ class K10CR1(CageRotator, ThorlabsQcodesInstrument):

     def _get_api_interface_from_dll(self, serial_number: str):
         """Retrieve the API interface for the Thorlabs device using its serial number."""
-        return self._dll.CageRotator.CreateCageRotator(serial_number)
+        return self._dll.CageRotator.CreateCageRotator(str(serial_number))

     def _post_connection(self):
         """
@@ -86,7 +86,7 @@ class K10CR1(CageRotator, ThorlabsQcodesInstrument):
         Will run after polling has started and the device/channel is enabled.
         """
         # Load any configuration settings needed by the device/channel
-        serial = self._serial_number
+        serial = str(self._serial_number)
         mode = self._startup_mode
         self._api_interface.LoadMotorConfiguration(serial, mode)
         self._configuration = self._api_interface.LoadMotorConfiguration(serial)
diff --git a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py
index 7746435e..90d4935b 100644
--- a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py
+++ b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py
@@ -32,7 +32,7 @@ class IGenericCoreDeviceCLI():
     def connect(self):
         """Connects to the device using its serial number."""
         try:
-            self._api_interface.Connect(self._serial_number)
+            self._api_interface.Connect(str(self._serial_number))
         except Exception as e:
             self.log.exception(f"Failed to connect to instrument with serial number {self._serial_number}")
             raise

This patch makes initialization work.

However, implementing functionality as parameters got me stuck. An example: from what I understand from the readme, there is no way to implement the MoveTo as setter for a position parameter because the function takes more than one argument which _set_thorlabs_decimal does not support.

@DCEM
Copy link

DCEM commented Dec 13, 2024

@thangleiter thank you for keeping up your efforts in testing this code.

I'm a bit unsure on how to handle the string serial number issue. To be honest i didn't even consider not using a string, that is why the typehint in K10CR1 is also str. Maybe because the pythonnet example from thorlabs does use the string type right away (K10CR1_pythonnet.py)

I would prefer to have the thorlabs implementation mimic the .Net API - and that will expect a string. I think it could be acceptable to have an automatic converstion as you suggested but that would be a convinice feature. I tried to stay with the original type, because automatically changing a type might cause issues. That is also why i converted the .Net decimal into a python decimal and not a float. This unfortuantely will cause issues when you want to do math with a value read from an insturement. A solution for this (the decimal issue) might be to conver to a float by default, but still get the decimal if you need it.
When setting a value that expects a decimal i convert to a dotnetdecimal automatically in that regard i think it could be acceptable to make the changes you suggested.
I am wondering if something weird might happen when/if automatically converting.
One solution might be to just expect a string if we decide to go with the automatic conversion I think we could consider defining a serial number parser somwhere, this way the behaviour can be changed at a single space if we need it to.

I don't think there are huge implications and I think I don't mind any of the solutions.

Regarding the feature implementation - I guess I didn't do a manual or even give a hint - sorry about that.

The idea is once again to mimic the thorlabs inheritance structure. Most of the movement stage related stuff will be in: GenericMotorCLI_AdvancedMotor.py and that should be accessilbe to K10CR1 right away (via: GenericAdvancedMotorCLI -> IntegratedStepperMotor -> CageRotator -> K10CR1 )

Imho this should result in K10CR1 already having a move_to method. Can you try a dir(K10CR1_instance) to see if is there. There should be a lot of parameters/methodes already implemented. (use help() to access docstrinigs at runtime)
You could also try the print_readable_snapshot(True) Qcodes method to see what you will already get from the implementation.
Non-Thorlabs API features i imlemented that should affect K10CR1:
the position parameter is settable
if set, it will calculate the time needed for the movment add 1 second and use that as timeout
for a rotaional axis it will porberbly use the RotationalStageValidator (that was needed for a rotaional axis that could not move indefinate)
i think only shortest distance is implemented an the positon will be normalized by modulo 360

if your device need a different behaviour the _set_position can be overwritten but at least per .Net API it should work right away.

if a feature is missing, it should be implemented in the respective class to allow the mimic to work.
K10CR1 does not seem to have too much special features, some are GetBowIndex, GetButtonParams, GetLEDSwitches, PotentiometerParameters (from: IntegreatedStepperMtor)

some functions like: GetMotorConfiguration are explicit marked as overwritten in the .Net help. but when you look at the definition, i don't think anything needs to be done on the qcodes implementation side.

All of this makes this implementation to be less accessible in the beginning.
The upside is that the with applying the modifications to the Thorlabs_Template_DotNet.py you should be able to get a lot of functionally running with little effort. (for other instruments than KD10CR1)

Also code redundency is minimized and if functionallity is missing in GenericAdvancedMotorCLI for example basically all instruments will get it as soon as it is implemented.

@thangleiter
Copy link
Contributor Author

@thangleiter thank you for keeping up your efforts in testing this code.

I'm a bit unsure on how to handle the string serial number issue. To be honest i didn't even consider not using a string, that is why the typehint in K10CR1 is also str. Maybe because the pythonnet example from thorlabs does use the string type right away (K10CR1_pythonnet.py)

I would prefer to have the thorlabs implementation mimic the .Net API - and that will expect a string. I think it could be acceptable to have an automatic converstion as you suggested but that would be a convinice feature. I tried to stay with the original type, because automatically changing a type might cause issues. That is also why i converted the .Net decimal into a python decimal and not a float. This unfortuantely will cause issues when you want to do math with a value read from an insturement. A solution for this (the decimal issue) might be to conver to a float by default, but still get the decimal if you need it. When setting a value that expects a decimal i convert to a dotnetdecimal automatically in that regard i think it could be acceptable to make the changes you suggested. I am wondering if something weird might happen when/if automatically converting. One solution might be to just expect a string if we decide to go with the automatic conversion I think we could consider defining a serial number parser somwhere, this way the behaviour can be changed at a single space if we need it to.

I don't think there are huge implications and I think I don't mind any of the solutions.

There is no way around supporting integer serial numbers because an instrument should also be loadable from a station yaml:

instruments:
  rotator:
    type: qcodes_contrib_drivers.drivers.Thorlabs.Thorlabs_K10CR1_DotNet.K10CR1
    init:
      serial: 55001014

Here, the serial will be parsed as an integer. In any case, I don't see why casting to str should be a problem. If it fails, it's a more informative exception than anything dotnet can offer I think.

Regarding the feature implementation - I guess I didn't do a manual or even give a hint - sorry about that.

The idea is once again to mimic the thorlabs inheritance structure. Most of the movement stage related stuff will be in: GenericMotorCLI_AdvancedMotor.py and that should be accessilbe to K10CR1 right away (via: GenericAdvancedMotorCLI -> IntegratedStepperMotor -> CageRotator -> K10CR1 )

Imho this should result in K10CR1 already having a move_to method. Can you try a dir(K10CR1_instance) to see if is there. There should be a lot of parameters/methodes already implemented. (use help() to access docstrinigs at runtime) You could also try the print_readable_snapshot(True) Qcodes method to see what you will already get from the implementation. Non-Thorlabs API features i imlemented that should affect K10CR1: the position parameter is settable if set, it will calculate the time needed for the movment add 1 second and use that as timeout for a rotaional axis it will porberbly use the RotationalStageValidator (that was needed for a rotaional axis that could not move indefinate) i think only shortest distance is implemented an the positon will be normalized by modulo 360

if your device need a different behaviour the _set_position can be overwritten but at least per .Net API it should work right away.

if a feature is missing, it should be implemented in the respective class to allow the mimic to work. K10CR1 does not seem to have too much special features, some are GetBowIndex, GetButtonParams, GetLEDSwitches, PotentiometerParameters (from: IntegreatedStepperMtor)

some functions like: GetMotorConfiguration are explicit marked as overwritten in the .Net help. but when you look at the definition, i don't think anything needs to be done on the qcodes implementation side.

All of this makes this implementation to be less accessible in the beginning. The upside is that the with applying the modifications to the Thorlabs_Template_DotNet.py you should be able to get a lot of functionally running with little effort. (for other instruments than KD10CR1)

Also code redundency is minimized and if functionallity is missing in GenericAdvancedMotorCLI for example basically all instruments will get it as soon as it is implemented.

Oh, I see, that was not clear to me. Knowing this, the following patches are required to get the position parameter to work:

diff --git a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/GenericMotorCLI_AdvancedMotor.py b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/GenericMotorCLI_AdvancedMotor.py
index 999a6403..11f968e8 100644
--- a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/GenericMotorCLI_AdvancedMotor.py
+++ b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/GenericMotorCLI_AdvancedMotor.py
@@ -1,4 +1,5 @@
 import logging
+from decimal import Decimal as PyDecimal
 from typing import Optional
 
 from qcodes.instrument.channel import InstrumentBase
@@ -90,6 +91,7 @@ class GenericAdvancedMotorCLI(GenericMotorCLI):
             'position',
             get_cmd=lambda: self._get_thorlabs_decimal('Position'),
             set_cmd=self._set_position,
+            set_parser=PyDecimal,
             vals=self._get_position_vals(),
             unit=self.advanced_limits.length_unit(),
             docstring='Position of the stage. '
@@ -178,9 +180,9 @@ class GenericAdvancedMotorCLI(GenericMotorCLI):
         if self.wait_for_movement():
             # Calculate required time for the movement
             time_needed = self._compute_move_time_ms(
-                max_distance,
-                self.advanced_limits.acceleration_max(),
-                self.home_parameters.velocity()
+                float(max_distance),
+                float(self.advanced_limits.acceleration_max()),
+                float(self.home_parameters.velocity())
             )
             timeout_ms = 2000 + time_needed
         else:
@@ -237,15 +239,15 @@ class GenericAdvancedMotorCLI(GenericMotorCLI):
         if self.wait_for_movement():
             # Calculate time required for movement based on the shortest distance
             time_needed = self._compute_move_time_ms(
-                shortest_distance,
-                self.advanced_limits.acceleration_max(),
-                self.home_parameters.velocity()
+                float(shortest_distance),
+                float(self.advanced_limits.acceleration_max()),
+                float(self.home_parameters.velocity())
             )
             timeout_ms = 1000 + time_needed
         else:
             timeout_ms = 0
 
-        self.move_to(position, timeout_ms)
+        self.move_to(float(position), timeout_ms)
 
     def move_to(self, position: DotNetDecimal, timeout_ms=0) -> None:
         """
@@ -570,4 +572,3 @@ class RotationalStageValidator(PyDecimalNumbers):
 
     def __repr__(self) -> str:
         return f"<RotationalStageValidator wrapping at {self.wrap_at} with range ({self._min_value}, {self._max_value})>"
-

Two more issues came up during testing:

  1. Decimal is not JSON serializable, which makes parameters that return this type incompatible with qcodes-style measurements which at the very least serialize a station snapshot. Might also cause problems with the sqlite database, I don't know.
  2. The destructor seems to be incomplete. Closing an instrument and re-opening it does not work:
In [14]: rotator.close()
In [15]: rotator = K10CR1('rotator', 55001014)
2024-12-16 10:12:16,122 ¦ qcodes.instrument.instrument_base ¦ ERROR ¦ DeviceManagerCLI ¦ connect ¦ 37 ¦ [rotator(K10CR1)] Failed to connect to instrument with serial number 55001014
Traceback (most recent call last):
  File "~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\DeviceManagerCLI.py", line 35, in connect
    self._api_interface.Connect(str(self._serial_number))
Thorlabs.MotionControl.DeviceManagerCLI.DeviceNotReadyException: Device is not connected
   at Thorlabs.MotionControl.DeviceManagerCLI.ThorlabsGenericCoreDeviceCLI.VerifyDeviceConnected(Int32 functionDepth)
   at Thorlabs.MotionControl.GenericMotorCLI.GenericMotorCLI.Connect(String serialNo)
2024-12-16 10:12:16,124 ¦ qcodes.instrument.instrument_base ¦ ERROR ¦ qcodes_thorlabs_integration ¦ __init__ ¦ 523 ¦ [rotator(K10CR1)] Failed to connect to Thorlabs device with serial number 55001014
---------------------------------------------------------------------------
DeviceNotReadyException                   Traceback (most recent call last)
Cell In[15], line 1
----> 1 rotator = K10CR1('rotator', 55001014)

File ~\Documents\Qcodes\src\qcodes\instrument\instrument_meta.py:36, in InstrumentMeta.__call__(cls, *args, **kwargs)
     31 def __call__(cls, *args: Any, **kwargs: Any) -> Any:
     32     """
     33     Overloads `type.__call__` to add code that runs only if __init__ completes
     34     successfully.
     35     """
---> 36     new_inst = super().__call__(*args, **kwargs)
     37     is_abstract = new_inst._is_abstract()
     38     if is_abstract:

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\Thorlabs_K10CR1_DotNet.py:48, in K10CR1.__init__(self, name, serial_number, startup_mode_value, simulation, polling_rate_ms, dll_directory, **kwargs)
     37 def __init__(
     38     self,
     39     name: str,
   (...)
     45     **kwargs
     46 ):
---> 48     super().__init__(
     49         name,                             # Instrument (Qcodes)
     50         serial_number=serial_number,      # ThorlabsQcodesInstrument
     51         startup_mode_value=startup_mode_value,
     52         simulation=simulation,            # ThorlabsQcodesInstrument
     53         polling_rate_ms=polling_rate_ms,  # GenericMotoCLI
     54         dll_directory=dll_directory,      # ThorlabsDLLMixin
     55         **kwargs)
     57     if '(Simulated)' not in self.model():
     58         self.snapshot(True)

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\IntegratedStepperMotorsCLI_CageRotator.py:24, in CageRotator.__init__(self, *args, **kwargs)
     23 def __init__(self, *args, **kwargs):
---> 24     super().__init__(*args, **kwargs)

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\IntegratedStepperMotorsCLI_IntegratedStepperMotor.py:13, in IntegratedStepperMotor.__init__(self, *args, **kwargs)
     12 def __init__(self, *args, **kwargs):
---> 13     super().__init__(*args, **kwargs)

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\GenericMotorCLI_AdvancedMotor.py:41, in GenericAdvancedMotorCLI.__init__(self, *args, **kwargs)
     40 def __init__(self, *args, **kwargs):
---> 41     super().__init__(*args, **kwargs)
     43     advanced_limits = AdvancedMotorLimits(self, 'advanced_limits')
     44     self.add_submodule('advanced_limits', advanced_limits)

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\GenericMotorCLI.py:35, in GenericMotorCLI.__init__(self, *args, **kwargs)
     31 if not isinstance(self, ThorlabsMixin):
     32     raise TypeError("GenericMotorCLI should only be mixed with "
     33                     "subclasses of ThorlabsMixin")            
---> 35 super().__init__(*args, **kwargs)
     37 motor_position_limits = LimitsData(self, 'motor_position_limits')
     38 self.add_submodule('motor_position_limits', motor_position_limits)

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\DeviceManagerCLI.py:124, in IGenericDeviceCLI.__init__(self, polling_rate_ms, api_interface, startup_mode_value, *args, **kwargs)
    121 self._set_api_interface(kwargs.pop('api_interface', api_interface))
    122 self._set_startup_mode_value(kwargs.pop('startup_mode_value', startup_mode_value))
--> 124 super().__init__(*args, **kwargs)
    126 if hasattr(self, '_edit_startup_mode'):
    127     self._edit_startup_mode()

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\qcodes_thorlabs_integration.py:521, in ThorlabsQcodesInstrument.__init__(self, simulation, *args, **kwargs)
    517     self._dll.DeviceManagerCLI.BuildDeviceList()
    519     self._set_api_interface(self._get_api_interface_from_dll(str(self._serial_number)))
--> 521     self.connect()
    522 except Exception as e:
    523     self.log.error(f"Failed to connect to Thorlabs device with serial number {self._serial_number}")

File ~\Documents\qcodes_contrib_drivers\src\qcodes_contrib_drivers\drivers\Thorlabs\private\DotNetAPI\DeviceManagerCLI.py:35, in IGenericCoreDeviceCLI.connect(self)
     33 """Connects to the device using its serial number."""
     34 try:
---> 35     self._api_interface.Connect(str(self._serial_number))
     36 except Exception as e:
     37     self.log.exception(f"Failed to connect to instrument with serial number {self._serial_number}")

DeviceNotReadyException: Device is not connected
   at Thorlabs.MotionControl.DeviceManagerCLI.ThorlabsGenericCoreDeviceCLI.VerifyDeviceConnected(Int32 functionDepth)
   at Thorlabs.MotionControl.GenericMotorCLI.GenericMotorCLI.Connect(String serialNo)

@DCEM
Copy link

DCEM commented Dec 16, 2024

We are using it in a station.yml:

  optical_stage:
    type: qcodes_contrib_drivers.drivers.Thorlabs.Thorlabs_BSCxxx_DotNet.ThorlabsBSCxxx
    init:
      serial_number: '70233194'

but I think it is acceptable to cast it as a string.
I think it should be done here. I cannot test it right now, but i think this is also the only place it needs to be done:

diff --git forkSrcPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py forkDstPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py
index 7746435ef41812477a888e61a965dac73146bb5d..1f44e4d06e306165971a442e41c1cb5c34ef63b4 100644
--- forkSrcPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py
+++ forkDstPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/DeviceManagerCLI.py
@@ -19,7 +19,7 @@ class IGenericCoreDeviceCLI():
     def __init__(self, *args, serial_number: str, **kwargs):
         super().__init__(*args, **kwargs)
 
-        self._serial_number = kwargs.pop('serial_number', serial_number)
+        self._serial_number = str(kwargs.pop('serial_number', serial_number))
 
         self.add_parameter(
             'busy',

The decimal issue is a thing. I am not sure if/when one might run into issues here. But I agree, using decimal is really anoying. I do remember reading in the thorlabs documentation that there are issues that can occur.
But to avoid the issues we got using decimal, we ended up converting to float.
The code actually does convert from .Net Decimal to Python decimal but there still were issues.

Maybe it is a good idea to have an instrument attribute like auto_decimal_conversion that could be True per default an then will output float, and accept float as input for all the instances where decimal is used.

I'm happy to look into that at some point, but I would like to take the "bigger picture" question regarding the thorlabs implementation into account.

Regarding the destructor - i think adding shut_down to it should be enough, but cannot test it right now.

diff --git a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py
--- a/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py
+++ b/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py
@@ -594,4 +594,8 @@
         return dict(zip(("vendor", "model", "serial", "firmware"), idparts))
 
+    def close(self) -> None:
+        self.shut_down()
+        super().close()
+
 
 numbertypes = Union[float, int, np.floating, np.integer, PyDecimal]

Edit: I managed to get a simulation running with the code. (still with issues)
but i realized the destructor should be corrected as follows: (including the dff above)

diff --git forkSrcPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py forkDstPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py
index fe2b2ea3e25605e0405e645e5ed6c7eb58ab5130..2639be4099fefff190f2cfc20b6f89efe3f3df0f 100644
--- forkSrcPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py
+++ forkDstPrefix/src/qcodes_contrib_drivers/drivers/Thorlabs/private/DotNetAPI/qcodes_thorlabs_integration.py
@@ -1,4 +1,3 @@
-import atexit
 import importlib
 import logging
 import math
@@ -491,8 +490,6 @@ class ThorlabsQcodesInstrument(IGenericCoreDeviceCLI, ThorlabsDLLMixin, Thorlabs
         self._simulation = kwargs.pop('simulation', simulation)
         super().__init__(*args, **kwargs)

-        atexit.register(self._exit_generic_device)
-
         # Import common DLLs.
         try:
             self._add_dll('Thorlabs.MotionControl.DeviceManagerCLI.dll')
@@ -538,16 +535,6 @@ class ThorlabsQcodesInstrument(IGenericCoreDeviceCLI, ThorlabsDLLMixin, Thorlabs
             self.log.error(f"Failed to initialize Thorlabs {self._model()} with serial number {self._serial_number}")
             raise

-    def _exit_generic_device(self) -> None:
-        """
-        Safely terminate operations at exit by:
-        * disconnect the device
-        * stop the simulation if needed
-        """
-        self.disconnect()
-        if self._simulation:
-            self._dll.SimulationManager.Instance.UninitializeSimulations()
     -
     def _import_device_dll(self):
         """Import the device-specific DLLs and classes from the .NET API."""
         raise NotImplementedError("This method should be implemented by subclasses.")
     @@ -599,7 +586,12 @@ class ThorlabsQcodesInstrument(IGenericCoreDeviceCLI, ThorlabsDLLMixin, Thorlabs
         return dict(zip(("vendor", "model", "serial", "firmware"), idparts))

     def close(self) -> None:
+        self.disconnect()
         self.shut_down()
+        
+        if self._simulation:
+            self._dll.SimulationManager.Instance.UninitializeSimulations()
+
         super().close()

@DCEM
Copy link

DCEM commented Dec 19, 2024

@thangleiter Thanks for raising the JSON serialization issue. This is just one challenge when dealing with decimals.

The Thorlabs .NET API uses decimals for real-world units. Converting these to floats can cause precision loss. Returning them as strings before serialization would preserve the exact numeric values.

Right now, having decimal parameters is not ideal. The helper functions that convert from DotNetDecimal to PyDecimal should convert to either a string or a float. There should be a straightforward way to choose the desired conversion at both the instrument and parameter levels.

I suggest making floats the default. This should be a keyword argument so that users can easily see it is an option.

Handling validators might become an issue when done like this. Maybe a validator that accepts strings and uses decimal internally could be an option.

I’d like input from @jenshnielsen on this decimal issue.

I’m willing to implement these changes, but I’d appreciate a decision on whether this code will be included so that the effort is well spent.

@thangleiter
Copy link
Contributor Author

I just realize I lied, the NumpyJSONEncoder that qcodes uses for dumping snapshots can handle decimals, so at least encoding works.

>>> json.dumps(decimal.Decimal(), cls=qcodes.utils.NumpyJSONEncoder)
'"0"'

@DCEM
Copy link

DCEM commented Dec 28, 2024

Ok., since json serialisation works, i thought about how to proceed.

I introduced :
I introduced a new constructor argument decimal_as_float to toggle .NET Decimal conversion to either Python Decimal or float.
There is no default, so the user has to make a concious choice on how to set ist.

To test and understand how to add more Instruments i wanted to try adding the ones with existing implementation.
An Issue with enums occured and i ended up doing the following changes to ThorlabsMixin:

  • 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.

Now the following devices are supported:
BSCxxx
K10CR1
KDC101 (this inkludes the PRM1Z8 actor)
KLSnnn
MFF10x
TDC001

I think now all existing kinesis instruments are supported.
I will have to update the manual on how to add further instruments, but you can take a look at the commits - after undrestanding the initial complexity it is somewhat easy i think.

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.

5 participants