search

system76 / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
93 stars 33 forks source link

Use EEPROM to store RGB parameters #13

Closed jackpot51 closed 3 years ago

jackpot51 commented 3 years ago

This increments the EEPROM version which will force a reset of EEPROM data. I have tested all changes in the configuration are stored, including all RGB modes, speeds, colors per layer and per key settings.

At some point we may want a save command to prevent unnecessary EEPROM wear.

ids1024 commented 3 years ago

At some point we may want a save command to prevent unnecessary EEPROM wear.

So as currently implemented, this will cause repeated EEPROM writes every time the brightness or speed slider is moved, and every move around the color wheel? Presumably that's definitely something we ultimately won't want to do to prevent wear (not sure how that impacts responsiveness as well.)

ids1024 commented 3 years ago

(not sure how that impacts responsiveness as well.)

Dragging around the color wheel (for instance) seems unusably unresponsive. Well at least that means it won't be wearing the eeprom too much with repeated writes...

The code looks fine otherwise, and it seems to persist settings correctly.

So I guess make this another API call? Not sure exactly when we would want it to be called. The other solution for responsiveness would be to only write the changed byte to the eeprom, but that then becomes a concern for wear.

jackpot51 commented 3 years ago

@ids1024 exactly. The issue right now is we need another command to save the settings to EEPROM. I can make that command and expose it in ectool, if you can do the work to call it in the configurator

jackpot51 commented 3 years ago

The new command and ectool implementation are available now

ids1024 commented 3 years ago

Seems good.

According to time sudo system76_ectool --access=hid led_save this takes quite close to 1 second. The Configurator should run these commands in the background without blocking the UI thread. Still working on the best way to do that while also not queuing up arbitrary numbers of commands writing to the same property.

But in any case, this should be good if a save command fits with the design we'll ultimately want.

jackpot51 commented 3 years ago

@ids1024 it does take longer than expected. I can probably make it faster by detecting which bytes need to be written.

jackpot51 commented 3 years ago

@ids1024 latest commit should improve performance of saves by only saving what is changed. It could still take a whole second if everything changes.