Qmk_firmware: [Bug] Recent split_util.c changes break split slave functionality

[elagil]

Describe the Bug

I am the developer of the Murcielago, a split keyboard that works with UART and uses EEPROM and USB connectivity for side detection. Its config.h includes:

#define EE_HANDS
#define SPLIT_USB_DETECT
#define SOFT_SERIAL_PIN E6

Previously, this worked fine. With a recent QMK update, the slave does not work anymore.

Before: QMK configurator default firmware worked
Now: QMK configurator default firmware makes slave do nothing

I reflashed the split EEPROM image to both sides, just to be sure, but it didn't help.

Were there split-related changes that are crucial for this use case?

System Information

• Keyboard: Murcielago
  
  
  
  • Revision (if applicable): Rev1
  
  
• Operating system: Windows
• AVR GCC version: 5.4.0
• QMK Firmware version: 0.9.7
• Any keyboard related software installed?
  
  
  
  • [ ] AutoHotKey
  
  
  • [ ] Karabiner
  
  
  • [x] None
  
  

[elagil]

I found that functionality breaks from https://github.com/qmk/qmk_firmware/releases/tag/0.8.175 onwards, where there are some changes in quantum/split_common/split_util.c

I tested the new release (0.9.7) with the old split_util.c (from 0.8.173) and it works fine. Somewhere in split_util.c, there is a change that breaks my slave functionality.

[semnil]

The following changes will work. 🤔

https://github.com/semnil/qmk_firmware/commit/ccc1b6b44c05c4602e5cfa90fe2b1b6b768dff10

It hasn't worked since this commit. 😭

https://github.com/qmk/qmk_firmware/commit/1816ad01d0efcc987b5aa7a833331b0f9d903d4f

@zvecr What do you think about this?

[fauxpark]

I assume this is because USB_Disable() actually does a lot more than clearing those two flags: https://github.com/abcminiuser/lufa/blob/master/LUFA/Drivers/USB/Core/AVR8/USBController_AVR8.c#L104

Do we need to clear USBE as well or can we just get away with calling USB_OTGPAD_Off()?

[elagil]

I assume this is because USB_Disable() actually does a lot more than clearing those two flags

I guess you are right. The rest seems to be unchanged logic-wise.

Do we need to clear USBE as well or can we just get away with calling USB_OTGPAD_Off()?

No, we don't need to clear USBE, as far as I can see. I just tried it with my board, and USBCON &= ~(_BV(OTGPADE)) is sufficient for the slave to run.

I found out that, within USB_Disable(), USB_INT_DisableAllInterrupts() is the problem. If commented out, it works as expected.

[elagil]

I dug a bit deeper, and found that

https://github.com/abcminiuser/lufa/blob/master/LUFA/Drivers/USB/Core/AVR8/USBInterrupt_AVR8.c#L42

is the problem. If commented out, it works fine. I am not sure why. Does QMK need the VBUS transition interrupt for something?

I may add that I use the Atmega32U4, to which this relates. Unfortunately, I have no ISP for AVR, so I cannot see where it hangs.

[fauxpark]

Just my speculation, but it could be that it's somehow causing USB_USBTask() to block on the slave side, where it instead should not be called at all since there's of course no USB stuff to process.
https://github.com/qmk/qmk_firmware/blob/develop/tmk_core/protocol/lufa/lufa.c#L966

[elagil]

At least wrapping it in

if(USB_IsInitialized)
{
  USB_USBTask();
}

does not help.

Disabling it completely on the slave with

if(false)
{
  USB_USBTask();
}

does not fix it either. So it is probably something else.

[fauxpark]

I notice that nowhere in USB_Disable() or any of the functions it calls does USB_DeviceState get reset to "unattached", which would make USB_DeviceTask() return early. Not sure whether that's a bug in LUFA, or if you're not supposed to use USB_Disable() in this way.

[elagil]

Adding USB_DeviceState = DEVICE_STATE_Unattached; to void USB_Disable(void) does indeed fix it.

In LUFA, this variable is only ever set to "unattached" in interrupts (USB bus event) or upon initialization. Therefore, it will never be set to unattached by LUFA, if the interrupts are disabled for USB.

[elagil]

I wouldn't call this a bug in LUFA, since USB_DeviceState is initialized with DEVICE_STATE_Unattached, but, ONLY after checking the VBUS pin voltage, is set to DEVICE_STATE_Powered.

In my case, the VBUS pin is shorted to VCC, as I have no diode in place. I relied on the software detection for assigning the master. LUFA just detects the VBUS voltage as expected.

For this case, and something like the buggy Elite C with the high reverse current Schottky, it would be nice to implement a workaround.

I suggest, simply

static inline void usbDisable(void) { USB_Disable(); USB_DeviceState = DEVICE_STATE_Unattached;}

in split_util.c, which works fine. I must admit that it is not very pretty. Adding it to the LUFA fork would also be fine.

[semnil]

static inline void usbDisable(void) { USB_Disable(); USB_DeviceState = DEVICE_STATE_Unattached;}

This change also worked on the same split keyboard Fortitude60 Rev1 😄
I want it to work anyway, even if it's not pretty. 😢

[elagil]

I want it to work anyway, even if it's not pretty.

I agree, I would also like this to work. However, I am not sure, where the best place is, to solve this. While that works in split_util.c, the USB_DeviceState variable technically resides within LUFA, so I would rather not touch it outside of it. If it ever changes its name, this will break.

@fauxpark do you have a suggestion?

[fauxpark]

If we've completely shut down the USB peripheral and its interrupts, why would we care about the state of the VBUS pin? In the context of QMK/split common at least, surely we can safely assume the device state is unattached, even if power "appears" to be coming from USB. After all, at this point we've timed out from polling usbHasActiveConnection() in usbIsActive(). But I'm probably missing something obvious...

[fauxpark]

I don't mind simply adding USB_DeviceState = DEVICE_STATE_Unattached;, it's not like the LUFA submodule will be updated under our feet as it's pointing at our own fork, not upstream/master. Plus it's pretty unlikely for that name to change in the foreseeable future.

[elagil]

If we've completely shut down the USB peripheral and its interrupts, why would we care about the state of the VBUS pin?

I think you are right about that in the context of QMK. However, if the VBUS pin actually reflects USB connectivity, declaring it unattached doesn't make sense. It may still be used for charging, or something like that. Connecting to a dumb charger will also lead to the connection timing out.

I don't mind simply adding USB_DeviceState = DEVICE_STATE_Unattached;

I can create a pull request, if you like.

[elagil]

PR is open!