yannbouteiller / vgamepad

Virtual XBox360 and DualShock4 gamepads in python
MIT License
162 stars 21 forks source link

`argtypes passes a union by value` - Potential Error/Note #5

Closed ES-Alexander closed 2 years ago

ES-Alexander commented 2 years ago

u/Jared246 ran into this yesterday on Reddit, which I expect is likely to be an issue in specific versions of Python (3.7.6 / 3.8.1). The issue was circumvented by commenting out DualShock-related lines in vigem_client.py (which wasn't important to them), but if it's the same as the linked issue I'd recommend at least a note in the README that those versions of Python shouldn't be used with this library.

If extra details are necessary they'll need to be tested for, or asked for from u/Jared246. I don't have experience with this library and unfortunately don't have time to set up a Windows VM and test it.

JaredSurette commented 2 years ago

(sorry if this is formatted wrong I've never used github like this only for small personal projects in the past)

hey, I'm the user that found the error. I am running python 3.8.1 (old IK I don't usually code that much and this is my first project back into it). I believe the error popped up when I had both "import vgamepad as vg" and "gp = vg.VX360Gamepad()" lines in my code.

error is as follows:

""Traceback (most recent call last): File "C:/Users/jared/Documents/Scripts/Python/ARDFlightSim/Sticktest.py", line 6, in import vgamepad as vg File "C:\Users\jared\Documents\Scripts\Python\ARDFlightSim\venv\lib\site-packages\vgamepad__init__.py", line 1, in from vgamepad.win.virtual_gamepad import VX360Gamepad, VDS4Gamepad File "C:\Users\jared\Documents\Scripts\Python\ARDFlightSim\venv\lib\site-packages\vgamepad\win\virtual_gamepad.py", line 6, in import vgamepad.win.vigem_client as vcli File "C:\Users\jared\Documents\Scripts\Python\ARDFlightSim\venv\lib\site-packages\vgamepad\win\vigem_client.py", line 174, in vigem_target_ds4_update_ex.argtypes = (c_void_p, c_void_p, DS4_REPORT_EX) TypeError: item 3 in argtypes passes a union by value, which is unsupported.""

I was able to remedy this error by commenting out line 174 in the file "vigem_client.py". Line says:

vigem_target_ds4_update_ex.argtypes = (c_void_p, c_void_p, DS4_REPORT_EX)

I'm not using this modules DualShock functionality so I figured "what's the worst that can happen if I just comment it out." so I did. It seems to be running fine now.

Thank you very much for your time in both making this module and supporting it. I'm not sure if any thing will get done about my problem specifically (bc its probably fixed if I just update my python) but its here for any future users who may encounter this issue on the same version

ES-Alexander commented 2 years ago

@JaredSurette thanks for providing the traceback and extra info.

(sorry if this is formatted wrong I've never used github like this only for small personal projects in the past)

Looks fine to me - congrats on making your first open-source contribution then :-) Only suggestion on the formatting would be to put the error traceback in a code block (so that things like \__init__.py don't appear as __init__.py), which you can do by putting three back-ticks (```) on their own line before and after it :-)


Given the recommendation here is basically to just add a note to the README, would you be interested in making that contribution? I'd recommend just adding a "Troubleshooting" section at the bottom with a note about the error and the python versions it affects. If you're not keen to then I or @yannbouteiller can do it instead, but could be a simple and low-risk opportunity to make a direct contribution to a library you're using :-)

If you click into the README.md file there should be a pencil icon in the top right corner, which says "Fork this project and edit the file". If you do that then you can edit it as appropriate, add your name to the contributors list, write a short+descriptive commit message, and commit the file. Then GitHub will prompt you to make a Pull Request with your changes to the main repo, which will allow @yannbouteiller to merge in your changes if they think they're reasonable/helpful.

yannbouteiller commented 2 years ago

Hi, thank you both for your involvement here!

I am examining the issue to understand whether this needs an actual fix. In fact, this line you have been commenting out is not even useful for any regular user of the library: it is for advanced (yet untested nor documented) use of the DS4 controller specifically, I could simply remove it, but on the other hand a few people seem interested in advanced DS4 use. @JaredSurette could you try updating your python version and check whether this resolves the issue when you uncomment line 174 please?

As a personal note, the issue comes from the extended DS4 report, which is defined as a Union here

yannbouteiller commented 2 years ago

Mmh, this probably does need a fix: https://bugs.python.org/issue16575

Looks like updating python would just hide a bug here.

ES-Alexander commented 2 years ago

From what I understand the underlying issue is with libffi, not Python, so it can't really be fixed in Python. It also may not actually cause issues on Windows, whereas the 'fix' of disallowing the current behaviour breaks multiple libraries that currently rely on it (which is why it was rolled back in the subsequent Python patches).

There's (much) more detail/history here: https://stackoverflow.com/a/62083120

yannbouteiller commented 2 years ago

Yes, the problem is that the vgamepad API that I intended to develop for passing extended reports to the DS4 controller relies on a vigem client C function that takes a union by value, and currently the documentation of ctypes does state that this is not supported. I could modify vigem client to add a function that takes this union by pointer (I have done this locally already in fact), but I don't like to branch the vigem client dlls away from their master branch. That being said, the change can be temporary (until libffi is fixed, if that ever happens) and will be invisible to the user.

yannbouteiller commented 2 years ago

Done, version 0.0.5 should work without having to comment stuff out :)