search

luxonis / XLink

A cross-platform library for communicating with devices over various physical links.
Apache License 2.0
11 stars 17 forks source link

suspicious/errant code for `bulk_chunklen` and `bcdusb` #61

Open diablodale opened 1 year ago

diablodale commented 1 year ago

I see suspicious code regarding the global bulk_chunklen, the local bulk_chunklen, and bcdusb used as an param and local. Can you explain more about the intention -or- review these locations to see if the code is correct?

bulk_chunklen

there is a global bulk_chunklen that has no atomic/mutex protecting it. https://github.com/luxonis/XLink/blob/457b23fb33e1146610e1a4e52defa7565046977a/src/pc/protocols/usb_host.cpp#L42

It is assigned only one place in the codebase. Within usb_open_device(). It is never read. It has no reason to exist. This global bulk_chunklen is set to a value during the open of a specific device yet libusb can open devices in parallel. Technically, if something read this var (which nothing does) then the var changes as different devices with different chunks are opened in parallel. https://github.com/luxonis/XLink/blob/457b23fb33e1146610e1a4e52defa7565046977a/src/pc/protocols/usb_host.cpp#L555

Then there is a local bulk_chunklen which hides the global with the same name. What is the relationship between the local and global vars with the same name? https://github.com/luxonis/XLink/blob/457b23fb33e1146610e1a4e52defa7565046977a/src/pc/protocols/usb_host.cpp#L573-L588

bcdusb

bcdusb starts as a local var in usb_boot(). It is an UINT16_T set to the value -1. This is suspicious signed/unsigned. It is never set or changed again. This is suspicious as it should therefore be a const/macro. https://github.com/luxonis/XLink/blob/457b23fb33e1146610e1a4e52defa7565046977a/src/pc/protocols/usb_host.cpp#L626

This local bcdusb is passed as a param to send_file() https://github.com/luxonis/XLink/blob/457b23fb33e1146610e1a4e52defa7565046977a/src/pc/protocols/usb_host.cpp#L651

in send_file() this bcdusb param is compared against hex 0x200 aka 512 decimal. This is suspicious since it will always be false because -1 stored in that uint16_t is (uint16_t)65535U ==> 65535 < 512 = false https://github.com/luxonis/XLink/blob/457b23fb33e1146610e1a4e52defa7565046977a/src/pc/protocols/usb_host.cpp#L578

diablodale commented 1 year ago

added commit that should resolve this issue.

⚠️🔬 The commit now uses the libusb bcdUSB value during the boot process of usb_boot(). If this is not wanted, it is an easy remove. FYI: my OAK-D and OAK-D-Lite sensors always report USB 2.00 while unbooted.

themarpe commented 1 year ago

added commit that should resolve this issue.

The commit now uses the libusb bcdUSB value during the boot process of usb_boot(). If this is not wanted, it is an easy remove. FYI: my OAK-D and OAK-D-Lite sensors always report USB 2.00 while unbooted.

Awesome, Thanks! Yes, MX's BL is USB2.0 only, I assume this was the reason this wasn't an issue before.

Otherwise, on the why's, its both probably an refactor artifact that stayed behind