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

Add support for ISSI drivers on both sides of a split keyboard #13842

Merged
merged 21 commits into from
Nov 1, 2021

Conversation

vladkvit
Copy link
Contributor

@vladkvit vladkvit commented Aug 1, 2021

When a split keyboard has a driver (such as the IS31FL3733) on each side of the split, this diff allows for full RGB animation support on both sides.

Description

Gotchas. This touches the "rgb_matrix" code and not the "led_matrix" code.
I tested this on my own split keyboard with a driver per side.

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).

Sorry, something went wrong.

@drashna
Copy link
Member

drashna commented Aug 1, 2021

Could this also be applied to the LED Matrix code?

@drashna drashna requested review from tzarc, a team and drashna August 1, 2021 21:17
@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 1, 2021

Could this also be applied to the LED Matrix code?

Probably. The two codebases seem quite similar.

@drashna
Copy link
Member

drashna commented Aug 1, 2021

Could this also be applied to the LED Matrix code?

Probably. The two codebases seem quite similar.

They are. Intentionally.

Also, is this dependent on i2c communication, can it be used with serial?

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 1, 2021

Could this also be applied to the LED Matrix code?

Probably. The two codebases seem quite similar.

They are. Intentionally.

Also, is this dependent on i2c communication, can it be used with serial?

This diff assumes serial as split protocol. Before this diff, RGB_SPLIT allowed the slave side to control I2C for ws2812 LEDs. This diff only extends the indexing functionality.

For I2C split protocol, I think the recent dual driver diff would make a split RGB keyboard work.

@drashna
Copy link
Member

drashna commented Aug 1, 2021

Okay, just wanted to make sure.

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 1, 2021

Some more gotchas:

  • The macro name could be better, would like feedback
  • It would be great if split WS2812 functionality was confirmed to still work
  • Style, should there be a semicolon at the end of the macro call?

@@ -213,6 +232,17 @@ typedef struct {
void (*flush)(void);
} rgb_matrix_driver_t;

#if defined(RGB_MATRIX_SPLIT)
# define RGB_MATRIX_FINISHED_ALL_LEDS \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be better as a function rather than a define, to keep filesize down (especially on AVR...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of potential issues. One is that the function definition changes depending on the define (it may or may not need k_rgb_matrix_split). Not sure how to go around that.
Second issue is that I don't know if inlining would work, if performance (over file size) is desired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure something like this would work fine:

bool rgb_matrix_finished_all_leds(void) {
#if defined(RGB_MATRIX_SPLIT)
    if (is_keyboard_left()) {
        return led_max < k_rgb_matrix_split[0];
    } else {
        return led_max < DRIVER_LED_TOTAL;
    }
#else
    return led_max < DRIVER_LED_TOTAL;
#endif
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@drashna
Copy link
Member

drashna commented Aug 3, 2021

For the docs, it would also be worth noting if both sides need to use the same address, or different addresses for the ISSI controller, and the like.

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 4, 2021

Could this also be applied to the LED Matrix code?

I can apply the changes to the LED matrix code as well (to avoid code divergence), but I don't know how to test it.

@drashna
Copy link
Member

drashna commented Aug 4, 2021

Could this also be applied to the LED Matrix code?

I can apply the changes to the LED matrix code as well (to avoid code divergence), but I don't know how to test it.

There are a few of us with ergodox infinity's that can test it out, actually. IIRC, @fauxpark does, as do I.

@fauxpark
Copy link
Member

fauxpark commented Aug 4, 2021

I don't, actually, just one of these: https://learn.adafruit.com/i31fl3731-16x9-charliplexed-pwm-led-driver

@drashna
Copy link
Member

drashna commented Aug 5, 2021

Compiling with ws2812:

Compiling: quantum/rgb_matrix/rgb_matrix_drivers.c                                                 quantum/rgb_matrix/rgb_matrix_drivers.c: In function 'setled':
quantum/rgb_matrix/rgb_matrix_drivers.c:228:36: error: 'index' undeclared (first use in this function)
         if (!is_keyboard_left() && index >= k_rgb_matrix_split[0]){
                                    ^~~~~
quantum/rgb_matrix/rgb_matrix_drivers.c:228:36: note: each undeclared identifier is reported only once for each function it appears in
quantum/rgb_matrix/rgb_matrix_drivers.c:228:45: error: 'k_rgb_matrix_split' undeclared (first use in this function); did you mean 'rgb_matrix_init'?
         if (!is_keyboard_left() && index >= k_rgb_matrix_split[0]){
                                             ^~~~~~~~~~~~~~~~~~
                                             rgb_matrix_init
quantum/rgb_matrix/rgb_matrix_drivers.c:235:34: error: 'return' with a value, in function returning void [-Werror]
     rgb_matrix_ws2812_array[i].r = r;
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
quantum/rgb_matrix/rgb_matrix_drivers.c:226:20: note: declared here
 static inline void setled(int i, uint8_t r, uint8_t g, uint8_t b) {
                    ^~~~~~

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 5, 2021

Compiling with ws2812:

Compiling: quantum/rgb_matrix/rgb_matrix_drivers.c                                                 quantum/rgb_matrix/rgb_matrix_drivers.c: In function 'setled':
quantum/rgb_matrix/rgb_matrix_drivers.c:228:36: error: 'index' undeclared (first use in this function)
         if (!is_keyboard_left() && index >= k_rgb_matrix_split[0]){
                                    ^~~~~
quantum/rgb_matrix/rgb_matrix_drivers.c:228:36: note: each undeclared identifier is reported only once for each function it appears in
quantum/rgb_matrix/rgb_matrix_drivers.c:228:45: error: 'k_rgb_matrix_split' undeclared (first use in this function); did you mean 'rgb_matrix_init'?
         if (!is_keyboard_left() && index >= k_rgb_matrix_split[0]){
                                             ^~~~~~~~~~~~~~~~~~
                                             rgb_matrix_init
quantum/rgb_matrix/rgb_matrix_drivers.c:235:34: error: 'return' with a value, in function returning void [-Werror]
     rgb_matrix_ws2812_array[i].r = r;
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
quantum/rgb_matrix/rgb_matrix_drivers.c:226:20: note: declared here
 static inline void setled(int i, uint8_t r, uint8_t g, uint8_t b) {
                    ^~~~~~

Hm, what was your compile command (keyboard)? So that I can replicate.

@drashna
Copy link
Member

drashna commented Aug 5, 2021

It's the crkbd. Make sure you compile with my keymap. It doesn't have rgb matrix enabled by default, but it is enabled on mine.

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 5, 2021

It's the crkbd. Make sure you compile with my keymap. It doesn't have rgb matrix enabled by default, but it is enabled on mine.

Just pushed a fix. It now compiles with the default keymap + RGB_SPLIT_ENABLE. I wasn't sure which keymap was yours; still not fully confident in the WS2812 logic either.

@drashna
Copy link
Member

drashna commented Aug 5, 2021

My keymap isn't actually in the crkbd folder, it's in a community layout, but it's drashna.

And yeah, that seems to fix it. And can verify that it works on the corne. Will probably test on the infinity later this week.

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 5, 2021

Nice. Out of curiosity, if you had RGB_ENABLE before, what was the behavior before this diff? Or did you have custom code for that?

Infinity -> I haven't applied the changes to the led_matrix code.

@firetech
Copy link
Contributor

firetech commented Aug 5, 2021

I can also test on Infinity if needed.

However, sorry if I'm being dense, but I don't really understand (or see) what this does in comparison with the existing RGB_MATRIX_SPLIT implementation in rgb_matrix_set_color (and LED_MATRIX_SPLIT/led_matrix_set_value)? I mean, I have (led_matrix) animations spanning both halves on my Infinity (which uses one IS31FL3731 per half) already?

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 6, 2021

It looks like Ergodox Infinity is currently using custom code in conjunction with LED_MATRIX_SPLIT.

The diff allows for 1) no custom code 2) not just symmetric keyboards 3) having proper animations, vs symmetric brightness values on both sides.

@firetech
Copy link
Contributor

firetech commented Aug 6, 2021

It looks like Ergodox Infinity is currently using custom code in conjunction with LED_MATRIX_SPLIT.

What are you referring to? I ported Ergodox Infinity to led_matrix, and I don't remember writing any such custom code. 😛

Also, I have working animations that are running waves and ripples etc. across both halves, not just symmetric ones...

That said, allowing for non-symmetric keyboards sounds like a nice feature. 😃

@vladkvit
Copy link
Contributor Author

vladkvit commented Aug 6, 2021

It looks like Ergodox Infinity is currently using custom code in conjunction with LED_MATRIX_SPLIT.

What are you referring to? I ported Ergodox Infinity to led_matrix, and I don't remember writing any such custom code. 😛

Also, I have working animations that are running waves and ripples etc. across both halves, not just symmetric ones...

That said, allowing for non-symmetric keyboards sounds like a nice feature. 😃

Custom code - fair enough. Non-symmetric brightness - maybe I'm just remembering the middle stages of trying to get it to work for my non-symmetrical keyboard.

The idea is to have g_is31_leds specify parameters for all LEDs on both sides of the board, and to fetch the correct one.

@stale
Copy link

stale bot commented Sep 21, 2021

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@vladkvit
Copy link
Contributor Author

vladkvit commented Sep 26, 2021

ping @drashna @fauxpark

I updated the code with changes to LED_MATRIX. The LED_MATRIX changes compile but are not tested. I can try modifying my own keyboard from RGB_MATRIX into an LED_MATRIX to see if it works, but would appreciate you taking a look.

@vladkvit vladkvit requested a review from fauxpark September 26, 2021 18:22
docs/feature_rgb_matrix.md Outdated Show resolved Hide resolved
docs/feature_led_matrix.md Outdated Show resolved Hide resolved
docs/feature_led_matrix.md Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Ryan <fauxpark@gmail.com>
@vladkvit vladkvit requested a review from fauxpark September 26, 2021 18:33
@tzarc tzarc merged commit a29ca1e into qmk:develop Nov 1, 2021
ptrxyz pushed a commit to ptrxyz/qmk_firmware that referenced this pull request Apr 9, 2022
…3842)

* Gets RGB working on a split keyboard with IS31FL3733. Currently needs small tweak to re-enable WS2812

* Added helper function

* Trying to integrate the function

* Moved functionality into a macro

* Swapped conditional for a macro everywhere

* Tidying up

* More code cleanup

* Documentation updates

* Fixed formatting via linter

* Switching to a function from a macro

* Fixed compile error

* Fixing WS2812 behavior. UNTESTED.

* Updated documentation about the driver addresses.

* Fixed code for WS2812

* Trying to add in LED_MATRIX support

* Updated effects for LED matrix

* Updated third-party effect defines.

* Ran format-c on modified files

* Apply suggestions from code review

Co-authored-by: Ryan <fauxpark@gmail.com>

* Move to static inline. Avoids issues with gcc v8+

* Move helper function for LED_matrix to static inline to avoid issues with gcc v8+

Co-authored-by: Vlad Kvitnevskiy <vladkvit@outlook.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants