c++ wrapper for libusb, add try/catch to prevent app crashes

[diablodale]

Adds c++ wrappers for the minimum set of features needed without introducing major Xlink code refactoring. fixes #59 I can't move much further until the questions below are answered.

• new dai::libusb namespace
• Uses the constructor approach for wrappers (not factory)
• Added usb_error for exceptions
• Added usb_context, usb_device, device_list, device_handle, config_descriptor
• Added methods like device_handle::claim_interface(), device_handle::set_configuration(), etc. to manage interfaces and fix resource leak bugs I found
• Workaround for thread-unsafe libusb_get_device_list()

Questions

1. can we shift XLink to be a C++14 project? the cmake's in XLink are still set to c++11. If no, then I'll need to change some code.
2. ~what code should make mvLog entries. Should that be in the wrapper? Or should the Caller do it with catch() { mvLog("had an error: " + myexception.what()); }. Pros and Cons for both. This is more a style and feature choice.~ The wrapper classes do the logging as it matched the majority of use cases.
3. ~Can I remove extern C from refLibusbDeviceByName()? I don't see a need for it. Does the device firmware code need it?~ I removed it.
4. Code comments in refLibusbDeviceByName() say it filters by Myraid devices. It actually doesn't. This can be added if needed.
5. ~I would like to pass between a few functions wrapper classes instead of raw pointers. Does your team have any concerns?~ Already changed some func signatures to use the new classes; usually as a reference.

[diablodale]

Apple's compiler appears to not match the behavior of all the others and is failing on use of std::pair in constexpr.

AppleClang 14.0.0.14000029
-- The CXX compiler identification is AppleClang 14.0.0.14000029
.....
/Users/runner/work/XLink/XLink/src/pc/protocols/usb_host.cpp:104:71: error: constexpr variable 'VID_PID_TO_DEVICE_STATE' must be initialized by a constant expression
static constexpr std::array<std::pair<VidPid, XLinkDeviceState_t>, 4> VID_PID_TO_DEVICE_STATE = {{
                                                                      ^                         ~~
/Users/runner/work/XLink/XLink/src/pc/protocols/usb_host.cpp:105:5: note: non-constexpr constructor 'pair<true, false>' cannot be used in a constant expression
    {{0x03E7, 0x2485}, X_LINK_UNBOOTED},
    ^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/__utility/pair.h:171:5: note: declared here
    pair(_T1 const& __t1, _T2 const& __t2)
    ^

I successfully compile with Clang 14.0.0 on my linux machine. As the internet writes, Apple's compiler is non-standard and quirky 🍏🪱. If anyone on the Luxonis team has access to a Mac, it would be great to have assist on that error. I suspect it is that Apple disabled or broke something with std::pair and constexpr. As a fallback, I can change the vidpid lookup to instead use a simple 3 column array.

[themarpe]

This is a large codebase change for which we lack both the extensive testing to be sure to pull it in as well as it requires redoing the mental model of current state. Seems like "libusb++" impl, which might be better suited as a standalone library.

For "just" the sake of #59 I'd rather see a more "non intrusive" function level try/catch.

Any improvements/fixes part of this PR that address some other specific problems, would be neat to be addressed by a separate PR.

---

Sorry for the delay on the feedback, but was headdown this week

[diablodale]

Hi, no worries. We spoke about this before -- that when our goals don't align we can do different things. And that's ok 🙂👍 We also spoke before about your group's testing methodology/group so I have some insight there too. Happy to discuss anytime here or offline. 🕊️✌️

As I first approach the crashing bugs with a try/catch around every function, it lead to several discoveries that changed my approach. Please do not take these as insults. Instead, I'm sharing with you my thought process and just describing what I saw in the USB code. The PoE code of Xlink I have not investigated.

• Difficult to read, understand, and --in general-- a difficult to maintain code base. Few code comments. Often no code comments. Critical variables with cryptic 2 or 3 letter names. For me to see where and how to catch errors/exceptions, I must understand the codebase and how it operates. Further...as an open source project, part of that ethos is to leave code better than it was before and to have it in a state where the "next" person can further contribute.
• The error handling in several locations was itself errant. Missing error handling and resource (usually releasing) mismanagement, errant error enums/ints passed along the call chain and misread as other enums, etc.
• Need for nested try/catches for specific scenarios where operations need to be retried and not fail once and return.
• Redundant code. One I found appeared in three places yet doing the same thing with different, sometimes slightly wrong, code.

These led me to the need for a simple c++ wrapper around the subset of libusb used by xlink. While it could be a separate project, I have no desire or need to do that work. It would require much more work to handle all the libusb functions, structs, async io, etc.

When I had the first few wrappers of resources like device_list or usb_device, it was simply inserting it where the C-call already was and then try/catching the wrapper's errors. But doing that very quickly simplified code where a chain of if/thiserror/thaterror/defaulterror is no longer needed and the "close/release" of the resource is automatic. Hundreds of lines of usb_host.cpp code was then unneeded. Then with resources (this is primarily about resources) that release themselves with RAII, I could then start to do the try/catch work that removed yet more (sometime incorrect) error handling of those resources.

Which got the PR to this current state. I understand you telling me your testing resources are elsewhere. And that's ok. 🙂👍

For now, I'm going to continue with my fork of xlink as I already feel more confident with its handling of USB (in particular error conditions). The new code is not ready yet, but already it resolves the issues I write above. Reliability with no fails/crashing for up to a week is important for my use cases so my priorities are oriented in that direction.

If there isn't any further life for this PR, I can close it and continue work in my fork.

[diablodale]

I have one ask. The mac-only compiler fail with constexpr ?might? have a workaround. I'd like to force-push a commit over itself several times trying different alternatives into this PR to test each approach. Any concerns with that?

[themarpe]

For now, I'm going to continue with my fork of xlink as I already feel more confident with its handling of USB (in particular error conditions). The new code is not ready yet, but already it resolves the issues I write above. Reliability with no fails/crashing for up to a week is important for my use cases so my priorities are oriented in that direction.

If there isn't any further life for this PR, I can close it and continue work in my fork.

I agree with your rundown - I would love to see this go further, but it also has to address a (big enough) pain point to warrant indepth review and manual testing (as XLink doesn't have a USB centric testing - TCPIP might be simpler to cover in this regards - develop_server). That said, I'd gladly keep up with developments here and further see what other issues it resolves, and re-review if/when the issue presses/becomes a pain point in our mainline.

I have one ask. The mac-only compiler fail with constexpr ?might? have a workaround. I'd like to force-push a commit over itself several times trying different alternatives into this PR to test each approach. Any concerns with that?

Not an issue at all

[themarpe]

Otherwise on the questions:

can we shift XLink to be a C++14 project? the cmake's in XLink are still set to c++11. If no, then I'll need to change some code.

IMO yes - underlying DepthAI is already at C++14, and it being the main consumer, it aligns nicely

Code comments in refLibusbDeviceByName() say it filters by Myraid devices. It actually doesn't. This can be added if needed.

I don't think this has to be changed unless code relies on it - so I'd just modify the comment instead. (was likely a culprit of a refactor to begin with)

[diablodale]

I've pushed the last commit to this branch. It includes markdown and mermaid documentation for usage and object model for the libusb wrapper. Its really quite simple. 🔢 This wrapper replaces much of the old Xlink C-style code for its usb transport.

Moving forward, fixes from this PR and more will usually be in my fork at https://github.com/diablodale/XLink/ Closing.