qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.1k stars 38.92k forks source link

[Bug] RGBLIGHT_SLEEP only turns off master half on split keyboard #22794

Open Mikastiv opened 9 months ago

Mikastiv commented 9 months ago

Describe the Bug

The define only works for the master keyboard on my setup when my pc is turned off. I am using a corne keyboard

Keyboard Used

corne/rev1

Link to product page (if applicable)

No response

Operating System

windows 10

qmk doctor Output

Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.1.1
Ψ QMK home: C:/Users/mlebl/qmk_firmware
Ψ Detected Windows 10 (10.0.19045).
Ψ QMK MSYS version: 1.7.2
Ψ Userspace enabled: False
Ψ Git branch: master
⚠ Git has unstashed/uncommitted changes.
Ψ - Latest master: 2023-12-31 18:27:03 -0500 (50a9658502) -- zig logo
Ψ - Latest upstream/master: 2023-12-31 19:25:58 +0100 (90811118b7) -- docs(skeletyl): fix readme instructions (#22791)
Ψ - Latest upstream/develop: None
Ψ - Common ancestor with upstream/master: 2023-12-23 17:49:37 -0600 (a1d29982dc) -- Add Momokai Aurora Image (#22728)
Ψ - Common ancestor with upstream/develop: None
Ψ All dependencies are installed.
Ψ Found arm-none-eabi-gcc version 10.1.0
Ψ Found avr-gcc version 8.5.0
Ψ Found avrdude version 7.0
Ψ Found dfu-programmer version 0.7.2
Ψ Found dfu-util version 0.11
Ψ Submodules are up to date.
Ψ Submodule status:
Ψ - lib/chibios: 2023-04-15 13:48:04 +0000 --  (11edb1610)
Ψ - lib/chibios-contrib: 2023-07-17 11:39:05 +0200 --  (da78eb37)
Ψ - lib/googletest: 2021-06-11 06:37:43 -0700 --  (e2239ee6)
Ψ - lib/lufa: 2022-08-26 12:09:55 +1000 --  (549b97320)
Ψ - lib/vusb: 2022-06-13 09:18:17 +1000 --  (819dbc1)
Ψ - lib/printf: 2022-06-29 23:59:58 +0300 --  (c2e3b4e)
Ψ - lib/pico-sdk: 2023-02-12 20:19:37 +0100 --  (a3398d8)
Ψ - lib/lvgl: 2022-04-11 04:44:53 -0600 --  (e19410f8)
Ψ QMK is ready to go, but minor problems were found

Is AutoHotKey / Karabiner installed

Other keyboard-related software installed

No response

Additional Context

I have pro-micro microcontrollers https://keebmaker.com/collections/parts/products/controllers-pro-micro-c?_pos=1&_fid=bdf159e90&_ss=c

drashna commented 9 months ago

To make sure, when updating the config, have you flashed both sides?

Mikastiv commented 9 months ago

Yes I did. I tried multiple times and it doesn't work for me. I would actually be willing to work on this if no body does and someone could guide me a bit; it annoys me to see my keyboard with half the lights on 😅

I tried to investigate a bit and from what I understood, the master keyboard is stuck in a loop in protocol_pre_task during suspend and never sends to the slave that is should suspend, but maybe I'm wrong

Mikastiv commented 8 months ago

I found why it was not working on my keyboard; here: https://github.com/qmk/qmk_firmware/blob/dcc47ea31b3f4ef097a2fc677bdbb9b2560d905a/tmk_core/protocol/lufa/lufa.c#L858-L860

USB_Device_RemoteWakeupEnabled is false for me so suspend_wakeup_condition() is never run and the lights suspend state is never synced to the keyboard slave. For myself. I added a matrix_scan call inside the suspend loop to run the same code from suspend_wakeup_condition() and it works. I don't know if this is a good solution or not. Thoughts?

Mikastiv commented 8 months ago

I would propose this as a fix

const bool wakeup = suspend_wakeup_condition();
if (USB_Device_RemoteWakeupEnabled && wakeup) {
    ...
}
Filios92 commented 8 months ago

I've been having the same issue (altough I think it didn't always happened on PC turn off) - I have been mitigating it by suspending RGB after 20s of idle / not using keyboard (which has some other issue, details below). Seems yours proposed fix helped!

(The other issue I have, which I thought could be related, is after calling rgblight_suspend the slave sometimes doesn't set it's rgblight_config.enable to the one indicated by master - maybe there is some race or maybe just serial comm error, but I haven't seen any. I have fixed it with (in rgblight_update_sync which I always call in rgblight_handlers_slave):

if (syncinfo->config.enable != rgblight_config.enable)
        syncinfo->status.change_flags |= RGBLIGHT_STATUS_CHANGE_MODE;
if (syncinfo->status.change_flags == 0) // not needed really
        return;

to force slave's enable update if changed (as the whole config is synced periodically either way, not just on status change) But this seems to me other issue / PR, as yours fix solved PC turn off issue, I may yet to check mine rgb_suspend fix alone after turning off PC)

JuanoD commented 2 months ago

24055 fixes this. We just have to wait for it to be merged into master