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

Rewrite usbrelay_py with ctypes #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PaulChanHK
Copy link

Signed-off-by: Paul Chan [email protected]

@darrylb123
Copy link
Owner

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

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

@darrylb123
Copy link
Owner

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

PaulChanHK commented Jan 9, 2023

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

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.

@PaulChanHK PaulChanHK changed the title Rewrite usbrelay_py with cffi Rewrite usbrelay_py with ctypes Jan 9, 2023
@darrylb123
Copy link
Owner

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

@darrylb123
Copy link
Owner

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

darrylb123 commented Jan 10, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants