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

[WIP] Reset source detection #5

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Duckle29
Copy link

@Duckle29 Duckle29 commented Sep 8, 2019

The ATMega32U4 has the hardware required to detect what the source of a reset is. Currently QMK doesn't use this, and as such requires the use of HWBE fuse and a pull-down on PE2.

Enabling this check would allow QMK-DFU to not require any components.

Bootloaders/DFU/makefile Outdated Show resolved Hide resolved
@drashna
Copy link
Member

drashna commented Sep 8, 2019

And honestly, I'd like to see a change more like:

		#ifdef CHECK_RESET_SOURCE
			if (!(MCUSR & (1 << EXTRF)) || (MagicBootKey == MAGIC_BOOT_KEY))
			//  JumpToApplication = true;				{
				JumpToApplication = true;
			}
		#endif

And in the makefile, adding something like:

ifeq ($(strip $(CHECK_RESET_SOURCE)), yes)
    OPT_DEFS += -DCHECK_RESET_SOURCE
endif

This would require changing this line:
https://github.com/qmk/qmk_firmware/blob/master/tmk_core/avr.mk#L328

To something like:

	make -C lib/lufa/Bootloaders/DFU/ MCU=$(MCU) ARCH=$(ARCH) F_CPU=$(F_CPU) FLASH_SIZE_KB=$(FLASH_SIZE_KB) BOOT_SECTION_SIZE_KB=$(BOOT_SECTION_SIZE_KB) CHECK_RESET_SOURCE=$(CHECK_RESET_SOURCE)

And adding CHECK_RESET_SOURCE = yes to your keyboard's rules.mk.

@fauxpark
Copy link
Member

fauxpark commented Sep 8, 2019

I'm not sure any of this is even needed. See this section of the BootloaderDFU documentation (apparently it is not doxygen'd ☹️):

https://github.com/abcminiuser/lufa/blob/6d9077370b613e9bf26d5d5e03481258873efa02/Bootloaders/DFU/BootloaderDFU.txt#L58-L70

If I understand this correctly, simply uncommenting the commented out lines will allow both situations to work. They are not commented out in upstream; I have no idea why it was done here - possibly because of this.

@Duckle29
Copy link
Author

Duckle29 commented Sep 8, 2019

It could also have been because without LTO it's too big. I don't think it's needed either but added the ifdefs just to be sure.

@fauxpark
Copy link
Member

fauxpark commented Sep 8, 2019

I'm doubtful of that, LTO has not really come into play until recently: abcminiuser#149

@fauxpark
Copy link
Member

fauxpark commented Sep 9, 2019

So, I can confirm by just uncommenting those lines, with HWB pulled low and default fuses (5E:99:C3), resetting from both the button and the keycode works. (I have to add LTO though, of course).
On cutting the HWB jumper and setting the BOOTRST fuse (5E:98:C3), I can only seem to reset with the physical button... is there something else I need to set/unset?

EDIT: Forgot about the HWBE fuse... but after changing that (CB) the RESET keycode just reboots the board. If I then change the BOOTRST fuse back to default, but keep the HWBE change, the reverse happens.

@Duckle29
Copy link
Author

Duckle29 commented Sep 9, 2019

It seems LUFA-DFU uses the magic key to escape the bootloader, which seems backward to what is described on the fourwalledcubicle site

The current check:

if (!(BootloaderAPI_ReadFuse(GET_HIGH_FUSE_BITS) & ~FUSE_BOOTRST))
{
/* If the reset source was not an external reset or the key is correct, clear it and jump to the application */
//if (!(MCUSR & (1 << EXTRF)) || (MagicBootKey == MAGIC_BOOT_KEY))
// JumpToApplication = true;
/* Clear reset source */
MCUSR &= ~(1 << EXTRF);
}

This check skips the bootloader either if the magic key is set, or if the reset wasn't the reset pin.

This means all resets but the reset pin jumps to the main application. Adding a further check for WDRF fixes the issue, but seems the wrong solution in my opinion. I think I'll have to ask in upstream LUFA if the use of the magic key to exit the bootloader is intended.

if ((!(MCUSR & (1 << EXTRF)) && (!(MCUSR & (1 << WDRF)))) || (MagicBootKey == MAGIC_BOOT_KEY))
{
	JumpToApplication = true;
}```

@Duckle29
Copy link
Author

Duckle29 commented Sep 9, 2019

So because these checks won't affect keyboards using the normal HWBE fuse bit, do we agree that the CHECK_RESET_SOURCE rule isn't required? I have to do a bit more thinking, but I currently don't think this can affect any current boards.

If the board has the BOOTRST fuse enabled, the first check will be used which (if my change to upstream is accepted) means it passes straight to the application on power-on and brown-out resets, entering bootloader on all other resets (Watch-dog-, external-, usb-, and jtag-reset)

I honestly don't know how it has worked before, as with the HWBE fuse set, the device will boot to bootloader if PE2 is low, which it always is on the pro-micro, in effect making it work just like BOOTRST, unless I have missed some documentation that causes HWBE to ignore PORF and BORF.

Since HWBE has been used as BOOTRST, the QMK fork will need a custom adaptation of the HWBE check, or rather use the BOOTRST check for both.

@drashna
Copy link
Member

drashna commented Sep 20, 2019

I still need to properly test this.

Also, there is a merge conflict on this now. If that could be resolved?

@drashna drashna requested a review from a team September 20, 2019 07:16
@Duckle29
Copy link
Author

@drashna Solved. Also as discussed on discord, I think HWBE only is relevant on actual reset pin resets, so no custom adaptation needed for that. That also makes more sense, it's just poorly documented in the datasheets that this is the case.

What do you think about removing the ifdef for this as discussed? Or is that what you meant you needed to test?

@drashna
Copy link
Member

drashna commented Sep 21, 2019

If that's the case, then removing the if-def should be fine.

My main concern is affecting other hardware configs, but if that's not a concern (and I vague remember that conversation, and it sounds like it's not), then that should be fine.

@Duckle29
Copy link
Author

I just changed the logic to the same logic in my upstream PR, and removed the ifdefs. As I see it, if a board has used QMK-DFU and BOOTRST prior to this, they have had a bricked keyboard that required flashing of a new keymap to function after any reset. Manual or power-up.

@Duckle29 Duckle29 changed the title Reset source detection [WIP] Reset source detection Oct 22, 2019
@drashna
Copy link
Member

drashna commented Oct 27, 2021

can probably close this now, as I think this is in the dfu bootloader again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants