mabuchilab / Instrumental

Python-based instrumentation library from the Mabuchi Lab.
http://instrumental-lib.readthedocs.org/
GNU General Public License v3.0
117 stars 77 forks source link

UC480 Thorlabs camera driver exposure facets, thorlabs TLPM powermeter, smaract SCU actuator drivers #144

Closed seb5g closed 2 years ago

natezb commented 2 years ago

Hi, thanks for the contribution. Would you like me to merge in this full branch, or just the first commit?

seb5g commented 2 years ago

Hello, Well I will probably continue working on adding things in the near future but would be great for the time being to have all these merged.

By the way, I was wondering why the instruments of type Motion doesn't share a common exposed interface that would allow their integration in other framework (the same is all applicable to other type of drivers)? For instance, I'm the developer of PyMoDAQ (https://github.com/CEMES-CNRS/PyMoDAQ) and use of your motion drivers would be made easier if they all have in common a set of base methods (like move_abs, move_rel, stop, check_position...). If all driver of type motion have these in common they could be called without the need to look at more specific methods for each instrument.

Another question: I'm not very familiar with cffi but found your nicelib package quite useful. However does all user have to process the header files before they can use C types dlls? Or should the processed header files be included in the package (for instance my _sculib.py ? I could not find automatic way that is using build_lib if the header file was not already processed.

Finally, did you have some thought about how to include .net assemblies in you driver framework?

Anyhow, I find Instrumental to be a very nice project!

natezb commented 2 years ago

Thanks, glad you've found Instrumental to be useful!

There's no deep reason why we don't have a common Motion API. I was probably waiting for us to have a few motion devices before deciding on the API, but it never got done. Ultimately we should add one, so feel free to contribute on that front.

As for your second question, I set the system up so the headers get processed upon first use by the end user. This way we don't have to deal with the question of distributing other companies' headers (or derivatives thereof). nicelib.load_lib() will build a low-level lib module if it can't be found.

We don't currently have any .NET drivers, but I'm not against adding them. I've never tried it, but it appears there is a package called Python.NET, which is probably what I'd use.

seb5g commented 2 years ago

Hello, I pushed some updates on smaract and some patches on the K10CR1 driver from thorlabs, could you merge them. Also, what's your next release planned?

Also I tried to create a driver on thorlabs new powermeter library TLPM but I'm having issues in parsing the header file. I ended writing my own driver using ctypes but it's kind of a waste... Could you provide some help?

natezb commented 2 years ago

Sorry for the delay, things have been busy. I'll try to look over this soon. Are you still having difficulties with the TLPM header file?

seb5g commented 2 years ago

Concerning the TLPM, I couldn't manage to make it work. I used classical ctypes way to create a pymodaq's plugin. Wish I could add this driver to instrumental but I'm kind of stuck...

natezb commented 2 years ago

Do you have an error traceback or something that you could post?

seb5g commented 2 years ago

Here is the traceback:

Connected to pydev debugger (build 192.7142.42)
C:\Miniconda3\envs\pymodaq_dev\lib\site-packages\nicelib\process.py:1472: UserWarning: Un-pythonable macro _VI_PTR: <generator>:2:30: before: ;
  warnings.warn("Un-pythonable macro {}: {}".format(macro.name, str(e)))
C:\Miniconda3\envs\pymodaq_dev\lib\site-packages\nicelib\process.py:1472: UserWarning: Un-pythonable macro ViPtr: <generator>:2:28: before: ;
  warnings.warn("Un-pythonable macro {}: {}".format(macro.name, str(e)))
Traceback (most recent call last):
  File "C:\Users\weber\AppData\Roaming\Python\Python38\site-packages\cffi\api.py", line 126, in _cdef
    self._parser.parse(csource, override=override, **options)
  File "C:\Users\weber\AppData\Roaming\Python\Python38\site-packages\cffi\cparser.py", line 389, in parse
    self._internal_parse(csource)
  File "C:\Users\weber\AppData\Roaming\Python\Python38\site-packages\cffi\cparser.py", line 412, in _internal_parse
    self._parse_decl(decl)
  File "C:\Users\weber\AppData\Roaming\Python\Python38\site-packages\cffi\cparser.py", line 505, in _parse_decl
    self._declare_function(tp, quals, decl)
  File "C:\Users\weber\AppData\Roaming\Python\Python38\site-packages\cffi\cparser.py", line 498, in _declare_function
    self._declare(tag + decl.name, tp)
  File "C:\Users\weber\AppData\Roaming\Python\Python38\site-packages\cffi\cparser.py", line 568, in _declare
    raise FFIError(
cffi.FFIError: multiple declarations of function TLPM_setDigIoOutput (for interactive usage, try cdef(xx, override=True))

Process finished with exit code 1

and this is the code:

from nicelib import build_lib, generate_bindings
from nicelib.process import declspec_hook, struct_func_hook
import os

header_info = {
    'win32': {
        'path': (
            r"{VXIPNPPATH}\WinNT\Include",
        ),
        'header': 'TLPM.h'
    },
    'win64': {
        'path': (
            r"{VXIPNPPATH64}\Win64\Include",
        ),
        'header': 'TLPM.h'
    },
}

lib_names = {'win32': f'{os.environ["VXIPNPPATH"]}\\WinNT\\Bin\\TLPM_32.dll',
             'win64': f'{os.environ["VXIPNPPATH64"]}\\Win64\\Bin\\TLPM_64.dll'}

def fastcall_hook(tokens):
    """Removes ``cdecl``, ``_cdecl``, and ``__cdecl``

    Enabled by default.
    """
    for token in tokens:
        if token not in ('__fastcall',):
            yield token

def build():
    build_lib(header_info, lib_names, '_tlpmlib', __file__, token_hooks=(declspec_hook, fastcall_hook))

# def bindings():
#     with open('bindings.py') as f:
#         generate_bindings(header_info, f)

if __name__ == '__main__':
    build()
    #bindings()
seb5g commented 2 years ago

and the header file:

TLPM.txt

natezb commented 2 years ago

The header you're trying to parse has a repeated declaration, and CFFI doesn't like that. I just pushed a change to NiceLib (mabuchilab/NiceLib@feb91e9001a56619f8d4a956743c8df3ceb6a2d2) that adds an override option to build_lib(). In your case you should try passing override=True, which should allow the repeated declaration.

seb5g commented 2 years ago

Hello there, using your new commit, I manage to build from the header and produced this new driver. Compatible with the new thorlabs brand of powermeters and tested with the PM101A.

I also produced a new driver for the motion stages from mad city labs

seb5g commented 2 years ago

by the way, I'm organising in next October a session of 5 days around my PyMoDAQ framework (pymodaq.cnrs.fr). 3 days will be devoted to training of new users while the remaining two days will be a kind of PyMoDAQ Days. With presentations of people using PyMoDAQ in their lab, round table about issues and new developement. I would also like to have an talk from people who are developing libraries such as Instrumental. It would be nice to have some input from people who are on the field of open-source software for lab instrumentation for some time and also see if/how bridges could be build between instrumental and pymodaq. Would you be interested?

seb5g commented 2 years ago

Hello, the last commit is to take into account the class defined in the paramset. When testing one of my new driver (smaract SCU), I remarked that the instrument class was not the good one. 3 are available in the driver: SCU, SCULinear or SCURotation. Despite the fact I used a paramset with the Linear version defined, instrumental returned the first one in the list which is not satisfactory. I propose therefore this modification

What about my proposal above? And why did'nt you merge this already by the way? Is there any issue?

seb5g commented 2 years ago

Hello, I'm sure you are quite busy, but I'm making much effort in adding new driver to the library, but I'm now facing the problem that I cannot publish pypi updates of my instrument plugin using instrumental new features because there is no new release of instrumental and couldn't find any schedule.

Did you have thought into my invitation?

natezb commented 2 years ago

I am sorry for how long it's taken to review this. I haven't been able to do this as part of my day job for several years, so I need to find spare time to address whatever issues there are. I also haven't used the library for a few years and don't have access to the vast majority of equipment, which makes it pretty challenging to verify that things work.

I made a few code review notes, and when those things are addressed I think everything should be good to merge. Sorry again for all the delays, and thanks for your contributions!

As for your proposal, it is interesting but as you can tell I don't have much energy to devote to this project at the moment. I'll think it over.

seb5g commented 2 years ago

I addressed most of your points with some questions remaining. Do you know when you will publish a new release on pypi? Do you need some help in maintaining instrumental?

natezb commented 2 years ago

I can probably make a release on PyPI within the next week. It's been awhile since a release, so some release procedures may have changed--that's the experience I remember from when I released NiceLib a few months back.

And yes, help with maintaining Instrumental would be valuable. There may be some things we could set up like adding flake8 and other checks to automatically run on PRs. Also adding some contribution guidelines.

seb5g commented 2 years ago

Following your advice for linting on PR I worked on setting up github actions (see commits) but then I just saw you're doing linting on appveyor. While I'm using github actions for my project I added the possibility here. It's very easy to add python versions, and OS as well (ubuntu or windows for instance) I also added an action (workflow) to trigger automated pypi publication when creating a release on github. It save the hassle to do it manually with twine...

Looking at appveyor config. I saw you dropped python 2.7 and 3.5. But then when I ran flake8 on the code, it found many issues related to python 2 things... I commented some and corrected other but maybe we should just remove them entirely?

Let me know how you would like to work on this?

natezb commented 2 years ago

Ok, I'll take a look over these further changes. In the future, it's probably better to create a new PR for unrelated changes. Every time more stuff gets added to this PR it takes longer to review and merge it.

I think I previously had automatic uploads to PyPI set up through TravisCI but it may not be working anymore.

My intention was to drop Python 2 support for future releases. But prior releases all had support for both 2 and 3, so there's going to be a lot of code written that way. I think it makes sense for all new code to be targeted at Python 3.6+ and to refactor Py2 stuff away when it makes sense, but bulk changes introduce a lot of unnecessary effort and risk of breakage.

seb5g commented 2 years ago

Sorry about the PR mixing everything, it seems my git wasn't configured correctly and pushed everything always on your repo...