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
Correct Periph compilation if Check Firmware is not enabled #26543
Correct Periph compilation if Check Firmware is not enabled #26543
Conversation
it is enabled by default on periph. |
@@ -127,7 +127,9 @@ void AP_Periph_FW::init() | |||
logger.init(g.log_bitmask, log_structure, ARRAY_SIZE(log_structure)); | |||
#endif | |||
|
|||
#if AP_CHECK_FIRMWARE_ENABLED |
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.
I think we should assert that AP_CHECK_FIRMWARE_ENABLED is enabled in AP_Periph. It is unsafe to have a peripheral without it. We need the crc checking or the user can brick their board
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.
I've added it as a separate default in defaults_periph.h
to make sure it's turned on.
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.
why not an assert? periph just won't work properly without it
the typedef won't exist in that case
7eb0339
to
72bc292
Compare
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.
There's no compiler output change for peripheral firmware after this PR.
@@ -127,7 +127,9 @@ void AP_Periph_FW::init() | |||
logger.init(g.log_bitmask, log_structure, ARRAY_SIZE(log_structure)); | |||
#endif | |||
|
|||
#if AP_CHECK_FIRMWARE_ENABLED |
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.
I've added it as a separate default in defaults_periph.h
to make sure it's turned on.
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.
i'd much prefer a #error and no #if
app_descriptor is guarded behing AP_CHECK_FIRMWARE_ENABLED, and that's only enabled by default when AP_OPENDRONEID_ENABLED is true.
Given the app descriptor seems to contain useful information useful in places where AP_OPENDRONEID_ENABLED is false, this is maybe a bit unfortunate.