search

keyboardio / Kaleidoscope

Firmware for Keyboardio keyboards and other keyboards with AVR or ARM MCUs.
http://keyboard.io
GNU General Public License v3.0
759 stars 258 forks source link

large emulated EEPROM sizes locks up firmware #685

Open mattvenn opened 5 years ago

mattvenn commented 5 years ago

Describe the bug The Raise keyboard uses the SAMD21G microcontroller and the Dygma fork of the SAMD Arduino framework: https://github.com/Dygmalab/ArduinoCore-samd/tree/algernons-hacks

The SAMD21 doesn't have EEPROM like the 32U4, so we are using a FlashStorage library to implement this: https://github.com/cmaglie/FlashStorage And setting the EEPROM_EMULATION_SIZE definition in boards.txt.

Using the fuses I have reserved 16k for emulated EEPROM, however I run into problems at EEPROM_EMULATION_SIZE settings larger than 8k.

To Reproduce

Change EEPROM_EMULATION_SIZE to 16384 or 16383. In both cases the firmware won't boot and doesn't establish USB connection.

I have compiled a simple test program that just prints out EEPROM.length() on the USB Serial and omits the Kaleidoscope libraries. This works as expected with a setting of 16k.

algernon commented 5 years ago

Hrm, this is interesting, because FlashStorage appears to be using 32-bit uints, and even in the Kaleidoscope bridge, we're still at uint16_t, while the 8k suggests an int16_t somewhere.

algernon commented 5 years ago

I think I have an idea why this happens! Most plugins that use EEPROM use an uint16_t to store the starting address of their eeprom slice. If on samd, the address is relative to the start of flash, and not relative to the start of the emulated eeprom, then it's quite easy to overflow that uint16_t... if we have a firmware that's around 8k in size, that'd explain the problem.

What's the size of the firmware you were testing with @mattvenn?

If my suspicion is correct, then we can fix this by introducing a - hardware specific - storage_offset_t, which would be uint16_t for 32u4, and uint32_t for SAMD, for example.

mattvenn commented 4 years ago

my firmware is about 17% with an 8k eeprom emulation turned on. Sounds plausible. What would be a fast way to test this? Just change all the uint16_t to uint32_t in the plugins I'm using?

On Sat, 14 Sep 2019 at 12:10, Gergely Nagy notifications@github.com wrote:

I think I have an idea why this happens! Most plugins that use EEPROM use an uint16_t to store the starting address of their eeprom slice. If on samd, the address is relative to the start of flash, and not relative to the start of the emulated eeprom, then it's quite easy to overflow that uint16_t... if we have a firmware that's around 8k in size, that'd explain the problem.

What's the size of the firmware you were testing with @mattvenn https://github.com/mattvenn?

If my suspicion is correct, then we can fix this by introducing a - hardware specific - storage_offset_t, which would be uint16_t for 32u4, and uint32_t for SAMD, for example.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope/issues/685?email_source=notifications&email_token=AAE223FNCRWZCCXH4VWEWHDQJS2BFA5CNFSM4IWO7RTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6WY3JQ#issuecomment-531467686, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE223DXOWIOOPFI7AA4M7LQJS2BFANCNFSM4IWO7RTA .

-- Matthew Venn web mattvenn.net twitter @matthewvenn https://twitter.com/matthewvenn

algernon commented 4 years ago

What would be a fast way to test this? Just change all the uint16_t to uint32_t in the plugins I'm using?

That should be enough to confirm the suspicion, yes. Or you can try with a very minimal firmware, and increase the EEPROM size slowly, just to rule out that strangely round 8k... if we can decrease firmware size, and up the eeprom size, that also suggests that my suspicion is correct.