Two related thread-safety fixes for the cond_timedwait + set_absolute_time
dance, both surfaced under TSan with multi-instance Mavlink and a fast
sim clock advance:
1. Signal loss between releasing _timed_waits_mutex and entering
pthread_cond_wait. set_absolute_time could have already broadcast
to a waiter that hadn't actually started waiting yet, and the
broadcast would be missed -> wait blocks forever.
Fix: cond_timedwait uses pthread_cond_timedwait with a short
wall-clock timeout (10 ms) and re-checks the timeout flag. Lost
signals turn into a maximum of one loop iteration of latency.
2. ABBA between (passed_lock -> _timed_waits_mutex) used by
cond_timedwait to mark `done`, and (passed_lock under
_timed_waits_mutex) used by set_absolute_time to broadcast. TSan
flagged the inversion immediately.
Fix: split set_absolute_time into three phases. Phase 1 marks
timed_outs and stages waiters onto a per-call signal_next list,
under _timed_waits_mutex only. Phase 2 broadcasts to each waiter,
outside _timed_waits_mutex but under a new _signaling_mutex held
for the duration. Phase 3 clears _setting_time under the
_timed_waits_mutex again.
The waiter's "dance" (when it sees _setting_time still true on
exit) acquires _signaling_mutex first, then _timed_waits_mutex,
guaranteeing it cannot return — and let its stack-local
passed_lock/passed_cond go out of scope — until set_absolute_time
has finished signaling.
The TimedWait::timeout flag also becomes std::atomic<bool> since it
is now read by cond_timedwait without holding _timed_waits_mutex.
The ~PublicationBase() destructor was header-inline, so its null-check
plus the orb_get_queue_size()/unadvertise() branch was emitted at every
publication destruction site (PX4 links with bfd ld, no ICF, no LTO).
Move it into Publication.cpp alongside the already out-lined
advertise()/publish(), mirroring #27581.
Saves 2.4 kB of flash on px4_fmu-v6x_default.
publish(), advertise() and get_instance() were instantiated per topic
type and the advertise-check in publish() was additionally inlined at
every call site. Move them into a type-independent PublicationMultiBase
compiled once.
Saves 5.5 kB of flash on px4_fmu-v6x_default.
test_multiple_semaphores_waiting() spawns a worker thread per TestCase, then
advances virtual time in a loop and completes each case in check(). Since
check() early-returns while !_thread_ready, a slow-to-start worker thread can
miss the whole loop: the loop advances past the case's timeout before the thread
registers, so the case is never completed. ~TestCase() then fails
EXPECT_TRUE(_is_done) and destroys a still-joinable std::thread, which calls
std::terminate() and aborts the test. This was flaky (~50% locally, worse under
CI load).
Wait for every worker thread to be ready before advancing virtual time. Also
re-comment the per-iteration std::cout that was left enabled.
Signed-off-by: Julian Oes <julian@oes.ch>
NuttX tasks are task_create() children, not pthreads, so px4_task_join has no
portable join and instead polls whether the task still exists. Document that it
returns no exit status, can wait on the wrong task if the PID is reused, and is
only safe because its sole on-target caller is WorkQueueManager shutdown (and
NuttX shutdown is a full reboot).
Signed-off-by: Julian Oes <julian@oes.ch>
px4_task_t is a plain int and callers hold -1 for "no task" (e.g. dataman's
g_task_id after stop). The bounds checks only tested id < PX4_MAX_TASKS, which is
true for negative ids, so px4_task_join(-1) and px4_task_delete(-1) would index
taskmap[-1] out of bounds. Check id >= 0 as well.
Signed-off-by: Julian Oes <julian@oes.ch>
process_add_subscription() (CONFIG_ORB_COMMUNICATOR only) read _data_valid and
the topic buffer contents while passing a pointer into _data straight to
send_message(), with no synchronization against a concurrent publisher. A
publisher's write() memcpy's into _data and sets _data_valid under ATOMIC_ENTER,
so the remote-init send can read a torn/partially-updated sample - most clearly
for o_queue == 1, where the read slot ((gen - 1) % 1) is exactly the slot being
written.
This is a pre-existing latent race (the previous px4::atomic<uint8_t*> _data only
guarded the pointer, never the bytes) that neither ASan nor TSan can catch because
CONFIG_ORB_COMMUNICATOR is not built in those configs. On VOXL2 it runs on the
muorb RX thread concurrently with publishers.
Snapshot the most recent sample into a temporary buffer under ATOMIC_ENTER, then
send it outside the critical section - mirroring copy(). The buffer is allocated
before the critical section (it cannot be allocated under ATOMIC_ENTER on NuttX)
and send_message() must run outside it anyway, since it can be slow and may call
back into DeviceNode. This only affects the rare subscription-add path, not the
publish hot path.
Signed-off-by: Julian Oes <julian@oes.ch>
The daemon server's client-handler thread spawns module tasks (e.g. dataman).
Start px4::init_once()/init() before creating px4_daemon::Server so platform init
- uORB, work queues, logging - completes with a happens-before edge to those
tasks. Otherwise a module task racing the still-running init reads globals like
uORB::Manager's instance pointer or the log message advertisement without
synchronization (ThreadSanitizer flags it) - and an atomic would not make it
correct, since the publication could simply be skipped.
_data (the lazily-allocated topic buffer) was a px4::atomic only to make the
double-checked-locking publish safe against concurrent readers. It is allocated
under the node lock() and written exactly once, so reading it under ATOMIC_ENTER
- which is that same lock() on POSIX - serializes the subscriber side against the
allocation without needing an atomic. On NuttX it is a set-once, word-aligned
pointer. The copy() guard is moved inside ATOMIC_ENTER accordingly. (Suggested in
review.)
DeviceNode::open() took the cdev lock only to call mark_as_advertised(), but
_advertised is a px4::atomic, so the single store needs no lock. (Suggested in
review.)
_interval_us and _last_update are read in SubscriptionCallbackWorkItem::call()
(and SubscriptionBlocking::call()) on the publishing thread, while the subscriber
sets them - e.g. from mixer_module's setMaxTopicUpdateRate() on a reconfigure.
That is a genuine cross-thread access that ThreadSanitizer flags.
Instead of making these fields atomic in the base SubscriptionInterval - which is
used single-threaded by 100+ subscriptions - parameterize the class on whether the
interval / last-update storage is atomic and expose two aliases:
SubscriptionInterval plain, single-threaded (the common case)
SubscriptionIntervalAtomic atomic, for callback subscriptions
SubscriptionCallback - and hence all callback subscriptions
(SubscriptionCallbackWorkItem, SubscriptionBlocking, CallbackHandler) - uses the
atomic variant; every other subscription stays plain with no synchronization
overhead.
The updated()/update()/copy() bodies stay out-of-line with explicit instantiation
of both variants, so they are not inlined at every call site (flash size).
The publisher fires callbacks via SubscriptionCallbackWorkItem::call(), which read
the subscriber's _last_generation (and called updated(), which reads it again) to
decide whether to schedule the work item. That made the subscriber's read cursor a
cross-thread field - publisher reads it while the subscriber writes it in copy() -
which TSan flagged and which we had wrapped in an atomic.
Instead, hand the publishing node's generation into call() and let the callback keep
its own _last_scheduled_generation cursor for the count gate (_required_updates).
call() runs under the node lock, so that cursor is only ever touched there and is
serialized - no atomic needed, and using the real generation (not a plain counter)
means a coalesced/missed call() does not drift the batch. The redundant updated()
generation check is dropped (call() only runs right after a publish, so it is always
true); the interval gate (_last_update / _interval_us) is kept unchanged.
With the publisher no longer reading it, _last_generation reverts to a plain unsigned
(subscriber-thread only). Behaviour is unchanged: the count and interval throttles
fire at the same rates (verified via work_queue status - gyro_fft 62.5 Hz,
flight_mode_manager 50 Hz) and TSan stays clean.
GCC does not provide __has_feature, and its preprocessor tokenizes both
operands of && before evaluating the operator, so
#if defined(__SANITIZE_ADDRESS__) || defined(__has_feature) && __has_feature(address_sanitizer)
failed to compile with 'missing binary operator before token "("' on
toolchains such as the voxl2 linaro GCC. Add the standard __has_feature
fallback shim so the check uses __SANITIZE_ADDRESS__ on GCC and the real
__has_feature on Clang. Behavior is unchanged.
Signed-off-by: Julian Oes <julian@oes.ch>
system_exit() (= _exit()) on Linux skips C++ destructors and atexit
hooks, including ASan's leak-and-error report. Under sanitizer builds
we want that report. Switch to plain exit() when __SANITIZE_ADDRESS__
is defined; native posix shutdown still uses _exit().
Two related fixes for HRT callout queue corruption under lockstep SIH:
1. WorkQueue::Run() popped a WorkItem, released the work-queue lock,
then called work->Run(). If another thread called Deinit on the
item in that window, wq->Remove was a no-op (item already popped)
and Deinit returned while Run() was still executing. Run() could
then call ScheduleDelayed → hrt_call_after on an object that the
caller of Deinit has since destructed and reused via placement new
— the hrt_call metadata ends up living inside another object's
fields, which later overwrite the flink pointer and tear the hrt
callout queue.
Fix: WorkItem carries a _run_in_progress atomic flag set by
WorkQueue::Run around work->Run(). Deinit spins on that flag after
wq->Remove so it cannot return while a Run() is still executing.
WorkQueue captures its worker tid in the constructor (the ctor
runs on the worker thread) so Deinit can skip the self-wait when
called from inside Run() itself (e.g. should_exit() paths that
invoke ScheduleClear).
2. _hrt_lock was a px4_sem_t and hrt_call_invoke unlocked around the
callback so callbacks could re-enter hrt_call_*. That also exposed
the queue to other threads mid-invocation. Switch _hrt_lock to a
PTHREAD_MUTEX_RECURSIVE pthread_mutex and hold it across the
callback — matches NuttX's enter_critical_section() nesting
semantics, lets callbacks reschedule themselves safely, and
prevents concurrent queue manipulation.
Together these eliminate the HRT queue tearing observed in the
SIH-at-20x MAVSDK integration soak (torn flink chains with the tail
unreachable from the head; orphan nodes pointing into reused
memory).
Move the helper from the header into WorkQueue.cpp. Drop the
SignalWorkerThread() call from request_stop() — request_stop sets
_should_exit but the worker is woken up via the existing exit path
in Detach already, so the extra signal here is redundant.
fuzztest's coverage instrumentation is incompatible with Thread
Sanitizer. Add px4_setup_gtest_without_fuzztest() macro to
cmake/px4_add_gtest.cmake that fetches GTest standalone and stubs out
fuzztest cmake functions. Guard all fuzztest-specific code on
TARGET fuzztest::fuzztest so it compiles cleanly without fuzztest.
Split set_absolute_time() into two phases: iterate the waiter list under
_timed_waits_mutex, then broadcast under a separate _broadcast_mutex.
This eliminates the lock-order-inversion cycle between cond_timedwait()
(holds passed_lock -> acquires _timed_waits_mutex) and set_absolute_time()
(held _timed_waits_mutex -> acquired passed_lock).
Fix tests to have each thread lock its own mutex before calling
cond_timedwait, as required by POSIX (the calling thread must own the
mutex passed to pthread_cond_wait). The previous cross-thread ownership
caused TSAN's deadlock detector to overflow its 64-entry limit.
A seq_cst px4::atomic emits a hardware `dmb` on ARM. That barrier only orders
accesses as seen by a second observer (another CPU core or DMA); for inter-thread
synchronisation on a single-core (uniprocessor) target there is no second core, so
the `dmb` is dead weight - only the compiler ordering is required.
On a NuttX build without CONFIG_SMP, keep full seq_cst semantics but emit a
compiler-only fence (__atomic_signal_fence, zero instructions) instead of the
`dmb`. This mirrors Linux, where smp_mb()/smp_rmb()/smp_wmb() collapse to a
compiler barrier on uniprocessor builds. SMP NuttX and POSIX are unchanged (real
barriers, so SITL under ThreadSanitizer keeps full ordering). 64-bit types (not
lock-free on a 32-bit core) keep the existing interrupts-off critical section,
which already provides both atomicity and ordering.
The public API is unchanged - no per-call ordering knob. Verified on
arm-none-eabi Cortex-M7 that the single-core path emits no `dmb` while keeping
atomicity, and that SMP/POSIX is unchanged:
load (single-core): ldr (signal_fence: 0 instr)
store (single-core): str
fadd (single-core): ldrex/strex loop, no dmb
load (seq_cst/SMP): dmb ish; ldr; dmb ish
store (seq_cst/SMP): dmb ish; str; dmb ish
fadd (seq_cst/SMP): dmb ish; ldrex/strex; dmb ish
Shrinks fmu-v6x by ~2.8 KB (dead barriers removed from atomics already in use),
with no behaviour change. Note: inter-thread ordering only - DMA/device sync
still needs explicit barriers.
Pxh lazily initializes the builtin app map from client handler threads. Concurrent first commands can both enter init_app_map() and mutate the static std::map at the same time.
Use pthread_once so the map is populated exactly once before process_line() or tab completion read it.
* fix(i2c_spi): prevent use-after-free when stopping driver instances
module_stop() deleted each instance before unlinking it from the global
i2c_spi_module_instances list. The instance *is* the intrusive list node,
so removeInstance()/List::remove() then dereferenced the freed node to fix
up the links. If another thread reused that heap block in the window, the
list got corrupted, sporadically leaving instances linked (a later start
reports "already running") or hard-faulting. This regressed in the 2020
array->linked-list refactor: the old array version only nulled an array
slot after delete, which was safe.
Unlink each instance before freeing it. Also only delete/unlink when the
task actually exited: if request_stop_and_wait() times out the work queue
may still reference the object, so leave it allocated and listed rather
than freeing it. module_stop() now returns -1 when an instance failed to
stop.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* Update platforms/common/i2c_spi_buses.cpp
* Update platforms/common/i2c_spi_buses.cpp
---------
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* platforms/common/uORB/uORB.h: Add definition for orb_sub_t and handle check functions
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
* uORB: Change subscriber id:s from int to orb_sub_t
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
* uxrce_dds_client: Change polling of transport device from px4_poll to poll
Use posix poll directly, there is no need to use px4_poll unless uORBs are
being polled.
The one used here is a normal filesystem/device poll, so we can use normal "poll",
this is the common pattern in the codebase.
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
* Fix linking for protected build
This fixes errors for memalign not linking on some configurations.
Memalign exists in nuttx kernel-side mm library and it may fail in configurations
where kernel and userspace are separated. This has no effect on other than
"CONFIG_BUILD_PROTECTED" or "CONFIG_BUILD_KERNEL" NuttX builds.
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
---------
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
Co-authored-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
* fix(bootloader): scrub uncorrectable flash ECC errors on STM32H7 before boot
A torn write to a flash row (power lost mid program/erase) can leave it
with inconsistent ECC. On H7 a CPU read of such a row raises an
uncorrectable double-bit ECC error -> bus fault, so the application hard
faults on every boot before printing anything, and only a mass erase
recovers it: reflashing does not, because neither the app nor the
bootloader image touches the parameter sector.
Scan the application and parameter flash with DMA right after clock_init
(a DMA read latches FLASH_SR DBECCERR instead of bus-faulting the CPU)
and erase any sector that holds an uncorrectable error. A corrupt
parameter sector then re-seeds to defaults on the next app boot; corrupt
app flash fails the image check and stays in the bootloader for reflash.
Mirrors ArduPilot's bootloader ECC scrub. H7-gated; the scan runs before
the interface DMA is brought up, so DMA1 stream 0 is uncontended.
* test(tools): add STM32H7 flash ECC fault injection tool
A bench helper that deterministically plants an uncorrectable (double-bit)
flash ECC error on an STM32H7, reproducing the torn-write parameter-store
brick. Used to validate the bootloader ECC scrub and the parameter re-seed
recovery on hardware: plant the fault, then confirm the board boots clean
instead of hard-faulting every boot.
brick_ecc.c is a freestanding stub that double-programs one flash word with
two different single-bit clears (the recipe that yields an inconsistent ECC);
brick_ecc.sh builds it, loads it into AXI SRAM over ST-Link, runs it, and
resets the board. README documents usage and how to retarget another board.
* fix(bootloader): require DMA transfer-complete for a clean ECC scan chunk
The stream self-disables on both transfer-complete and transfer-error.
A non-ECC bus error (unreachable scan buffer, rejected FIFO config)
previously read as a clean chunk, silently turning the scan into a
no-op. Require TCIF so an unread chunk takes the skip-the-scan
fallback instead of passing.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
---------
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* feat: implemented serial passthrough
* fix: used make format
* fix: changed function order to match other drivers
* fix: moved passthrough from systemcmds to drivers
* fix: renamed BITBANG_TIMER to UART_BITBANG_TIMER
* fix: used make format
* feat: added PASSTHRU_EN guard to start of dshot&pwm_out
* fix: made changing ESC channels more stable
* fix: adjusted naming of guards
* fix: changed include guard of bitbang
* fix: removed unused variable SER_PASS_BAUD
* fix: adjusted comments
* fix: adjusted print_usage() to match other drivers
* fix: remove bitbang_write_byte from public API and some buffer guard
* fix: added Serialpassthrough&Bitbang to exclude list of allyesconfig.py
* fix: added missing flag to print_usage()
* refactor(uORB): out-line PublicationBase/Subscription methods to save flash
Move the type-independent PublicationBase::advertise()/publish() and
Subscription::copy()/update() bodies out of the headers into
Publication.cpp / Subscription.cpp, leaving only declarations inline.
PX4 links with bfd ld (no identical-code folding) and without LTO, so
these header-inline methods were emitted per translation unit: GCC both
.isra-cloned them (~39 publish, 25 advertise, 17 copy, 12 update copies)
and fully inlined them into many callers. Out-lining collapses all of
that to a single shared definition each.
This is the out-of-line follow-up to #27526, which hoisted advertise/
publish from the Publication<T> template into the non-template
PublicationBase but kept them inline in the header. The methods were
already emitted as separate clones, so callers already pay a bl and the
publish/read hot paths are unchanged -- this is pure code motion.
Saves 10.4 KB .text on auterion_fmu-v6x (1,949,568 -> 1,939,168).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(uORB): out-line SubscriptionCallback register/unregister to save flash
Move SubscriptionCallback::registerCallback()/unregisterCallback() out of the
header into a new SubscriptionCallback.cpp. Like the Publication/Subscription
methods, these were emitted per translation unit (PX4 links with bfd ld, no
ICF, no LTO): GCC .isra-cloned them and inlined them into every WorkItem-based
subscriber's setup path.
registerCallback() runs only at subscription/callback setup (cold path), so
out-lining is pure code motion with no hot-path change.
Saves 4.9 KB .text on auterion_fmu-v6x (1,939,168 -> 1,934,128).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(uORB): out-line remaining Subscription methods to save flash
Extends this PR's pattern to the rest of Subscription: move the type-independent methods still defined in the header (constructors, destructor, copy/move assignment, updated(), advertised()) into Subscription.cpp so they are emitted once instead of inlined into every translation unit.
updated()/advertised() inline the Manager::updates_available / is_advertised (DeviceNode) chain, and the constructors were inlined at every subscription site, so most of the remaining duplication lived there. ~9.4 KB .text saved on ark_fmu-v6x_default.
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* src/drivers/cdcacm_autostart: Include posix.h for px4_close
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
* platforms/nuttx/CMakeLists.txt: Fix linking of nuttx libaries for memalign
This fixes memalign not found in linking step for some boards
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
* Add "flock" to macos.sh setup script
"flock" is not standard on macOS, and a dependency was missing from the
macOS setup path.
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
* Fix clang-tidy errors in Bitset.hpp and in src/lib/matrix
Fix the "bugprone-dynamic-static-initializers" linter errors.
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
* systemlib/hardfault_log: Fix clang-diagnostics error
Fix for
"[error] clang-diagnostic-error [error]
use of undeclared identifier XCPTCONTEXT_REGS".
XCPTCONTEXT_REGS is defined in nuttx irq.h, so include that.
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
* uORB: Fix clang-tidy error "bugprone-dynamic-static-initializers"
Fix the clang-tidy error appearing on uORBManager _Instance variable by
adding a getter for the reference to the _Instance and using that instead.
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
* CI: Add default ubuntu mirrors as fallback
In case of specified aws mirror doesn't have the package idicated by the
metadata, a the default ubuntu mirror as a backup.
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
---------
Signed-off-by: Jukka Laitinen <jukka.laitinen@tii.ae>
GCC13 does aggresive optimizations that causes a RWW violation.
Causing the bootloader to crash while updating.
Add compiler arguements to reduce optimizations fixing the issue.
On a non-secure bootloader the VERIFY_SIG opcode was answered explicitly
instead of falling through like an older bootloader without the opcode,
which could desync the following BOOT command and fail the upload.
Compile the case only when BOOTLOADER_USE_SECURITY is set so non-secure
builds reject it via the default unknown-command path.
Signed-off-by: Julian Oes <julian@oes.ch>
The DSHOT bit width was selected at compile time from a single timer
clock (STM32_APB1_TIM5_CLKIN) and applied board-wide. On boards whose
timers run on different clocks (e.g. APB1 vs APB2) the non-APB1 timers
could emit DSHOT at the wrong rate; this was only flagged with a
#pragma message rather than handled.
Compute the bit width per timer from that timer's own clock_freq (which
io_timers[] already carries), using a fixed DSHOT600 reference so the
width depends only on the timer clock, not the selected DSHOT rate. The
selection rule is unchanged, so register values are identical for every
current board.
Keeping the width rate-independent is what preserves the duty cycle:
MOTOR_PWM_BIT_0/1 are absolute CCR counts calibrated for the fixed ARR,
so a rate-varying ARR would distort them.
Signed-off-by: Julian Oes <julian@oes.ch>
* feat(secure_bootloader): add ed25519 key and signing helpers
Scaffolding for PX4 secure-boot firmware signing, split into two
self-contained scripts:
- generate_signing_keys.py: produces <name>.json (private+public hex)
for use by sign_firmware.py and <name>.pub (C-array public key)
for inclusion in the bootloader build via CONFIG_PUBLIC_KEYn.
Refuses to overwrite existing private-key files.
- sign_firmware.py: pads an input .bin to a 4-byte boundary and
appends a 64-byte ed25519 signature, producing a file that drops
directly into the flash slot described by the image TOC.
Optionally appends an R&D certificate binary after the signature.
Replaces the signing path of the old Tools/cryptotools.py that was
removed along with the log-encryption cleanup; the new layout keeps
bootloader-signing tooling separate from log encryption to avoid
confusing the two independent crypto surfaces.
* fix(bootloader): panic if px4_get_secure_random is ever called
sw_crypto's crypto_open() unconditionally references
px4_get_secure_random from its XCHACHA20 path, so even a bootloader
that only performs ed25519 signature verification (and never touches
stream ciphers) pulls in an undefined symbol at link time.
The app build resolves it through nuttx_random.c, which is gated
behind CONFIG_CRYPTO_RANDOM_POOL and only compiled in when the
NuttX random pool is enabled. That config isn't on in the tiny
bootloader NuttX defconfig, and enabling it would pull in a pile
of kernel code the bootloader doesn't need.
Supply the symbol locally, but make it call up_assert() instead of
returning zeros. Silently handing out predictable bytes would be a
serious security bug if anyone later enables the XCHACHA20 path in
the bootloader without wiring up a real RNG. Aborting makes the
mistake impossible to miss.
* fix(bootloader): add PROTO_VERIFY_SIG opcode for upload-time signature check
Today a signed-boot failure is invisible to the uploader: verify_app()
runs inside jump_to_app() after PROTO_BOOT has already rebooted the
chip, so the host sees a successful upload followed by the device
silently staying in the bootloader. There is no protocol-level signal
that the image that was just written is not going to run.
Add PROTO_VERIFY_SIG (0x39) so the host can ask the bootloader to run
find_toc() + verify_app(0) *before* the reboot and get a concrete
OK / FAILED / INVALID answer back over the still-open USB connection.
The new opcode is gated on BOOTLOADER_USE_SECURITY: bootloaders built
without secure boot return cmd_bad (INSYNC/INVALID) so an uploader
can tell "I don't know this command" apart from "verification failed".
Two subtleties required care:
1. PROG_MULTI deliberately defers the very first word of the app
image to a RAM variable (first_word) and only commits it to flash
inside PROTO_BOOT, so a partial upload can never become bootable.
But verify_app() reads directly from flash, so if we verified
before committing first_word, even a valid image would always
fail (the first four bytes at APP_LOAD_ADDRESS would still be
0xffffffff). The handler therefore mirrors the first half of the
PROTO_BOOT handler: gate on STATE_ALLOWS_REBOOT, program the
deferred first word, then run the crypto check.
2. On failure we deliberately do not try to "undo" the first-word
write — H7 flash programming granularity makes it impossible to
revert in place, and the device was going to end up in the same
reject state at the next boot either way. The improvement is
purely that the uploader sees the failure before REBOOT instead
of after.
Chose opcode 0x39 to stay clear of ArduPilot's 0x28 (READ_MULTI) and
0x40 (CHIP_FULL_ERASE), which PX4 does not currently use but which
an AP-compatible uploader might.
ArduPilot also has bootloader secure boot (monocypher ed25519, like
us) but their verification runs at boot time, not upload time — so
neither project currently solves the "upload silently wrote a bad
image" problem. This puts PX4 ahead on that.
* feat(Tools): add --image_signed to mark signed firmware for uploader
px_uploader.py only needs to run signature verification over USB for
images that actually carry a signature — asking the bootloader to
verify an unsigned image would always fail, and the extra round trip
is wasted time for the common unsigned case. It therefore needs a
reliable way to tell the two apart.
We cannot tell by inspecting the bytes: a sign_firmware.py output is
just the raw .bin with 64 bytes of ed25519 signature glued on the
end, indistinguishable in content from an unsigned image of the same
padded length. The natural place to put the flag is the .px4 JSON
envelope, which already carries board_id / version / summary / etc.
Add --image_signed to px_mkfw.py, which sets "image_signed": true
in the emitted JSON. The flag has no effect on what bytes actually
end up on the device — it just tells the uploader "this blob has a
signature, please verify it before booting".
* feat(Tools): verify firmware signature before reboot
After PROG_MULTI + GET_CRC, send a new VERIFY_SIG opcode (0x39) to
the bootloader to check the ed25519 signature over the freshly
flashed image *before* sending REBOOT. This way a signature failure
is reported as a clean error from the uploader script, instead of a
silent "device stays in bootloader after reboot" that leaves the user
guessing.
The uploader always probes VERIFY_SIG. The bootloader is the source
of truth for whether secure boot is enabled:
- INSYNC/OK -> verification passed, proceed to REBOOT
- INSYNC/FAILED -> raise; "Signature does not verify against any
trusted key" if the firmware claims to be signed,
"Secure bootloader rejected an unsigned image"
otherwise (= helpful guidance for the common
misconfiguration of uploading default firmware
to a secureboot bootloader)
- INSYNC/INVALID -> bootloader has no secure boot. Quietly proceed
unless the firmware metadata says image_signed,
in which case raise.
- recv timeout -> assume a pre-VERIFY_SIG bootloader, proceed.
Surfaces a "Verifying image signature... passed" line on the upload
status path when the firmware is marked signed, so users get a clear
positive signal that the secure-boot pipeline ran end-to-end.
Also drop the redundant logger.error in the upload() loop, which was
duplicating the error message printed by the top-level handler in
main() for every UploadError path (not specific to verify_signature,
but only became obvious once these clean error messages started
firing in normal usage).
* fix(cmake): fix .px4board variant resolution to require exact match
px4_config.cmake matched the requested CONFIG against each candidate
.px4board with `MATCHES`, which is a regex partial match. For a
config like `px4_fmu-v6x_bootloader_secureboot`, the iteration over
.px4board files (alphabetical glob order) would match
`bootloader.px4board` first — because "px4_fmu-v6x_bootloader" is a
prefix of "px4_fmu-v6x_bootloader_secureboot" — and stop, silently
selecting the wrong board config.
Use `STREQUAL` instead so each candidate has to match the full
requested CONFIG. Existing single-label cases (e.g. exact match on
`px4_fmu-v2_default` or `px4_fmu-v2`) are unaffected because they
were already exact in practice; this just plugs the prefix-match
hole that any future `<label>_<suffix>` variant would trip over.
* fix(nuttx): support bootloader_<variant> labels
Boards can ship bootloader variants beyond the default `bootloader`
label — e.g. a `bootloader_secureboot` that adds crypto + keystore
Kconfig on top of the same source tree. Two pieces of build glue
were hardcoded to the exact label `bootloader` and need to relax to
match any `bootloader_*` label:
1. platforms/nuttx/CMakeLists.txt picked the bootloader linker
script and bootloader-specific library list only when the label
was exactly `bootloader`. Any other label (including
`bootloader_secureboot`) silently fell through to the app build,
producing nonsensical link flags. Match `^bootloader` instead,
and explicitly set SCRIPT_PREFIX to `bootloader_` so all
bootloader variants share the single existing linker script
regardless of their full label.
2. platforms/nuttx/cmake/px4_impl_os.cmake selects the NuttX config
subdirectory by exact label match, falling back to `nsh` when no
matching directory exists. For `bootloader_secureboot` that fell
through to `nsh`, dragging in a full app-style NuttX with cromfs,
networking, and the full heap subsystem — a 128 KB bootloader
sector overflowed by ~50 KB. Add an intermediate fallback: if the
label starts with `bootloader` and a `bootloader/` subdir exists,
use that instead of `nsh`.
Both changes are backward-compatible: existing single-label
`bootloader` builds take exactly the same path as before. They only
gain the ability for boards to add `bootloader_<suffix>.px4board`
files without duplicating bootloader build wiring.
* feat(build): auto-sign secure-boot images via BOARD_SECUREBOOT
Add a Kconfig pair that boards can opt into:
CONFIG_BOARD_SECUREBOOT -- bool: sign the .px4 with ed25519
CONFIG_BOARD_SECUREBOOT_KEY -- string: path to JSON private key
(default: Tools/test_keys/test_keys.json)
When set, the .px4 build rule inserts a Tools/secure_bootloader/sign_firmware.py
step between the unsigned .bin and px_mkfw.py, and passes --image_signed
so the .px4 envelope's metadata flags it for the uploader's VERIFY_SIG
step. The unsigned .bin is still produced alongside the .px4, so users
can sign with their own key out-of-tree if they prefer.
A BOARD_SECUREBOOT_KEY environment variable overrides the Kconfig path
at build time, mirroring the override pattern used for CONFIG_PUBLIC_KEYn
in stub_keystore. This makes release builds practical:
BOARD_SECUREBOOT_KEY=/secure/path/release.json make px4_<board>_secureboot
Relative paths in the Kconfig are resolved against the repo root (not
the build directory) so the same value works regardless of where the
build runs from.
Default builds are byte-identical to before; the new code path is gated
on CONFIG_BOARD_SECUREBOOT being set.
* feat(boards): add secureboot demo variant + docs
Two coordinated build variants demonstrate end-to-end secure boot
on px4_fmu-v6x without touching the default builds:
px4_fmu-v6x_secureboot -- the app, with TOC + signing
px4_fmu-v6x_bootloader_secureboot -- the matching secure bootloader
App side (secureboot.px4board):
- Selects nuttx-config/scripts/secureboot-script.ld via
CONFIG_BOARD_LINKER_PREFIX. The script is a copy of the default
layout plus a fixed 0x800 reservation past the vector table for
the image TOC, and an empty .signature section at end-of-FLASH
so sign_firmware.py knows where the appended ed25519 signature
will land.
- Compiles src/toc.c (a four-entry IMAGE_MAIN_TOC matching the
layout used elsewhere in the tree) into drivers_board, gated on
PX4_BOARD_LABEL == secureboot so the default build still
produces the existing layout.
- Sets CONFIG_BOARD_SECUREBOOT=y so the .px4 build rule signs the
image with the upstream test key by default.
Bootloader side (bootloader_secureboot.px4board):
- Enables CONFIG_BOARD_CRYPTO + DRIVERS_SW_CRYPTO + DRIVERS_STUB_KEYSTORE,
which links monocypher and the stub keystore into the bootloader.
- Bakes Tools/test_keys/key0.pub in as CONFIG_PUBLIC_KEY0, paired
with the test_keys.json the app variant signs with.
- hw_config.h gates BOOTLOADER_USE_SECURITY, BOOTLOADER_SIGNING_ALGORITHM
(CRYPTO_ED25519), and BOARD_IMAGE_TOC_OFFSET (0x800) on PX4_CRYPTO,
so they only activate in this variant.
Together the two variants implement the workflow:
make px4_fmu-v6x_bootloader_secureboot # build + flash via SWD once
make px4_fmu-v6x_secureboot upload # signs with test key, verifies
Bootloader fits in 128 KB (~57 KB used) thanks to --gc-sections
stripping the unused libtomcrypt code; nothing in the bootloader
binary depends on monocypher beyond the ed25519 verifier.
Replace the bundled test key for production with
Tools/secure_bootloader/generate_signing_keys.py output and update
both .px4board files (or set BOARD_SECUREBOOT_KEY at build time) —
see docs/en/advanced_config/bootloader_secure_boot.md.
* fix(boards): cap H7 bootloader linker scripts at 128 KB
Roughly half of the H7 boards in tree had `LENGTH = 2048K` for the
bootloader sector in their bootloader_script.ld, even though the
matching app linker script places APP_LOAD_ADDRESS at 0x08020000 —
i.e. the bootloader actually only owns the first 128 KB sector.
The 2048K constraint is wrong: it lets a bootloader that grew past
the 128 KB sector silently overflow into the app sector at link
time and corrupt the start of the app on flash. The linker should
fail the build instead.
The current default bootloader is ~46 KB, well under 128 KB on every
affected board, so this change is a no-op for default builds. It
just plugs a footgun for anyone adding bootloader features (secure
boot, extra UI, network boot, ...) that would have otherwise
silently grown into the app's flash range.
Boards fixed:
3dr-style H7 reference boards: cubepilot/cubeorange,
cubepilot/cubeorangeplus, holybro/durandal-v1, narinfc/h7
vendor variants: corvon/743v1, cuav/nora, cuav/x7pro,
gearup/airbrainh743, hkust/nxt-dual, hkust/nxt-v1,
matek/h743, matek/h743-mini, matek/h743-slim,
micoair/h743, micoair/h743-aio, micoair/h743-lite,
micoair/h743-v2, x-mav/ap-h743r1, x-mav/ap-h743v2
Boards already correct (LENGTH = 128K) are unchanged.
* fix(ci): add pynacl to Python requirements
Tools/secure_bootloader/sign_firmware.py uses PyNaCl for ed25519
signing, and the .px4 build rule for boards with CONFIG_BOARD_SECUREBOOT
calls it as part of the normal build (e.g. px4_fmu-v6x_secureboot).
Without pynacl in the dev requirements, those builds fail in CI and
fresh dev setups.
* docs(update): Subedit
* docs(update): Fix example error I made
* fix(docs): document BOOT and SIG1 regions
* fix(platforms): style fix
* fix(ci): workaround to get pip dependency
* fix(ci): fall back to plain pip on older build containers
The voxl2 build image ships a pip that predates --break-system-packages
(added in pip 23.0.1), so the install step blew up with "no such option".
Modern containers enforce PEP 668 and need the flag; older ones don't
support it but also don't enforce PEP 668, so plain pip works there.
Try the modern flag first, fall back if pip rejects it.
Signed-off-by: Julian Oes <julian@oes.ch>
* fix(build): strip BUILD_DIR_SUFFIX from CONFIG in cmake-build
cmake-build was passing the full build-dir name (including any
BUILD_DIR_SUFFIX like _replay or _failsafe_web) as -DCONFIG=, which
the board-lookup in cmake/px4_config.cmake then tried to match
against <vendor>_<model>_<label> .px4board files. That used to work
by accident because px4_config.cmake matched with regex MATCHES, but
since the switch to STREQUAL (needed to disambiguate
bootloader_secureboot from bootloader) the suffixed CONFIG no longer
matches anything and LABEL ends up empty, tripping a CMake error in
kconfig.cmake.
Pass the bare CONFIG and keep the suffix only on the build dir so
both concerns are independent.
Signed-off-by: Julian Oes <julian@oes.ch>
---------
Signed-off-by: Julian Oes <julian@oes.ch>
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
Auto-selects DSHOT_MOTOR_PWM_BIT_WIDTH per timer clock at compile time
so ARK FPV emits DSHOT at the same bit period as the rest of the fleet.
The hardcoded WIDTH=20 truncates the prescaler on ARK FPV's 160 MHz APB1
(PLL1N=40, floor(160e6/600e3)=266 not divisible by 20), giving a 1.706us
bit period and 66.7%/33.3% duty cycles (vs DSHOT600 spec 75%/37.5%).
The new ladder tries WIDTH in {20, 19, 18} and picks the first that
divides cleanly. ARK FPV lands on W=19, matching the 1.75us bit period
of the 240 MHz boards. Duty cycles improve to 70%/35%. 240/96/108/84 MHz
boards keep W=20 (unchanged). 200 MHz boards (CubeOrange/Orange+,
h7extreme) fall through to W=20 (unchanged, fix needs W=22 with scaled
BIT_1/BIT_0 -- out of scope).
A #pragma message (not #warning, which -Werror=cpp would treat as an
error) flags boards where APB1 and APB2 timer clocks would emit DSHOT
at different rates -- currently only fmu-v4pro (APB1=90, APB2=180 MHz).
The whole block is guarded on STM32_APB2_TIM8_CLKIN so STM32F1 IO
co-processor boards (no TIM8, header still pulled in by io_timer.c)
keep W=20 silently.
Supersedes #27401.
