Repository navigation
Conversation
Assisted-by: Codex:GPT-6
Assisted-by: Codex:GPT-6
There was a problem hiding this comment.
There are two separate changes here:
- changing queue size for libusb backend
- adding read stats API
I've left my comments for new API, and as for read queue size - replacing one constant with another is not something I'm willing to accept. A propper fix would be to introduce an API to change the queue size in runtime and implement for all platforms properly. I remember starting working in that direction, but can't recall how it ended.
As for the issue you're facing - you're free to use an updated fork, and thanks for the contribution.
But this will not end up in main branch as is.
Closing for now.
| #define libusb_init fake_init | ||
| #define libusb_exit fake_exit | ||
| #define libusb_submit_transfer fake_submit | ||
| #include "../../libusb/hid.c" |
There was a problem hiding this comment.
nope, even for a test code I'd not accept hacks like this; this may be fine as a one-time check using agentic testing/implementation, but not something I'd want to see long-term in main branch
this can be tested in automated way w/o real hardware, by properly registering a virtual usb device; I've started having an infrastructure for that somewhere, maybe we will get back to this later
| #define HID_API_HAS_QUEUE_STATS 1 | ||
| int HID_API_EXPORT HID_API_CALL hid_get_queue_stats( | ||
| hid_device *dev, unsigned long long *received, | ||
| unsigned long long *dropped, unsigned int *max_queue); |
There was a problem hiding this comment.
having a new API function with implementation only for one backend is not something I can be happy with
I likely understand your motivation here (hard to test the implementation properly w/o having a HW/setup/etc.), but having a new API function is a contract someone would have to maintain which I'd like to avoid
| if (num_queued > 30) { | ||
| dev->reports_received++; | ||
| unsigned int queue_depth = (unsigned int)num_queued + 2; | ||
| if (queue_depth > dev->queue_max) | ||
| dev->queue_max = queue_depth; | ||
|
|
||
| /* Preserve the validated 1,024-report queue limit; prevent | ||
| unbounded memory growth if the application never reads. */ | ||
| if (num_queued > 1022) { |
There was a problem hiding this comment.
I hate the magic number 30 here, but magic number 1022 is not better at all
Not me, an community contribution: #787 |
libusb: retain 1024 input reports and expose native queue loss counters
Bursty input can overflow the small libusb receive queue and silently discard old
reports. Port the locally validated 1024-report limit, received/dropped/peak
counters, mutex-protected
hid_get_queue_stats()snapshot and close-time logging.The existing queue and oldest-report discard policy are preserved. Peak depth is
measured before eviction and can reach 1025. Other backends return unsupported;
existing APIs retain their behavior.
The original hardware validation transferred 22658 reports with zero drops and
matching Python/C counters. New Linux checks compile both native backends and
exercise the actual callback using mocked USB operations: overflow boundary,
FIFO retention, invalid pointers and a 22658-report zero-drop synthetic run.
Address/undefined sanitizers passed with leak detection disabled due to sandbox
tracing restrictions. No hardware was accessed. Native builds used cc because
CMake/Autotools are unavailable. Other platforms need CI.
Companion Python contribution: stman/cython-hidapi branch feat/libusb-queue-stats.
Assisted-by: Codex:GPT-6