Fix build failures caused by #12947. #15019
Conversation
|
Looks good to me, tested to fix build failure on my boards. |
Looking at this now |
There was a problem hiding this comment.
Why does moving the #if guards outside of the kinetis_hsrun_disable() function help with build error? I would have assumed that for non-MK66F18 boards, the functions compile to an empty function and the compiler removes them entirely as an optimization?
Anyway, the change looks correct except for one place where I added a comment.
| // FlexRAM is configured as traditional RAM | ||
| // We need to reconfigure for EEPROM usage | ||
|
|
||
| # if defined(MK66F18) | ||
| kinetis_hsrun_disable(); | ||
| FTFL->FCCOB0 = FTFE_FCCOB0_CCOBn_SET(0x80); // PGMPART = Program Partition Command |
There was a problem hiding this comment.
FCCOB0 needs to be set outside of MK66F18, too, so this #if directive is too broad.
I only changed FTFL->FCCOB0 = 0x80; to FTFL->FCCOB0 = FTFE_FCCOB0_CCOBn_SET(0x80); in PR #12947
There was a problem hiding this comment.
Why does moving the #if guards outside of the kinetis_hsrun_disable() function help with build error?
Because those functions are unused on other platforms, and warnings occur as a result... which are treated as errors.
There was a problem hiding this comment.
I only changed FTFL->FCCOB0 = 0x80; to FTFL->FCCOB0 = FTFE_FCCOB0_CCOBn_SET(0x80); in PR #12947
FTFE_FCCOB0_CCOBn_SET does not exist on non-MK66F18 boards, and ends up failing the build.
There was a problem hiding this comment.
On develop CI:
Compiling: tmk_core/common/chibios/eeprom_teensy.c
tmk_core/common/chibios/eeprom_teensy.c:105:12: error: 'kinetis_hsrun_enable' defined but not used [-Werror=unused-function]
static int kinetis_hsrun_enable(void) {
^~~~~~~~~~~~~~~~~~~~
tmk_core/common/chibios/eeprom_teensy.c:47:12: error: 'kinetis_hsrun_disable' defined but not used [-Werror=unused-function]
static int kinetis_hsrun_disable(void) {
^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
...and:
Compiling: tmk_core/common/chibios/eeprom_teensy.c
tmk_core/common/chibios/eeprom_teensy.c: In function 'eeprom_initialize':
tmk_core/common/chibios/eeprom_teensy.c:237:24: error: implicit declaration of function 'FTFE_FCCOB0_CCOBn_SET' [-Werror=implicit-function-declaration]
FTFL->FCCOB0 = FTFE_FCCOB0_CCOBn_SET(0x80); // PGMPART = Program Partition Command
^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
There was a problem hiding this comment.
I only changed FTFL->FCCOB0 = 0x80; to FTFL->FCCOB0 = FTFE_FCCOB0_CCOBn_SET(0x80); in PR #12947
FTFE_FCCOB0_CCOBn_SET does not exist on non-MK66F18 boards, and ends up failing the build.
Gotcha. We should just change it back to FTFL->FCCOB0 = 0x80 then. The macro does no changes, I only used it for descriptiveness.
There was a problem hiding this comment.
FCCOB0 needs to be set outside of MK66F18, too, so this #if directive is too broad.
I only changed FTFL->FCCOB0 = 0x80; to FTFL->FCCOB0 = FTFE_FCCOB0_CCOBn_SET(0x80); in PR #12947
Are we missing a header or something, then?
There was a problem hiding this comment.
The macro is defined in lib/chibios-contrib/os/common/ext/CMSIS/KINETIS/MK66F18.h, but not for other devices. Not sure why.
As i said, easiest to change it back to just 0x80.
| static int kinetis_hsrun_disable(void) { | ||
| #if defined(MK66F18) | ||
| static int kinetis_hsrun_disable(void) { |
There was a problem hiding this comment.
Maybe do this instead:
#if defined(MK66F18)
static int kinetis_hsrun_disable(void) {
// ... the real implementation
}
#else
static inline int kinetis_hsrun_disable(void) { return 0; }
#endif(static inline does not cause an error if the function ends up unused)
There was a problem hiding this comment.
(static inline does not cause an error if the function ends up unused)
Fair point. Probably easier that way.
There was a problem hiding this comment.
Swapped the definitions to static inline, still broken with FTFE_FCCOB0_CCOBn_SET.
|
@stapelberg @sigprof should be good to go, now. Would be great for confirmation! Thanks! |
Looks good, thanks for the fix! |
Description
#12947 caused a whole host of problems with other boards. This fixes their build errors, and likely causes issues for boards that picked up new functionality from the previous PR, most likely around partitioning.
Ping @stapelberg -- you'll need to confirm the fixes, otherwise I'll end up having to revert the original changes entirely.
Types of Changes
Checklist