luxonis / XLink

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

refactor getWinUsbMxId() to fix luxonis/XLink#57 #60

Closed diablodale closed 1 year ago

diablodale commented 1 year ago

complete refactor of getWinUsbMxId() to fix luxonis/XLink#57

Ready for review. Some things to keep in mind

diablodale commented 1 year ago

This needs testing on various Windows hardware/setups to ensure the new Win32 path -> libusb path code works and the assumptions it makes are valid. I'll test on three machines: Win11 usb3, Win10 usb3, Win10 usb2 👴

diablodale commented 1 year ago

Good results on my 3 windows test machines. One was additionally interesting...

I used my updated multiple_devices_test from my depthai-core PR + additional debug output so I can watch for behaviors. The

cpu windows usb ports/hubs notes
intel 10th gen i7 (2020) Win 11 USB 3, no hubs tests worked as expected
intel 3rd gen i7 (2012) Win 10 USB 3, USB 2, USB hubs tests worked as expected
intel core 2 duo (2007) Win 10 USB 2, no hubs tests work better than expected 🤩

That last computer, 16 years old, still works. Yes the frames come in slowly from 3 devices while some share the same controller's usb2 bandwidth. Yet, it works with 3 OAKs attached. And works...better. Better meaning that when the Device() for the 3 OAKs is destructed, and it waits for reboot of the device, it FINDS the rebooted devices almost always. I would say it finds all three devices more than 80% of the time. In comparison, when the same test is run on the 1st and 2nd computers, it usually finds the 1st rebooted device but usually does NOT FIND the 2rd and 3rd devices...a rough estimate is a 40% success rate of reboots.

I continue to think the following:

themarpe commented 1 year ago

Thanks a bunch for this exploration! I'll create corresponding PRs on DepthAI to go through some of our Windows testing infra as well

themarpe commented 1 year ago

FATAL logs would likely have to be removed for that, if you have another commit ready with that, will then reference that

diablodale commented 1 year ago

Can you/your team do some dev unit testing on Windows machines with this PR? I'd like to have one or two other people review/test this code before removing the debugging code. If your dev's don't have the time, then -yes- I will remove the debug and also rebase the commits together

themarpe commented 1 year ago

We will - if you have a commit with that removed I can reference your repo /w changes (can be separate from this PR/branch), in DepthAI and get the testing in

diablodale commented 1 year ago

We will - if you have a commit with that removed I can reference your repo /w changes (can be separate from this PR/branch), in DepthAI and get the testing in

Done. This PR's head has CI ready code. For dev unit testing, I recommend using one commit backwards 4ac4914f188d72cb12aaab331c0c15b67706b20a

themarpe commented 1 year ago

Changes went through our Windows testing as well - I'll go ahead and merge to develop for further testing in core as well Thanks!