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.

Sign up for GitHub

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

Jump to bottom

fix: split_common i2c slave backlight not disabled #3886

Merged
merged 1 commit into from
May 7, 2019
Merged

fix: split_common i2c slave backlight not disabled #3886

merged 1 commit into from
May 7, 2019

Conversation

rclasen
Copy link
Contributor

@rclasen rclasen commented Sep 10, 2018

slave side matrix scanning copied the matrix twice to the i2c slave
buffer... - though, for i2c one copy was put in the wrong offset.

The wrong i2c copy (offset 1) overwrote the unused RGBLIGHT part of the
buffer... and when using more than 5 columns per half also the original
matrix buffer.

removed code that does the unnecessary second copy.

@drashna drashna added the core label Sep 10, 2018
@drashna
Copy link
Member

drashna commented Sep 10, 2018

@That-Canadian and/or @nooges, this looks good?

@rclasen
Copy link
Contributor Author

rclasen commented Sep 10, 2018

sorry for adding this to an existing PR... but 39624f1 won't apply cleanly without the initial buffer fix

@rclasen
Copy link
Contributor Author

rclasen commented Sep 11, 2018

BTW: commit message for 39624f1 is wrong regarding eeprom writes... backlight_set doesn't mess with the eeprom. Though, the code duplication still exists. let me know if I shall reword the message.

@drashna
Copy link
Member

drashna commented Sep 13, 2018

Adding to PRs is absolutely fine!

And yeah, please make sure that messages/documentation in code is accurate/correct.

@rclasen
Copy link
Contributor Author

rclasen commented Sep 13, 2018

ok, updated the commiit message. Code was fiine.

@drashna drashna mentioned this pull request Sep 13, 2018
Merged
@drashna
Copy link
Member

drashna commented Oct 2, 2018

@nooges or @That-Canadian, does this look good?

@cflee
Copy link
Contributor

cflee commented Oct 13, 2018

I think the BACKLIT_DIRTY = true; isn't necessary anymore, just left there as a result of the merge?

@rclasen
Copy link
Contributor Author

rclasen commented Oct 13, 2018

you just need 3111dabb0fed4693b25eec5b313ab48cff952eb3 - the other commits were obsoleted by the alternative fix in #3683.

shall I rebase this on master?

@cflee
Copy link
Contributor

cflee commented Oct 13, 2018

I think that is a good idea, we just need that commit on master.

@rclasen
Copy link
Contributor Author

rclasen commented Oct 13, 2018

ok, rebased.

@drashna
Copy link
Member

drashna commented Jan 25, 2019

There have been a bunch of changes, would you mind updating this?

@drashna drashna added the bug label Jan 25, 2019
@drashna
Copy link
Member

drashna commented Mar 22, 2019

@rclasen there have been a number of changes to the split code recently, and this may no longer be an issue.

If you could test out the newer code?
If it's still an issue, could you rebase and update the PR.
Otherwise, could you close it?

skullydazed
skullydazed approved these changes Apr 5, 2019
View reviewed changes
Copy link
Member

@skullydazed skullydazed left a comment

Choose a reason for hiding this comment

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

This looks good, although I'm told it may be outdated. If so it should be closed, and if not I think it's ok to merge.

@drashna
Copy link
Member

drashna commented Apr 5, 2019

This has been moved:

https://github.com/qmk/qmk_firmware/blob/master/quantum/split_common/transport.c#L71

@rclasen rclasen changed the title fix: split_common slave buffer population fix: split_common i2c slave backlight not disabled May 5, 2019
@rclasen
Copy link
Contributor Author

rclasen commented May 5, 2019

I've just pushed an updated patch that does the same fix in transport.

As the other bits of the original PR were addressed by other means I've updated the title to reflect the remaining bit.

@rclasen
fix split_common i2c backlight disabling
6a3c334 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
@drashna drashna merged commit 12e6d41 into qmk:master May 7, 2019
KauyonKais pushed a commit to KauyonKais/qmk_firmware that referenced this pull request May 8, 2019
@rclasen
[Split] fix split_common i2c backlight disabling (qmk#3886)
6073adc 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
KauyonKais pushed a commit to KauyonKais/qmk_firmware that referenced this pull request May 8, 2019
@rclasen @KauyonKais
[Split] fix split_common i2c backlight disabling (qmk#3886)
b0b8622 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
@Lenbok Lenbok mentioned this pull request May 10, 2019
Closed
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 11, 2019
@rclasen @drashna
[Split] fix split_common i2c backlight disabling (qmk#3886)
5d9a4c4 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019
@rclasen @Timbus
[Split] fix split_common i2c backlight disabling (qmk#3886)
39991d1 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
ridingqwerty pushed a commit to ridingqwerty/qmk_firmware that referenced this pull request Jan 10, 2020
@rclasen @drashna
[Split] fix split_common i2c backlight disabling (qmk#3886)
e5c9323 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
JeffreyPalmer pushed a commit to JeffreyPalmer/qmk_firmware that referenced this pull request Feb 27, 2020
@rclasen @JeffreyPalmer
[Split] fix split_common i2c backlight disabling (qmk#3886)
671c7cf 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@rclasen @drashna
[Split] fix split_common i2c backlight disabling (qmk#3886)
09b2b53 
slave backlight was always on - as get_backlight_level() doesn't
indicate if the backlight is enabled or not.

also updated the corosponding code for serial transport to stop peeking
directly at 'internal' backlight_config structure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skullydazed skullydazed skullydazed approved these changes

@fauxpark fauxpark fauxpark approved these changes

@ezuk ezuk Awaiting requested review from ezuk

@fredizzimo fredizzimo Awaiting requested review from fredizzimo

@jackhumbert jackhumbert Awaiting requested review from jackhumbert

@mechmerlin mechmerlin Awaiting requested review from mechmerlin

@yanfali yanfali Awaiting requested review from yanfali

Assignees
No one assigned
Labels
bug core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rclasen @drashna @cflee @fauxpark @skullydazed