-
Notifications
You must be signed in to change notification settings - Fork 19
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
dai::Response:*::success
not initialized, some code paths then test garbage mem values
#4
Comments
Do you have an example of this codepath? The case mentioned below does receive the response and fills out this value. Or have you observed otherwise? (eg a response containing a garbage value?) |
Yes, the three lines of code above in my OP. Also, I would like to change the success and errorMsg pair. |
My bad - reread the comment. In this case its not a garbage response (as in correct response that doesn't have the value set).
And in (afaik 2) other places. Also could memset/zeroinit the T response in
and
To not require making a release for bootloader |
Unfortunately that won't completely work. I already checked. 😉 Can't use response = {} or T{}. Because those problem classes have user-defined default constructors which do not initialize the member variables. Therefore the compiler constructed constructor that would do a If you don't want to make a firmware release "soon", then I still recommend I make a PR that does corrects the issue. In parallel, I make a PR in |
Idea 2 - use a macro to know when the header is being compiled for host or the device. I feel like I've seen a define somewhere that does that. For example struct BootApplication : BaseResponse {
// Common
BootApplication() : BaseResponse(BOOT_APPLICATION) {}
// Data
#ifdef(HOST)
uint32_t success = 0;
char errorMsg[64] = {0};
#else
uint32_t success;
char errorMsg[64];
#endif
static constexpr const char* VERSION = "0.0.14";
static constexpr const char* NAME = "BootApplication";
}; Then the device doesn't need new code, while the host gets the correction. |
Sounds good on setting the success then / doing checks - other means would get less nice, quickly. Regarding the host/device idea, possibility, but we currently require that shared commit matches on both core & BL (or FW), so it'd still need an update on the other end. |
Got it. I will
|
For the second point, I think checking the return value of the actual receiving/parsing might be better, where appropriate |
- manually init class vars as workaround for luxonis/depthai-bootloader-shared#4
- fixes luxonis#4 - related to luxonis/depthai-core#347
- manually init class vars as workaround for luxonis/depthai-bootloader-shared#4
- manually init class vars as workaround for luxonis/depthai-bootloader-shared#4
- fixes #4 - related to luxonis/depthai-core#347
- manually init class vars as workaround for luxonis/depthai-bootloader-shared#4
@themarpe, is the underlying bootloader initialization fix released in firmware? |
Yes, can be removed now, if the core is targeting the updated main (which it is), will have that in (& underlying BL was also released for it). Won't modify how previous version of BL's worked, but responses from coming FW will remain structured in same manner as before. |
Hmmm. Is it possible for the old bootloader with this bug to be used with new depthai-core? If that is all true, then I think it better to leave the workaround in place. I can make a PR to update the code comments to not remove it. Do you know which bootloader version includes the fix? I can also list that in the comments. |
Sorry for the delay - rechecked - the BL will respond in the same manner as before, so incoming data will remain the same (therefore unaffected). The means of parsing the incoming data will be the same due to shared code change & core change essentially creating the same outcome - "If eg: So LGTM to continue |
Three classes within
dai::Response:*
have asuccess
member that is later directly tested in branching likeif(resp.success) ...
However, there are multiple codepaths in which
success
is never initialized and therefore holds a random garbage value.Which then leads to failures in branching.
Easy fix. The classes should set a default init of
0
aka false.Setup
main
anddepthai-core
mainRepro
Found during code cleanup I'm in process doing. Here is one example...
The branch which tests
if(resp.success)
https://github.com/luxonis/depthai-core/blob/c49cd08cb1cad7d526ca2c2a2b561f0dcfa7020d/src/device/DeviceBootloader.cpp#L691Expected
Initialize all member fields. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c41-a-constructor-should-create-a-fully-initialized-object
Fix
I have a PR forthcoming, fix for this specific case is easy.
The text was updated successfully, but these errors were encountered: