search

keyboardio / Model01-Firmware

The "standard" Keyboardio Model 01 Firmware sketch.
GNU General Public License v3.0
172 stars 302 forks source link

Build error on Arch Linux: variable 'keymaps' with dynamic initialization put into program memory area #53

Closed keidax closed 6 years ago

keidax commented 6 years ago

With some help from this thread I was able to get make flash running, up until I hit this error: 

$ make flash
BOARD_HARDWARE_PATH="/home/gabe/Arduino/hardware" /home/gabe/Arduino/hardware/keyboardio/avr/libraries/Kaleidoscope/bin//kaleidoscope-builder flash
Building output/Model01-Firmware/Model01-Firmware (0.0.0-gv1.22-13-gc7fa) ...
In file included from /home/gabe/Arduino/hardware/keyboardio/avr/libraries/Kaleidoscope/src/Kaleidoscope.h:26,
                 from /home/gabe/src/Model01-Firmware/Model01-Firmware.ino:17:
/home/gabe/src/Model01-Firmware/Model01-Firmware.ino: In function '(static initializers for /home/gabe/src/Model01-Firmware/Model01-Firmware.ino)':
/home/gabe/Arduino/hardware/keyboardio/avr/libraries/Kaleidoscope/src/layers.h:11:13: error: variable 'keymaps' with dynamic initialization put into program memory area
   const Key keymaps[][ROWS][COLS] PROGMEM = { layers };  \
             ^~~~~~~
/home/gabe/src/Model01-Firmware/Model01-Firmware.ino:131:1: note: in expansion of macro 'KEYMAPS'
 KEYMAPS(
 ^~~~~~~
exit status 1
make: *** [/home/gabe/Arduino/hardware/keyboardio/avr/build-tools/makefiles//rules.mk:72: flash] Error 1

I'm not familiar enough with Arduino to guess what might be going wrong here, maybe it's not detecting right board definition?

FWIW I have not made any modifications yet, this is straight from master

noseglasses commented 6 years ago

Can you please post the definition of layers in your sketch.

The KEYMAPS macro must only be passed compile time constant data. Please compare the default sketch. All macros that are used to define keycodes evaluate to compile time constants.

I suspect layers to be something that is not compile time constant. That's propaply why the compiler complains about dynamic initialization.

obra commented 6 years ago

Just at a guess, Arch isn't using the standard Arduino compiler suite (avr-gcc 4.9.x)

As a debugging step, can you download an unpackaged copy of the arduino IDE from arduino.cc and check to see if that will build your firmware correctly?

On Wed, May 23, 2018 at 11:35 PM noseglasses notifications@github.com wrote:

Can you please post the definition of layers in your sketch.

The KEYMAPS macro must only be passed compile time constant data. Please compare the default sketch. All macros that are used to define keycodes evaluate to compile time constants.

I suspect layers to be something that is not compile time constant. That's propaply why the compiler complains about dynamic initialization.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Model01-Firmware/issues/53#issuecomment-391604888, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaNsd3sLqSltmk4AFFxeMMecGwOLuks5t1lSdgaJpZM4ULczi .

keidax commented 6 years ago

Indeed, Arch's avr-gcc package provides version 8.1.0, and building with the downloaded toolchain worked as expected. I didn't realize the version numbers diverged so much. Supporting Arch's version is probably out of the scope of this project?

obra commented 6 years ago

I’d be thrilled to review and probably accept patches to improve portability.

On May 24, 2018, at 8:46 PM, Gabriel Holodak notifications@github.com wrote:

Indeed, Arch's avr-gcc package provides version 8.1.0, and building with the downloaded toolchain worked as expected. I didn't realize the version numbers diverged so much. Supporting Arch's version is probably out of the scope of this project?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

noseglasses commented 6 years ago

It would be interesting to find out, what exactly caused avr-gcc 8.1 to complain. This is likely to fire back as soon as Arduino is going to use later versions of avr-gcc.

I suspect one of the keycode definitions to be a problem.

Finding out, which one it is can be achieved by taking and assigning a keycode to a constexpr, somewhere in the sketch e.g.

auto constexpr test = ShiftToLayer(FUNCTION);

If the compiler complains about the assignment with something like '...is not a constant expression', the respective keycode is one of the culprits.

Assignment to PROGMEM data comes with the same restrictions as with constexpr. Everything must be evaluated at compile time. This means, that e.g. reinterpret_cast cannot be used.

@keidax: Does the error also occur with the default firmware sketch, when compiled with avr-gcc 8.1? If not, could you please post the keymap definition of your sketch.

keidax commented 6 years ago

@noseglasses Yes, the original error was with the default sketch. I'll take a stab at narrowing down the cause this weekend.

keidax commented 6 years ago

@noseglasses It seems the issue might be in the definition of RESTRICT_ARGS_COUNT macro. Trying to compile the results of the preprocessed code gives a message like


/home/gabe/src/Model01-Firmware/Model01-Firmware.ino:140:13: error: temporary of non-literal type '<lambda()>' in a constant expression
             );
             ^
/home/gabe/src/Model01-Firmware/Model01-Firmware.ino:138:15: note: '<lambda()>' is not literal because:
              []{ static_assert(sizeof(const char) == sizeof("" ), "\n" "\n***************************************************************" "\n******************** READ THIS CAREFULLY! *********************" "\n***************************************************************" "\n" "\nFile: " "/home/gabe/src/Model01-Firmware/Model01-Firmware.ino" "\nLine: " "133" "\n" "\nStrange arguments found in invocation of " "KEYMAP_STACKED" "." "\n" "\nPlease make sure to pass exactly " "64" " macro arguments to" "\n" "KEYMAP_STACKED" ". Also make sure that there are no dangling" "\ncommas at the end of the argument list." "\n" "\nThis is the superfluous part at the end of the macro" "\narguments list: \'" "" "\'" "\n" "\n***************************************************************" "\n***************************************************************" "\n***************************************************************" "\n" ); },
               ^
cc1plus: note:   '<lambda()>' is a closure type, which is only literal in C++17 and later
noseglasses commented 6 years ago

Thanks for finding the issue. It's actually one of my own PRs that introduced RESTRICT_ARGS_COUNT. It seems that I accidentally sneaked some c++17 code into Kaleidoscope. Sorry that you had trouble with it. I just found a c++11 compliant workaround but I just have a smartphone with me for the next two weeks. ASAP I will PR a fix.

noseglasses commented 6 years ago

If someone wants to try a possible fix until I (or someone else) submit a PR, here is a short description of the necessary change to macro RESTRICT_ARGS_COUNT in file macro_helpers.h.

The macro basicaly wraps a lambda function that contains a static_assert. 

[]{ static_assert(...) }

The lambda is non-constexpr in c++11. It can be replaced by a constexpr anonymous temporary struct instance.

(struct{static_assert(...)}){}

This solution is fully c++11 compliant and accepted by gcc 8.1.