qmk / qmk_firmware

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

Discussion: Bootmagic confuses people a lot #2389

Closed skullydazed closed 5 years ago

skullydazed commented 6 years ago

We've been seeing an uptick lately in bootmagic related issues. It seems that a lot of people accidentally trigger bootmagic while plugging in their keyboards and then aren't sure how to fix it. I've been talking with @jackhumbert a bit about this, and it seems there are several factors at play here:

To help with these situations we'd like to do the following:

Given the scope of the change, and the potential change in behavior, we'd like to collect community feedback. Please share your thoughts on this issue and/or the proposed solutions. This issue will remain open for at least one week, until 2/19, or the discussion dies down if it continues that long.

drashna commented 6 years ago

My vote is for it to be disabled by default.

But clear documentation of the feature and how to enable/disable the features is needed, regardless of what happens. As it stands, every time it comes up, I end up having to dig just to find the keys that enable it.

yanfali commented 6 years ago

I think the only issue may be PCBs that have lost their reset button, e.g. the DZ60 type C. On that keyboard you may actually want to leave it on as it's their only way to get into the bootloader without removing the PCB and shorting the MCU.

vifon commented 6 years ago

My vote is for leaving it enabled by default and documenting it thoroughly. Changing the default key might help too. Half of the time Bootmagic gets to be useful, the user actually discovers it for the first time, for example to flash a half-bricked keyboard without unscrewing it and reaching to the reset button.

wilba commented 6 years ago

You might want to consider a "bootmagic lite" which I use. I disable the full bootmagic and just call this in matrix_init_kb(), thus there is always a fail-safe way of both starting in bootloader and resetting the EEPROM state (which can be handy during development and also behaviour due to the EEPROM state being "stuck").

void bootmagic_lite(void)
{
    // The lite version of TMK's bootmagic.
    // 100% less potential for accidentally making the
    // keyboard do stupid things.

    // We need multiple scans because debouncing can't be turned off.
    matrix_scan();
    wait_ms(DEBOUNCING_DELAY);
    matrix_scan();

    // If the Esc and space bar are held down on power up,
    // reset the EEPROM valid state and jump to bootloader.
    // Assumes Esc is at [0,0] and spacebar is at [4,7].
    // This isn't very generalized, but we need something that doesn't
    // rely on user's keymaps in firmware or EEPROM.
    if ( ( matrix_get_row(0) & (1<<0) ) &&
        ( matrix_get_row(4) & (1<<7) ) )
    {
        // Set the Zeal60 specific EEPROM state as invalid.
        eeprom_set_valid(false);
        // Set the TMK/QMK EEPROM state as invalid.
        eeconfig_disable();
        // Jump to bootloader.
        bootloader_jump();
    }
}

It's not generalized enough to put into common code, but something like this can give users a virtual "reset button" regardless of their keymap. I've lost count of the number of user support issues that were solved just by telling the user to "hold space+esc when you plug in USB".

alex-ong commented 6 years ago

leave it enabled and make it more visible. Disabling Bootmagic is bad for things like http://kbfirmware.com/ since a large amount of first time users don't pull the source code and build themselves. Even something small like a link to boot-magic explained on the kbfirmware.com website would probably be a good enough "fix". Obviously a better fix would be for kbfirmware.com to support the common build things (e.g. disable mouse keys, disable one shot, disable this disable that in a list of tick boxes so the online-compiler can just do it for them)

the first time i built a keyboard powered by TMK i used kb.sized.io. I wondered why N Key rollover wasn't working, and i wasn't bothered to go download the source and AVR toolchain to compile. A quick google told me about boot-magic and space+n.

henrebotha commented 6 years ago

We absolutely must add something about this to the FAQ.

@alex-ong I hear you, but to me that just says that external projects that depend on QMK need to stay up to date with QMK changes. We shouldn't be hamstrung by compatibility changes. If nothing else, kbfirmware.com et al can just pin their dependency to the last commit before we disable bootmagic by default.

lbibass commented 6 years ago

Yeah, bootmagic has saved my butt many times, especially with the DZ60 type-c. Please keep it enabled, at least for specific builds/PCBs that might need it badly.

skullydazed commented 6 years ago

Thanks for the feedback so far everyone. Keep it coming!

I think both @drashna and @yanfali make good points about what the default should be. Comments seem to be in favor of keeping it enabled right now due to boards (like the dz60) that make the hardware reset button hard or impossible to access, but the thumbs up seem to favor disabling it. We won't commit one way or the other yet but will continue to listen to feedback.

@alex-ong You make some good points but we don't control that site. It's also on a pretty old version of qmk at this point. We're working on our own configurator (http://config.qmk.fm) that is currently in an alpha state that can hopefully address those points.

henrebotha commented 6 years ago

@skullydazed Bit off topic but I get a cert warning from Chrome on config.qmk.fm.

skullydazed commented 6 years ago

@henrebotha looks like we don't have SSL enabled for that yet. Thanks for the note I've edited my post.

drashna commented 6 years ago

Yeah, for the boards that have hard to access reset buttons, or no reset buttons, then leaving it enabled would be a good idea. However, as long as they have the RESET code in the keymap, then they shouldn't need the bootmagic option either.

But also, something that should be kept in mind (both for and against) is that this can always be override "per user". (and having the rules.mk option available in the configurer would be perfect)

drashna commented 6 years ago

Also, as mentioned in the gitter chat: Command and bootmagic heavily overlap. Documentation should include this (state it, and even list both methods for each action)

yanfali commented 6 years ago

I'd be happy to draft an initial markdown documenting what I can understand of bootmagic. The bits I don't really get are DEBUG related. That would probably require a core dev to chime in. Let me know if you'd like a PR to start on that.

skullydazed commented 6 years ago

@yanfali That'd be great to get a start on it. I haven't had a chance to write anything yet so I encourage you to write up what you can and let others fill in the blanks.

skullydazed commented 6 years ago

Thanks for the feedback everyone. I've incorporated many of these suggestions into the documentation. Special thanks to @yanfali for kicking that off, his PR provided a good base to build from. I'm considering what we have complete for now, but if you have suggestions or corrections for the docs please let us know either as a PR or a comment here.

Next steps are to consider behavioral changes.

As a first step I'm going to disable both bootmagic and command on most community maintained keyboards. I have heard a good argument made that the dz60 should continue to have one or the other enabled by default, as some versions of those do not have hardware RESET buttons. If you think there are other boards that should have a similar exemption from this change please suggest it here, along with your rationale.

I keep thinking about @Wilba6582's suggestion for a bootmagic lite. One way I think we can do that is to wrap individual bootmagic and command functions in #ifdef's, so they can be enabled and disabled as needed. This way most features would be disabled by default, but could be easily enabled by those who need them. Does anyone have any thoughts about this potential change?

yanfali commented 6 years ago

I like the idea of bootmagic lite. It would be a consistent constant while troubleshooting keyboards on r/mk and r/olkb

yiancar commented 6 years ago

+1 for the bootmagic lite, literally that would solve most problems with a simple keycombo

drashna commented 5 years ago

Aside from maybe the bootmagic lite stuff, I think this can be safely closed now, since we have several PRs that have expanded the bootmagic documentation.

yiancar commented 5 years ago

Agreed. I personally keep using bootmagic lite. At some point I might write some docs on how to create such function. as it should be individually made