Bootloader misinterpets gpregret bit fields

[markzac]

This has been noted a couple of times but your treatment of bit fields is just wrong in the bootloader. It not only disagrees with the documentation but it's so flawed that it interferes with valid gpregret operations and consistent upgrade techniques. Can you please please look into this? I'm including my recommended changes here. I think you could argue the first two, although not convincingly. The third is just wrong - it's not checking the bit fields correctly.

Thank you,

 Mark

diff --git a/components/libraries/bootloader/nrf_bootloader.c b/components/libraries/bootloader/nrf_bootloader.c
index 19bdc9c9..4d2e59ed 100644
--- a/components/libraries/bootloader/nrf_bootloader.c
+++ b/components/libraries/bootloader/nrf_bootloader.c
@@ -246,7 +246,7 @@ static bool crc_on_valid_app_required(void)
((nrf_power_gpregret2_get() & BOOTLOADER_DFU_GPREGRET2_MASK) == BOOTLOADER_DFU_GPREGRET2)
&& (nrf_power_gpregret2_get() & BOOTLOADER_DFU_SKIP_CRC_BIT_MASK))
{
- nrf_power_gpregret2_set(nrf_power_gpregret2_get() & ~BOOTLOADER_DFU_SKIP_CRC);
+ nrf_power_gpregret2_set(nrf_power_gpregret2_get() & ~BOOTLOADER_DFU_SKIP_CRC_BIT_MASK);
ret = false;
}
else
@@ -332,7 +332,7 @@ static void dfu_enter_flags_clear(void)
&& (nrf_power_gpregret_get() & BOOTLOADER_DFU_START_BIT_MASK))
{
// Clear DFU mark in GPREGRET register.
- nrf_power_gpregret_set(nrf_power_gpregret_get() & ~BOOTLOADER_DFU_START);
+ nrf_power_gpregret_set(nrf_power_gpregret_get() & ~BOOTLOADER_DFU_START_BIT_MASK);
}

if (NRF_BL_DFU_ENTER_METHOD_BUTTONLESS &&
@@ -370,7 +370,8 @@ static bool dfu_enter_check(void)
}

if (NRF_BL_DFU_ENTER_METHOD_GPREGRET &&
- (nrf_power_gpregret_get() & BOOTLOADER_DFU_START))
+ ((nrf_power_gpregret_get() & BOOTLOADER_DFU_GPREGRET_MASK) == BOOTLOADER_DFU_GPREGRET)
+ && (nrf_power_gpregret_get() & BOOTLOADER_DFU_START_BIT_MASK))
{
NRF_LOG_DEBUG("DFU mode requested via GPREGRET.");
return true;

[Sigurd]

Hi,

Thank you for reporting this.

markzac said:

think you could argue the first two, although not convincingly. The third is just wrong - it's not checking the bit fields correctly.

BOOTLOADER_DFU_START and BOOTLOADER_DFU_SKIP_CRC is defined like this,

#define BOOTLOADER_DFU_START    (BOOTLOADER_DFU_GPREGRET | BOOTLOADER_DFU_START_BIT_MASK)      /**< Magic number to signal that bootloader should enter DFU mode because of signal from Buttonless DFU in main app.*/
#define BOOTLOADER_DFU_SKIP_CRC (BOOTLOADER_DFU_GPREGRET2 | BOOTLOADER_DFU_SKIP_CRC_BIT_MASK)  /**< Magic number to signal that CRC can be skipped due to low power modes.*/

so

nrf_power_gpregret2_set(nrf_power_gpregret2_get() & ~BOOTLOADER_DFU_SKIP_CRC);

nrf_power_gpregret_set(nrf_power_gpregret_get() & ~BOOTLOADER_DFU_START);

should be correct.

But I agree with you that the third one looks wrong. I will report that to the developers.

[testy]

Sigurd said:

I will report that to the developers.

Thanks for looking, but issue still persists (although reported several times) and SDK 17.0.0 is out uncorrected.

With above mentioned rectifications my bootloader runs, when GPREGRET = 0xB1, or 0xB3 or 0xB5 or 0xB7 (application sees then LSbit cleared, i.e. 0xB0, 0xB2, 0xB4, 0xB6), doesn't run, when GPREGRET = 0xB9, 0xBB, 0xBD, 0xBF.

But bootloader doesn't hang forever (contrary to SDK 15-17), when GPREGRET != 0xB1 (fortunately).