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

nRF52840: MCU to MCU updates via activation pin and CircuitPython filesystem programming #283

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

Conversation

m-mcgowan
Copy link

@m-mcgowan m-mcgowan commented Oct 21, 2022

Overview

This PR implements changes in the bootloader to enable MCU to MCU updates, in a similar manner to the built-in ROM bootloaders provided on STM32L4 and ESP32 devices. The host MCU can programmatically enter bootloader mode by asserting a DFU pin on reset, which places the device in DFU bootloader mode, with the DFU protocol running over UART rather than USB serial.

Additionally, the bootloader protocol has been extended with commands to allow erasing and writing to QSPI flash. This can be used to flash a pre-prepared FAT filesystem image to QSPI flash, such as for use with CircuitPython, enabling factory programming of CircuitPython scripts along with the CircuitPython runtime.

All changes are opt-in only and are intended to be fully backwards compatible, meaning no changes to boards that don't have the new configuration flags defined.

Configuration Changes

Boards that support DFU activation via a pin should define DFU_PIN_ACTIVATION=1.

The button pin, DFU activation pin and FRST pin configurations now specify a pull direction to allow active high or low depending upon how that GPIO is used. The defaults reflect was was previously being used (BUTTON_PULL).

New optional defines PIN_DFU_ACTIVATE and PIN_DFU_ACTIVATE_PULL allow a particular board to define the DFU activation pin and pull direction. When these defines are present, DFU pin activation is compile-time enabled. These defines are present only for the Adafruit nRF52480 express board, which uses BUTTON_2 - feather pin D2. This was previously the FRST signal, but factory reset functionality has been removed prior to this PR, and so it was safe to reuse this.

The define DFU_EXTERNAL_FLASH enables the protocol extensions for programming the QSPI flash. It is defined as 0 by default, and defined as 1 only for the adafruit_nRF52480_express board.

When DFU activation pin is enabled, both USART and USB CDC serial are both compiled in, and selected at runtime. When the DFU pin is not asserted, the bootloader functions as it did before - USB CDC and MSD is enabled for use with adafruit-nrfjprog and UF2. UART is still used by default for boards with the MCU subvariant nrf52, as was the case prior to this PR.

Other Implementation Details

  • I added some critical sections around the hci memory pool and hci transport code, to prevent race conditions accessing shared state between the main program thread and code running on UART interrupts.

  • When DFU is started via the GPIO, the device is not restarted when the application is flashed, in case the MCU wishes to flash more binaries, such as the QSPI filesystem. bootloader_app_start() previously both registered the app and launched the app. The function has been factored out into functions so the app can be registered without necessarily starting it.

Testing

The code has been tested extensively on the Adafruit feather nrf52840 express, but not on any other boards. With the conditional compilation around most of the new DFU activation pin functionality and QSPI flash, regressions to other boards are not expected, although testing these would be prudent.

@m-mcgowan m-mcgowan changed the title nRF52840: MCU to MCU updates via activation pin and QSPI nRF52840: MCU to MCU updates via activation pin and CircuitPython filesystem programming Oct 21, 2022
@bsatrom
Copy link

bsatrom commented Nov 8, 2022

@hathach can you take a look at this PR, let us know if the changes are acceptable or if not, what we need to do to get it into a good spot for merging into main?

@hathach
Copy link
Member

hathach commented Nov 18, 2022

sorry, I was a bit busy for now, will check this out whenever I could !!

@bsatrom
Copy link

bsatrom commented Dec 9, 2022

Hey @hathach have you had a chance to review this yet?

@m-mcgowan
Copy link
Author

@hathach Is there any way I can help you with reviewing this? I'm happy to walk you though the PR and talk through the changes, the motivation behind them etc. It would be great to have this merged, so anything I can do to help just let me know. Thanks!

@hathach
Copy link
Member

hathach commented Mar 1, 2023

sorry for late response, I am in the middle of reviewing this, however look like the additional code cause the flash to be overflowed. Let me think how to shrink the size of bootloader quickly first, then we can merge to this and continue with the PR.

LD feather_nrf52840_express_bootloader-0.7.0-16-g047531a-dirty.out
/home/hathach/.local/xPacks/@xpack-dev-tools/arm-none-eabi-gcc/11.2.1-1.2.2/.content/bin/../lib/gcc/arm-none-eabi/11.2.1/../../../../arm-none-eabi/bin/ld: _build/build-feather_nrf52840_express/feather_nrf52840_express_bootloader-0.7.0-16-g047531a-dirty.out section `.text' will not fit in region `FLASH'
/home/hathach/.local/xPacks/@xpack-dev-tools/arm-none-eabi-gcc/11.2.1-1.2.2/.content/bin/../lib/gcc/arm-none-eabi/11.2.1/../../../../arm-none-eabi/bin/ld: region FLASH overflowed with .data and user data
/home/hathach/.local/xPacks/@xpack-dev-tools/arm-none-eabi-gcc/11.2.1-1.2.2/.content/bin/../lib/gcc/arm-none-eabi/11.2.1/../../../../arm-none-eabi/bin/ld: section .bootloaderConfig LMA [00000000000fd800,00000000000fd857] overlaps section .text LMA [00000000000f4000,00000000000fd9e7]
/home/hathach/.local/xPacks/@xpack-dev-tools/arm-none-eabi-gcc/11.2.1-1.2.2/.content/bin/../lib/gcc/arm-none-eabi/11.2.1/../../../../arm-none-eabi/bin/ld: region `FLASH' overflowed by 728 bytes
collect2: error: ld returned 1 exit status

PS: oops, never mind, it is due to my setup with debug option. All is good now, but I am making an PR to optimize space anyway. Will continue with reviewing

src/main.c Outdated
bootloader_app_start();
// register the app
uint32_t app_addr = bootloader_app_register();
if (!dfu_activate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we reset when dfu_activate=1, we can just skip the teardown and simply reset. Teardown is only needed when jumping to app. I will make changes accordingly.

@hathach
Copy link
Member

hathach commented Mar 1, 2023

I need to push changes to your fork to update PR, would you mind give me the write permission ?

@m-mcgowan
Copy link
Author

hathach

Sure thing, this is done! Thank you for starting to review.

// binaries to flash (e.g. QSPI flash data)
if (PIN_DFU_ACTIVATE_PRESENT && dfu_activate)
{
NVIC_SystemReset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reset immediately here with activation would reduce the modification of existing code. I don't have the set up to test, please double check to see if this commit stills work on your side. Then we can move on with the reviewing (it may take a couple more commits I think).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the tear down isn't strictly necessary, but it does no harm either, right?!. :-) I would vote for leaving the code as is, since it's already well tested. I only changed the code that was strictly necessary to keep the changes to the bare minimum.

If you feel very strongly for the change then of course we can test it.

Copy link
Member

@hathach hathach Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do the testing if possible, reset here will allow us to keep the rest of main.c unchanged, we could also revert changes to the bootloader.c file as well. I will also make a couple more commits anyway mostly to moving/refactoring changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will test this today and will post back with results. Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it didn't work. I can try to find out why later, but from the perspective of the programmer, everything was ok. All the packets were ACKed and it successfully programmed the CPy interpreter and an image for the flash filesystem. But on reset, the nRF stays in the bootloader.

I'll look deeper into it and check the memory contents and look for other clues as to why it's not working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, that is weird, probably one of the teardown function is indeed required. I will also try to do some check as well

Copy link
Author

@m-mcgowan m-mcgowan Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll review the changes today to see if I have any suggestions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the changes and retested for a few hours. Initially things weren't working for me, but eventually I managed to make it work with no code changes needed. I'm going to retest today (and write up the testing process since it's quite involved and easy to make a mistake.)

I'll let you know when the testing is done and we can merge this soon!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still seeing intermittent errors. It is like the application doesn't "stick".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the root cause, but we are able to make this work in cases when the application is the last binary flashed (so qspi filesystem then the app). When the application is flashed first, then the QSPI filesystem, the application isn't saved. I believe the error is on our end (most likely related to timing), because I see the same result when I revert the recent changes to the PR back to my original PR, which I know was working, so something else must have changed. So I would say this PR is good to merge as is.

@GlibSkunk
Copy link

I would also appreciate the DFU over UART pins for the Feather nRF52840 Express, so thanks for the work guys! One request though if possible... could a new "Magic Number" also be added to the GPREGRET which points directly to DFU over UART?

In my scenario I communicate with the feather through UART exclusively. In my application code, I would then put in a uart command to set GPREGRET to the "DFU_MAGIC_SERIAL_UART_ONLY_RESET" and trigger a reset. This would avoid requiring an additional pull on the DFU pin.

First time Github poster, so I hope this request isn't completely out of line.

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

Successfully merging this pull request may close these issues.

4 participants