darrylb123 / usbrelay

Control usb relay - based on hidapi
GNU General Public License v2.0
315 stars 98 forks source link

Rewrite usbrelay_py with ctypes #97

Open PaulChanHK opened 1 year ago

PaulChanHK commented 1 year ago

Signed-off-by: Paul Chan 19663576+PaulChanHK@users.noreply.github.com

darrylb123 commented 1 year ago

Paul, I'm not the original author of the python bindings and know almost nothing about python myself. This PR changes from a C library to a pure python binding. I can see the appeal, but we already have a functioning binding. I had never heard of CFFI until the PR so please educate me. Can you explain the reason for doing this at all? Can you define any changes to dependencies or user interface? If there is any changes to the documentation, can you include that as well?

darrylb123 commented 1 year ago

Please also note, there are some very recent changes to the libusbrelay.c and libusbrelay_py.c which bumped the library versions.

darrylb123 commented 1 year ago

Tested the PR locally. Worked fine with one module. Segfaults with 2.

import usbrelay_py count = usbrelay_py.board_count() print("Count: ",count) Count: 2 boards = usbrelay_py.board_details() Segmentation fault (core dumped)

It could be because of changes to libusbrelay made recently.

PaulChanHK commented 1 year ago

Hi Darryl,

This PR changes from a C library to a pure python binding. I can see the appeal, but we already have a functioning binding. I had never heard of CFFI until the PR so please educate me.

Paul: Sorry I mixed up the terms cffi with ctypes. The code in this PR is actually written with Python ctypes. Below page provide a quick introduction to ctypes and cffi. https://eli.thegreenplace.net/2013/03/09/python-ffi-with-ctypes-and-cffi

Can you explain the reason for doing this at all?

Paul:

I use usbrelay in a test environment running Ubuntu but the env. is actually not tied to a particular Ubuntu version or Python (minor) version. Without ctypes binding, we need to build one python package for one Python version - e.g. Python 3.5, Python 3.8 and Python 3.9 that is inconvenient. It would be better to have python-usbrelay from apt and usbrelay_py from PyPI directly installable.

Use of ctypes simplifies the code and make usbrelay immediately available to any Python 2 or Python 3 versions provided that the prerequisite libffi is installed in the system (and ctypes supported in CPython and Pypy since Python 2.5).

Can you define any changes to dependencies or user interface?

Paul: See the next one about dependency. For user APIs, there is no change in terms of importing usbrelay_py module and the API definitions. Some ctypes-mapped C-APIs are unused as they are mainly added for completeness of the C-APIs full set and for future expansion.

If there is any changes to the documentation, can you include that as well?

Paul: Do you mean the installation dependency like prerequisite libffi ? It should be included by Python installation package. So, I think we don't need to mention this.

BTW, I can start a new PR to avoid cffi in the branch name for clarity, if necessary.

PaulChanHK commented 1 year ago

Tested the PR locally. Worked fine with one module. Segfaults with 2.

import usbrelay_py count = usbrelay_py.board_count() print("Count: ",count) Count: 2 boards = usbrelay_py.board_details() Segmentation fault (core dumped)

It could be because of changes to libusbrelay made recently.

I only has one ucreatefun relay to test with. Can we build a debug usbrelay.so for test purposes ? like by returning some fixed HID return values for API/Python tests.

darrylb123 commented 1 year ago

I'll see if I can get one done today.

darrylb123 commented 1 year ago

Did a little debugging It's segfaulting on second iteration so I checked that the address pointer was incrementing correctly. Added to libusbrelay.c printf("Sizeof relay_boards: %ld\n",sizeof(relay_board)); Added to libusbrelay_py.py print(sizeof(relay_board_t)) Result:

import usbrelay_py print(usbrelay_py.board_details()) Sizeof relay_boards: 24 <<<<<<< libusbrelay.c 20 <<<<<<<<<<<<<<<<<<<libusbrelay_py.py Segmentation fault (core dumped)

darrylb123 commented 1 year ago

Found that the struct pack = 1 needs to be commented out so that it uses native alignment that returns a struct size of 24, same as the C library. Still doesn't fix the segfault. any reference to the array of structs after the first one fails. for i in range(0, counts): board = POINTER(relay_board_t).from_address(address).contents print(board.state) # boards.append((board.relay_count)) # boards.append((board.serial.decode("utf-8"), board.relay_count, board.state, # board.path.decode("utf-8"), board.module_type,)) address = address + sizeof(relay_board_t)

For the moment, wouldn't it be better to create a test library that just returns an array of values and prove you can iterate through the array?