Qmk_firmware: Bootmagic doesn't work.

[Lithium768]

Describe the Bug

I'm using bface board.
I found that bootmagic does not work with default debounce = 5.
However, bootmagic will work properly when I set debounce to 0.

System Information

• Keyboard: bface
  
  
  
  • Revision (if applicable):
  
  
• Operating system: Windows 10, Archlinux
• AVR GCC version: 9.2.0-1
  
• ARM GCC version: not used
  
• QMK Firmware version: 0.6.452
  
• Any keyboard related software installed?
  
  
  
  • [ ] AutoHotKey
  
  
  • [ ] Karabiner
  
  
  • [ ] Other:
  
  

Additional Context

my keymap rules.mk

TAP_DANCE_ENABLE = yes
BOOTMAGIC_ENABLE = full
SPACE_CADET_ENABLE = no

BACKLIGHT_ENABLE = no
RGBLIGHT_ENABLE = no

[drashna]

I see that you're on avr-gcc 9.2. Could you downgrade to 8.3 and see if the issue persists?

[drashna]

Also, tagging @alex-ong since this may be related to the debounce API stuff.

[yanfali]

We only support gcc 8.3 and 7 3 on avr platforms at this time. All other versions have produced bad code so this could just be a compiler issue.

[Lithium768]

I've downgraded AVR-GCC to 8.3, but the compiled firmware still doesn't work except debounce = 0.

[alex-ong]

thanks for the tag; i thought that boot-magic skipped matrix.c code entirely but that must have changed since the last time i checked...

[alex-ong]

Hmm from my check:

    /* do scans in case of bounce */
    print("bootmagic scan: ... ");
    uint8_t scan = 100;
    while (scan--) { matrix_scan(); wait_ms(10); }
    print("done.\n");
    #after this check contents of matrix[] to check which keys are down

So looks like it uses the regular keyboard scanning code. matrix_scan() calls debounce() inside it, so it _should_ behave the same as before the debounce_api stuff. So in _theory_ it's not debounce_api related (though of course it could be; i don't use bootmagic and i've never tested it before/after the debounce_api changes)

Another thing that changed was the default debounce algorithm, which moved from a sleep (1ms) version to a sleepless version that relies on timer_elapsed(). timer_elapsed() should work, since timer_init() is called by keyboard.c before bootmagic() occurs. Currently with DEBOUNCE=0 working and DEBOUNCE=5 not working, we can narrow it down to this bit of code in sym_g.c.

#if DEBOUNCE > 0
static uint16_t debouncing_time;
void debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed)
{
  if (changed) {
    debouncing = true;
    debouncing_time = timer_read();
  }

  if (debouncing && timer_elapsed(debouncing_time) > DEBOUNCE) {
    for (int i = 0; i < num_rows; i++) {
      cooked[i] = raw[i];
    }
    debouncing = false;
  }
}
#else //no debouncing.
void debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed)
{
  for (int i = 0; i < num_rows; i++) {
    cooked[i] = raw[i];
  }
}
#endif

The only things that comes to mind are

• timer_read() or timer_elapsed() not working properly either due to the wait_ms(10) in bootmagic.c or them not working this early in the initialization phase, though timer_init() was called by this point, and timer_elapsed() should work regardless of time difference up to sizeof(int16)
• changed is being passed in as the wrong value, either always being true or always being false. Though since it use keyboard.c and matrix.c and the keyboard works normally i don't see how the implementation could be wrong...

I don't have a spare keyboard (danggit) so i find these things hard to debug (if i change firmware on my keyboard to a non working version i'm kind of keyboardless...)

are you familiar with git bisect @Lithium768? could you help find when it broke? Otherwise i'll have a look when i have some spare time.

[Lithium768]

@alex-ong
I think timer_elapsed() is not working properly.
Because when I replaced

/* do scans in case of bounce */
    print("bootmagic scan: ... ");
    uint8_t scan = 100;
    while (scan--) { matrix_scan(); wait_ms(10); }
    print("done.\n");
    #after this check contents of matrix[] to check which keys are down

with

/* do scans in case of bounce */
    print("bootmagic scan: ... ");
    uint8_t scan = 100;
    while (scan--) { matrix_scan();
    uint16_t waiting_time;

    waiting_time = timer_read();
    while (timer_elapsed(waiting_time) < 10);
    }

, the keyboard stopped booting and seemed to be stuck in while (timer_elapsed(waiting_time) < 10);.

[Lithium768]

In tmk_core/protocol/vusb/main.c line 61~65

    keyboard_init();
    host_set_driver(vusb_driver());

    debug("initForUsbConnectivity()\n");
    initForUsbConnectivity();

line 28~42

/* This is from main.c of USBaspLoader */
static void initForUsbConnectivity(void)
{
    uint8_t i = 0;

    usbInit();
    /* enforce USB re-enumerate: */
    usbDeviceDisconnect();  /* do this while interrupts are disabled */
    while(--i){         /* fake USB disconnect for > 250 ms */
        wdt_reset();
        _delay_ms(1);
    }
    usbDeviceConnect();
    sei();
}

Before initForUsbConnectivity();, the interrupt is not set, so timer_elapsed() cannot work.

[alex-ong]

Alright so we know the problem, how should we fix it?

Cli() and sei() in keyboard init()?

Rewrite debounce api to support is_bootloader()?

[Lithium768]

Temporarily, I just put keyboard_init(); after the initForUsbConnectivity(); and it works.
Maybe there's something better to do.

[Lithium768]

Perhaps rewriting the debounce API to support is_bootloader() will prevent similar things from happening again.

[fauxpark]

IMO the AVR timer_init() should call sei(), considering the global interrupt flag is required to be set for the ISR to work.

[zvecr]

Debounce having know about bootloader breaks encapsulation.
Changing the init order for only one platform brings inconsistencies in behaviour.

I think we should look into things like the lufa init order to see any inconsistencies, for instance there is a sei() call here:
https://github.com/qmk/qmk_firmware/blob/92f9b6c3bdff939bc562d640e02c3aebaa204e17/tmk_core/protocol/lufa/lufa.c#L1036

[fauxpark]

@zvecr the ATmega32A and 328P use V-USB, not LUFA.

[zvecr]

@fauxpark oh I know. I was implying that we should compare the two for inconsistencies and potentially align them.

[alex-ong]

@zvecr i was thinking something along the lines of
debounce(raw_matrix, debounced_matrix, changed)
becoming
debounce(raw_matrix, debounced_matrix, current_timestamp, changed)

That way you can feed it fake timestamps, though this is also hacky. That seems clunky though, especially for debounce algorithms that don't need the current_timestamp.

I'm glad that we've figured out the problem and are now just deliberating on the best solution though.

[yiancar]

This is now fixed

[zvecr]

Fixed by #8198