Nrf52840 usbboot by hathach · Pull Request #1004 · adafruit/circuitpython

hathach · 2018-07-09T19:32:20Z

this PR add uf2 bootloader, also up-gradable via CDC with nrfutil and OTA. It also add the MSC with internal flash + CDC as serial for circuitpython. To go into uf2 mode, press button0 (on pca10056) while reset (double reset won't work for nrf series).

There is still (at least) some known bugs.

On Linux the very first time we connect to cp with minicom, there is ~10 seconds delay. Afterwards (welcome prompt is printed) the terminal can disconnect/reconnect and work normally. On windows, it seems to be OK. I am suspecting some background task is not run properly at first. Spend a few hours to debug, but I want to make the initial PR for testing first ( come back to this later)
The flash filesystem only work when SD is disabled. When SD is enabled, application basically only queue the flash operation (erase + write), SD will report complete asynchronously via callback (write buffer must remain valid until then). Some sync method is needed, we come back to this later.

@arturo182 to continue to use hwuart (jlink) for serial instead of usb cdc, you can change this macro from 0 to 1. The name of macro is not good at all, I will try to
find a better name for it later or if you could, please suggest one. https://github.com/adafruit/circuitpython/blob/nrf52840_usbboot/ports/nrf/boards/pca10056/mpconfigboard.h#L40

PS: First time I pushed an PR to circuitpy, let's me know if anything I should changes to get merged.

…usbboot

define CIRCUITPY_BOOT_OUTPUT_FILE will cause mp_hal_stdout_tx_strn() to invoke before serial_init() is called. Solution is skipped output to serial if it is not inited.

add crc bypass magic to bootloader settting

[WIP] nRF52840 USB Bootloader

… Makefile since several board use it

dhalbert · 2018-07-10T17:48:48Z

Let's get this merged into master, and then you can work in master instead of a branch, and submit PR's more often. Then we can keep up with each other's work. We try to have each PR add a particular feature or solve a particular bug, as opposed to adding multiple things that have been in the works for weeks.. It's fine to have multiple PR's and it makes it easier to review each set of changes.

dhalbert · 2018-07-10T17:52:29Z

In other words: fork your own hathach/circuitpython. Keep it up to date with adafruit/circuitpython. Work in a feature or bugfix branch. When you're ready to merge, submit a PR for that branch, after bringing the branch up to date if necessary with master. After the PR is accepted, you update your fork from master, then delete the branch, and start a new branch for the next thing.

This workflow is covered in detail in @kattni's new Guide: https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github.

hathach · 2018-07-10T17:53:42Z

Ok, so after merged, I will fork the cp. Then work on my fork and submit PR from there, right? I kind of new with the PR things on cp repo

hathach · 2018-07-10T17:54:42Z

Ah. Super thanks, I am totally new. Never read that doc before :)

dhalbert · 2018-07-10T17:55:41Z

Yes, exactly. It's somewhat more complicated but it's fairly standard, and it keeps adafruit/circuitpython clean from work branches. You have the freedom to push to your fork whenever you want for backup purposes.

hathach · 2018-07-10T17:58:09Z

Thanks for explanation @dhalbert :D

arturo182 · 2018-07-10T18:02:37Z

Are we sure we want to merge 50+ commits without any squashes?

dhalbert · 2018-07-10T18:04:16Z

@hathach Feel free to contact any of us: @dhalbert, @tannewt, @kattni, @arturo182 for explanations, either in GitHub, HipChat, or discord. We're all happy to get you going on this -- it's not intuitive and takes some rote learning to get it right.

dhalbert · 2018-07-10T18:06:13Z

@arturo182 Yes, we're fine with many commits. It represents a lot of work and the individual commits can be helpful when bisecting or just looking back. We used to try to keep a cleaner commit chain, doing rebases more often, etc., and it just caused more issues and was harder to explain. Now we're fine with merge commits, etc. The history is already huge and a few more commits doesn't matter.

arturo182 · 2018-07-10T18:11:22Z

Ok them :) But they're still the issue that this PR is in conflict with #1011 so maybe that one can be merged and then this one could be fixed?

dhalbert · 2018-07-10T18:13:55Z

Right. #1011 first and then fix this one to remove the merge conflicts. I can work on that if doing the updating to master is difficult on this one. But I will wait until the code changes requested by you and @tannewt are done.

tannewt

Looks good to me. @arturo182 and @dhalbert should approve as well. Thanks!

hathach · 2018-07-11T14:36:51Z

resolve conflict with #1011

arturo182 · 2018-07-11T14:49:25Z

-# If the build directory is not given, make it reflect the board name
-BUILD ?= $(if $(SD),build-$(BOARD)-$(SD_LOWER),build-$(BOARD))
+# Build directory with SD
+BUILD = $(if $(SD),build-$(BOARD)-$(SD_LOWER),build-$(BOARD))


Unsure why we would want to remove the capability to override the build directory? I think the ?= should stay.

ah right, sorry, I tried to get SD default by board (e.g pca10056 default is s140, feather52832 is S132) and thought it would be fixed. Just change include order a bit to get that.

arturo182 · 2018-07-11T14:52:14Z

+#include "tusb.h"
+
+void run_background_tasks(void) {
+#ifdef NRF52840_XXAA


I'm wondering if there's a better way to do this, could we only include this file if we're building for 840, then we wouldn't need the macro check

There could be other background service running besides usb, e.g ble task handling.

arturo182 · 2018-07-11T14:53:04Z

-boot-flash:
-	@:$(call check_defined, SERIAL, example: SERIAL=/dev/ttyUSB0)
-	$(NRFUTIL) dfu serial --package $(BOOTLOADER_PKG) -p $(SERIAL) -b 115200
+NRF_DEFINES += -DADAFRUIT_FEATHER52


I don't think this define is used anywhere, I have removed it before in another PR.

arturo182 · 2018-07-11T14:53:25Z

-boot-flash:
-	nrfjprog --program $(BOOTLOADER_FILENAME).hex -f nrf52 --chiperase --reset
+NRF_DEFINES += -DNRF52840_XXAA
+NRF_DEFINES += -DADAFRUIT_FEATHER52840


Same with this define

arturo182 · 2018-07-11T14:58:16Z

 #define CIRCUITPY_AUTORELOAD_DELAY_MS 500
+
+// Temp (could be removed) 0: usb cdc (default), 1 : hwuart (jlink)
+#define CFG_HWUART_FOR_SERIAL        0


this should probably have a better name, fitting the define convention with reversed logic like MICROPY_HW_USB_CDC ?

arturo182 · 2018-07-11T14:58:30Z


+
 NRF_DEFINES += -DNRF52840_XXAA
+NRF_DEFINES += -DADAFRUIT_FEATHER52840


arturo182 · 2018-07-11T14:59:09Z

+/* Internal Flash API
+ *------------------------------------------------------------------*/
+static inline uint32_t lba2addr(uint32_t block) {
+  return ((uint32_t)__fatfs_flash_start_addr) + block * FILESYSTEM_BLOCK_SIZE;


Wrong indent level

you got an eagle eye :D

arturo182 · 2018-07-11T14:59:42Z

+uint32_t  internal_flash_get_block_size(void);
+uint32_t  internal_flash_get_block_count(void);
+void      internal_flash_irq_handler(void);
+void      internal_flash_flush(void);


No need to align functions

I find it easier to read :D

arturo182 · 2018-07-11T15:04:22Z


 #if (MICROPY_PY_BLE_NUS == 0)
+
+#if !defined( NRF52840_XXAA) || ( defined(CFG_HWUART_FOR_SERIAL) && CFG_HWUART_FOR_SERIAL == 1 )


If you define a default value in mpconfigport.h then no need to check for defined

It is temporarily, it is only defined in pca10056, feather52840 won't have it since it does not have jlink to begin with. I am still not sure if we should keep it hmmm

I guess it might be ok to remove the define and just have it always on, as you say, the feather doesn't have jlink, the dongle doesn't have jlink and the PCA10056 has a separate USB connector for the nRF USB, so everyone can use the CDC.

Ok, but let's remove it later when CDC is tested reliably enough, we still need a easy switch to jlink for comparison for now.

arturo182 · 2018-07-11T15:08:32Z

+*/
+/**************************************************************************/
+
+#ifdef NRF52840_XXAA


No need for the macro, we only include this file in the Makefile if we are building for nrf52840

tannewt

The build is failing because the ports/atmel-samd/peripherals submodule is deleted. Try git submodule update --init --recursive from the top level.

hathach · 2018-07-11T19:25:58Z

@tannewt superb !!! Thanks a lot, that fixes the annoying travis build failed. Somehow it is deleted by accident .

arturo182 · 2018-07-11T19:41:04Z

Hey @hathach looks like the docs fail to build, make sure to exclude the tinyusb directory from the doc build like I did here: 0686857

hathach · 2018-07-12T08:41:41Z

@arturo182 thanks, finally the travis build is passed now :)

arturo182 · 2018-07-12T09:04:40Z

Ok then, I think we can merge this and make a new PR fixing rest of my comments :)

tannewt · 2018-07-12T18:47:22Z

Thank you all!

hathach and others added 30 commits April 24, 2018 00:19

add bootloader hex

50e7a4c

add missing files

ce9f05f

Added note on USB CDC bootloader

0de8697

Added built to example

d0e00d7

update gitignore

3d8bb96

Merge branch 'master' into usbboot

5365e51

Merge branch 'usbboot' of github.com:microbuilder/circuitpython into …

dadf831

…usbboot

update the bootloader hex for feather52840

05f36eb

fix #809

53593d1

define CIRCUITPY_BOOT_OUTPUT_FILE will cause mp_hal_stdout_tx_strn() to invoke before serial_init() is called. Solution is skipped output to serial if it is not inited.

Added board specific READMEs

78f806c

update nrfutil to 0.5.2d with singlebank option

52ebfd0

Merge branch 'master' into usbboot

6d4a2f5

Merge remote-tracking branch 'upstream/master'

9bb4ded

Merge branch 'master' into usbboot

c854e2d

allow to flash feather52 and feather52840 with jlink

c10899d

add crc bypass magic to bootloader settting

add feather nrf52 uart rts, cts support

2b3436e

update nrf52840 bootloader hex and target

418e28c

update nrf52840 bootloader hex

35d38db

Merge pull request #934 from microbuilder/usbboot

9fc0ec6

[WIP] nRF52840 USB Bootloader

Merge branch 'master' into nrf52840_usbboot

6d2702e

update bootloader for feather nrf52840

6143710

Merge branch 'master' into nrf52840_usbboot

f5be5ed

update bootloader hex with uf2

4722371

update tools/uf2 to support hex input for uf2conv.py

4d45565

add uf2, bootloader target

5454768

update reamde

8d065fe

update pin map for feather52840

ecd3993

update bin files

bed7310

move dfu-gen dfu-flash dfu-bootloader bootloader uf2 target to common…

4920cc4

… Makefile since several board use it

clean up makefile

8fbe1d7

tannewt previously approved these changes Jul 10, 2018

View reviewed changes

arturo182 mentioned this pull request Jul 11, 2018

Add board files for PCA10059 - the nRF52840 dongle #996

Closed

Merge branch 'master' into nrf52840_usbboot

10846d3

hathach dismissed tannewt’s stale review via 10846d3 July 11, 2018 14:35

arturo182 suggested changes Jul 11, 2018

View reviewed changes

tannewt reviewed Jul 11, 2018

View reviewed changes

hathach added 3 commits July 12, 2018 01:37

merge review

f98c7f7

clean up

4b1f880

fix travis build

873e202

exclude usb from doc build

862ae2f

arturo182 approved these changes Jul 12, 2018

View reviewed changes

tannewt merged commit 75f48a5 into master Jul 12, 2018

dhalbert deleted the nrf52840_usbboot branch August 1, 2018 03:25



		NRF_DEFINES += -DNRF52840_XXAA
		NRF_DEFINES += -DADAFRUIT_FEATHER52840


		#if (MICROPY_PY_BLE_NUS == 0)

		#if !defined( NRF52840_XXAA) \|\| ( defined(CFG_HWUART_FOR_SERIAL) && CFG_HWUART_FOR_SERIAL == 1 )

Conversation

hathach commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhalbert commented Jul 10, 2018

Uh oh!

dhalbert commented Jul 10, 2018

Uh oh!

hathach commented Jul 10, 2018

Uh oh!

hathach commented Jul 10, 2018

Uh oh!

dhalbert commented Jul 10, 2018

Uh oh!

hathach commented Jul 10, 2018

Uh oh!

arturo182 commented Jul 10, 2018

Uh oh!

dhalbert commented Jul 10, 2018

Uh oh!

dhalbert commented Jul 10, 2018

Uh oh!

arturo182 commented Jul 10, 2018

Uh oh!

dhalbert commented Jul 10, 2018

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

hathach commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

hathach commented Jul 11, 2018

Uh oh!

arturo182 commented Jul 11, 2018

Uh oh!

hathach commented Jul 12, 2018

Uh oh!

arturo182 commented Jul 12, 2018

Uh oh!

tannewt commented Jul 12, 2018

Uh oh!

Reviewers

Assignees

hathach commented Jul 9, 2018 •

edited

Loading