adafruit / Adafruit_nRF52_Bootloader

USB-enabled bootloaders for the nRF52 BLE SoC chips
MIT License
438 stars 393 forks source link

Improvement/fix of system wake up by BUTTON_DFU GPIO #196

Closed lyusupov closed 3 years ago

lyusupov commented 3 years ago

When an app is doing a system wake up by press and release of the BUTTON_DFU - the nRF bootloader makes unwanted entrance into DFU mode (_because of BUTTONDFU is active). This is not a behaviour that we all are looking for, right ?

This PR gives an option to bypass the bootloader's DFU mode by use of the MCU retention register.

This is an example of the feature usage in an Arduino sketch:

#define DFU_MAGIC_SKIP        0x6d

NRF_POWER->GPREGRET = DFU_MAGIC_SKIP;
pinMode(GPIO_BUTTON_DFU, INPUT_PULLUP_SENSE /* INPUT_SENSE_LOW */);

uint8_t sd_en;
(void) sd_softdevice_is_enabled(&sd_en);

if ( sd_en ) {
  sd_power_system_off();
} else {
  NRF_POWER->SYSTEMOFF = 1;
}
hathach commented 3 years ago

sorry for the late response, could you tell which mcu you are using 832 or 840 ? This is indeed an improvement, though I have a plan to probably remove BUTTON_DFU on 840, and rely solely on the double-reset, this may still be needed for 832 though.

lyusupov commented 3 years ago

which mcu you are using 832 or 840 ?

840

lyusupov commented 3 years ago

I think we should actually make it more generic to skip all other DFU mode and jump straight to app.

The focus of this particular PR is to fix the issue while applying minimal risk to break any other of the bootloader functions. I've also noticed a similar proposal from someone else that had been rejected in past: https://github.com/adafruit/Adafruit_nRF52_Bootloader/issues/162

Let me know if you have the time to make the changes, else I will do as follow up.

Please, feel yourself free to alter or extend this PR on your discretion.

hathach commented 3 years ago

I think we should actually make it more generic to skip all other DFU mode and jump straight to app.

The focus of this particular PR is to fix the issue while applying minimal risk to break any other of the bootloader functions. I've also noticed a similar proposal from someone else that had been rejected in past: #162

Let me know if you have the time to make the changes, else I will do as follow up.

Please, feel yourself free to alter or extend this PR on your discretion.

hmm, that is weird, now I don't see any reasons to reject that PR. I probably had a bad day at that time. I will see if I could revise and merge that one.

hathach commented 3 years ago

I have pushed the update to skip DFU entirely and merge PR. Thank you very much for bringing this up.

lyusupov commented 3 years ago

Thank you for this feature release!

I have one question for you to ask: When I do:

NRF_POWER->GPREGRET = DFU_MAGIC_SKIP;
pinMode(GPIO_BUTTON_DFU, INPUT_PULLUP_SENSE);
NRF_POWER->SYSTEMOFF = 1;

Will the 'double-reset' function to operate as expected afterwards ? ( Or the execution thread will proceed into the app at first ? )

hathach commented 3 years ago

with current code, it will skip the entire dfu including the double reset.

lyusupov commented 3 years ago

This means that, to achieve direct entrance into DFU mode from deep sleep, it will be necessary to click the reset button three times:

is that correct?

Use of three clicks instead of regular two is not too bad, but will look a bit confusing, isn't it?

hathach commented 3 years ago

It should not since pin reset should reset the gpregret. Have you tried it out on the hw

lyusupov commented 3 years ago

Yes, I confirm by my test on the hardware that two reset clicks are sufficient.

hathach commented 3 years ago

thank for confirmation

lyusupov commented 3 years ago

This test condition

  && !dfu_skip 

located here has no sense anymore. We should consider to remove it sometime, right?

hathach commented 3 years ago

yeah right, we should remove it. just make PR for it #204