boards/nrf52840dongle: Support a FLOSS programmer #21466
Conversation
chrysn
added
Area: build system
github-actions
bot
added
Area: doc
crasbe
added
Type: new feature
|
I wonder if it wouldn't be nice to include a That script should include a check if |
| FLASHFILE = $(HEXFILE) | ||
| FLASHDEPS += $(HEXFILE).zip | ||
| FLASHER = nrfutil | ||
| FFLAGS = dfu usb-serial --port=${PORT} --package=$(HEXFILE).zip |
There was a problem hiding this comment.
| FFLAGS = dfu usb-serial --port=${PORT} --package=$(HEXFILE).zip | |
| FFLAGS = dfu usb-serial --port=$(PORT) --package=$(HEXFILE).zip |
There was a problem hiding this comment.
I'd rather leave it as is in this commit so that it is visible in the commit that this line was not modified. (The GitHub diff settings choice mark it as a new line, but eg. a git diff color-moved does show those as unmodified).
| FFLAGS = $(FLASHFILE) | ||
| endif | ||
|
|
||
| ifeq (1,$(USING_NRF_BOOTLOADER)) |
There was a problem hiding this comment.
| ifeq (1,$(USING_NRF_BOOTLOADER)) | |
| ifneq (,$(filter nrfutil nrfdfu,$(PROGARMMER))) |
Since the number of programmer variants is very limited (two), we could omit the extra variable here.
NRF_BOOTLOADER might be misleading, since there is (at least) one other bootloader for the nRFs, the Adafruit nRF52 Bootloader.
There was a problem hiding this comment.
I think there is value to having this independent, because it could also be used with an external programmer but leaving the bootloader in place.
Even if you accept that rationale, let's leave this open for the naming issue. I think that the adafruit bootloader is similar enough, but I'll want to check that anyway while looking at knurling-rs/nrfdfu-rs#16, and then I'll see if there is a better name to use here.
There was a problem hiding this comment.
That is a valid point, but then the parameter must be documented somewhere.
There is a similar parameter for the Adafruit nRF52 Bootloader called UF2_SOFTDEV to keep the SoftDevice in place. However that wouldn't protect the Bootloader from overriding with a JLink.
| @@ -1,31 +1,41 @@ | |||
| # This board uses the vendor's serial bootloader | |||
|
|
|||
| PROGRAMMER ?= nrfutil | |||
There was a problem hiding this comment.
I think if nrfdfu works well, we could set it as default too.
There was a problem hiding this comment.
Yes, why not! It is consistent with our preference for FLOSS tools.
|
Successfully tested on my nrfdfu /home/nils/Programming/RIOT/tests/periph/cpuid/bin/nrf52840dongle/tests_cpuid.elf
[INFO nrfdfu] Sending init packet...
[INFO nrfdfu] Sending firmware image of size 16776...
[INFO nrfdfu] Done.
sleep 2
/home/nils/Programming/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "115200" -ln "/tmp/pyterm-nils" -rn "2025-05-07_10.14.54-tests_cpuid-nrf52840dongle"
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2025-05-07 10:14:54,278 # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2025-05-07 10:14:55,282 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,283 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,284 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,284 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,285 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,286 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,286 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,286 # READY
2025-05-07 10:14:55,287 # Help: Press s to start test, r to print it is ready
2025-05-07 10:14:55,287 # START
2025-05-07 10:14:55,287 # main(): This is RIOT! (Version: 2025.07-devel-136-gcab6f-HEAD)
2025-05-07 10:14:55,287 # Test for the CPUID driver
2025-05-07 10:14:55,287 # This test is reading out the CPUID of the platforms CPU
2025-05-07 10:14:55,287 #
2025-05-07 10:14:55,287 # CPUID_LEN: 8
2025-05-07 10:14:55,288 # CPUID: 0xfe 0xf4 0xfc 0x4e 0xe7 0x55 0x40 0xc0
2025-05-07 10:14:55,288 # { "threads": [{ "name": "main", "stack_size": 1536, "stack_used": 460}]}
2025-05-07 10:14:57,077 # Exiting Pyterm |
The makefiles currently do test for whether the flasher is installed, but only give a generic error message. As for auto-adding, I'm a bit unsure. Our general approach there is highly inconsistent (some tools have a pkg and auto-build, some just expect things to be there, and there is the idea floating on chat but not in an issue yet to make every Python thing with dependencies just run Also, I wouldn't know how to best do it: |
Disclaimer: I'm completely unfamiliar with Rust (at least the Programming Language, I got some experience dealing with rusty metal) and Cargo, but the documentation says there is the So it should just work with |
|
RIOT/doc/doxygen/src/flashing.md Lines 110 to 113 in 96e202f This paragraph has to be updated as well with this PR. |
|
@chrysn what is the reason for this still being a draft ? |
|
Mainly that I'd hope for a nrfdfu release soonish so we don't tell people to (But I could probably just declare that a secondary issue, address the review comments and make it ready -- just can't do that right now) |
You know what would solve that? A Makefile in |
No it wouldn't: Unless we go through our own local installations unconditionally and never use locally installed tools (and I'm rather confident people don't want that -- most of RIOT development works from a plain git checkout with just distro-provided software), installations would fail with hard-to-debug errors for people who already have the latest version of the tool installed. Be my guest to come up with an elaborate scheme to run installed tools to probe for their version number, fall back to the tools we autobundle, and sensibly cache all that information so CI runs don't grind to a halt from all the auto-testing, but that level of complexity has no place in this PR. |
That is not true for all tools. The aforementioned picotool for example is always fetched from remote.
Who could deny an invitation like that? :) PKG_NAME := nrfdfu
PKG_URL := https://github.com/knurling-rs/nrfdfu-rs.git
PKG_VERSION := 6ec635d1f2bef827c8029832a384283c057f9698
PKG_LICENSE := MIT
PKG_BUILD_DIR := build
REMOTE_CARGO_TOML_RAW_URL := https://raw.githubusercontent.com/knurling-rs/nrfdfu-rs/$(PKG_VERSION)/Cargo.toml
# make sure Cargo is installed
CARGO_EXISTS := $(shell command -v cargo 2>/dev/null)
ifeq (,$(CARGO_EXISTS))
$(error 'cargo' is not installed or not in PATH)
endif
# parse the Cargo.toml file to get the version of the nrfdfu util
REMOTE_VER := $(shell curl -sSL $(REMOTE_CARGO_TOML_RAW_URL) | grep '^version' | head -n1 | sed 's/version *= *"\(.*\)"/\1/')
GLOBAL_VER_TMP := $(shell \
cargo install --list 2>/dev/null | grep "^$(PKG_NAME) v" | sed -E 's/.*v([0-9.]+).*/\1/' )
# a version number may or may not have a URL at the end
GLOBAL_VER = $(firstword $(GLOBAL_VER_TMP))
LOCAL_VER_TMP := $(shell \
cargo install --root $(PKG_BUILD_DIR) --list 2>/dev/null | grep "^$(PKG_NAME) v" | sed -E 's/.*v([0-9.]+).*/\1/' )
LOCAL_VER = $(firstword $(LOCAL_VER_TMP))
# Installation Decision Tree
# Note: it is up to the user to run `make clean` from time to time to remove
# old installations. It is assumed that the local installation is newer than
# the global installation and therefore preferred if present.
INSTALL_TARGET=
ifneq (,$(GLOBAL_VER))
# check if there is a global installation
ifeq ($(GLOBAL_VER),$(REMOTE_VER))
# check if the global and remote installation have the same version
ifeq (,$(LOCAL_VER))
# create a symlink to the global installation if no local
# installation is present
INSTALL_TARGET = link
$(info [INFO] Using global nrfdfu util...)
endif
else
# it is unlikely that the global installation is newer than the remote
# version, therefore install the remote version.
ifeq (,$(LOCAL_VER))
INSTALL_TARGET = local
endif
endif
else
ifeq (,$(LOCAL_VER))
# no local installation present
INSTALL_TARGET = local
$(info [INFO] No nrfdfu util found...)
endif
endif
$(CURDIR)/nrfdfu: $(INSTALL_TARGET)
.PHONY: local link clean distclean
local:
@echo "[INFO] Installing $(PKG_NAME) ($(REMOTE_VER)) from Git repo..."
@cargo install --root $(PKG_BUILD_DIR) --git $(PKG_URL) --rev $(PKG_VERSION) --quiet
@cp $(PKG_BUILD_DIR)/bin/nrfdfu .
@chmod a+x nrfdfu
link:
@echo "[INFO] Creating a symlink for nrfdfu..."
@ln -s ~/.cargo/bin/nrfdfu nrfdfu
clean::
@rm -rf nrfdfu
distclean:: clean
@rm -rf $(PKG_BUILD_DIR)Option 1: Option 2: No Option 3: Option 4: The only "downside" of this solution is that apparently Also idk how to keep cargo from printing the warning messages, but they should be fixed upstream in the |
boards/nrf52840dongle/doc.md
Outdated
| the bootloader. | ||
| If some other firmware is running or RIOT crashed, you need to enter the bootloader | ||
| manually by pressing the board's reset button. | ||
| #### nrfdfu |
There was a problem hiding this comment.
This paragraph should also mention that cargo, libudev-dev and pkg-build are mandatory dependencies that have to be installed first with the distribution's package manager.
There was a problem hiding this comment.
Not any more 🎉: The upcoming 0.2.0 release (which I'm waiting for before rebasing/fixing this all) has a change in that does away with all host dependencies but cargo, and cargo is already a build dependency of RIOT (even when not developing in Rust: some boards have Rust based RIOT modules on by default).
It is true for the tools I've worked with, but you're right, it's a bit of wild west with tools.
No, but re/ wild west, I think we should aspire to use a single mechanism for the tools we need no matter whether they're part of the flashing process or of the building process.
I was about to quote a movie, but given that 1990s movies' language isn't suitable here, I'll post a picture that shows that indeed I'm both happy and impressed:
I'll have some review comments (in particular, I think that if we go with a tag rather than a hash, or the version in addition, we might avoid pulling all the source for nothing if the local package is installed -- and given we have a bad habit of copy-pasting, it might already be a good time to pull out the bulk into an included "this is a Rust tool" generic Makefile), but it looks quite cool already. do you have that code as a branch somewhere, or can I craft a commit with that content and you as the author? Re. upstream, they're already active fixing the warnings. The version they advertise in their Cargo.toml is at least consistent with common practice of only altering the Cargo.toml version before a release -- personally I prefer having it 1.2.3 at the tagged version and 1.2.4-alpha.0 or something like that right after the release, but that's a lot of back-and-forth commits (and Rust doesn't have too much flexibility in what is in a version). |
The Makefile is not totally up to standard, it does not use the
That is part of something I had in mind lately. Many times, the
It's not in a branch, I just made it locally. You can craft a commit.
Okay, I don't know how Rust typically does it. Doxygen for example bumps the version number after a release, so that the master branch has a newer version than the last release, which is convenient for version checks such as this. Of couse there would be a million cornercases like someone installed the current master version globally etc, but realistically, the Makefile covers most of the common cases and avoids unncecessary reinstalls. |
|
@crasbe isn't your decision tree missing the case where there is no global installation and the local version is older than the remote version ? |
Yes and no. Checks for that would make the decision tree much more compilated and IMO the user/developer can do a |
|
This is becoming the very discussion I hoped to not have in a PR that is just about replacing one bootloading-tool-the-user-has-to-install with another bootloading-tool-the-user-has-to-install. Can we complete this with a (properly updated to list installation requirements) plain tool replacement, and move the full maintenance of on-demand install tools into a follow-up PR? (Happy to review that one, but we're now mixing very separate concerns in one thread.) |
|
Sure. Let me know when this PR is ready for final review. |
| When building a program for this board to be flashed using other methods | ||
| (e.g. using the board's Tag-Connect footprint), | ||
| setting `USING_NRF_BOOTLOADER=1` in the makefiles aligns the program suitable to be flashed after the bootloader. |
There was a problem hiding this comment.
Wouldn't it make sense to set USING_NRF_BOOTLOADER ?= 1 by default and just write something like "if you want to override the original bootloader with external tools, set it to zero.
Warning: you will not be able to program the dogle without external tools anymore. You will have to reflash the original bootloader afterwards."
There was a problem hiding this comment.
One has to use an external programmer to lock oneself out, but then I think that working on the full device unless something else is specified is at least consistent with what we had and have in other places.
There are multiple bootloaders, and people do flash other bootloaders in, so I'd say that once a debugger is connected, it's a stretch to assume that the bootloader is still there.
What I would appreciate if we had was a way to say "hey I have this board and a debugger attached, RIOT please undo what you did and do a factory reset", and I'm starting to gather the metadata for that, but that's something bigger.
There was a problem hiding this comment.
One should assume that when someone gathers a J-Link and a Tag Connect, that they know what they are doing, indeed.
My personal taste is that it's nicer to explicitly break something rather than implicitly, but I also understand your position.
For reference: Mikolai started working on such an "undo RIOT changes" in #21330 by reflashing the Adafruit Bootloader, but that is riddled by oddities in the adafruit-nrfutil Adafruit nRF52 Bootloader...
There was a problem hiding this comment.
Yes, "explicit" would be better, but for now I think we should prioritize "consistent".
(On the long run I'd like to have something like BOOTLOADER along with BOARD, where the former has a default depending on the latter; not sure how to do that switch in a way that it's apparent to all users what is happening).
|
There still seems to be an issue with Release 0.2.0, it did not compile successfully on my Linux Mint machine. I wasn't sure where to post this, but considering you're basically the only recent contributor to that project... |
|
brief response from mobile: rustup update
|
|
Just to clarify beforehand: I'm just trying to figure out how to make the installation process as seamless as possible for the user. I'm not expecting support from you nor do I want to derail this PR. We can also move this discussion to the Unfortunately The best approach would probably to uninstall rust with Apparently newer versions of Ubuntu also ship with Rust 1.75.0: https://launchpad.net/ubuntu/+source/rustc From what I've read, it is possible to specify a minimum required Rust version in the |
|
That's good input and resulted in knurling-rs/nrfdfu-rs#28. For the larger issue of having a suitable Rust, yes we should get better catching that in the build system, but this is no new requirement from here: We've had Rust code as part of non-demo examples for a while (microbit-v2 running the saul example had the lsm303agr driver since 2022), and while the cargo/rustc shipped by distros has sometimes been recent enough to build native, those rustc builds don't ship libcore for thumb targets, so effectively users need a rustup Rust. For some users this is mitigated by BUILD_IN_DOCKER -- and for those we can (and should) enhance usability without requiring rustup (given they have a perfectly sufficient rust version in their build but not their flashing environment). Lowering version requirements would be a step, but the better step is probably integrating with cargo-binstall / cargo-quickinstall (which doesn't even need local cargo when done right) -- and that's something I could do in the follow-up to #21466 (comment). |
|
So what is the game plan with this? Do you want to create a fixup-version 0.2.1 for |
4 participants
Contribution description
The nrfutil programmer used for nrf52840dongle was never FLOSS (AFAICT it always had a 3-clause BSD license plus but-only-use-it-for-our-hardware), and installation has become way more convoluted since they abandoned the Python version.
There is a free implementation that's generally more hassle-free, too, as it works right from the .elf without going the extra detour over a programmer-specific zip file. [edit:] It is set as the new default.
Testing procedure
While there has not been a new release of nrfdfu:
cargo install --git https://github.com/knurling-rs/nrfdfu-rs/(later it should becargo install nrfdfu)make -C tests/periph/cpuid BOARD=nrf52840dongle all flash term. (No need to setPROGRAMMER=nrfdfu-- it is the default now).Note that unlike nrfutil, this tool discards the PORT= variable -- it doesn't deal in serial port terms, it looks at USB devices with the right uid/vid; there's a PR to identify the USB device through its serial number, which IMO should be the way to go for us anyway.
Issues/PRs references