-
Notifications
You must be signed in to change notification settings - Fork 77
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
base: main
Are you sure you want to change the base?
Thorlabs kinesis drivers #262
Conversation
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.
Stacking classmethod and property is deprecated in CPython
@thangleiter since the other PR got merged, there are merge conflicts now - could you resovle those?
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 |
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. |
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). 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. 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. 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. |
Same here, it's been a while.
I never had this problem with our devices since they have dedicated classes in the C API. I would expect it's either in
I implemented this by decorating base class methods with the prefix (see, #262 (comment)).
Can you connect to the same instrument concurrently? Otherwise what do you mean by loading the settings? Are they persistent?
See above.
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. |
If I remember correctly I tried that, but it returned something different (someting like "Benchtop Stepper Device").
No, I don't think it is possible to connect to an instrument concurrently.
If I saw it correctly you chose to have a folder for each instrument and then a filename by implementation type?
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. |
Since I would argue for retaining the APT versions of the drivers for backwards compatibility, the 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?
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 |
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. |
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.