Add SmartEEPROM Support

[zvecr]

Based on recent discussions, the expected behaviour for enabling SmartEEPROM is slightly different than that proposed within #49.

Main differences are:

• Slightly different EEPROM provisioning
  • 2k vs 4k to be confirmed
• No longer requires running 2 commands
• Forced update to values if different to expected values
  • Allow this to be skipped via a flag

Examples:

mdloader --first --download ~/qmk_firmware/massdrop_alt_default.bin --restart # reconfigure if settings different
mdloader --first --download ~/qmk_firmware/massdrop_alt_default.bin --restart --ignore-eep # ignore any SmartEEPROM config - prints warning if different

[TheZoc]

Nice to see someone else trying to get this feature merged!

I have a few questions/suggestions about the changes:

1. Why was the virtual eeprom size bumped to 2048?
   Considering the lifespan of the flash memory, the smallest block should be used to avoid faster degradation.
   I already thought that 1024 was too much, since the user data is ~36 bytes only, but it could be useful for VIA support if the user decides to use it.

As I side note, I suggest that #defines are avoided inside functions as they can leak outside it easily and could cause future issues.

2. The prompt confuses me: It will be displayed after the keyboard was already accessed.
   It makes me really uncomfortable needing to type anything in a software after the hardware access started - especially as the default setting.
   Also, many users won't have multiple keyboards available to input data after the flashing software has started.
   That's why I previously added a --forceeep command line for it, with no toggle.

I recommend that change to be reverted, as it could be confusing in terms of UX; and dangerous since it requires user intervention after the flash process has already started. IMHO, the SmartEEPROM settings shouldn't be touched unless the user is explicit about it on the command line.

I hope this helps! :)

[zvecr]

For context, a lot of the requested changes are from internal discussions with Drop and myself.

As I side note, I suggest that #defines are avoided inside functions as they can leak outside it easily and could cause future issues.

Done, was just being lazy....

Why was the virtual eeprom size bumped to 2048?

if you dig into the data sheet, going up to 4k of eeprom gives the same wear leveling expectations while giving up the same amount of flash. The value of 2k hasn't been 100% confirmed yet, but the intentions are to support more than the default QMK eeconfig settings.

The prompt confuses me: It will be displayed after the keyboard was already accessed.
It makes me really uncomfortable needing to type anything in a software after the hardware access started - especially as the default setting.
Also, many users won't have multiple keyboards available to input data after the flashing software has started.
That's why I previously added a --forceeep command line for it, with no toggle.
I recommend that change to be reverted, as it could be confusing in terms of UX; and dangerous since it requires user intervention after the flash process has already started. IMHO, the SmartEEPROM settings shouldn't be touched unless the user is explicit about it on the command line.

The change is to try and optimise the end user experience to those users who are less technical. Having multiple commands that might have to be run is going to cause confusion. And blindly changing the value without the users knowledge was deemed to be less than ideal. The compromise was the contents of this PR. A command line option still exists to provide coverage of not wanting to change it, or wanting to avoid interaction.

Theres a meeting later today, so will bring up your concurs. Though theres no guarantee the behaviour will revert to that of your previous PR.

[TheZoc]

For context, a lot of the requested changes are from internal discussions with Drop and myself.

Ah, great to know they're helping :)

Why was the virtual eeprom size bumped to 2048?

if you dig into the data sheet, going up to 4k of eeprom gives the same wear leveling expectations while giving up the same amount of flash.

Ah, yeah, just saw the table on page 655 :)

The change is to try and optimise the end user experience to those users who are less technical. Having multiple commands that might have to be run is going to cause confusion. And blindly changing the value without the users knowledge was deemed to be less than ideal. The compromise was the contents of this PR. A command line option still exists to provide coverage of not wanting to change it, or wanting to avoid interaction.

I understand the reasoning. Still, I still recommend to remove the option for user input during the flashing process.
If the users are less technical, they might try crazy things like plugging and unplugging they keyboard during the flashing process to try to answer the prompt - and there's no way to continue without answering to that prompt. This can leave the hardware in a bad state or cause physical damage: There will be no way to imagine what the user will try to proceed, since it will be waiting indefinitely at that prompt.

Also, less technical users probably won't have multiple keyboards lying around to to try have two keyboards plugged at the same time.

What I suggest is the previous behavior, from #49:

• If the user doesn't specify anything, leave SmartEEPROM setting as is. You might want to detect and display a message if that's a requirement.
• If the user specifies to update the SmartEEPROM setting, display a warning if it's already configured. Do not touch current configuration and provide information on how to force to update it. If not configured, configure as expected.
• If the user specifies to update the SmartEEPROM and specifies to force it to reconfigure, reconfigure the SmartEEPROM.

Of course, change the names of the CLI parameters as it seems correct for you and Massdrop. I'm just suggesting this to avoid potential customer complaints about bricked keyboards (etc.) in the future :)

[zvecr]

If the users are less technical, they might try crazy things like plugging and unplugging they keyboard during the flashing process to try to answer the prompt - and there's no way to continue without answering to that prompt. This can leave the hardware in a bad state or cause physical damage: There will be no way to imagine what the user will try to proceed, since it will be waiting indefinitely at that prompt.

At this point its only performed a read, so wont be any less of a defence against corruption/bicking than the point at which a write is performed on this PR or #49. Both assume it will not be interrupted during the running of write_user_row. The use case of a single keyboard and no way to answer the prompt, is something I think its worth to consider.

[TheZoc]

If the users are less technical, they might try crazy things like plugging and unplugging they keyboard during the flashing process to try to answer the prompt - and there's no way to continue without answering to that prompt. This can leave the hardware in a bad state or cause physical damage: There will be no way to imagine what the user will try to proceed, since it will be waiting indefinitely at that prompt.

At this point its only performed a read, so wont be any less of a defence against corruption/bicking than the point at which a write is performed on this PR or #49. Both assume it will not be interrupted during the running of write_user_row.

I'm not going to be able to cover all the possible situations, but I'll try to describe a simple one that might be food for thought.

Assume non-technical user with 2 keyboards. Keyboard 1 (KB1) is a CTRL, Keyboard 2 (KB2) is a unknown keyboard that runs on similar chip, but has unknown capabilities and quirks at this time.

1. User plugs KB1 and set to DFU mode. Use a mouse to paste the command line to start the firmware update process.
2. User sees the prompt and can't proceed.
3. User unplugs KB1 and plug KB2. Types the answer in a notepad-like application and copy with the mouse, to proceed with the flashing process.
4. User plugs back KB1.
5. User sets KB1 to DFU mode.
6. User attempts to proceed with the flashing utility.

There's a fair number of problems that can happen in that.

• The address of KB1 COM port has changed.
• Once the keyboard COM port is detected, it is never validated again. It will try to write into an invalid memory address.
• In the example above, KB2 was plugged after KB1 was removed - but it wasn't unplugged later, possibly messing up device memory addresses.
• If KB2 has a different DFU procedure, it could also create a COM port with the same address as previously taken by KB1. In that case, the flashing utility might try to overwrite it, possibly bricking the keyboard with invalid data.

Especially when flashing something only recoverable by hardware intervention (e.g. JTAG or some other invasive procedure), I suggest to be extra careful and always assume the worst. People can do some really weird stuff 😢

The use case of a single keyboard and no way to answer the prompt, is something I think its worth to consider.

😄