qmk / qmk_firmware

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

[Bug] ErgodoxEZ prevents Windows computer from sleeping #10624

Closed alexcrichton closed 4 years ago

alexcrichton commented 4 years ago

Hello! I recently got an Ergodox EZ keyboard and have been getting used to it. One thing I've noticed, however, is that when my computer goes to sleep it immediately wakes up. I'm basically having the same issue described in https://github.com/qmk/qmk_firmware/issues/375, but that was closed years ago and the various comments there don't seem to apply to the current source (and fiddling with the source proved unsuccessful on my part).

I am unfortunately very new to this keyboard so I don't know much about how to debug further, but I would be happy to get more information if needed!

System Information

mute2120 commented 4 years ago

Having this same issue, It appeared after the recent update to oryx/wally. It is also preventing my computer from shutting down properly with my ergodox ez plugged in.

stash commented 4 years ago

I found a workaround for this by disabling the wake-on-keypress code. Sadly, you'll need to use a mouse or something else to wake the computer. Add NO_USB_STARTUP_CHECK = yes to rules.mk.

stash commented 4 years ago

I've narrowed the bug down, but I'm not sure how to fix it.

I think this is because of the changes in the firmware19 branch of zsa/qmk_firmware, not doing power down the same way when entering suspend mode. But again, I'm not certain; this is a strong clue at best. It looks like they're already reverting what I suspect was the offending commit: https://github.com/zsa/qmk_firmware/commit/edd58256f58b44b00f8ae811941c63862693e4c4

What I've found is that by disabling this code in https://github.com/zsa/qmk_firmware/blob/firmware19/tmk_core/protocol/lufa/lufa.c#L1186-L1194 with the NO_USB_STARTUP_CHECK makes the computer not wake:

#if !defined(NO_USB_STARTUP_CHECK)
        while (USB_DeviceState == DEVICE_STATE_Suspended) {
            print("[s]");
            suspend_power_down();
            if (USB_Device_RemoteWakeupEnabled && suspend_wakeup_condition()) {
                USB_Device_SendRemoteWakeup();
            }
        }
#endif

I added some LED-blinking debugging to the suspend_wakeup_condition function in https://github.com/zsa/qmk_firmware/blob/firmware19/tmk_core/common/avr/suspend.c#L147-L155 .

bool                       suspend_wakeup_condition(void) {
    matrix_power_up();
    matrix_scan();
    matrix_power_down();
    for (uint8_t r = 0; r < MATRIX_ROWS; r++) {
        if (matrix_get_row(r)) return true;
    }
    return false;
}

Basically I blinked the right-side LED indicators based on what row is triggering. I was suspecting Left-Hand-Nonsense(tm), but it appears to be random! The call to matrix_get_row(r) seems to be returning random noise. I suspect that the keyboard is neglecting to handle a low power mode or something (I'm an electronics noob, so I'm not sure why; just a hypothesis)

stash commented 4 years ago

cc @fdidron , see comments above

fauxpark commented 4 years ago

@drashna ?

drashna commented 4 years ago

Actually, we found that the "convert to custom matrix lite" seems to be the cause of the issue, specifically.

I would like to dig into it further, but ... outright reverting 11c308d436180974b7719ce78cdffdd83a1302c0 works, for now.

Also .... it seems to be windows specific....

stash commented 4 years ago

@drashna it looks like matrix_power_up and matrix_power_down were defined for the ergodox-ez before, but are left as no-ops after the "convert to matrix lite" patch (https://github.com/qmk/qmk_firmware/commit/11c308d436180974b7719ce78cdffdd83a1302c0). Both of those functions are being called in suspend_wakeup_condition. Could that have something to do with this bug?

drashna commented 4 years ago

yup. Re-adding the matrix_power_up function fixes the issue.

That's 100% on me. Testing this with florian, to confirm, and will have a PR to fix this.

Specifically, it seems that adding this to the end of the matrix.c file is all that is needed to fix the issue.

void matrix_power_up(void) {
    mcp23018_status = init_mcp23018();

    unselect_rows();
    init_cols();

    // initialize matrix state: all keys off
    for (uint8_t i=0; i < MATRIX_ROWS; i++) {
        matrix[i] = 0;
    }

}
alexcrichton commented 4 years ago

@drashna I can confirm that fix works for me as well, thanks again for all with the help on this!

stash commented 4 years ago

@drashna Can confirm this also works (just de-duplicates some code):

void matrix_power_up(void) {
    matrix_init_custom();

    // initialize matrix state: all keys off
    for (uint8_t i = 0; i < MATRIX_ROWS; i++) {
        matrix[i] = 0;
    }
}

Tested by re-flashing, putting system to sleep (system didn't wake). Pressing a key on the kb then wakes the system as before.

Thank you for your help!

drashna commented 4 years ago

That probably does, though, I think i'd rather keep it separate.

I also plan on adding a comment, so this doesn't happen again in the future