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

libusb_set_option support #75

Open
mcuee opened this issue Aug 6, 2021 · 23 comments
Open

libusb_set_option support #75

mcuee opened this issue Aug 6, 2021 · 23 comments
Labels
C API Issues which need support from C libusb API

Comments

@mcuee
Copy link

mcuee commented Aug 6, 2021

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
Copy link
Author

mcuee commented Aug 7, 2021

Ref: FYI. I have not been able to get usbdk to work under pyusb yet.
pyusb/pyusb#200

@mcuee
Copy link
Author

mcuee commented Aug 8, 2021

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
Copy link
Author

mcuee commented Aug 20, 2021

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.
pyusb/pyusb#200

@vpelletier
Copy link
Owner

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

  • not having to write a lot of C code. Such simple wrapper is definitely not a lot of code, so I am not bothered about this.
  • not requiring a compilation step when installing the module (to avoid requiring a working build environment on users' machines). This is somewhat solved by python wheels, but not entirely: there will always be architectures & calling conventions I will not be building a wheel for, so users on these archs will get build issues no one else sees.

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
Copy link
Author

mcuee commented Sep 1, 2021

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 New NO_DEVICE_DISCOVERY option to replace WEAK_AUTHORITY option libusb/libusb#935

@vpelletier
Copy link
Owner

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:

  • libusb_set_int_option, libusb_set_string_option, etc, each taking the context, the option enum and one argument. I believe this currently means only one such function (and hopefully not enough will be shoe-horned in this to require a lot of variants).
  • libusb_set_option(libusb_context *, enum libusb_option, struct option_value *) with struct option_value being some multi-"personality" structure (maybe an union of all the possible option argument structures ?)

@tormodvolden
Copy link

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
Copy link
Author

mcuee commented Sep 7, 2021

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

@vpelletier vpelletier added the C API Issues which need support from C libusb API label Sep 9, 2021
@mcuee
Copy link
Author

mcuee commented Sep 9, 2021

libusb/libusb#987

One option is to improve libusb using Env variables.

@xloem
Copy link

xloem commented Nov 2, 2021

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
Copy link
Owner

vpelletier commented Nov 3, 2021

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:

  • on Windows x86, variadic functions are apparently always cdecl(even when explicitly declared stdcall), and on other x86 OSes it seems to be cdecl
  • cdecl has the caller handle the stack cleanup, and ctypes must be able to do this anyway
  • 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). Callbacks would be an issue, but the above would only mean they could be defined as cdecl functions and everything should work.

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
Copy link

xloem commented Nov 3, 2021

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
Copy link
Owner

  • 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
Copy link

xloem commented Nov 4, 2021

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
Copy link
Owner

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

  • easy/seems unlikely to be an issue:
    • I explicitly load it as cdecl on Windows (which will be needed for LIBUSB_OPTION_USE_USBDK)
    • there are no floats (of any size)
    • there are no composite types (structs...) passed by value
    • integers passed do not lose any bit when cast into a same-size-as-void * integer type (or slightly larger if there is no perfect match)
  • probably easy:
    • there are few enough arguments that they never have to be pushed on the stack
    • there are no types larger than a register (hopefully no arch has pointers larger than a register)
    • there are no signed types (or at least no negative values passed in them), or I will likely get a bug about sign-extension (or about lack of sign-extension)

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

@xloem
Copy link

xloem commented Nov 5, 2021

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
Copy link
Owner

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
Copy link
Author

mcuee commented Mar 6, 2024

@vpelletier

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

Ref:

@xloem
Copy link

xloem commented Mar 6, 2024

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
Copy link
Owner

vpelletier commented Mar 6, 2024

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
Copy link

wynwxst commented May 11, 2024

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

@vpelletier
Copy link
Owner

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
Copy link

wynwxst commented May 12, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C API Issues which need support from C libusb API
Projects
None yet
Development

No branches or pull requests

5 participants