-
Notifications
You must be signed in to change notification settings - Fork 154
Depthai USB Support #1390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Depthai USB Support #1390
Conversation
bugbot run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: USB Fallback Missing Error Handling
The createSession()
USB fallback implementation in DeviceGate.cpp
is missing an error check after reading the USB response header. Unlike other USB methods in the file, it does not verify request.RequestNum == RESPONSE_ERROR
. This allows the code to proceed with memory allocation and JSON parsing using potentially invalid request.RequestSize
or error data, which can lead to memory corruption, crashes, or incorrect behavior.
src/device/DeviceGate.cpp#L237-L244
depthai-core/src/device/DeviceGate.cpp
Lines 237 to 244 in fa9efd6
XLinkPlatformGateRead(&request, sizeof(request)); | |
char *respBuffer = new char[request.RequestSize + 1]; | |
XLinkPlatformGateRead(respBuffer, request.RequestSize); | |
respBuffer[request.RequestSize]= '\0'; | |
auto resp = nlohmann::json::parse(respBuffer); | |
delete[] respBuffer; | |
spdlog::debug("DeviceGate createSession response: {}", resp.dump()); |
Bug: USB Firmware Upload Fails JSON Binary Handling
The USB communication path incorrectly embeds the firmware package (a std::vector<uint8_t>
) directly into a JSON object. Unlike the HTTP path which uses multipart form data for binary uploads, JSON does not natively support raw binary data. This format mismatch will prevent the server from correctly processing the firmware package.
src/device/DeviceGate.cpp#L260-L266
depthai-core/src/device/DeviceGate.cpp
Lines 260 to 266 in fa9efd6
nlohmann::json uploadFwpBody = {{"sessionId", sessionId}, | |
{"file", package}}; | |
request.RequestNum = UPLOAD_FWP; | |
request.RequestSize = uploadFwpBody.dump().size(); | |
XLinkPlatformGateWrite(&request, sizeof(USBRequest_t)); | |
XLinkPlatformGateWrite((void*)uploadFwpBody.dump().c_str(), uploadFwpBody.dump().size()); |
Comment bugbot run
to trigger another review on this PR
Was this report helpful? Give feedback by reacting with 👍 or 👎
src/device/DeviceGate.cpp
Outdated
return false; | ||
} | ||
|
||
char *respBuffer = new char[request.RequestSize + 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use C++ here - eg a vector /w reserve or resize and pass its "data()" to XLink to not have to manually new & delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TheMutta!
I'm thinking if I'd be safer here to define an interface (pure virtual class) and then do two implementations, so we have compile-time safety to a degree that we implemented all current (and potential future) calls to gate.
What do you think?
cmake/depthaiDependencies.cmake
Outdated
XLink | ||
GIT_REPOSITORY https://github.com/luxonis/XLink.git | ||
GIT_TAG 87785828fabdb1718760bb0a044405d5bbfbb3a2 | ||
GIT_TAG d46c049e30d2a56f7bbec2e25e7e2fa5c09ec6d0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we cross link the XLink PR in this PRs description? Thanks!
src/device/DeviceGate.cpp
Outdated
throw std::invalid_argument("Gate only supports RVC3 and RVC4 platforms"); | ||
} | ||
this->platform = deviceInfo.platform; | ||
this->platform =deviceInfo.platform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace change
src/device/DeviceGate.cpp
Outdated
struct USBRequest_t { | ||
uint16_t RequestNum; | ||
uint32_t RequestSize; | ||
}__attribute__((packed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this cross platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2c, can just skip packed by making both uint32_t or swapping them around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely cross platform
shared/depthai-shared
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be removed
src/device/DeviceGate.cpp
Outdated
request.RequestSize = 0; | ||
XLinkGateWrite(&request, sizeof(USBRequest_t)); | ||
|
||
XLinkGateRead(&request, sizeof(request)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue with Gate being in an "wrong state / timeout"
Pass a timeout explicitly here, instead of using a default.
Align the timeout, with Gate impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pin down "who" retries in this case when Gate isn't yet available
Co-authored-by: TheMarpe <[email protected]>
Co-authored-by: TheMarpe <[email protected]>
Purpose
This branch adds the ability to Depthai to communicate with Gate via USB through XLink
Specification
All the API calls in the DepthaiGate class use USB communication if TCP/IP is not available
None / not applicable
Dependencies & Potential Impact
All the dependencies are already integrated in XLink
Deployment Plan
The changes don't affect the normal functioning via TCP/IP and don't break existing features, they can be rolled out easily.
Testing & Validation
The branch was tested by using all the API calls, which all completed successfully.