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

Feature: add get_report_descriptor wrapper #186

Merged
merged 2 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions chid.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ cdef extern from "<hidapi.h>":
int hid_read(hid_device* device, unsigned char* data, int max_length) nogil
int hid_read_timeout(hid_device* device, unsigned char* data, int max_length, int milliseconds) nogil
int hid_set_nonblocking(hid_device* device, int value)
int hid_get_report_descriptor(hid_device* device, unsigned char *data, int length) nogil
int hid_send_feature_report(hid_device* device, unsigned char *data, int length) nogil
int hid_get_feature_report(hid_device* device, unsigned char *data, int length) nogil
int hid_get_input_report(hid_device* device, unsigned char *data, int length) nogil
Expand Down
32 changes: 32 additions & 0 deletions hid.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,38 @@ cdef class device:
result = hid_send_feature_report(c_hid, cbuff, c_buff_len)
return result

def get_report_descriptor(self, int max_length=4096):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_length is not a parameter, but a constant in the scope of hid_get_report_descriptor usage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and it is possible to limit the return length of the report descriptor.

In most case there is no interest in doing so, but I added it as an option anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, you'll blindly truncate the buffer/descriptor that way, technically it will work, but there is no sane reason to do so. By having it as an API parameter, one might think that it could make sense to limit the buffer, but by the HID Standard - it doesn't make any sense.

Copy link
Member

@prusnak prusnak Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Youw I kind of agree there is no sane reason to do so, but why does the C API provide a way how to define max_length? And same for get_input_report and get_feature_report?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, in C API it is not **max**_length. That is the actual length of the buffer, that is provided externally. In C and other "low-level" languages you have so many options if how to pass data around, and in this particular case (and in all other functions, like input/output/feature reports, etc.) it is just a matter of choosen design, that the buffer is allocated by the caller, and the API function simply needs to know the usable size of the buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Youw so what do you suggest? Rename max_length to length or just remove it entirely? I am open to any suggestion.

If max_length is removed, a buffer of length 4096 is always generated since we don't know the exact length of the report descriptor beforehand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we don't know the exact length of the report descriptor beforehand

That's the point - you never know it beforehand. You may only know it for specific known device, in which case you probably don't need the descriptor at al, as you probably know already what reports are there.

Yes, the suggestion is to remove it completely.

I want to try the cytho native array optimisation, so I might as well remove this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a new PR to remove the parameter: #189

"""Return the report descriptor up to max_length bytes.
If max_length is bigger than the actual descriptor, the full descriptor will be returned.

:param max_length: Maximum number of bytes to read, must be positive
:type max_length: int

:return:
:rtype: List[int]
:raises ValueError: If connection is not opened.
:raises IOError: If read error
"""
if self._c_hid == NULL:
raise ValueError('not open')

cdef unsigned char* cbuff
cdef size_t c_descriptor_length = max(1, max_length)
cdef hid_device * c_hid = self._c_hid
cdef int n
result = []
try:
cbuff = <unsigned char *>malloc(max_length)
with nogil:
n = hid_get_report_descriptor(c_hid, cbuff, c_descriptor_length)
if n < 0:
raise IOError('read error')
for i in range(n):
result.append(cbuff[i])
Comment on lines +354 to +360
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert in python/cython, but this looks like the most inefficient (biggest overhead) possible way to perform such operation.
If this article is right, it should be much better alternative.

Or at least create a stack temporary variable instead of making a heap one.

Copy link
Contributor Author

@ldkv ldkv Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert in Cython either, I just adapted the existing code to add this new method (see the method get_feature_report).

If you think there is a better way to do it, feel free to create a PR for it. My work here is done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Youw yes, PR with an efficiency fix would be greatly appreciated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I came up with #190.
It appears cython.array is not needed here, as I initially though, assuming list(cbuf[:n]) does what I expect it shuld:

  1. zero-overhead memory view over cython C array
  2. python-implementaiton-optimized implementation of python list creation from memory view

finally:
free(cbuff)
return result

def get_feature_report(self, int report_num, int max_length):
"""Receive feature report.

Expand Down
Loading