Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bootmagic doesn't work. #6560

Closed
3 tasks
7ith1um opened this issue Aug 17, 2019 · 18 comments
Closed
3 tasks

Bootmagic doesn't work. #6560

7ith1um opened this issue Aug 17, 2019 · 18 comments

Comments

@7ith1um
Copy link

7ith1um commented Aug 17, 2019

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
Copy link
Member

drashna commented Aug 17, 2019

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

@drashna
Copy link
Member

drashna commented Aug 17, 2019

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

@yanfali
Copy link
Contributor

yanfali commented Aug 17, 2019

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.

@7ith1um
Copy link
Author

7ith1um commented Aug 18, 2019

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

@alex-ong
Copy link
Contributor

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
Copy link
Contributor

alex-ong commented Aug 18, 2019

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.

@7ith1um
Copy link
Author

7ith1um commented Aug 18, 2019

@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);.

@7ith1um
Copy link
Author

7ith1um commented Aug 18, 2019

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
Copy link
Contributor

alex-ong commented Aug 18, 2019

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

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

Rewrite debounce api to support is_bootloader()?

@7ith1um
Copy link
Author

7ith1um commented Aug 18, 2019

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

@7ith1um
Copy link
Author

7ith1um commented Aug 18, 2019

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

@fauxpark
Copy link
Member

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

@zvecr
Copy link
Member

zvecr commented Aug 18, 2019

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:

@fauxpark
Copy link
Member

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

@zvecr
Copy link
Member

zvecr commented Aug 18, 2019

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

@alex-ong
Copy link
Contributor

alex-ong commented Aug 18, 2019

@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
Copy link
Contributor

yiancar commented May 4, 2020

This is now fixed

@zvecr
Copy link
Member

zvecr commented May 7, 2020

Fixed by #8198

@zvecr zvecr closed this as completed May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants