Feature: add get_report_descriptor wrapper #186
Conversation
ldkv
force-pushed
the
feature/get_report_descriptor
branch
from
cc81563 to
f8f4524
Compare
| @@ -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): | |||
There was a problem hiding this comment.
max_length is not a parameter, but a constant in the scope of hid_get_report_descriptor usage
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I created a new PR to remove the parameter: #189
| 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]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@Youw yes, PR with an efficiency fix would be greatly appreciated
There was a problem hiding this comment.
Added wrapper for
get_report_descriptorfunction.Quite straightforward I think.
Requested by #165