qmk / qmk_firmware

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

[Bug] SPLIT_USB_DETECT prevents keyboard detection on cold boot #8990

Closed elagil closed 2 years ago

elagil commented 4 years ago

Describe the Bug

If #define SPLIT_USB_DETECT is used for a split board, the board is not detected by the PC on a cold boot or after repowering a USB hub that it is attached to. For example, my board is attached to a monitor with a USB hub. When I switch off and on that monitor, the keyboard is not detected anymore.

System Information

Additional Context

I am guessing, there is some timeout happening on qmk, prior to the PC detecting the board. The board does not show up in device manager at all (in Windows).

Tweaking the timeouts, this can be repaired, but I don't know if it is consistent across different machines. Therefore, I am not sure if this is really a bug, but rather an unfortunate default setup.

I was thinking that the board halves should check for being USB master, as long as none of the two found a USB connection. What do you think?

zvecr commented 4 years ago

I was thinking that the board halves should check for being USB master, as long as none of the two found a USB connection. What do you think?

@elagil unfortunately that change is not simple at all given the current code base assumptions. is_keyboard_master is assumed to be singleton, with no scope for dynamic change. Updating it to assume the slave role, and retry the usb check, mean things like handedness (in its current form) will break.

For reference, what did you have to change the timeouts to?

elagil commented 4 years ago

I understand. Having a first look, it would indeed be a substantial change. My timeout is 5000 ms with 50 ms polling interval, which works fine. The slave stays unresponsive for five seconds, of course, but it is acceptable for my use.

I also noticed that there is no warning when compiling TIMEOUT/POLL_INTERVAL >= 256, which breaks the for loop that handles the polling (uint8_t counter variable). It would be good to add a check there.

dpapavas commented 4 years ago

I've also stumbled across this in the process of developing a SPI-based split transport for a hand-wired keyboard I'm finishing up on. Let me also add that, in my case this issue is a bit more than a simple nuisance, as SPI is push-pull and bidirectional. Having both ends ending up thinking they're slaves might lead (given that the SS line will probably float low most of the time and the SCK line might fluctuate from picked up noise) to both ends configuring the MISO line as an output and potentially trying to drive it to different levels, as they attempt to communicate. This would lead to a short, which, if it has happened in my case, seems to have luckily be dealt with by the clamping diodes.

This would not be a case in the case of I2C-based transport as far as I can see, but I don't know about soft-serial (probably not there either).

@elagil unfortunately that change is not simple at all given the current code base assumptions. is_keyboard_master is assumed to be singleton, with no scope for dynamic change. Updating it to assume the slave role, and retry the usb check, mean things like handedness (in its current form) will break.

Perhaps I misunderstand your comment, but if I read you correct, that you say that it would not be easy to implement timing out and configuring as a slave, only to try to revert that decision later, if no master seems to be present after some time, then I think there's a simpler solution.

They only time the keyboard doesn't know which end has the active USB connection is when the USB port it is on, is powered but no driver has been loaded to enable it for communication. This can take shorter or longer periods of time (for instance on my work laptop, I connect the keyboard to a USB port provided by a dongle, which doesn't start working until after the Linux kernel has started booting), but at any rate, if there's no USB connection there's no reason for either half to commit to any role. It wouldn't be able to do anything useful anyway, as far as I can see.

I have tried a proof of concept implementation of the following approach and it seems to work: Instead of SPLIT_USB_TIMEOUT, define a signaling pin, which is connected on both halves. In my case it can be any of the SPI pins, but for I2C the SCL line could be used, or for soft-serial, the one data line. This is initially configured as input with the internal pull-up enabled, so it's held high. The usbIsActive check then becomes something like:

bool usbIsActive(void) {
    while (readPin(SIGNALING_PIN)) {
        if (usbHasActiveConnection()) {
            return true;
        }
        wait_ms(SPLIT_USB_TIMEOUT_POLL);
    }

    usbDisable();

    return false;
}

So each half keeps polling until one discovers a USB connection and becomes master. It then only needs to configure its end of the signaling line as output and pull it low for the other side to give up and commit to being a slave. In my case this happens automatically as I use the SS pin for signaling, which the master pulls low to select the slave, so my proof-of-concept implementation was literally a couple of lines. The same should be achievable for I2C if the SCL line is used, but I don't know about soft-serial. Once the relationship is established, it can't change without a power cycle, so there's no need to make it reversable.

Let me know what you think. If you find the solution acceptable, I'd be happy to polish it up into a PR, but I'd need some help testing, as I don't have any I2C, or soft-serial based boards.

elagil commented 4 years ago

Your method will certainly work and seems to require few changes to the protocol between the halves. I don't see conflicts, but I am not a core developer and may be wrong. I would help you testing it (on AVR with UART).

You will also require some additional code for changing pin functionality, which I am not yet seeing in your example.

For reference, I have implemented a software-version in https://github.com/qmk/qmk_firmware/pull/9742, but there seem to be unresolved incompatibilities that I didn't work on yet.

dpapavas commented 4 years ago

I would help you testing it (on AVR with UART).

Thanks for the offer. You mention UART; are you referring to soft-serial, which I assumed uses software bit-banging, instead of the hardware UART, or is there some other communication option I'm not aware of?

You will also require some additional code for changing pin functionality, which I am not yet seeing in your example.

The example code was meant to illustrate the negotiation logic, but for my proof-of-concept implementation (which seems to be working fine so far) I practically only needed another line; something like setPinInputHigh(SIGNALING_PIN) inside keyboard_pre_init_kb. In practice it may have to be a bit more complex than that, but I'd like to avoid it, both as a matter of principle, but also because I'd like to avoid code that explicitly sets a shared line as output and tries to signal. If that code runs on both halves inadvertently, it might lead to a short, as previously mentioned. I'll look into it more once a core developer confirms that the method seems reasonable overall and would be acceptable.

elagil commented 4 years ago

are you referring to soft-serial

Yes, sorry for the confusion.

runs on both halves inadvertently, it might lead to a short, as previously mentioned

If left and right halves are determined prior to the detection of the master, the two halves could use different pins for signalling. Then, there would be no conflict.

dpapavas commented 4 years ago

If left and right halves are determined prior to the detection of the master, the two halves could use different pins for signalling. Then, there would be no conflict.

Yes, that is also an option. Nevertheless, that wouldn't work when configured to detect left/right sides based on master/slave. So in that case we'd either need to fall back to timeouts and not supporting detection during cold boots, or using the same line. I don't think we need to complicate things for the end user though. Being careful not to allow both sides outputting on the same line isn't such a big deal; I merely commented that I'd happily avoid it if possible, all else being equal.

willthree123 commented 3 years ago

I am now running into the same problem too, I have a hardwired split keyboard. This keyboard won't work when the computer start-up until I re-plugin.

I have read all the related comments and issues, but I still don't get it. What should I change/ add to my code to prevent this to happen?

What I have tried

A. Does not work at start up, both master and slave works.

#define SOFT_SERIAL_PIN D2
#define SPLIT_USB_DETECT
//#define SPLIT_USB_TIMEOUT 5000
#define MASTER_LEFT

B/C. Does work at startup, but the slave (right half) does not.

#define SOFT_SERIAL_PIN D2
#define SPLIT_USB_DETECT
#define SPLIT_USB_TIMEOUT 5000
#define MASTER_LEFT
#define SOFT_SERIAL_PIN D2
#define MASTER_LEFT

Rules.mk

# MCU name
MCU = atmega32u4
BOOTLOADER = caterina

EXTRAKEY_ENABLE ?= yes  # Audio control and System control(+450)
SPLIT_KEYBOARD = yes

RGBLIGHT_ENABLE = yes
RGBLIGHT_DRIVER = WS2812

VIA_ENABLE = yes
LTO_ENABLE = yes

AUDIO_ENABLE ?= no
BOOTMAGIC_ENABLE = lite
BLUETOOTH_ENABLE ?= no
COMMAND_ENABLE = no
CONSOLE_ENABLE = no
MOUSEKEY_ENABLE = no
MIDI_ENABLE ?= no
NKRO_ENABLE = no
SLEEP_LED_ENABLE ?= no
UNICODE_ENABLE ?= no

Config.h

#ifndef CONFIG_H
#define CONFIG_H

#include "config_common.h"

/* USB Device descriptor parameter */
#define VENDOR_ID       0x7766
#define PRODUCT_ID      0x0001
#define DEVICE_VER      0x0001
#define MANUFACTURER    willthree123
#define PRODUCT         wf50
#define DESCRIPTION     "wf50"

/* key matrix size */
#define MATRIX_ROWS 10
#define MATRIX_ROWS_PER_SIDE (MATRIX_ROWS / 2)
#define MATRIX_COLS 6

/* key matrix pins */
#define MATRIX_ROW_PINS { C6, D7, E6, B4, B5 }
#define MATRIX_COL_PINS { B6, B2, B3, B1, F7, F6 }
#define MATRIX_ROW_PINS_RIGHT { C6, D7, E6, B4, B5 }
#define MATRIX_COL_PINS_RIGHT { F6, F7, B1, B3, B2, B6 }
#define UNUSED_PINS
/* COL2ROW or ROW2COL */
#define DIODE_DIRECTION COL2ROW
#define DEBOUNCE 5

#define SPLIT_USB_DETECT 
#define SOFT_SERIAL_PIN D2

/* ws2812 RGB LED */
#define RGB_DI_PIN D0
#define RGBLED_NUM 10 
#define RGBLIGHT_SPLIT
#define RGBLIGHT_ANIMATIONS

#define TAPPING_TERM    200
#define IGNORE_MOD_TAP_INTERRUPT 
#define LOCKING_SUPPORT_ENABLE
#define LOCKING_RESYNC_ENABLE
#define IS_COMMAND() ( \
    get_mods() == (MOD_BIT(KC_LCTL) | MOD_BIT(KC_RCTL)) || \
    get_mods() == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RSFT)) \
)
#define USB_MAX_POWER_CONSUMPTION 500

#endif
dpapavas commented 3 years ago

I'm afraid it can't be fixed with simple reconfiguration, unless your board supports properly detecting the plugged-in side by the voltage on the VBUS pad (some boards, such as the Teensy, don't support this, as they allow the voltage from the plugged in master side to reach the VBUS pad on the slave side as well).

If your board does support "VBUS detection", then you should be able to remove the #define SPLIT_USB_DETECT line and it should work (or at least I think so, as I've never had the chance to use VBUS-based split detection mysel).

willthree123 commented 3 years ago

Thanks for your help!

FYI, I am using arduino pro micro, I assume this is be cheap and not support the proper USB detection

If your board does support "VBUS detection", then you should be able to remove the #define SPLIT_USB_DETECT line and it should work

I have tried your suggestions and this is what many people suggested me. However, the slave would not work even if I do not restart/ cold boot the computer.

I have posted more details and video on Reddit.

https://www.reddit.com/r/olkb/comments/np60t6/my_handwired_keyboard_would_not_work_every_time/

This is actually really sad when you designed and soldered your very first (QMK split) keyboard, even make a custom firmware. But the keyboard does not work on start up.😢

elagil commented 3 years ago

I made a pull request in the past (that was rejected) for forcing a master side. If you don't need to switch your master side, you may want to implement this: https://github.com/qmk/qmk_firmware/pull/10353

dpapavas commented 3 years ago

FYI, I am using arduino pro micro, I assume this is be cheap and not support the proper USB detection

No, the Pro Micro should support VBUS detection, at least according to the documentation:

Pro Micro boards can use VBUS detection out of the box and be used with or without SPLIT_USB_DETECT.

That said, judging by the schematics, I would only expect this to be true of the 3.3V version, not the 5V version.

daskygit commented 3 years ago

The 5V clones are usually fine, they don't connect the SJ1 jumper and fit a 5V regulator instead. Recent batches seem to have a poor choice of diode with too much leakage, much like the elite-c rev3.

@willthree123 You never responded to my discord query. I'm surprised the second configuration doesn't work with the increased timeout. Have you flashed both sides individually?

willthree123 commented 3 years ago

Sorry, Dasky. I did not check the QMK Discord group. I flashed them individually (without the serial pin connected also)

Since my keyboard firmware is custom. Therefore, I tried flashing "Let's Split" firmware (only changing the LED and soft Serial Pin) yesterday. The keyboard still having the same problem. Only one side works on start-up, but the other doesn't. Many people told me that Pro Micro (ATmega32U4) doesn't need SPLIT_USB_DETECT. However, I have NEVER made the split function worked without it.

SPLIT_USB_DETECT Master(Start Up) Slave(Start Up) Master(Normal) Slave (Normal)
✕ ✓ ✕ ✓ ✕
✓ ✕ ✕ ✓ ✓

Source code of my custom firmware: https://github.com/willthree123/WF50

I haven't tried elagil's solution yet, I surely will try at the weekend.

marek-antozi commented 3 years ago

Sorry, Dasky. I did not check the QMK Discord group. I flashed them individually (without the serial pin connected also)

Since my keyboard firmware is custom. Therefore, I tried flashing "Let's Split" firmware (only changing the LED and soft Serial Pin) yesterday. The keyboard still having the same problem. Only one side works on start-up, but the other doesn't. Many people told me that Pro Micro (ATmega32U4) doesn't need SPLIT_USB_DETECT. However, I have NEVER made the split function worked without it.

| SPLIT_USB_DETECT | Master(Start Up) | Slave(Start Up) | Master(Normal) | Slave (Normal)| | ✕ | ✓ | ✕ | ✓ | ✕ | | ✓ | ✕ | ✕ | ✓ | ✓ |

Source code of my custom firmware: https://github.com/willthree123/WF50

I haven't tried elagil's solution yet, I surely will try at the weekend.

I am using #define EE_HANDS instead of defining master side and slave part of my keyboard with Pro Micros was not working without #define SPLIT_USB_DETECT. Unfortunately, keyboard was not detected properly at the boot. I use #define SPLIT_USB_TIMEOUT 5000, but only for master side and it works. Keyboard is detected properly since boot.

willthree123 commented 3 years ago

I solved my problem by changing the pro micros. I found that the problem is my pro micros, they could not detect VBUS (which should), VBUS is used to detect master/slave. I swap the blue ones with the black ones, works fine now,

omscarr commented 3 years ago

Hi, I have the same issue.

I have an Keeb.io Iris rev 2.5 split keyboard and it doesn't work when it's plugged in before the pc is powered on.

My pro-micros are made by "sparkfun" and they're 5v versions.

The firmware I'm running is a little old. I had an issue with master / slave detection, and one side's keypresses being really laggy. I had to pull in @nooges I2C-pullup-fix branch to fix it but I don't think that PR got completed.

willthree123 commented 2 years ago

@omscarr

My problem was solved by changing HARDWARE.

It seems that my pro micro clone couldn’t detect VBUS and cause it couldn’t assign slave and master. I swapped the blue micro clone with black PCB ones. The problem was solved!

My keyboard has been used for over 3 months now.

daskygit commented 2 years ago

The original issue should be resolved by #18599 which is opt-in and at the time of posting is currently in develop branch only.

If you experience any further issues using SPLIT_USB_DETECT and SPLIT_WATCHDOG_ENABLE together please open an issue or jump in the QMK Discord.