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 66 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 are missing coverage. Please review.

Project coverage is 10.56%. Comparing base (608a88d) to head (553f207).

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.

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.

None yet

5 participants