luxonis / xlink Goto Github PK

A cross-platform library for communicating with devices over various physical links.

License: Apache License 2.0

CMake 8.47% C 71.01% C++ 20.51%

xlink openvino

xlink's Introduction

XLink

This is a "fork" of a library that used to be openvino repositories third party dependency - XLink. The original library was removed from openvino repositories in openvinotoolkit/openvino#15131 .

Changes

Some tweaks to CMake files to enable installation of this target and being able to be used with Hunter package manager

xlink's People

Contributors

Stargazers

Watchers

Forkers

hellovai laorient diablodale wstnturner ibaigorordo metavai spectacularai fraxul moverseai meta-flutter lepalom ramonbroox figurerobotics traversaro asa parcos-remy onthegrid007 xgreat

xlink's Issues

Better memory management

To provide better memory management the following changes are proposed:

Add the capability to set a callback for allocating memory (eg. XLinkSetAllocator(alloc, dealloc), ...)
Modify XLinkPlatformAllocateData & XLinkPlatformDeallocateData to call the callback instead
Rename XLinkReleaseData and XLinkReleaseSpecificData to XLinkFreeData and XLinkFreeSpecificData respectively
Add a ReleaseData equivalent, which performs everything the same with exception of actually freeing the data

xlink contribution branch unclear (develop is very behind master)

I have a PR ready for xlink, but the branch on which to submit it is unclear.
develop is usually the branch to use for Luxonis projects.
But develop is many commits behind master. So many that it is unreliable to use for a PR.

What should I do @themarpe ?

if libusb is used -- LGPL licensing causes static/dynamic compile flags, doc updates, etc.

Introducing libusb obj/binary code into the depthai project, distributing it, etc. introduces libusb's LGPL licensing.
https://www.gnu.org/licenses/gpl-faq.html#LGPLStaticVsDynamic

I like tools that "help keep honest people honest". And to help prevent accidents.
I request tools/processes in depthai to manage the inclusion/flags of dependencies that have restrictive licensing (libusb is just the first one...)

@themarpe writes

Yeah, LGPL isn't the most open licence but will work okay as long as we note a couple of things.
There are 2 ways to go:

make libusb a dll (as is already on macos and Linux platforms). Windows might make this a tad harder because of the lack of rpath

compile depthai-core as dll with libusb static

The third option of having static libusb and static core, comes with a caveat that consuming application must then either be: provided in an object form to be able to relink another libusb or be opensource. This case should be documented so people aren't introducing licencing incompatible code into their project.
Note, we'll default to build libusb as a dll, so the change shouldn't affect anyone (with exception of Windows users regarding installing the libusb dll along their app)
https://softwareengineering.stackexchange.com/questions/312758/does-providing-object-files-satisfy-lgpl-relink-clause#312759
Core doesn't have to become LGPL licenced as there is the exemption of being able to relink with another version of libusb, which an opensource library consuming it fits into (in dll case).
Disclaimer, I'm am not a lawyer - this is my understanding from reading through various sources.

I am in aligment with that (as a non-lawyer).

Suggested work

adjust depthai project(s) cmake to enable those three major options. BUILD_SHARED is already avail, but there will need to be more granularity to specify which components are shared/dynamic and which are static so to comply with licensing
add licensing section to https://docs.luxonis.com/en/latest/ (No content exists there today)
depthai docs list those major three options and the cmake/compile flags to use
add DLL loading path management to Windows codepath to enable rpath similar behavior. I can help with this. I have mature code 8+ years that does this.

FYI, OpenCV has a cmake OPENCV_ENABLE_NONFREE. It adjusts the cmake flags/files to help devs be more honest and prevent accidential license issues.

libusb logs `libusb: warning [libusb_exit] device 2.0 still referenced` on app exit

usbLinkOpen() in the develop branch calls libusb_ref_device() to increment the ref, but I can not find any code that will decrement it using libusb_unref_device().

This is suspicious and may have unwanted sideaffects. libusb doc writes that ref/unref need to be matched and when ref=0 a device is finally destroyed.

libusb_ref_device() only exists one place...it is within refLibusbDeviceByName()

XLink/src/pc/protocols/usb_host.cpp

Line 244 in 457b23f

libusb_ref_device(devs[i]);

That function above refLibusbDeviceByName() is called by code in three locations...

`usb_boot()` location

Looks correct to me. It has a matching unref.

XLink/src/pc/protocols/usb_host.cpp

Lines 632 to 665 in 457b23f

    
               if(refLibusbDeviceByName(addr, &dev) == X_LINK_PLATFORM_SUCCESS){ 
        
                   break; 
        
               } 
        
               std::this_thread::sleep_for(milliseconds(10)); 
        
           } while(steady_clock::now() - t1 < DEFAULT_CONNECT_TIMEOUT); 
        
           if(dev == nullptr) { 
        
               return X_LINK_PLATFORM_DEVICE_NOT_FOUND; 
        
           } 
        
           auto t2 = steady_clock::now(); 
        
           do { 
        
               if((res = usb_open_device(dev, &endpoint, h)) == LIBUSB_SUCCESS){ 
        
                   break; 
        
               } 
        
               std::this_thread::sleep_for(milliseconds(100)); 
        
           } while(steady_clock::now() - t2 < DEFAULT_CONNECT_TIMEOUT); 
        
           if(res == LIBUSB_SUCCESS) { 
        
               rc = send_file(h, endpoint, mvcmd, size, bcdusb); 
        
               libusb_release_interface(h, 0); 
        
               libusb_close(h); 
        
           } else { 
        
               if(res == LIBUSB_ERROR_ACCESS) { 
        
                   rc = X_LINK_PLATFORM_INSUFFICIENT_PERMISSIONS; 
        
               } else if(res == LIBUSB_ERROR_BUSY) { 
        
                   rc = X_LINK_PLATFORM_DEVICE_BUSY; 
        
               } else { 
        
                   rc = X_LINK_PLATFORM_ERROR; 
        
               } 
        
           } 
        
           if (dev) { 
        
               libusb_unref_device(dev);

`usbLinkBootBootloader()` location

Looks correct to me. It has a matching unref.

XLink/src/pc/protocols/usb_host.cpp

Line 714 in 457b23f

auto refErr = refLibusbDeviceByName(path, &dev);

`usbLinkOpen` location

Looks suspicious. It calls refLibusbDeviceByName which increments the ref. But nothing calls unref. If I guess, I would think there should be an unref in usbLinkClose but there is not.

XLink/src/pc/protocols/usb_host.cpp

Line 687 in 457b23f

if(refLibusbDeviceByName(path, &dev) == X_LINK_PLATFORM_SUCCESS){

`getNextAvailableLink()` could leave availableXLinks[] array inconsistent

When getNextAvailableLink() starts to populate a xLinkDesc_t within availableXLinks[], if an error occurs it could leave that entry in the array in an inconsistent state. I believe it prematurely populates link->id = id;

XLink/src/shared/XLinkDevice.c

Lines 509 to 516 in fc76e5b

    
           xLinkDesc_t* link = &availableXLinks[i]; 
        
           link->id = id; 
        
           if (XLink_sem_init(&link->dispatcherClosedSem, 0 ,0)) { 
        
               mvLog(MVLOG_ERROR, "Cannot initialize semaphore\n"); 
        
               XLINK_RET_ERR_IF(pthread_mutex_unlock(&availableXLinksMutex) != 0, NULL); 
        
               return NULL; 
        
           }

Setup

XLink at fc76e5b
all OS, all compilers

Repo

Found in code review of that commit.

line 509 gets the address of an entry in availableXLinks[].
Changes that entry's id from INVALID_LINK_ID to an id retrieved from getNextAvailableLinkUniqueId()
Then attempts XLink_sem_init()
In scenarios where that fails, function returns NULL to the caller.

Result

The entry in availableXLinks[i] has been changed. It is not INVALID_LINK_ID. Instead, it is inconsistent...
...it has a valid link->id
...but the link->dispatcherClosedSem is invalid/uninitialized
This is inconsistent.

Expected

The entry in availableXLinks[] should be marked an invalid/empty xLinkDesc_t since the initialization was not successful.

Fix

Move link->id = id;. Put it just before the final call to pthread_mutex_unlock().
Yes, it is possible for that last pthread_mutex_unlock() to fail. Yet if it does, that is a separate issue. availableXLinks[] itself will be valid and correctly initialized -- it will be consistent.

add_flag() doesn't add flags to any C++ files in project

add_flag() in this project's cmake doesn't add flags to C++ files.
I suspect when c++ files were added to the project, the dev didn't update cmake to support them.

Trivial fix now in my fork. Will link to this issue below. Easy cherry pick.

mvLog header kills system `attribute()` macro; causes chaos and mass failures

I discovered that the XLinkLog.h header redefines the critical system macro __attribute__.
The rule is...don't use, change, or redefine things (e.g. macros) with double underscores. Period no exceptions.
https://devblogs.microsoft.com/oldnewthing/20230109-00/?p=107685

XLink is working around this issue by careful ordering of header includes. Not a good solution. One header that is broken by this issue is libusb.h. Other code/headers may be affected silently.

Setup

all os, platforms

Repro

Setup a Windows machine to compile with mingw
Create any .cpp file in the XLink project, e.g. src\pc\protocols\myfile.cpp
Edit your cpp file to have the following

#define MVLOG_UNIT_NAME xLinkUsb

#ifdef XLINK_LIBUSB_LOCAL
#include <libusb.h>
#else
#include <libusb-1.0/libusb.h>
#endif

#include "XLink/XLinkLog.h"

int myvar = 1;

config, build

Your build should be successful
Now...change the order so that your file has this code...

#define MVLOG_UNIT_NAME xLinkUsb
#include "XLink/XLinkLog.h"

#ifdef XLINK_LIBUSB_LOCAL
#include <libusb.h>
#else
#include <libusb-1.0/libusb.h>
#endif

int myvar = 1;

Result

uncountable number of errors, deep within the system headers, usually related to intrinsics. For example...

[build] C:/Users/WDAGUtilityAccount/Desktop/mingw64/lib/gcc/x86_64-w64-mingw32/12.2.0/include/mmintrin.h: In function '__m64 _mm_cvtsi32_si64(int)':
[build] C:/Users/WDAGUtilityAccount/Desktop/mingw64/lib/gcc/x86_64-w64-mingw32/12.2.0/include/mmintrin.h:79:54: error: cannot convert a vector of type '__vector(2) int' to type '__m64' {aka 'int'} which has different size
[build]    79 |   return (__m64) __builtin_ia32_vec_init_v2si (__i, 0);
[build]       |                                                      ^
[build] C:/Users/WDAGUtilityAccount/Desktop/mingw64/lib/gcc/x86_64-w64-mingw32/12.2.0/include/mmintrin.h: In function 'int _mm_cvtsi64_si32(__m64)':
[build] C:/Users/WDAGUtilityAccount/Desktop/mingw64/lib/gcc/x86_64-w64-mingw32/12.2.0/include/mmintrin.h:122:39: error: cannot convert '__m64' {aka 'int'} to '__vector(2) int'
[build]   122 |   return __builtin_ia32_vec_ext_v2si ((__v2si)__i, 0);
[build]       |                                       ^~~~~~~~~~~
[build]       |                                       |
[build]       |                                       __m64 {aka int}
[build] <built-in>: note:   initializing argument 1 of 'int __builtin_ia32_vec_ext_v2si(__vector(2) int, int)'
[build] C:/Users/WDAGUtilityAccount/Desktop/mingw64/lib/gcc/x86_64-w64-mingw32/12.2.0/include/mmintrin.h: In function '__m64 _mm_packs_pi16(__m64, __m64)':
[build] C:/Users/WDAGUtilityAccount/Desktop/mingw64/lib/gcc/x86_64-w64-mingw32/12.2.0/include/mmintrin.h:161:43: error: cannot convert '__v4hi' {aka 'short int'} to '__vector(4) short int'
[build]   161 |   return (__m64) __builtin_ia32_packsswb ((__v4hi)__m1, (__v4hi)__m2);
[build]       |                                           ^~~~~~~~~~~~
[build]       |                                           |
[build]       |                                           __v4hi {aka short int}
[build] <built-in>: note:   initializing argument 1 of '__vector(8) char __builtin_ia32_packsswb(__vector(4) short int, __vector(4) short int)'

Notes

The bug is in include\XLink\XLinkLog.h. It redefines the system macro __attribute__. That is forbidden. And causes chaos like libusb header failures and potentially silent failures elsewhere. For example, in usb_host.cpp 4 local headers and 5+ system headers will no longer have a functioning __attribute__ macro.

XLink/include/XLink/XLinkLog.h

Lines 48 to 53 in d209b7e

    
           #if (defined (WINNT) || defined(_WIN32) || defined(_WIN64) ) 
        
           #define __attribute__(x) 
        
           #define FUNCATTR_WEAK static 
        
           #else 
        
           #define FUNCATTR_WEAK 
        
           #endif

LICENSE file missing

What's the license of this repo?

Please include LICENSE file.

Remove public build flags

Flags __PC__ and XLINK_USE_MX_ID_NAME have to be properly passed through to users of this library (even transitively). To make 3rdparty integration easier (non-CMake usage), this flags should preferably be removed with their defaults set appropriately.

errant xlink error checking, deref null ptr, string/memory out of bounds, memory leak, etc.

XLink has various unsafe usage found by the MSVC code analyzer.
Some are code defects and can lead to abnormal program flow and/or crash.

My intention is to fix code defects, and to decrease noise so that important warnings/errors can be seen.
I have a PR ready that addresses all issues except one in the CONCERN section below

Setup

Microsoft Windows [Version 10.0.19044.1415]
MSVC v16.11.8, with code analyzer enabled, and CppCoreCheck.dll, yet disabling several warnings
XLink master eef5a51

Repro

config for x64, debug, static lib
and with msvc compile flags to include /analyze:plugin EspXEngine.dll /wd26440 /wd26496 /wd26462 /wd26460 /wd26481 /wd26493 /wd26446 /wd26482 /wd26485 /wd26812 /wd26476 /wd26490 /wd26472 /wd26495 /wd26432 /wd26457 /wd6031

Set env var for the analyzer...

esp.extensions=CppCoreCheck.dll
esp.annotationbuildlevel=ignore

build Xlink

Result

I read the below compiler+analyzer output and investigated the code.
Almost all the issues the analyzer found are legitimate bugs (not style issues).

[main] Building folder: depthai-core XLink
[build] Starting build
[proc] Executing command: "C:\Program Files\CMake\bin\cmake.exe" --build c:/njs/depthai-core/build --config Debug --target XLink -j 14 --
[build] [14/24   4% :: 1.621] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\usb_mx_id.c.obj
[build] [15/24   8% :: 1.855] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkLog.c.obj
[build] [16/24  12% :: 1.883] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkSemaphore.c.obj
[build] [17/24  16% :: 1.941] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkStream.c.obj
[build] [18/24  20% :: 1.953] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkPrivateFields.c.obj
[build] [19/24  25% :: 1.960] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDispatcherImpl.c.obj
[build] C:\njs\XLink\src\shared\XLinkDispatcherImpl.c(346) : warning C6246: Local declaration of 'stream' hides declaration of the same name in outer scope. For additional information, see previous declaration at line '254' of 'c:\njs\xlink\src\shared\xlinkdispatcherimpl.c'.: Lines: 254
[build] C:\njs\XLink\src\shared\XLinkDispatcherImpl.c(417) : warning C6246: Local declaration of 'stream' hides declaration of the same name in outer scope. For additional information, see previous declaration at line '254' of 'c:\njs\xlink\src\shared\xlinkdispatcherimpl.c'.: Lines: 254
[build] C:\njs\XLink\src\shared\XLinkDispatcherImpl.c(546) : warning C6011: Dereferencing NULL pointer 'ret'. : Lines: 542, 543, 545, 546
[build] [20/24  29% :: 1.970] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkPrivateDefines.c.obj
[build] [21/24  33% :: 1.989] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDeprecated.c.obj
[build] [22/24  37% :: 2.005] Building C object XLink\CMakeFiles\XLink.dir\src\pc\PlatformDeviceSearch.c.obj
[build] [23/24  41% :: 2.020] Building C object XLink\CMakeFiles\XLink.dir\src\pc\PlatformData.c.obj
[build] [23/24  45% :: 2.045] Building C object XLink\CMakeFiles\XLink.dir\src\pc\PlatformDeviceControl.c.obj
[build] C:\njs\XLink\src\pc\PlatformDeviceControl.c(660) : warning C6387: 'devPathWriteBuff' could be '0':  this does not adhere to the specification for the function 'strncpy'. : Lines: 643, 644, 656, 658, 659, 660
[build] C:\njs\XLink\src\pc\PlatformDeviceControl.c(662) : warning C6053: The prior call to 'strncpy' might not zero-terminate string 'devPathWriteBuff'.: Lines: 643, 644, 656, 658, 659, 660, 662
[build] [23/24  50% :: 2.058] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDevice.c.obj
[build] C:\njs\XLink\src\shared\XLinkDevice.c(331): warning C4244: '+=': conversion from 'int64_t' to 'long', possible loss of data
[build] C:\njs\XLink\src\shared\XLinkDevice.c(333): warning C4244: '-=': conversion from 'int64_t' to 'long', possible loss of data
[build] C:\njs\XLink\src\shared\XLinkDevice.c(325): warning C4101: 'end': unreferenced local variable
[build] [23/24  54% :: 2.097] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\tcpip_host.c.obj
[build] C:\njs\XLink\src\pc\protocols\tcpip_host.c(393): warning C4133: 'function': incompatible types - from 'tcpipHostCommand_t *' to 'const char *'
[build] C:\njs\XLink\src\pc\protocols\tcpip_host.c(169) : warning C6001: Using uninitialized memory 'size'.: Lines: 166, 167, 169
[build] C:\njs\XLink\src\pc\protocols\tcpip_host.c(186) : warning C6011: Dereferencing NULL pointer 'ipaddrtable'. : Lines: 166, 167, 169, 170, 173, 175, 176, 181, 182, 183, 184, 185, 186
[build] [23/24  58% :: 2.184] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkStringUtils.c.obj
[build] [23/24  62% :: 3.113] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_time.c.obj
[build] [23/24  66% :: 3.202] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_synchapi.c.obj
[build] [23/24  70% :: 3.236] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\usb_boot.c.obj
[build] C:\njs\XLink\src\pc\protocols\usb_boot.c(807) : warning C6244: Local declaration of 'bulk_chunklen' hides previous declaration at line '48' of 'c:\njs\xlink\src\pc\protocols\usb_boot.c'.: Lines: 48
[build] [23/24  75% :: 3.295] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_pthread.c.obj
[build] C:\njs\XLink\src\pc\Win\src\win_pthread.c(184) : warning C6387: 'Temp_value_#19' could be '0':  this does not adhere to the specification for the function 'GetProcAddress'. : Lines: 184, 181, 182, 184
[build] C:\njs\XLink\src\pc\Win\src\win_pthread.c(205) : warning C6387: 'Temp_value_#33' could be '0':  this does not adhere to the specification for the function 'GetProcAddress'. : Lines: 205, 203, 205
[build] [23/24  79% :: 3.300] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\pcie_host.c.obj
[build] [23/24  83% :: 3.343] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_semaphore.c.obj
[build] [23/24  87% :: 3.441] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkData.c.obj
[build] C:\njs\XLink\src\shared\XLinkData.c(463): warning C4244: '+=': conversion from 'int64_t' to 'long', possible loss of data
[build] C:\njs\XLink\src\shared\XLinkData.c(465): warning C4244: '-=': conversion from 'int64_t' to 'long', possible loss of data
[build] [23/24  91% :: 3.545] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDispatcher.c.obj
[build] [23/24  95% :: 3.830] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_usb.c.obj
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(432): warning C4090: 'function': different 'const' qualifiers
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(432): warning C4267: 'function': conversion from 'size_t' to 'ULONG', possible loss of data
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(423): warning C4267: 'initializing': conversion from 'size_t' to 'USHORT', possible loss of data
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(917): warning C4996: 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(152) : warning C6255: _alloca indicates failure by raising a stack overflow exception.  Consider using _malloca instead.
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(563) : warning C28182: Dereferencing NULL pointer. 'deviceInterfaceDetailData' contains the same NULL value as 'LocalAlloc()`560' did. : Lines: 545, 546, 548, 549, 550, 553, 554, 558, 559, 560, 563
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(718) : warning C26451: Arithmetic overflow: Using operator '*' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '*' to avoid overflow (io.2).
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(828) : warning C6011: Dereferencing NULL pointer 'pDevList->infos'. : Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(832) : warning C6387: 'pDevList->infos+i' could be '0':  this does not adhere to the specification for the function 'SetupDiEnumDeviceInfo'. See line 828 for an earlier location where this can occur: Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828, 827, 828, 827, 832
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(833) : warning C6387: 'pDevList->infos+i' could be '0':  this does not adhere to the specification for the function 'SetupDiGetDeviceRegistryPropertyA'. See line 828 for an earlier location where this can occur: Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828, 827, 828, 827, 832, 833
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(841) : warning C6011: Dereferencing NULL pointer 'pDevList->vidpids'. : Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828, 827, 828, 827, 832, 833, 836, 837, 841
[build] [24/24 100% :: 3.873] Linking C static library XLink\XLinkd.lib
[build] Build finished with exit code 0

Concern -- need feedback

src\pc\protocols\usb_boot.c there are two vars same name bulk_chunklen. One is global static. The other is function local

Problem 1 -- same name
Problem 2 -- look at how the static is used. It is set (in non Windows codepath) but never read by any codepath. The local var is set/read. Like 823-827 makes me suspect that it should be reading the global bulk_chunklen as it might have had its value restricted by line 706 -- where libusb declares the max packet size for this usb endpoint. This is suspicious.

I suspect a code defect concerning these bulk_chunklen vars. I don't have the USB experience or the hardware-level visibility to test this. 🤔 What is your view on this topic? I want to update the PR to address this (or open a separate bug for your USB team member to investigate).

FYI

In win_usb.c the two call sites for SetupDiGetDeviceInterfaceDetail() have inconsistent code, no error handling, memory leak due to missing LocalFree(), and use of problematic Windows apis like _alloca(). The call site get_mx_id_device_path() is definitely used. I didn't readily find a client-side codepath that would call the other retreive_dev_path().

_alloca() raises a Windows SEH exception on error and the existing code will crash the main program.
SEH exceptions are Windows-only and require Windows specific wrapping via __try __except __finally or the program crashes.
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca?view=msvc-170
https://docs.microsoft.com/en-us/cpp/cpp/structured-exception-handling-c-cpp?view=msvc-170

I resolved difference in the two call sites, handled leaks and errors, and changed to use malloc(). There is no need for the other inconsistent memory alloc functions.

If you want to use _alloca() at one of the call sites, the adjusted code should be similar to... 😐

__try
{
    detData = (PSP_DEVICE_INTERFACE_DETAIL_DATA)_alloca(reqLen);
}
__except (GetExceptionCode() == STATUS_STACK_OVERFLOW)
{
    // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca
    // when stack overflows, attempt stack restore
    if (!_resetstkoflw())
        mvLog(MVLOG_FATAL, "Stack corrupt after _alloca() failure for SetupDiEnumDeviceInterfaces()\n");
    SetupDiDestroyDeviceInfoList(devInfo);
    return USB_DEV_NONE;
}

duplicate decrement in `XLinkPlatformFindDevices()`

XLinkPlatformFindDevices() errantly double-decrements when looking for TCPIP devices
line 118 is wrong and should be removed

XLink/src/pc/PlatformDeviceSearch.c

Lines 115 to 124 in d209b7e

    
           TCPIP_rc = getTcpIpDevices(in_deviceRequirements, out_foundDevices, sizeFoundDevices, &numFoundDevices); 
        
           *out_amountOfFoundDevices += numFoundDevices; 
        
           out_foundDevices += numFoundDevices; 
        
           sizeFoundDevices -= numFoundDevices; 
        
           // Found enough devices, return 
        
           if (numFoundDevices >= sizeFoundDevices) { 
        
               return X_LINK_PLATFORM_SUCCESS; 
        
           } else { 
        
               sizeFoundDevices -= numFoundDevices; 
        
           }

Setup

all os, platforms
xlink used for depthai-core v2.21.2, master, and develop

Repro

Found during code review. Referencing above, line 118 should be removed. Line 123 does the correct decrement.

I also caution that lines 89 and 120 are missing a dangerous scenario. If numFoundDevices > sizeFoundDevices that means that memory was overwritten/corrupted. That scenario must never occur.

assert test in `pcie_host` are always true; suggests logic or coding bug earlier

Found by compiler warnings. These asserts are always true and therefore useless asserts. This suggests a logic failure before, wrong variable types, coding error, etc.

XLink/src/pc/protocols/pcie_host.c

Line 148 in d209b7e

ASSERT_XLINK_PLATFORM_R(bufSize >= 0, PCIE_INVALID_PARAMETERS);

XLink/src/pc/protocols/pcie_host.c

Line 224 in d209b7e

ASSERT_XLINK_PLATFORM_R(bufSize >= 0, PCIE_INVALID_PARAMETERS);

They are always true because their param is an unsigned variable, therefore it is be definition always >=0 making these asserts always true.

OSX build issue (MSG_NOSIGNAL).

Hello.
I ran into an issue while building something with XLink as a dependency; basically "PlatformData.c" uses MSG_NOSIGNAL, but OSX doesn't have that :(.
Can we add a switch for apple (flag = 0 maybe?).

workaround libusb bug in `libusb_free_device_list(ptr, 1)`

libusb_free_device_list(ptr, 1) does not handle the scenario of having null entries in the device list. It will errantly stop its unref walk of the list thinking it has reached the terminating null. libusb/libusb#1287

Fix now in my fork of Xlink. Will auto link to the commit below.

Idea: consider merging this repo into depthai-core?

This xlink fork seems only used at the moment in the depthai-core library.

Is this library used anywhere else? If not, it might be simpler to include this xlink code in a subfolder in the depthai-core repository.

Notable benefits:

Easier building without usage of hunter
No seperate versioning of xlink and depthai-core, so no configuration management of the compatibility between the two

Downsides:

xlink cannot be used stand-alone (is this an actual use case at the moment?)

This is an idea I had when browsing through the code-base, but maybe I'm missing something? Is there any reference to the original xlink library? I was not able to find it.

Clang immintrin bug

HI I have been trying to deduce this bug from the depthai-core library and found out that it is a simple fix but do not know how to make a pull request...

In the file:
XLink/src/pc/protocols/usb_host.cpp

The std headers just need to be moved to the top of the file for this bug to be fixed... I assumed it was a clang bug but it turns out its an order of header bug.

This would make the xlink library clang compatible if this single issue is fixed

Create a tag

Please, could you create a tag of this repo that had relation with depthai-core release?

I have seen in #46 that maybe it would be merged in the main directory, but in the meantime create a tag could not be difficult.

mvLog() is double-spacing; how use mvLog() in regards to trailing newline?

XLink double-spaces the mvLog output often. Nothing breaks, but when the log volume is large and multiple things are logging, the doublespacing doesn't help. 😉

While writing XLink code, I see inconsistencies of mvLog() format strings. Some have a trailing \n and some do not.
On Windows and Linux, this trailing newline in the format string causes doublespaced log output on the console.

This can be resolved by choosing a policy, enforcing it, and then adjusting the mvLog calls to follow that policy. Bulk changes are easy with tools like vscode its regex search/replace.

Setup

XLink at develop 457b23f
Windows and Linux. Maybe other platforms
MSVC v16.11.26 and g++ v11.3.0

Repo

Found by watching logs and then mvLog code review. Easy repro is to...

add the following code at the top of the function XLinkPlatformFindDevices

mvLog(MVLOG_FATAL, "Test line 1\n");
mvLog(MVLOG_FATAL, "Test line 2\n");
mvLog(MVLOG_FATAL, "Test line 3\n");

config, build for Debug
set env var XLINK_LEVEL=fatal the value must be lowercase
run the test examples/list_devices

Result

Both Windows and Linux have double-spaced log entries

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:58  Test line 1

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:59  Test line 2

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:60  Test line 3

Expected

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:58  Test line 1
F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:59  Test line 2
F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:60  Test line 3

Notes

This is due to use of \n in the format string. When code calls mvLog(MVLOG_FATAL, "Test line 1\n"); with a trailing space, this causes double-spaced entries due to a trailing newline also added by mvLog() itself. There are 4 platform scenarios. 3/4 adds a newline. 1/4 (Android) doesn't.

XLink/src/shared/XLinkLog.c

Lines 162 to 186 in 457b23f

    
           #ifdef __ANDROID__ 
        
               // Convert to Android logging enumeration 
        
               enum android_LogPriority logPrio = ANDROID_LOG_DEBUG + (lvl - MVLOG_DEBUG); 
        
               //__android_log_print(logPrio, UNIT_NAME_STR, headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line); 
        
               __android_log_vprint(logPrio, UNIT_NAME_STR, format, args); 
        
               // __android_log_print(logPrio, UNIT_NAME_STR, "%s", ANSI_COLOR_RESET); 
        
           #else 
        
               fprintf(stdout, headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line); 
        
               vfprintf(stdout, format, args); 
        
               fprintf(stdout, "%s\n", ANSI_COLOR_RESET); 
        
           #endif 
        
           #elif defined __shave__ 
        
               printf(headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line); 
        
               printf(format, args); 
        
               printf("%s\n", ANSI_COLOR_RESET); 
        
           #endif 
        
           #ifdef __RTEMS__ 
        
               } 
        
               else 
        
               { 
        
                   printk(headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line); 
        
                   vprintk(format, args); 
        
                   printk("%s\n", ANSI_COLOR_RESET);

incorrect use of memset corrupts `XLinkPlatformFindDevices()` out param

memset() is used incorrectly causing corruption and/or not nulling the struct(s). For now, it appears an annoyance for debugging though definitely errant code. I recommend fixing.

Setup

all os, platforms, compilers
xlink that is used in depthai-core v2.21.2 b7c3aca and current master

Repro

found in code review during isolation of device closing errant behavior

XLink/src/pc/PlatformDeviceSearch.c

Line 52 in d209b7e

memset(out_foundDevices, sizeFoundDevices, sizeof(deviceDesc_t));

should be

memset(out_foundDevices, 0, sizeFoundDevices * sizeof(deviceDesc_t));

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

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) {

Compile With LLVM Clang

Hi, even though I have been using github for some 5 years now, I still have not once done a pull-request. Meaning I am not looking for any credit, hell I just want the damn thing fixed so I don't have to keep providing another flag to depthai-core when building to tell it where the fixed xlink library is.

In the file pc/protocols/tcpip_host.h there is the function declaration for the public close socket function. The param is int socketfd, but the definition in tcpip_host.c has SOCKET sock as its param. I do not know if SOCKET is supposed to macro to int but just fixing that one thing allows the static lib to be built using the latest LLVM Clang Binarys on Windows. I am going to check now if depthai-core can also work out of the box now with LLVM Clang, if not, I will also do a similar message on that repo stating my findings so someone else can fix that. Then you can promote LLVM Clang as a working toolchain. ;)

p.s. yes I would also be greatful for some links on where to learn pull-requests, creation, management, and whatever else so I don't have to keep doing it this way...

Many thanks,
-Peter F

vidpid->state lookup container improvement; changing to `std::array`

While refactoring XLink using libusb wrappers, I saw that the vidpid -> xlink state lookup is implemented as std::unordered_map vidPidToDeviceState. For XLink's use case, this is a poor choice as it uses more memory, more binary size, and more cpu than a simple solution with std::array.

I wanted to learn/play with some new ai and benchmarking tools. They all agree to switch to std::array.
I am changing it in my refactor.

Setup

XLink at all current major branches
all platforms and compilers

Repro

First found with code review. godbolts and benchmarks below.

with only 4 elements, std::array is more efficient in code and memory than std::unordered_map
std::array has zero memory overhead and no allocations; can store the values at compile-time; search by comparing (at most) 8 ints
std::vector has memory overhead of ~24 bytes (on 64bit); runtime has to insert values into heap; search by jumping to heap location of vector data and comparing (at most) 8 ints
std::unordered_map has memory overhead of 72+ bytes (on 64bit) + ~20 bytes per element; runtime has to insert values; search by hash calc, hash calc, binary op, jump to calculated hash bucket pointer, jump to buckets linked list, walk linked list and compare 2 ints for each entity until find the match (walking list up to 4 times...likely less)

For this use-case of fixed never-changing elements and very small size, std::array is the best choice.

At https://godbolt.org/z/KM3E9Gdfd is code for both today's std::ordered_map and a new impl with std::array. You can chose which you want with -DUSE_UNMAP or -DUSE_ARRAY. After it compiles, notice several things

Result

The std::array implementation has compile-time created the array and stored it in the binary for immediate use by the app. This results in faster load times, no overhead, no memory allocations. gcc is also optimizing the compiled code given it knows what is in the array at compile-time. In contrast, the map implementation will have the raw initialization data stored somewhere in the binary and then it has to make heap allocations for each created entry into the map and make copies of the initialization data into them.
The binary size difference is substantial. array = 95,505 and unordered_map = 497,156. The map is 4x larger. (additional 400kb) in filesize 😥. All that extra code for nothing since this map is never mutated and t-i-n-y.

When benchmarked https://quick-bench.com/q/CNuAH-ebWBdson0Epl-F--C3ZXI the array implementation is 3.8x faster than map.

Slightly altered code to generate random vidpids show array still faster (1.3x faster) but not as dramatic. https://quick-bench.com/q/orjPkib5xLpID0nitgWJc-VowTY. I think this is due to the addional load of generating random numbers and that since a fixed search vidpid isn't known at compile time, then gcc can't optimize as aggressively.

The speed increase is not really significant. If that were the only thing I would say this is microoptimization ;-)
However, the file size is significant. Saving 400kb in filesize justifies the trivial change to std::array.

xlink is errant calling exit() which forcibly destroys the main app itself -- should never force exit apps

After much debug and fixing thread/ownership issues in depthai startup/shutdown, I isolated that XLink is calling exit() which force terminates the main app itself. Naturally this is errant. XLink library should never exit() or abort() or quick_exit(). Instead it should use tools like assert(), returning error values from the function, etc.

Setup

Microsoft Windows [Version 10.0.19043.1348]
Xlink 4c14908
MSVS 2019 Community v16.11.6

Repro

Search codebase for exit( and all 3 are errant...
or...

Fix the threading/ownership issues in depthai startup/shutdown luxonis/depthai-core#257
config, build=debug for depthai (which will pull in xlink)
Run repeatedly the example bootloader_version.exe in msvc debugger

Result

Crashes and/or app suddendly terminates with exit code=1

Expected

No crash, and clean exit with exit code = 0

Logs

After the hacks for threading/ownership above, enabling logging in XLink, adding a few mvLog() calls in XLink itself, and compiling XLink locally using DEPTHAI_XLINK_LOCAL I isolated the issue.

Found device with name: 14442C10C1A1C6D200-ma2480
[2021-11-15 16:16:36.412] [debug] connlinkid= 0    streamId= 0
creating... init::watchdogThread 
Version: 0.0.15
DeviceBootloader about to be closed...
::close before join()
[2021-11-15 16:16:36.414] [debug] connlinkid= 0    streamId= 1
init::watchdogThread 2 before loop
init::watchdogThread 2 after loop 1
E: [global] [         0] [] addEvent:321	Condition failed: event->header.flags.bitField.ack != 1
E: [global] [         0] [] addEventWithPerf:333	 addEvent(event) method call failed with an error: 3
F: [global] [         0] [EventRead00Thr] dispatcherEventReceive:91	Duplicate id detected. 
E: [global] [         0] [] XLinkReadData:183	Condition failed: (addEventWithPerf(&event, &opTime))
E: [global] [         0] [EventRead00Thr] handleIncomingEvent:568	Assertion Failed and exit(EXIT_FAILURE): stream 
F: [global] [         0] [Scheduler00Thr] dispatcherResponseServe:776	no request for this response: XLINK_WRITE_RESP 1
init::watchdogThread 2 after readRaw
init::watchdogThread 2 before sleep in try
init::watchdogThread 2 after sleep in try
init::watchdogThread 2 exiting thread lambda...
The program '[11784] bootloader_version.exe' has exited with code 1 (0x1).

Look above at handleIncomingEvent:568 Assertion Failed and exit(EXIT_FAILURE): stream
This is ASSERT_XLINK(stream);
which is a macro that expands to (I added the extra "exit()" in the log so I can see behavior)

#define ASSERT_XLINK(condition) do { \
            if(!(condition)) { \
                mvLog(MVLOG_ERROR, "Assertion Failed and exit(EXIT_FAILURE): %s \n", #condition); \
                exit(EXIT_FAILURE); \
            } \
        } while(0)
    #endif // ASSERT_XLINK

I can see that original coder has a release/debug for these exit(). Since depthai and hunter compile the build_type as specified, and therefore apps link in that specified build type, it is errant to ever compile an exit() and provide that in any easy to use system via cmake, hunter, etc.

I see 3 places where exit() is called. I believe all of them should be altered to never exit().

remove the debug versions of those two macros

the 3rd function which hardcodes exit(1) can be replaced with something similar to..

mvLog(MVLOG_ERROR, "connect(usbFdWrite,...) returned < 0\n");
close(usbFdRead);
close(usbFdWrite);
usbFdRead = -1;
usbFdWrite = -1;
return X_LINK_PLATFORM_ERROR;

After these XLink fixes, the example bootloader_version.exe is so far successful 100% of the time.
Further...other tests which would sometimes fail previously like rgb_preview.exe are also successful.
I do see other failures on other tests, so I'll keep searching for the source of those fails as a separate issue.

CLOCK_REALTIME should be CLOCK_MONOTONIC

Throughout depthai-core and Xlink I believe CLOCK_REALTIME is wrong. It should be CLOCK_MONOTONIC but there is a problem in POSIX's semaphore API. 😕

The code everywhere is relative time when CLOCK_xxxx is used with apis like clock_gettime().

However...

CLOCK_REALTIME is like a clock on your kitchen wall. That clock can move forward or backward due to LAN servers updating network time, NTP servers changing time, leap seconds, etc. It is not reliable for use in depthai-core and xlink.
CLOCK_MONOTONIC is monotonic time since some unspecified starting point, it can never be changed or set by an admin, server process, NTP, leap seconds, or anything else

Changing all the code to use CLOCK_MONOTONIC will ensure that watchdogs, semaphore timeouts, PoE devices, and all kinds of other sensitive timing thing are not affected by clock changes.

There is a problem. XLink uses POSIX apis like pthread_mutex_timedlock(), pthread_cond_timedwait(), and sem_timedwait(). They all accept an epoch-time parameter. But clocks based on epoch are realtime wall-clocks. They vary positive and negative as described above. This is a known issue with those POSIX apis.
https://www.club.cc.cmu.edu/~cmccabe/blog_time_and_time_again.html
https://bugs.eclipse.org/bugs/show_bug.cgi?id=569084

Other mutex and semaphore apis like Boost, Windows, etc. do not have this bug.

`getWinUsbMxId()` path construction is errant and doesn't find libusb devices

getWinUsbMxId() does not correctly transform Win32 location paths into libusb port paths. This causes failure(s) to find USB devices.

getWinUsbMxId() is a fallback function that is used only on Windows when libusb finds a X_LINK_BOOTED device that it can't access due to other app/code having that device already open via libusb; there is no shared access.

getWinUsbMxId() uses the Win32 native SetupDi api to query for usb devices. Then the function attempts to map the Win32 "Location path" into a libusb path of ports. Unfortunately, this mapping is errant and therefore devices are often not found since the path's will not match.

Setup

Microsoft Windows [Version 10.0.22621.1702] aka Windows 11
VS2019 v16.11.25
depthai-core v2.21.2 + fixes via https://github.com/diablodale/depthai-core/tree/device-tests-close-threads
XLink develop 88d81da with a LOT of debug output
Three OAK usb devices

Repro

Add lots of debug to trace behaviors of Xlink 😉
Change tests/color_camera_node_test line 46 to be a forever while(true) loop
Connect 3 OAK devices via USB to the same computer. I recommend at least two of them on separate controllers.
Open two terminals on the computer
Run color_camera_node_test in first terminal and wait until it is in the loop. Observe the debug output. LEAVE IT RUNNING!
Run color_camera_node_test in second terminal and wait until it is in the loop. Observe the debug output

Result

First terminal

Notice the probe of the 3 devices by running the small program on each. Then the chosen device is connected+started.

F: [global] [         0] [ThreadN] getLibusbDeviceMxId:353      attempting small program...
F: [global] [         0] [ThreadN] getUSBDevices:173    getLibusbDeviceMxId() returned: Success
F: [global] [         0] [ThreadN] getUSBDevices:201    inserting...
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:353      attempting small program...
F: [global] [         0] [ThreadN] getUSBDevices:173    getLibusbDeviceMxId() returned: Success
F: [global] [         0] [ThreadN] getUSBDevices:201    inserting...
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:353      attempting small program...
F: [global] [         0] [ThreadN] getUSBDevices:173    getLibusbDeviceMxId() returned: Success
F: [global] [         0] [ThreadN] getUSBDevices:201    inserting...
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:353      attempting small program...
F: [global] [         0] [ThreadN] getUSBDevices:173    getLibusbDeviceMxId(1844301031490A1300) returned: Success
F: [global] [         0] [ThreadN] getUSBDevices:201    inserting found device into result
...

Second terminal

Here the bug is exposed. It again probes all three devices. The 1st and 2nd can run the small program to probe them. However, the 3rd device is running in the other terminal and therefore can not be opened. It is "libusb device is BOOTED and LIBUSB_ERROR_ACCESS" which leads to the windows-only fallback function getWinUsbMxId().

getWinUsbMxId() calls Win32 SetupDi apis, parses for serial numbers in the DeviceInstanceId, and attempts to transfor the Windows path into a libusb port path. In the logs below "seeking..." is the params passed to getWinUsbMxId(). It is seeking vid=03e7 pid=f63b at libusb_path="3.20".

Remember that terminal 1 has one device open. Look below in the "candidate" entries. Each candidate is the vid, pid, and custom mapping path contructed; just before the if() test. Easy to see in the devbug all three OAK devices since they all have the same vid=03e7.

UNBOOTED are...
pid=2485 simulatedpath=1.9
pid=2485 simulatedpath=1.1

BOOTED is...
pid=f63b simulatedpath=1.20 👎😢

The function was given libusb port path "3.20" yet the simulated mapping generated "1.20". This is errant.

F: [global] [         0] [ThreadN] getLibusbDeviceMxId:353      attempting small program...
F: [global] [         0] [ThreadN] getUSBDevices:173    getLibusbDeviceMxId() returned: Success
F: [global] [         0] [ThreadN] getUSBDevices:201    inserting found device into result
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:353      attempting small program...
F: [global] [         0] [ThreadN] getUSBDevices:173    getLibusbDeviceMxId() returned: Success
F: [global] [         0] [ThreadN] getUSBDevices:201    inserting found device into result
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:330      libusb_open: Access denied (insufficient permissions)
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:335      libusb device is BOOTED and LIBUSB_ERROR_ACCESS
F: [global] [         0] [ThreadN] getWinUsbMxId:1077   seeking   03e7  f63b  3.20
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485  1.9
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 0bda  0316  1.18
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 0bda  0316  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  f63b  1.20
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  f63b  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 048d  ce00  1.6
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 048d  ce00  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 04f2  b68b  1.13
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 04f2  b68b  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 8087  0026  1.14
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 8087  0026  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485  1.1
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485  
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:330      libusb_open: Access denied (insufficient permissions)
F: [global] [         0] [ThreadN] getLibusbDeviceMxId:335      libusb device is BOOTED and LIBUSB_ERROR_ACCESS
F: [global] [         0] [ThreadN] getWinUsbMxId:1077   seeking   03e7  f63b  3.20
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485  1.9
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 0bda  0316  1.18
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 0bda  0316  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  f63b  1.20
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  f63b  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 048d  ce00  1.6
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 048d  ce00  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 04f2  b68b  1.13
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 04f2  b68b  
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 8087  0026  1.14
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 8087  0026
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485  1.1
F: [global] [         0] [ThreadN] getWinUsbMxId:1136   candidate 03e7  2485
F: [global] [         0] [ThreadN] getUSBDevices:173    getLibusbDeviceMxId() returned: Access denied (insufficient permissions)
F: [global] [         0] [ThreadN] getUSBDevices:201    inserting found device into result
[2023-05-12 22:28:47.296] [depthai] [warning] Insufficient permissions to communicate with X_LINK_BOOTED device having name "3.20". Make sure udev rules are set
...

These are the Windows Device Manager location paths for the two running Luxonis Devices during the above logs.

# this is the device running in terminal 1
USB\VID_03E7&PID_F63B\1844301031490A1300
# its location paths
PCIROOT(0)#PCI(1400)#USBROOT(0)#USB(20)
ACPI(_SB_)#ACPI(PCI0)#ACPI(XHC_)#ACPI(RHUB)#ACPI(SS04)


# this is the device running in terminal 2
USB\VID_03E7&PID_F63B\14442C10C1A1C6D200
# its location paths
PCIROOT(0)#PCI(1B00)#PCI(0000)#PCI(0200)#PCI(0000)#USBROOT(0)#USB(3)
ACPI(_SB_)#ACPI(PCI0)#ACPI(RP17)#ACPI(PXSX)#ACPI(TBDU)#ACPI(XHC_)#ACPI(RHUB)#ACPI(SS01)

Expected

device on libusb path "3.20" is found using the Win32 apis and its MXID returned

XLink crashes app if any exceptions occur in XLink cpp files

Xlink can easily crash due to the newer C++ code within it yet having no exception handling.

For example, the file usb_host.cpp has a lot of C++ code in it and most of it can throw exceptions. However, there is no exception handling in functions, e.g. getUSBDevices(), to prevent their doomed cascade upward into standard-C code...which causes an app crash/segfault. 💥

Since exceptions can be throw by trivial uses like std::string(...) or container::at(), I recommend that the top-level functions in all cpp files be marked noexcept. Then add try/catch to all needed places to catch any exceptions and return relevant error codes instead. All modern editors like vscode and analyzers provide clear feedback of functions marked noexcept yet calling things which can throw.

Setup

all os, platforms, compilers
XLink in v2.21.2, master, develop, and for many past versions

Repro

Edit usb_host.cpp and insert as the first line of the function getUSBDevices() the following: throw std::runtime_error("crash me");
config and build. I used VS2019.
attach a USB oak device
run tests/color_camera_node_test

Result

App immediately crashes.

Expected

App somehow gracefully fails, in this case it should report it could find no usb devices.

XLinkOpenStream StreamId desynchronization

XLinkOpenStream has an issue where if both host and device try to open streams, streamId can get "desynchronized`.
Example:

The above diagram shows how host tries to open stream A and device stream B, but the results are that both streams now point to same IDs, which results in all data transfers being delivered to wrong locations.

Possible solution is to replace the creation of "unique" stream ID on remote with creating one locally for local conversion between ID<->name while replace using stream name as an unique ID.

Events would then be sent with name instead of ID, but this doesn't diverge too much from current architecture, as stream name has a fixed place in event header already.

	if(refLibusbDeviceByName(addr, &dev) == X_LINK_PLATFORM_SUCCESS){
	break;
	}
	std::this_thread::sleep_for(milliseconds(10));
	} while(steady_clock::now() - t1 < DEFAULT_CONNECT_TIMEOUT);

	if(dev == nullptr) {
	return X_LINK_PLATFORM_DEVICE_NOT_FOUND;
	}

	auto t2 = steady_clock::now();
	do {
	if((res = usb_open_device(dev, &endpoint, h)) == LIBUSB_SUCCESS){
	break;
	}
	std::this_thread::sleep_for(milliseconds(100));
	} while(steady_clock::now() - t2 < DEFAULT_CONNECT_TIMEOUT);

	if(res == LIBUSB_SUCCESS) {
	rc = send_file(h, endpoint, mvcmd, size, bcdusb);
	libusb_release_interface(h, 0);
	libusb_close(h);
	} else {
	if(res == LIBUSB_ERROR_ACCESS) {
	rc = X_LINK_PLATFORM_INSUFFICIENT_PERMISSIONS;
	} else if(res == LIBUSB_ERROR_BUSY) {
	rc = X_LINK_PLATFORM_DEVICE_BUSY;
	} else {
	rc = X_LINK_PLATFORM_ERROR;
	}
	}

	if (dev) {
	libusb_unref_device(dev);

	xLinkDesc_t* link = &availableXLinks[i];
	link->id = id;

	if (XLink_sem_init(&link->dispatcherClosedSem, 0 ,0)) {
	mvLog(MVLOG_ERROR, "Cannot initialize semaphore\n");
	XLINK_RET_ERR_IF(pthread_mutex_unlock(&availableXLinksMutex) != 0, NULL);
	return NULL;
	}

	#if (defined (WINNT) \|\| defined(_WIN32) \|\| defined(_WIN64) )
	#define __attribute__(x)
	#define FUNCATTR_WEAK static
	#else
	#define FUNCATTR_WEAK
	#endif

	TCPIP_rc = getTcpIpDevices(in_deviceRequirements, out_foundDevices, sizeFoundDevices, &numFoundDevices);
	*out_amountOfFoundDevices += numFoundDevices;
	out_foundDevices += numFoundDevices;
	sizeFoundDevices -= numFoundDevices;
	// Found enough devices, return
	if (numFoundDevices >= sizeFoundDevices) {
	return X_LINK_PLATFORM_SUCCESS;
	} else {
	sizeFoundDevices -= numFoundDevices;
	}

	#ifdef __ANDROID__
	// Convert to Android logging enumeration
	enum android_LogPriority logPrio = ANDROID_LOG_DEBUG + (lvl - MVLOG_DEBUG);
	//__android_log_print(logPrio, UNIT_NAME_STR, headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line);
	__android_log_vprint(logPrio, UNIT_NAME_STR, format, args);
	// __android_log_print(logPrio, UNIT_NAME_STR, "%s", ANSI_COLOR_RESET);
	#else
	fprintf(stdout, headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line);
	vfprintf(stdout, format, args);
	fprintf(stdout, "%s\n", ANSI_COLOR_RESET);
	#endif

	#elif defined __shave__
	printf(headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line);
	printf(format, args);
	printf("%s\n", ANSI_COLOR_RESET);

	#endif
	#ifdef __RTEMS__
	}
	else
	{
	printk(headerFormat, mvLogHeader[lvl], UNIT_NAME_STR, timestamp, threadName, func, line);
	vprintk(format, args);
	printk("%s\n", ANSI_COLOR_RESET);

	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;

luxonis / xlink Goto Github PK

xlink's Introduction

XLink

Changes

xlink's People

Contributors

Stargazers

Watchers

Forkers

xlink's Issues

Suggested work

usb_boot() location

usbLinkBootBootloader() location

usbLinkOpen location

Setup

Repo

Result

Expected

Fix

Setup

Repro

Result

Notes

Setup

Repro

Result

Concern -- need feedback

FYI

Setup

Repro

Setup

Repo

Result

Expected

Notes

Setup

Repro

bulk_chunklen

bcdusb

Setup

Repro

Result

Setup

Repro

Result

Expected

Logs

Setup

Repro

Result

First terminal

Second terminal

Expected

Setup

Repro

Result

Expected

Recommend Projects

Recommend Topics

Recommend Org

`usb_boot()` location

`usbLinkBootBootloader()` location

`usbLinkOpen` location