-
Notifications
You must be signed in to change notification settings - Fork 420
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
base: master
Are you sure you want to change the base?
Conversation
…ctivation takes precedence. (Some solutions don't need FRST for BLE OTA updates, and those that do can define FRST as a separate pin)
…er being active when the DFU Activation pin is asserted
# Conflicts: # src/main.c
@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? |
sorry, I was a bit busy for now, will check this out whenever I could !! |
Hey @hathach have you had a chance to review this yet? |
@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! |
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.
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) { |
There was a problem hiding this comment.
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.
I need to push changes to your fork to update PR, would you mind give me the write permission ? |
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(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
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. |
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
andPIN_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.