Huge improvement in stability,reliability and speed, restored dual_bank support and signed firmware verification

[ejtagle]

This pull request fixes a race condition that was present in the original Nordic bootloader code that caused FLASH update failures randomly, and was the main culprit of the forced MTU=20 in the code.
Also, readded the DUAL BANK support (disabled by default) and SIGNED FIRMWARE verification (also disabled by default)

Without those fixes, it was very easy to crash the DFU process (tested on both android and ios)... 1 of 10 times it failed.
With this patch, it was impossible to crash (tested for about 1 month of daily/hourly updates, more than 300 updates, not a single crash!)

To be precise, when i say update, i mean Bluetooth DFU update

Hope this will be accepted! - Any doubts, just ask!

[dhalbert]

Please don't substitute tabs for spaces in the C code. A lot of the new code is using tabs for indentation.

[ejtagle]

Please don't substitute tabs for spaces in the C code. A lot of the new code is using tabs for indentation.

I saw that, I didn't know those tabs were "on purpose". Again, if you want to keep those tabs, i am fine with that also (In fact, i myself prefer tabs, but as most of the codebase was using spaces... ;) )

[fanoush]

this is big patch handling multiple unrelated issues, it would be better to submit each one separately so they are smaller and could be possibly merged separately(?)

1. bugfixes
2. dual bank update
3. encryption

BTW I am not maintainer of this project so this is just a suggestion, feel free to ignore it

[ejtagle]

this is big patch handling multiple unrelated issues, it would be better to submit each one separately so they are smaller and could be possibly merged separately(?)

1. bugfixes
2. dual bank update
3. encryption

BTW I am not maintainer of this project so this is just a suggestion, feel free to ignore it

I thought about that, but there are dependencies between patches, and reviewing them one by one, means opening a pull request after the previous one was merged. That means probably more than a year in time of reviews? ... Are you willing to spend/wait that time ? .. ;)
I also don't have that kind of time available. I prefer to answer all the questions here about each commit. The commits themselves can be cherry picked if some of them are not desired (but I think all of them are interesting, if trying to develop a commercial product ;) )

Besides, my repo is also available, in "open source" projects, the risk is that a folk could become the main repository, if it fixes problems the previous main repository does not fix, and does not introduce new bugs. I truly want to contribute to the main repo, don't want to be the "new unofficial" arduino bootloader repo for the nrf52!

[Copilot]

Pull request overview

This PR significantly improves Bluetooth DFU reliability and adds support for dual-bank firmware updates and signed firmware verification. The changes fix a critical race condition in the Nordic bootloader that caused random flash update failures, allowing higher MTU values and improving transfer speeds.

Changes:

• Fixed race condition in Nordic bootloader causing flash update failures
• Increased MTU from 20 to 247 and HCI RX buffer queue size from 8 to 16
• Added dual-bank and signed firmware support (both disabled by default)

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
src/usb/usb_desc.c	Conditionally compiles MSC/UF2 descriptors based on SIGNED_FW flag
src/usb/usb.c	Conditionally initializes UF2 module when not using signed firmware
src/usb/tusb_config.h	Conditionally enables MSC device class
src/sdk_config.h	Increases HCI RX buffer queue size from 8 to 16
src/main.c	Increases BLE parameters (MTU, event length, queue sizes) and enables 2M PHY
src/flash_nrf5x.h	Updates function signatures for erase and write operations
src/flash_nrf5x.c	Refactors flash write to handle page boundaries correctly
src/dfu_init.c	Adds signed firmware verification using tinycrypt library
lib/tinycrypt	Adds tinycrypt submodule for cryptographic operations
lib/sdk11/components/libraries/bootloader_dfu/dfu_types.h	Adds dual-bank region definitions
lib/sdk11/components/libraries/bootloader_dfu/dfu_transport_ble.c	Implements latency management to prioritize flash writes
lib/sdk11/components/libraries/bootloader_dfu/dfu_single_bank.c	Adds DFU timeout handling and refactors erase operations
lib/sdk11/components/libraries/bootloader_dfu/dfu_dual_bank.c	New file implementing dual-bank DFU support
lib/sdk11/components/libraries/bootloader_dfu/dfu_bank_internal.h	Adds DFU timeout interval definition
lib/sdk11/components/drivers_nrf/pstorage/pstorage_raw.c	Fixes race condition in flash operation handling
lib/sdk11/components/ble/ble_services/ble_dfu/ble_dfu.c	Retries notification sends on resource errors
README.md	Documents dual-bank and signed firmware features
Makefile	Adds build support for signed firmware and dual-bank options
.gitmodules	Adds tinycrypt submodule

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[hathach]

Hm, it has been a while and Nordic may update their nrf dfu app. I couldn't dfu with latest ios and android app both with this pr and current tip of master. I may miss/forgot something. @ejtagle could you provide a bit more on your test setup especially os and app version

PS: I see we need to change the DFU app's Number of Packet to something small, seems to be HCI_RX_BUF_QUEUE_SIZE. Somehow the default value is too high i.e 23 #306 (comment)

[ejtagle]

Hm, it has been a while and Nordic may update their nrf dfu app. I couldn't dfu with latest ios and android app both with this pr and current tip of master. I may miss/forgot something. @ejtagle could you provide a bit more on your test setup especially os and app version

PS: I see we need to change the DFU app's Number of Packet to something small, seems to be HCI_RX_BUF_QUEUE_SIZE. Somehow the default value is too high i.e 23 #306 (comment)

Yes, PRN must be set to 8 or less. The Main reason is that writing to flash is handled asynchronously, so most newer phones are able to send data faster than the bootloader is able to write them: eventually the bootloader runs out of buffers, and flashing fails.

Enabling prn=8 forces the Mobile app to wait until data is actually written to flash

[fanoush]

The Main reason is that writing to flash is handled asynchronously,

well, not exactly, when nrf52 is writing to flash the cpu is completely halted including interrupts

The CPU is halted if the CPU executes code from the flash while the NVMC is writing to the flash
the 'if the CPU executes code from the flash' was added for 52840, it was not there for 52832, so maybe with 52840 code copied to RAM would run, but since the softdevice is in flash it does not matter, everything is halted while writing to flash

so most newer phones are able to send data faster than the bootloader is able to write them: eventually the bootloader runs out of buffers, and flashing fails.

softdevice is scheduling flash writes only when it would not collide with BLE timing, so increasing MTU, making connection interval shorter,allowing more packets per interval and otherwise tuning BLE for speed is counterproductive in this case. So to allow more stable experience even without enabling notifications it would be probably better to make BLE timings more conservative and find good balance betwen flash and BLE speed

EDIT: notifications add even more BLE traffic so there is even less spare time for flash writes, so it would be best if PRN could be turned off, then the upload speed could be better

[ejtagle]

The Main reason is that writing to flash is handled asynchronously,

well, not exactly, when nrf52 is writing to flash the cpu is completely halted including interrupts

The CPU is halted if the CPU executes code from the flash while the NVMC is writing to the flash the 'if the CPU executes code from the flash' was added for 52840, it was not there for 52832, so maybe with 52840 code copied to RAM would run, but since the softdevice is in flash it does not matter, everything is halted while writing to flash

so most newer phones are able to send data faster than the bootloader is able to write them: eventually the bootloader runs out of buffers, and flashing fails.

softdevice is scheduling flash writes only when it would not collide with BLE timing, so increasing MTU, making connection interval shorter,allowing more packets per interval and otherwise tuning BLE for speed is counterproductive in this case. So to allow more stable experience even without enabling notifications it would be probably better to make BLE timings more conservative and find good balance betwen flash and BLE speed

EDIT: notifications add even more BLE traffic so there is even less spare time for flash writes, so it would be best if PRN could be turned off, then the upload speed could be better

You are absolutely right, and at the same time, wrong... Trust me, i've tried what you say, and was unable to increasre speed: Let me try to explain ...

-Execution is halted while writing/erasing flash: TRUE
-Softdevice is scheduling flash writes only when it would not collide with BLE timing:TRUE
-Increasing MTU, is counterproductive in this case: NOT TRUE

Why: because what softdevice schedules is the call to write a block of flash, not each byte, each block. If the block is bigger, then there are less blocks to write, so transferring the whole firmware image takes less blocks and writing is much faster.

Regarding PRN... Yes, notificaciones take bandwidth, so they slowdown firmware Transfer.

BUT writes are asynchronous to the reception of packets, there is no way to notify the Mobile app that the write has completed (besides the PRN notification), so we end with the same problem... We need to límit the write speed and the PRN is the only mechanism allowed by the nordic DFU protocol...
And, unfortunately, there is not enough RAM to cache huge blocks... Maximizing the block size is key to increase write speed

Each packet write ends calling dfu_data_pkt_handle , and that function calls pstorage_store... That schedules a flash write. So, the bigger the packets, the less writes to flash and the faster transfers we got

[fanoush]

Why: because what softdevice schedules is the call to write a block of flash, not each byte, each block. If the block is bigger, then there are less blocks to write, so transferring the whole firmware image takes less blocks and writing is much faster.

that is not how the hardware works, writing is done by CPU in 32bit values one by one (and checking READY(NEXT) register before or after each write), there is no DMA that would do it for you with CPU being free to do something else = it cannot write 'blocks' that would be somehow faster like you say. If you send bigger block softdevice splits it internally into smaller parts according to timings so it can be sure it will not miss any important interrupt or event when CPU is halted by writing. So the speed of flash writing has some upper limit (see tWRITE timing in docs linked above) and is slowed down only by softdevice deciding that it is not safe to write next 32bit value now.

The scheduling of flash writes internally is similar to how softdevice Radio Timeslot API works. In fact it would be possible to do the flash writing ourselves by our custom code inside timeslot api callback since the code there has full access to all hardware and can poke NVMC registers directly.

In ideal state when softdevice is not bothered by BLE restrictions, the CPU writing speed into the flash is constant no matter how big or small 'blocks' are and so it does not make sense to accept data much faster. Even if you have some buffers in RAM, they will fill up and you need to wait for writing them in the end anyway so total time before the device flashing is done and it can safely reboot into new firmware will be the same.

We need to límit the write speed and the PRN is the only mechanism allowed by the nordic DFU protocol...

You can also limit writing speed by configuring MTU and connection interval so the phone is simply not able to send the data much faster than the device is able to write into flash.

[fanoush]

There is nice summary of the flash writing issue here https://docs.nordicsemi.com/bundle/sds_s140/page/SDS/s1xx/flash_mem_api/flash_mem_api.html
"Make flash writes in as small chunks as possible to increase the probability of success and reduce the chance of affecting Bluetooth Low Energy performance."

[ejtagle]

Why: because what softdevice schedules is the call to write a block of flash, not each byte, each block. If the block is bigger, then there are less blocks to write, so transferring the whole firmware image takes less blocks and writing is much faster.

that is not how the hardware works, writing is done by CPU in 32bit values one by one (and checking READY(NEXT) register before or after each write), there is no DMA that would do it for you with CPU being free to do something else = it cannot write 'blocks' that would be somehow faster like you say. If you send bigger block softdevice splits it internally into smaller parts according to timings so it can be sure it will not miss any important interrupt or event when CPU is halted by writing. So the speed of flash writing has some upper limit (see tWRITE timing in docs linked above) and is slowed down only by softdevice deciding that it is not safe to write next 32bit value now.

The scheduling of flash writes internally is similar to how softdevice Radio Timeslot API works. In fact it would be possible to do the flash writing ourselves by our custom code inside timeslot api callback since the code there has full access to all hardware and can poke NVMC registers directly.

In ideal state when softdevice is not bothered by BLE restrictions, the CPU writing speed into the flash is constant no matter how big or small 'blocks' are and so it does not make sense to accept data much faster. Even if you have some buffers in RAM, they will fill up and you need to wait for writing them in the end anyway so total time before the device flashing is done and it can safely reboot into new firmware will be the same.

We need to límit the write speed and the PRN is the only mechanism allowed by the nordic DFU protocol...

You can also limit writing speed by configuring MTU and connection interval so the phone is simply not able to send the data much faster than the device is able to write into flash.
,
We are thinking and talking the same things, but there is something that is not completely right on the assumptions Nordic did when they wrote that article:
-We all agree that the cpu gets halted while FLASH writes are being carried out
-We all agree that FLASH is always written in 32 Words
-I think we all agree the softdevice is a state machine that is being run by interrupts (Rx complete, timers, events complete)
-Softdevice restricts access to flash write/erase registers

So, basically, the only thing that the softdevice must do is to calculate how much time will be needed to write / erase data to flash, and if halting the cpu for that time would interfere with the scheduling of reception/transmission of ble packets, and if that happens, split writes into several parts... Erases can't be split, but writes can be (and the bootloader does not do erase while receiving firmware packets, it erases the full flash before that.
... So, in our case, that we are just behaving as a client (Peripheral role), there will be nearly no conflict at all: in fact, I have never seen flash write failures
Even reducing MTU to 20 bytes, the phone is able to overrun the flash write queue...
I suspect the phrase ""Make flash writes in as small chunks as possible to increase the probability of success and reduce the chance of affecting Bluetooth Low Energy performance." Is outdated , as block writes can be split in 32words units by the softdevice, so, only if setting connection intervals to less than tWrite, and slave latency to 0, and requiring to send notifications or any packets from the bootloader to the phone could make a flash write to fail - none of those assumptions are true in our case
Basically, we don't need to deal with flash write failures. The only thing that will happen is the splitting of blocks of flash writes into smaller pieces transparently by the softdevice, and throttling of the average speed of writes ... Giving the softdevice larger blocks allows it a more aggressive scheduling of flash writes, and results in higher write throughout (but, of course, too much data can overrun the internal bootloader buffers.. so you end up requiring prn=8 to keep as full as possible the write queue without overrunning it, and allow softdevice to be more aggressive at writing data faster 😉

[hathach]

out of curiosity, is this the main race condition that causes the crash sometime while DFUing ?

[hathach]

perfect !! Thank you very much, I don't mind adding dual bank + signed firmware support as long as it is disabled by default since keep the current behavior intact. The rest changes look great, It is still not clear to me which part is the race condition (please elaborate for the reference), but it surely got resolved. And I am happy to merge this when ci passed.

PS: I made a typo fix along with other clean up, update readme to state PRN <= 8 and other minor clean up. Also fix the dfu_bl_image_validate() return value as well.

[fanoush]

Erases can't be split

actually for 52840 they can, they cared to add it as well as READYNEXT for faster writing, 52832 did not have it, so let's hope the implemented both in S140 to ease the pain of flash writing :-)

EDIT: oh, the READYNEXT is broken when running from flash and won't help anyway so it is not used https://docs.nordicsemi.com/bundle/errata_nRF52840_Rev2/page/ERR/nRF52840/Rev2/latest/anomaly_840_233.html
https://devzone.nordicsemi.com/f/nordic-q-a/77666/does-the-softdevice-use-readynext-in-sd_flash_write-nrf52840-errata-rev2-233-nvmc-readynext-not-generated , it would help only when running from RAM