vpelletier / python-libusb1

Python ctype-based wrapper around libusb1
GNU Lesser General Public License v2.1
168 stars 65 forks source link

libusb_set_option support #75

Open mcuee opened 2 years ago

mcuee commented 2 years ago

It seems to me libusb_set_option() is missing here. https://libusb.sourceforge.io/api-1.0/group__libusb__lib.html#gaf6ce5db28dac96b1680877a123da4fa8

https://libusb.sourceforge.io/api-1.0/group__libusb__lib.html#ga07d4ec54cf575d672ba94c72b3c0de7c For the enum libusb_option, probably LIBUSB_OPTION_USE_USBDK is a bit special.

Use the UsbDk backend for a specific context, if available. This option should be set immediately after calling libusb_init(), otherwise unspecified behavior may occur. Only valid on Windows.

mcuee commented 2 years ago

Ref: FYI. I have not been able to get usbdk to work under pyusb yet. https://github.com/pyusb/pyusb/issues/200

mcuee commented 2 years ago

Something like this on the low level, but I am not so sure how to add the higher level changes.

diff --git a/usb1/libusb1.py b/usb1/libusb1.py
index 21b682b..98bbe0a 100644
--- a/usb1/libusb1.py
+++ b/usb1/libusb1.py
@@ -706,6 +706,13 @@ libusb_log_level = Enum({
     'LIBUSB_LOG_LEVEL_DEBUG': 4,
 })

+libusb_options = Enum({
+    'LIBUSB_OPTION_LOG_LEVEL' : 0,
+    'LIBUSB_OPTION_USE_USBDK' : 1,
+    'LIBUSB_OPTION_NO_DEVICE_DISCOVERY' : 2,
+    'LIBUSB_OPTION_WEAK_AUTHORITY' : 3,
+})
+
 #int libusb_init(libusb_context **ctx);
 libusb_init = libusb.libusb_init
 libusb_init.argtypes = [libusb_context_p_p]
@@ -717,6 +724,10 @@ libusb_exit.restype = None
 libusb_set_debug = libusb.libusb_set_debug
 libusb_set_debug.argtypes = [libusb_context_p, c_int]
 libusb_set_debug.restype = None
+#int libusb_set_option(libusb_context * ctx, enum libusb_option option, ...)           
+libusb_set_option = libusb.libusb_set_option
+libusb_set_option.argtypes = [libusb_context_p, c_void_p, c_int] #FIXEM handle va list
+libusb_set_option.restype = c_int
 #const struct libusb_version * libusb_get_version(void);
 try:
     libusb_get_version = libusb.libusb_get_version
mcuee commented 2 years ago

Some updates: I figured out a potential workaround for usbdk. But I do not have the proper fix. The issue may or may not be applicable to python-libusb1. https://github.com/pyusb/pyusb/issues/200

vpelletier commented 2 years ago

It is very unfortunate that libusb chose to add a variadic in their API, as these are basically impossible to wrap with ctypes (I was trying to find a positive mention of this in either the documentation or python code, but fail to - the closest I have is a not-merged, possibly-abandoned merge request to add python's ... to signal variadic declarations in ctypes). IIRC whether it works or fails will depend on the calling convention the library is compiled for and the number of arguments. "fails" can mean a lot of things here, in my understanding: from callee pulling the argument from the wrong register or decoding it incorrectly, to popping it from a stack where it was never pushed.

What I did on a previous project was to have a C file with glue functions with fixed arguments (because I knew which forms my ctypes wrapper needed) calling into the variadic. In the case of that module, getting these functions was so essential that I saw no other way. Which means such module now requires a compilation step...

There are two reasons why I chose ctypes (as opposed to a C module):

Reading pyusb/pyusb#200 , I kind of agree that this is a ctypes "bug" (I would rather call it an entirely missing feature which gives the illusion of working on some common archs), but IMHO it is also a very poor design decision from libusb1 to use a variadic in their API. Back when libusb_set_option only handled LIBUSB_OPTION_LOG_LEVEL it was not a big loss so I skipped that function, but it being involved in such important feature is annoying.

mcuee commented 2 years ago

So it seems libusb_set_option does pose some challenges to python bindings. I am not a programmer myself and I only know a little bit of C and Python.

Just wondering if it is possible to split the python binding into different functions. 1) set_option_log_level_option (optional, since env variable can be used instead) 2) set_option_usbdk (for Windows only) 3) set_option_weak_authority (for Android and Linux only) 4) set_option_no_enumeration (for Android and Linux only) -- this is merged in https://github.com/libusb/libusb/pull/935

vpelletier commented 2 years ago

Just wondering if it is possible to split the python binding into different functions.

This is definitely possible, but I can understand libusb maintainers not wanting to multiply such functions.

Alternatives which would be ABI-call-friendly (but definitely not as good-looking as a sweep-complexity-under-compiler's-rug variadic) and which I can think of (I'm not a seasoned C dev) are:

tormodvolden commented 2 years ago

The simplest for now and foreseeable future would be if libusb could keep libusb_set_debug() and not deprecate it. Other than that, libusb has only two other options and they are simple flags without option argument: LIBUSB_OPTION_USE_USBDK and LIBUSB_OPTION_NO_ENUMERATION (previously named LIBUSB_OPTION_WEAK_AUTHORITY) Any new options in the pipeline that I am aware of are also just flags. Then the language bindings for libusb_set_option() wouldn't need to deal with arguments.

It is nice for a library to be future-proof and get everything right from the beginning, and the new libusb_set_option() is enormously scalable and elegant with variadic macros, however it is wonderful over-engineering for a problem that libusb doesn't have, and rather should try to avoid (lots of options with various arguments).

mcuee commented 2 years ago

@tormodvolden This seems to be a good suggestion. Therefore I create libusb issue libusb/libusb#990.

mcuee commented 2 years ago

https://github.com/libusb/libusb/issues/987

One option is to improve libusb using Env variables.

xloem commented 2 years ago

It is very unfortunate that libusb chose to add a variadic in their API, as these are basically impossible to wrap with ctypes (I was trying to find a positive mention of this in either the documentation or python code, but fail to - the closest I have is a not-merged, possibly-abandoned merge request to add python's ... to signal variadic declarations in ctypes). IIRC whether it works or fails will depend on the calling convention the library is compiled for and the number of arguments. "fails" can mean a lot of things here, in my understanding: from callee pulling the argument from the wrong register or decoding it incorrectly, to popping it from a stack where it was never pushed.

I briefly glanced at this as I have an open pull request to libusb that adds options that take a parameter. I was unaware of the python concern.

I believe the failure concern may be resolvable for the current implementations, if the maximum number of parameters is always passed. Extra ones will simply be ignored, and I expect libusb could likely document that to make it clear. Some legacy code passes NULL pointers to denote that there are no further parameters.

vpelletier commented 2 years ago

I believe the failure concern may be resolvable for the current implementations, if the maximum number of parameters is always passed. Extra ones will simply be ignored, and I expect libusb could likely document that to make it clear. Some legacy code passes NULL pointers to denote that there are no further parameters.

I read some more about variadic, and now I wonder why I was under the impression that variadics could cause crashes:

So... was I mistaken entirely and variadic are actually fine, and the python patches are just adding some syntactic sugar ? Or am I still not re-identifying the real issue ?

One thing I saw but do not yet understand is that on (at least) x86-64 registers are used in an order which depend on argument type (integer and float, at least), but if variadic follows the same convention as regular calls (and the argument types are correct to begin with, but it is not the kind of issue I am worried about - at least I think) then the order should be consistent with the one expected by the callee.

I'll search some more, but another day: I wanted to post an update here quickly.

EDIT: fix a few sentence structures and recover a few dropped words

xloem commented 2 years ago

it looks like small types are promoted to int and double in variadics. so this seems mostly an issue for machines where sizeof(int) != sizeof(void*).

I see the maintenance issue.

I believe right now my android branch might be the only code that uses the variadic arguments, and it takes a pointer to the java vm for old android apis where it can't reasonably be enumerated. somebody may improve on that code at some point, which then could use an auxiliary function instead of set_option.

vpelletier commented 2 years ago
  • as the name mangling differs between calling conventions, ctypes is able to infer the calling convention to use for every library symbol (at least those loaded by name).

I checked python's ctype code, and while there is code checking the stdcall name mangling, it is after the code loading the non-mangled version. And indeed, loading libusb using ctype's WinDLL and CDLL I only get the cdecl flag (#define FUNCFLAG_CDECL 0x1) set when loading from CDLL. I also accessed libusb_init for comparison, as that function should be stdcall.

Python 3.10.0 (tags/v3.10.0:b494f59, Oct  4 2021, 18:46:30) [MSC v.1929 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import ctypes
>>> libusb = ctypes.WinDLL(r'libusb-1.0.24\VS2019\MS32\dll\libusb-1.0.dll')
>>> bin(libusb.libusb_set_option._flags_)
'0b0'
>>> bin(libusb.libusb_init._flags_)
'0b0'
>>> c_libusb = ctypes.CDLL(r'libusb-1.0.24\VS2019\MS32\dll\libusb-1.0.dll')
>>> bin(c_libusb.libusb_set_option._flags_)
'0b1'
>>> bin(c_libusb.libusb_init._flags_)
'0b1'

Looking at objdump and IDA free outputs, I see that all functions are listed twice. As a side note, on the MS32 version, the stdcall variant is never mangled for some reason, while for the MS64 it is:

$ objdump.exe -x MS32/dll/libusb-1.0.dll | grep -E 'libusb_(set_option|init)'
        [   0] _libusb_set_option
        [ 122] libusb_init
        [ 121] libusb_init
        [ 155] libusb_set_option
$ objdump.exe -x MS64/dll/libusb-1.0.dll | grep -E 'libusb_(set_option|init)'
        [   0] _libusb_set_option
        [ 121] libusb_init
        [ 122] libusb_init@4
        [ 155] libusb_set_option

Getting back from my own off-topic Windows concerns and towards a more concrete solution: I have the option to only fetch libusb_set_option when libusb is CDLL. I expect android to be cdecl, and this is where this function matters anyway.

it looks like small types are promoted to int and double in variadics. so this seems mostly an issue for machines where sizeof(int) != sizeof(void*).

Would they also be promoted on non-variadic cdecl calls ? Genuine question, as my google-fu has been unable to give me the answer so far. I may have to disassemble some test code built for a sizeof(int) != sizeof(void*) arch to find out.

xloem commented 2 years ago

Thanks for the work you're putting in looking into this.

Would they also be promoted on non-variadic cdecl calls ?

No, it sounds like this happens only when the types for function parameters are not specified. Here is a link: https://azrael.digipen.edu/~mmead/www/Courses/CS120/VariadicFunctions.html

Others here may be more experienced with C than I am.

vpelletier commented 2 years ago

So, my current understanding is that libusb_set_option may work as long as:

...so maybe it can reliably work ? But my confidence in myself is low.

xloem commented 2 years ago

all the issues look unlikely to crop up, but it does seem kind of like an advanced call where it's best to understand c variadic functions if using it with new parameters.

vpelletier commented 4 months ago

Following the recent libusb 1.0.27 release, I checked the API this module is not wrapping yet, and am happy about the new libusb_init_context function. I have pushed an early version of what should be the next release, which side-steps the need to call (and hence wrap) libusb_set_option.

I do not think I have an android setup to test usb on (I'm not actually sure what is needed... maybe I have ?). Testing and reviews very welcome.

mcuee commented 4 months ago

@vpelletier

I do not know how to test libusb for Anrdoid either. But @xloem or @CraigHutchinson should be able to help.

Ref:

xloem commented 4 months ago

I think somebody would need to port this library into android using an android python development framework such as kivy or beeware. This may have already been done in which case the maintainer would have the most experience.

EDIT: Mistake, the below information only relates to the above linked non-root PR which is still unmerged. Then the android side of the framework would be augmented to pass the android-specific java pointer in for non-root functionality. Alternatively (even better), there is a new android java API call implemented since this development started, that is usable in some way to get this pointer from C. It may be easy to use. Ideally such an improvement would be contributed straight to libusb.

vpelletier commented 4 months ago

I could have a first try, by following https://wiki.termux.com/wiki/Termux-usb , replacing the C code with:

#!/usr/bin/env python3
import sys
import usb1

usb_fd = int(sys.argv[1], 10)

with usb1.USBContext(
    with_device_discovery=False, # Note: I'll probably rename this parameter
    log_level=usb1.LOG_LEVEL_DEBUG,
) as ctx:
    handle = ctx.wrapSysDevice(usb_fd)
    dev = handle.getDevice()
    import pdb; pdb.set_trace()

and ran with:

$ termux-usb -l
# ...
$ termux-usb -r /dev/bus/usb/001/002
# ...
$ termux-usb -e ./test_android.py /dev/bus/usb/001/002

I could not get a device behing an OTG-to-A hub to appear, but I could plug one of the few OTG devices I have (a SeekThermal camera) and could open it and trigger a bit of IO (retrieving the manufacturer string descriptor, list of languages...).

So at least the basics are working, and I identified a few things which should be improved.

As I have no idea what the camera's protocol is, I will not be able to test much more of the API with it. On the side of Linux devices with gadget support (which would allow much more testing), both of my devices currently do not boot, and I've been procrastinating on figuring out what makes them sad.

wynwxst commented 1 month ago

From, what I gather, a variadic is a function that has a dynamic amount of arguments?

vpelletier commented 1 month ago

Correct, and while ctypes is able to reliably know what to do for a call with a fixed number of arguments, it cannot (or at least, it does not implement it) when it is variable. IIRC it is then always up to the caller to unwind the stack (while in some ABI this is up to the callee), and some argument types are promoted to larger registers (? not sure).

This makes this fall into the arch- and possibly also OS-dependent bucket, where many "works for me" bugs strive.

wynwxst commented 1 month ago

Correct, and while ctypes is able to reliably know what to do for a call with a fixed number of arguments, it cannot (or at least, it does not implement it) when it is variable. IIRC it is then always up to the caller to unwind the stack (while in some ABI this is up to the callee), and some argument types are promoted to larger registers (? not sure).

This makes this fall into the arch- and possibly also OS-dependent bucket, where many "works for me" bugs strive.

out of curiosity can you not dynamically allocate the argtypes? Since the first option will always be ctx if I remember correctly For example

# assume imported libusb-1.0.dll as libusb
import ctypes
libusb_set_option = libusb.libusb_set_option
libusb_set_option.argtypes = [libusb_context_p,c_int,c_int] #if I understand correctly, ctx will always be there and the only variable part is onwards with log level and so on
def variadic_func(*args):
 if len(args) == 2: # only 2 args so ctx and log level
  libusb_set_option.argtypes = [libusb_context_p,c_int]
 if len(args) == 3:
  libusb_set_option.argtypes = [libusb_context_p,c_int,c_int]
 # or if the rest are all type ints (for x no. of args)
 argtypes = [libusb_context_p]
 ctx = args[0]
 args = args[1:]
 for i in args:
  argtypes.append(c_int)
 libusb_set_option.argtypes = [ctx,*argtypes]
 args = [ctx,*args]
 libusb_set_option(*args)

I'm not sure if I understood correctly and I'll quickly run it to see if I haven't forgotten to add anything