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

Fix serial backlight fix breaking #3683

Merged
merged 2 commits into from
Sep 28, 2018
Merged

Fix serial backlight fix breaking #3683

merged 2 commits into from
Sep 28, 2018

Conversation

cflee
Copy link
Contributor

@cflee cflee commented Aug 18, 2018

#3586 seems to be breaking rgblight for I2C split boards, see r/olkb comment thread and thread2

This PR reverts it and offers a substitute fix: tweak keyboard_slave_loop() to always run backlight_set() on every loop, ignoring the BACKLIT_DIRTY flag, which is effectively what #3586 has done except in the slave matrix scan instead.

Technically it only needs to run every time there's a serial transaction from master since that will always update the backlight byte, so we could change the serial code to update the BACKLIT_DIRTY flag. But it seems like the intent in the rgblight section in keyboard_slave_loop() is that serial mode doesn't use the flags, so I think this makes sense. I also don't want to mess with the serial code...

Note: I don't have a serial split board to test this with, but my lets_split_eh which uses I2C works with this.

@SethBarberee
Copy link
Contributor

I’ll get to testing it later today/tomorrow

@drashna
Copy link
Member

drashna commented Aug 18, 2018

@nooges and @That-Canadian This look okay to you? And if you have time, does it work fine?

@drashna drashna added the bug label Aug 18, 2018
@SethBarberee
Copy link
Contributor

SethBarberee commented Aug 18, 2018

From my testing on my EE_HANDS Serial Iris, everything works fine. Nice work!

@Lenbok
Copy link
Contributor

Lenbok commented Sep 4, 2018

@drashna Is it OK to merge this fix? I don't want to update qmk on my board while the current master has the bug that this fixes.

@drashna
Copy link
Member

drashna commented Sep 13, 2018

Hmm, it looks like this may have some conflicts with #3886

@cflee
Copy link
Contributor Author

cflee commented Sep 13, 2018

@drashna I will look at that PR more closely again, but it seems like the first and third commit roughly achieve what this PR does (but by setting BACKLIT_DIRTY instead of the split_util.c change here to isolate that flag to I2C only), so the remaining diff in the second commit is:

diff --git a/quantum/split_common/matrix.c b/quantum/split_common/matrix.c
index 6a9ce66d27..457db143dd 100644
--- a/quantum/split_common/matrix.c
+++ b/quantum/split_common/matrix.c
@@ -237,7 +237,7 @@ int i2c_transaction(void) {
             if (err) goto i2c_error;
             
             // Write backlight 
-            i2c_master_write(get_backlight_level());
+            i2c_master_write( backlight_config.enable ? backlight_config.level : 0 );
             
             BACKLIT_DIRTY = false;
         }

@evantravers
Copy link
Contributor

Where does this stand? Is there anything I can do to help?

@evantravers
Copy link
Contributor

@cflee I can try and test this… what are the testing steps?

@cflee
Copy link
Contributor Author

cflee commented Sep 25, 2018

@evantravers I guess you also have a Let's Split Eh? If your branch is ahead of this or up to date with master, you could either manually apply the changes as displayed in the Files changed tab, or cherry pick the two commits from this PR into your branch.

Without this fix on latest master, rgblight changes shouldn't be working on the slave side; with this fix rgblight should work fine.

@evantravers
Copy link
Contributor

@cflee I'm going to assume that I need to flash that firmware onto both halves, right?

@evantravers
Copy link
Contributor

It works for me on my let's split eh.

Turns out you only need to flash that change on the slave… but I flashed on both to be sure.

@paulbdavis
Copy link
Contributor

paulbdavis commented Sep 27, 2018

Also working on my let's split eh?

My user setup is on top of the current master, with these two commits applied on top and everything is working fine, but I do not have a setup to test for regressions to non-RGB backlighting (maybe just a different config setup can test that on my hardware?).

@drashna drashna merged commit 7d2d0c6 into qmk:master Sep 28, 2018
@cflee cflee deleted the cflee/3586-fix branch September 28, 2018 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants