LED Matrix: Task system

[fauxpark]

Description

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[firetech]

@fauxpark Is something missing here? In trying to pick #9657 back up, it took me a few hours to realize that the reason I only got the lights to work some times (specifically after flashing back and forth from an older firmware) was that led_matrix_eeconfig.flags was getting reset on every reboot due to it now being 8 bytes long. eeconfig.h only gives it 6 bytes of storage...

[fauxpark]

I'm not sure what you mean, it works fine for me. The LED Matrix code is almost identical to RGB Matrix now, so for the most part, whatever bugs it has have been duplicated to LED Matrix.

[firetech]

sizeof(led_matrix_eeconfig) returns 8 (bytes) now, but tmk_core/common/eeconfig.h has only reserved bytes 28-33 (6 bytes) for it. The latter two bytes end up overlapping EECONFIG_KEYMAP_UPPER_BYTE and going beyond EECONFIG_SIZE, respectively. Slightly undefined behaviour, which is probably why it can work for you but not for me. I see now that RGB Matrix seems to have the same issue, which is what the // EECONFIG needs to be increased to support this comment in both led_matrix_types.h and rgb_matrix_types.h refers to.

Anyway, commenting out uint16_t reserved; from the led_eeconfig_t typedef (there to match up with HSV hsv; in rgb_config_t) "solved" my issue, so I'm quite sure my issue was caused by the eeconfig size.

[fauxpark]

rgb_config_t (and led_eeconfig_t) is actually only 5 bytes long, because enable and mode share the first byte and the struct is packed. But you are correct that with the RGB/LED Matrix split code introduced into develop (#11055), it now overflows the allotted block by one byte. This will probably need to be fixed before develop merges into master at the end of the month. @XScorpion2 ?

EDIT: Now I understand why it's working for me - I'm not testing on a split board (nor do I have one to test on).

[firetech]

Hmm. sizeof(led_matrix_eeconfig) returns 8 on my ergodox_infinity. Is that just a "feature" of packed struts or is it actually not getting packed in my build?

[fauxpark]

I have a theory... could you try removing the _MSC_VER ifdefs around the pragma packs? Those lines should be forcing the structs to be aligned on a 1-byte boundary, but since we are never going to be compiling on the Microsoft C compiler (AFAIK QMK only really supports GNU GCC), they won't be taken into consideration. Thus the alignment, at least for led_eeconfig_t, will probably be 2 bytes, due to the uint16_t reserved field.

I'm not sure why those ifdefs are there, GCC seems to support these pragmas since at least 4.4.4.

[firetech]

Seems to have solved it partly... sizeof now returns 6, and the flags no longer seem to get overwritten, but they don't seem to be stored properly either. If I don't have led_matrix_set_flags(LED_FLAG_ALL) in my keyboard init, the lights shortly blink on boot and then don't work.

EDIT: previously just a set_flags in the init didn't seem to be enough, it seemed like I had to set it more often (with reservation for mistakes due to slight sleep deprivation at the time).

[XScorpion2]

rgb_config_t (and led_eeconfig_t) is actually only 5 bytes long, because enable and mode share the first byte and the struct is packed. But you are correct that with the RGB/LED Matrix split code introduced into develop (#11055), it now overflows the allotted block by one byte. This will probably need to be fixed before develop merges into master at the end of the month. @XScorpion2 ?

EDIT: Now I understand why it's working for me - I'm not testing on a split board (nor do I have one to test on).

enable is 2 bits, but ya, was 5 bytes in total, now 6 bytes with that change in development. Eeprom was updated in that branch as well to account for the increased size.

[fauxpark]

Looks like I can't count, or read...

[fauxpark]

@firetech could you also try splitting reserved into two uint8_ts instead of removing the ifdefs?

[firetech]

@firetech could you also try splitting reserved into two uint8_ts instead of removing the ifdefs?

Same result, 8 bytes with the ifdefs, 6 without.

[firetech]

Hmm... I've noticed that EEPROM_SIZE defaults to 32 on some keyboards, mine included (#if defined(K20x) in tmk_core/common/chibios/eeprom_teensy.c). If I try to increase that to 64 (which seems to be the next supported step based on the EEESIZE defines on the lines below), my keyboard hangs during boot... :/

(Due to how eeprom_read_block is implemented for this chip, it just silently falls back to reading bytes 28-32 and calls it a day. No wonder flags is getting zeroed...)

EDIT: apparently, I can't read... Literally immediately after #define EEPROM_SIZE 32:

/*
    ^^^ Here be dragons:
        NXP AppNote AN4282 section 3.1 states that partitioning must only be done once.
        Once EEPROM partitioning is done, the size is locked to this initial configuration.
        Attempts to modify the EEPROM_SIZE setting may brick your board.
*/

Soooo, I guess I'm stuck with a 32 byte EEPROM (at least my board wasn't bricked).