Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] EECONFIG_RGB_MATRIX_SPEED is unused #7916

Closed
robhaswell opened this issue Jan 16, 2020 · 3 comments
Closed

[Bug] EECONFIG_RGB_MATRIX_SPEED is unused #7916

robhaswell opened this issue Jan 16, 2020 · 3 comments
Labels

Comments

@robhaswell
Copy link
Contributor

@robhaswell robhaswell commented Jan 16, 2020

EECONFIG_RGB_MATRIX_SPEED is defined but never read. This is less of a bug report and more of a manual linting error.

Describe the Bug

EECONFIG_RGB_MATRIX_SPEED appears in the following lines:

It appears that this value is never read except to store to the eeprom - it is never read out of the eeprom. Additionally the animation speed is now a property of the rgb_matrix_config struct. I believe this is a leftover from the RGB matrix overhaul and should be removed.

@robhaswell

This comment has been minimized.

Copy link
Contributor Author

@robhaswell robhaswell commented Jan 16, 2020

There is also a comment // TODO: Combine these into a single word and single block of EEPROM which seems redundant if these references are removed.

@fauxpark

This comment has been minimized.

Copy link
Member

@fauxpark fauxpark commented Jan 17, 2020

This has to do with the size of the RGB Matrix config struct. From rgb_matrix_types.h:

typedef union {
    uint32_t raw;
    struct PACKED {
        uint8_t enable : 2;
        uint8_t mode : 6;
        HSV     hsv;
        uint8_t speed;  // EECONFIG needs to be increased to support this
    };
} rgb_config_t;

enable and mode make up a single byte (the numbers after them denote the amount of bits allocated). hsv is three bytes, and of course speed is a byte. That's five bytes all up.

But wait... this is supposed to fit into the four bytes of a uint32! The RGB Matrix code gets around this by using eeprom_read/update_block() and passing in the size of the struct, instead of eeprom_read/update_dword(). Thus EECONFIG_RGB_MATRIX_SPEED is defined at EEPROM address 32, even though there are no functions that actually operate on just that specific byte.

As for the comment below it, that is a similar situation which relates to this PR: #6110
wherein some new entries were added to the keymap_config struct, however all the bits of the raw byte were already allocated, so it had to be split up into two non-consecutive bytes in EEPROM.

@robhaswell

This comment has been minimized.

Copy link
Contributor Author

@robhaswell robhaswell commented Jan 17, 2020

Ah I see, thanks for explaining.

@robhaswell robhaswell closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.