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

[diablodale]

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.

XLink/src/pc/protocols/usb_host.cpp

Line 42 in 457b23f

static unsigned int bulk_chunklen = DEFAULT_CHUNKSZ;

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.

XLink/src/pc/protocols/usb_host.cpp

Line 555 in 457b23f

bulk_chunklen = ifdesc->endpoint[i].wMaxPacketSize;

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?

XLink/src/pc/protocols/usb_host.cpp

Lines 573 to 588 in 457b23f

	int bulk_chunklen = DEFAULT_CHUNKSZ;
	twb = 0;
	p = const_cast<uint8_t*>((const uint8_t*)tx_buf);
	int send_zlp = ((filesize % 512) == 0);
	if(bcdusb < 0x200) {
	bulk_chunklen = USB1_CHUNKSZ;
	}
	auto t1 = steady_clock::now();
	mvLog(MVLOG_DEBUG, "Performing bulk write of %u bytes...", filesize);
	while(((unsigned)twb < filesize) || send_zlp)
	{
	wb = filesize - twb;
	if(wb > bulk_chunklen)
	wb = bulk_chunklen;

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.

XLink/src/pc/protocols/usb_host.cpp

Line 626 in 457b23f

uint16_t bcdusb=-1;

This local bcdusb is passed as a param to send_file()

XLink/src/pc/protocols/usb_host.cpp

Line 651 in 457b23f

rc = send_file(h, endpoint, mvcmd, size, bcdusb);

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

XLink/src/pc/protocols/usb_host.cpp

Line 578 in 457b23f

if(bcdusb < 0x200) {

[diablodale]

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]

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