search

QCoDeS / Qcodes_contrib_drivers

A collection of community-contributed QCoDeS drivers for instruments
https://qcodes.github.io/Qcodes_contrib_drivers/
MIT License
46 stars 84 forks source link

Thorlabs kinesis drivers #262

Open thangleiter opened 1 year ago

thangleiter commented 1 year ago

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.

astafan8 commented 1 year ago

@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 commented 1 year ago

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

thangleiter commented 1 year ago

I merged the branch and kept both versions for discussion. The drivers @julienbarrier added would still need to be re-implemented, but the idea of this PR is that this should be rather straightforward. As an example, I implemented (a part of) the TDC001 driver using the proposed changes (lives mostly in qcodes_contrib_drivers/drivers/Thorlabs/private/kinesis/cc.py).

Again, since a lot of the instruments have a lot of common functionality, there is only one dll interface class (ThorlabsKinesis) whose methods can be marked using the @register_prefix(prefixes) decorator to be forwarded to a specific KinesisInstrument subclass. For instance, both the TDC001 (with CC prefixed dll functions) and the K10CR1 (with ISC prefixed functions) support different rotation modes but are controlled using different dlls. Instead of having to implement different methods for each specific dll call, one can just mark the given method with the CC and ISC prefixes. In a nutshell:

class ThorlabsKinesis:
    ...
    @register_prefix(['CC', 'ISC'])
    def set_rotation_modes(self, mode):
        self.get_dll_function(mode)

class TDC001(KinesisInstrument, prefix='CC'): pass
class K10CR1(KinesisInstrument, prefix='ISC'): pass
class ABCDEF(KinesisInstrument, prefix='FOO'): pass

hasattr(TDC001, 'set_rotation_modes')
# True
hasattr(K10CR1, 'set_rotation_modes')
# True
hasattr(ABCDEF, 'set_rotation_modes')
# False

Instrument subclasses then mostly only need to declare qcodes parameters to implement a driver.

codecov[bot] commented 7 months ago

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.

:exclamation: 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 :warning:
..._drivers/drivers/Thorlabs/private/kinesis/enums.py 0.00% 110 Missing :warning:
...b_drivers/drivers/Thorlabs/Thorlabs_PM100D/tlpm.py 0.00% 65 Missing :warning:
...rivers/drivers/Thorlabs/private/kinesis/structs.py 0.00% 33 Missing :warning:
...rib_drivers/drivers/Thorlabs/private/kinesis/cc.py 0.00% 23 Missing :warning:
...ib_drivers/drivers/Thorlabs/private/kinesis/isc.py 0.00% 23 Missing :warning:
...ib_drivers/drivers/Thorlabs/Thorlabs_MFF10x/apt.py 0.00% 20 Missing :warning:
...rivers/drivers/Thorlabs/Thorlabs_MFF10x/kinesis.py 0.00% 18 Missing :warning:
...ib_drivers/drivers/Thorlabs/Thorlabs_K10CR1/apt.py 0.00% 10 Missing :warning:
...ib_drivers/drivers/Thorlabs/Thorlabs_PRM1Z8/apt.py 0.00% 7 Missing :warning:
... and 5 more
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thangleiter commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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, https://github.com/QCoDeS/Qcodes_contrib_drivers/pull/262#issuecomment-1798342827).

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 commented 5 months ago

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)](https://github.com/QCoDeS/Qcodes_contrib_drivers/pull/262#issuecomment-1798342827)).

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 commented 5 months ago

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 commented 5 months ago

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 commented 3 months ago

@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 commented 2 months ago

@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:

Drawbacks:

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 commented 2 months ago

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.