The per-channel send mutex is genuinely recursive: callers hold lock_send()
while the MAVLink C library re-enters send_start()/send_finish() on the same
mutex via MAVLINK_START_UART_SEND. On NuttX a recursive mutex only works with
CONFIG_PTHREAD_MUTEX_TYPES; without it pthread_mutexattr_settype() is a no-op
and the nested lock deadlocks. Only a handful of boards happened to enable the
option (for Zenoh), so mavlink could deadlock at startup on most boards, e.g.
when bringing up a USB link.
Enable CONFIG_PTHREAD_MUTEX_TYPES on the NuttX boards that build mavlink, add
a compile-time guard so a board missing the option fails the build loudly
instead of deadlocking silently, and check the settype() return. Enabling the
option is free in flash and RAM on these boards since they already enable
CONFIG_PRIORITY_INHERITANCE / CONFIG_PTHREAD_MUTEX_ROBUST.
Signed-off-by: Julian Oes <julian@oes.ch>
The mission manager's _shared_state_mutex was recursive only because the
count updaters (update_geofence_count / update_safepoint_count) called the
public update_active_mission(), which re-took the same lock. That is the
"public method calls another public locking method" antipattern, not a
genuine need for recursion.
Split the state update into an update_active_mission_locked() helper that
assumes the lock is held and returns the mission_s to publish. The count
updaters call it while already holding the lock and publish after unlocking;
the public update_active_mission() locks around the helper. The mutex is now
a plain, statically-initialized (PTHREAD_MUTEX_INITIALIZER) mutex, dropping
the pthread_once runtime init and the non-portable recursive attribute.
This removes the dependency on CONFIG_PTHREAD_MUTEX_TYPES for this mutex,
which is not enabled on most NuttX boards.
Signed-off-by: Julian Oes <julian@oes.ch>
The AirBrainH743 breaks out a single I2C bus (I2C1) that also carries the
onboard DPS310 baro and IIS2MDC mag. Because the bus was marked internal,
the external-compass autostart (which scans external buses with -X) never
probed it, so an external GPS/compass on that bus was never bound.
Mark I2C1 external so the external-mag autostart scans it, and add a
px4_i2c_device_external() override (guarded by BOARD_OVERRIDE_I2C_DEVICE_EXTERNAL)
that keeps the onboard baro and mag classified as internal. This mirrors the
approach used on fmu-v6c. The onboard baro/mag are now started explicitly by
bus and address (-X -b 1) since -I no longer matches the external bus.
Also enable the i2cdetect command, which is handy for probing the shared
external bus on this board.
Signed-off-by: Julian Oes <julian@oes.ch>
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>
The trajectory time-stretch is skipped when the max-error param is at or below
FLT_EPSILON, so zero is a valid 'disabled' setting. Lower the minimum of
MPC_XY_ERR_MAX and MPC_Z_ERR_MAX from 0.1 to 0 so users can actually select it,
and document it in both params.
Also fix the setMaxAllowedVerticalError() doc comment, which incorrectly
claimed zero was the default: the MPC_Z_ERR_MAX param default is 1 m.
Signed-off-by: Julian Oes <julian@oes.ch>
These helpers are no longer called from production code after the smoother
was reworked to clamp XY and Z velocity components independently. Their only
remaining references were in TrajectoryConstraintsTest.cpp, which is not
registered in CMake and therefore never compiled. Drop both the functions and
the orphaned tests.
Signed-off-by: Julian Oes <julian@oes.ch>
PositionSmoothing already has a horizontal-error gate (MPC_XY_ERR_MAX)
that slows trajectory integration when the smoother's virtual position
runs ahead of the drone in XY. The same time-stretch was applied to
all three axes, but it was only ever computed from XY error. So:
- Z lag never slowed Z down: a noisy altitude reference (e.g. mission
setpoint jittering as home altitude is refined in flight) could
walk the commanded z 5+ m above the mission target while horizontal
tracking stayed fine. The jerk-limited smoother integrates each
small step into a velocity ramp, and successive ramps superpose
into multi-meter altitude excursions.
- Z lag dragging XY: when a Z stretch eventually was added, applying
the same scalar to all axes meant a ±1 m altitude jitter could
cap forward flight speed during a straight mission leg. That's
almost never what we want.
Fix: compute time_stretch_xy and time_stretch_z independently using
their own _max_allowed_*_error thresholds and apply per-axis. Default
the new MPC_Z_ERR_MAX to 1 m (tighter than XY's 2 m because altitude
holds are usually crisper). Other PositionSmoothing users (Orbit,
GotoControl) keep zero as the vertical error cap, which is the
explicit "disable Z stretch" sentinel — so behaviour there is
unchanged.
ScheduleOnInterval and registerCallback can both invoke Run() on
another thread (the work queue / publisher thread). configure_gripper
sets state Run() reads, so it must complete before the callbacks are
armed — otherwise a fast first iteration can race with the gripper
initialization.
set_interval_us() and the initial _time_stamp_last_loop write must
happen before the uORB subscription callback is armed — otherwise a
publisher on another thread can invoke Run() concurrently with init().
Run() reads _time_stamp_last_loop and depends on the interval being
set; observing zero-init values briefly leads to either a giant first
dt or an over-rate first iteration.
PositionSmoothing::_generateVelocitySetpoint computed a single
constrained 3D velocity vector and then ran clampToXYNorm /
clampToZNorm in series. The XY clamp could shorten the Z component
(and vice versa), so a Z speed limit ended up capping forward speed
and a tight altitude error capped horizontal motion. Compute XY and
Z velocity components independently using their own speed limits so
they don't mutually clamp.
Companion to the per-axis time-stretch fix (commit added later in
the chain). Together they decouple the horizontal and vertical
trajectory dynamics in the smoother.
When a mission is finished we need to make sure to send the mission
current message and not drop it. Otherwise, MAVSDK tests might not get
the update that the mission has been finished.
The per-channel send mutex array was defined without static, giving it external
linkage in the single-binary firmware even though it is only used within
mavlink_main.cpp (its status/buffer siblings are already static). Make it static
to match and avoid a latent cross-TU name clash.
Signed-off-by: Julian Oes <julian@oes.ch>
get_status()/get_buffer() returned per-instance _mavlink_status_early/
_mavlink_buffer_early fallbacks because the constructor touches the channel
status (via mavlink_update_parameters() -> setProtocolVersion()) before
set_instance_id() has assigned a channel, and set_instance_id() then copied
those into the static per-channel arrays.
Instead, don't touch channel state before we own a channel: setProtocolVersion()
now only caches _protocol_version until _instance_id is valid, and set_instance_id()
applies the cached version to the freshly-claimed channel's status flags. This lets
get_status()/get_buffer() index the per-channel arrays directly and removes the two
early buffers. The end state is identical (the buffer copy always copied a
zero-initialized buffer, since nothing writes it before assignment).
Signed-off-by: Julian Oes <julian@oes.ch>
Replace the terse "triggers TSAN" note with a TODO explaining that 'sensors
status' reads sensor module state from the shell thread without synchronization,
so it should be re-enabled once that race is fixed.
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.
tx_message_count (every send) and tx_buffer_overruns took _tstatus_mutex on the
hot path just to increment a counter. Make them px4::atomic, like the existing
_bytes_* counters, bumped lock-free from either thread's send path and folded
into _tstatus (cumulative) under the mutex at the ~1 Hz publish. The mutex is
kept for the genuinely shared low-frequency fields. (Suggested in review.)
The param access mutex must be recursive because a layered param lookup
re-enters the lock on the same thread (Layer::get() -> _parent->get()).
Initialize it in param_init() - which runs once before any thread accesses
parameters - via pthread_mutexattr_settype(PTHREAD_MUTEX_RECURSIVE), replacing
the previous pthread_once lazy init. This avoids the non-portable glibc static
initializer PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP, which is unavailable on QURT
and gated behind _GNU_SOURCE elsewhere (broke the voxl2 and SITL builds).
_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.)
Add a test_and_ci page covering the PX4_ASAN / PX4_TSAN / PX4_MSAN / PX4_UBSAN
build variables, the Linux `sudo sysctl vm.mmap_rnd_bits=28` workaround for the
sanitizer runtime's startup memory mapping, and the TSan suppressions file
(test/sitl_tsan.supp) for the Gazebo simulation transport. Linked from SUMMARY.md
and the test_and_ci index.
Running SITL with the Gazebo bridge under ThreadSanitizer reports many data races
inside libzmq and gz-transport (and the GZBridge glue) - third-party simulation
transport that is not in the flight path or the SIH-based CI and that we cannot
fix. Add a suppressions file so the TSan signal stays usable for finding real
races. Use via TSAN_OPTIONS="suppressions=test/sitl_tsan.supp".
init() registered the vehicle_local_position callback before calling
set_interval_us() and initialising _time_stamp_last_loop. The moment the callback
is registered it fires (and Run() is scheduled) on the publisher's thread, racing
those writes - ThreadSanitizer flags both. Register the callback last, once the
object is fully initialised.
_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.
Times perf_count and perf_begin/perf_end in tight loops with hrt, plus a
plain non-atomic counter loop as reference, to quantify the overhead of
the atomic perf_counter change. Run with: microbench microbench_perf
MavlinkMissionManager keeps several pieces of mission state as static
class members so all instances share one view of the active mission:
_mission_dataman_id, _fence_dataman_id, _safepoint_dataman_id,
_current_seq, _count[], _crc32[]. Multiple instances' receiver
threads (and their send() ticks) read and write these concurrently,
which TSan flags.
Add a static pthread_mutex_t _shared_state_mutex (recursive, so the
nested call inside update_geofence_count -> update_active_mission is
fine) initialized once via pthread_once, and take it around every
read-modify-write or read-then-publish path:
- update_active_mission
- update_geofence_count
- update_safepoint_count
- send() (covers _current_seq / _count / _crc32 access)
- check_active_mission (the publisher of the dataman bump)
Mutex declaration is in the public section so the pthread_once
initializer can reach it; it remains a class-level static.
Three closely-related races flagged by TSan with multi-instance
mavlink under SIH:
- _protocol_version is touched by both threads (task_main writes it
in setProtocolVersion via mavlink_update_parameters; receiver
reads it via getProtocolVersion and itself writes it on auto-
upgrade to v2). Make it px4::atomic<uint8_t>.
- setProtocolVersion's get_status()->flags RMW touches the per-
channel mavlink_channel_statuses[] global, which the receiver
thread also reads/writes from mavlink_frame_char_buffer. Take
lock_send() around the body, matching the existing pattern around
mavlink_parse_char in the receiver.
- get_use_hil_gps() read _param_mav_usehilgps directly from the
receiver thread while task_main was writing it via updateParams.
Add an atomic snapshot _use_hil_gps_atomic, refresh it in
mavlink_update_parameters(), and return the atomic load from the
getter. Same pattern as _boot_complete and the other cross-thread
state in this class.
Drive-by: the receiver's "upgrade to v2" check at the top of
MavlinkReceiver::run was reading _mavlink.get_status()->flags
outside lock_send(). mavlink_parse_char() copies the channel-
global flags into the receiver-local _status, so read _status.flags
instead — no lock needed and no race.
Two fixes for the singleton MavlinkCommandSender used across all
Mavlink instances:
1. initialize() races between instances. Multiple Mavlink::start()
calls hit it concurrently; the existing nullptr check on _instance
is not atomic, and re-running px4_sem_init on a semaphore another
thread is already using is a TSan-flagged data race. Move the
actual init into do_initialize() and call it through pthread_once,
which is portable across POSIX and NuttX.
2. check_timeout() is invoked every Mavlink main loop tick on every
instance. Under load they queue up on the singleton's lock and
form a convoy that delays heartbeat sending. Switch to try_lock:
if another instance is already inside, skip this cycle and retry
on the next tick. The wait was never useful here — the work is
"scan for timed-out commands", which is fine to defer one tick.
Per-channel mavlink_status_t and the per-instance telemetry_status_s
are touched by both the main mavlink thread (sending streams,
publishing status) and the receiver thread (mavlink_parse_char,
counting RX bytes). Until the library is refactored so per-channel
state is owned by the instance and TX seq becomes atomic, take the
existing recursive lock_send() / lock_telemetry_status() helpers in
all the spots that needed them but didn't have them:
mavlink_main.cpp:
- Wrap the per-iteration streams / ulog / events / forwarding loop
with one lock_send() so a stream's send() never races with the
receiver's parse_char on the same channel status. The mutex is
recursive, so nested helpers that take it again are fine.
- lock_send() around the individual sends in handleMavlinkShellOutput,
handleCommands and handleAndGetCurrentCommandAck (these run after
the streams loop, but with the lock dropped between them).
mavlink_receiver.cpp:
- lock_send() around mavlink_parse_char() only — NOT around
handle_message(), because some handlers (e.g. param requests
flowing into configure_stream_threadsafe) busy-wait for the main
mavlink thread, which itself wants lock_send. Holding it across
handle_message produces a circular wait.
- lock_send() around request_message() invocations in
handle_request_message_command (synchronous send paths from RX).
- lock_telemetry_status() around the RX-side _tstatus update so it
doesn't race with publish_telemetry_status() on the main thread.
mavlink_parameters.cpp:
- lock_send() in the hash-param value send, send_param() and
send_error() — these run on both the main thread (no outer lock
in some paths) and the RX thread.
mavlink_mission.cpp:
- lock_send() around every mavlink_msg_mission_*_send_struct call.
streams/LINK_NODE_STATUS.hpp:
- lock_telemetry_status() around the tstatus copy.
Mirrors the comments left in code: the proper fix is to refactor the
shared globals out of the MAVLink library; this is the
"add the lock everywhere it was missing" interim.
The receiver thread reads _mavlink_start_time via get_start_time() in
its first iterations, so writing it after _receiver.start() races
with the receiver. Move the assignment up so the value is published
before the receiver can observe it.
Three closely related thread-safety fixes around state shared between
the Mavlink main thread, the receiver thread, and configure_stream
callers from arbitrary threads:
- _boot_complete becomes px4::atomic_bool. set_boot_complete() runs
on the main mavlink thread; boot_complete() is read from receiver
threads to gate forwarding decisions.
- _subscribe_to_stream becomes px4::atomic<char *>. Producers
(configure_stream_threadsafe on any thread) hand off a stream name
to the consumer (main mavlink thread, in check_requested_subscriptions);
the rate field is sequenced by the atomic store/load on the pointer.
Wall-clock usleep replaces px4_usleep here because in lockstep mode
px4_usleep waits in sim time, which can stall startup if the main
thread isn't advancing time yet.
- mavlink_system.sysid / .compid are written exactly once across
all instances using compare_exchange latches. The values are
singletons (every instance reads the same param), but the field is
read concurrently by _mav_finalize_message_chan_send, so two
instances writing the same value at construction is still a race
TSan flags. Latch only on a *valid* write so an early constructor
with params not yet loaded doesn't permanently lock the latch
closed.
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.
Move registerCallback() after all member variable writes in
SensorSelectionUpdate() and reorder Start() to complete initial
sensor selection before registering the sensor_selection callback.
This prevents Run() from reading partially-written members like
_selected_sensor_device_id and _filter_sample_rate_hz on the work
queue thread while Start() is still initializing them.
This fixes TSAN issues in unit tests.
This fixes TSAN issues on destruction of AtomicTransaction happening
during the unit tests.
The static _MutexHolder instance wraps a pthread_mutex_t in a C++ class,
creating a destructor ordering problem during process exit when worker
threads may still hold the lock. Replace with a raw pthread_mutex_t
initialized via pthread_once, eliminating the C++ destructor entirely.
perf counters are written on the hot path (perf_begin / perf_end /
perf_count) and read concurrently by the `perf` command and the logger,
which ThreadSanitizer flags as data races.
Wrap the counter fields in PerfVar: px4::atomic on POSIX (where TSan runs
and native 64-bit atomics are essentially free) and a plain scalar on
flight hardware (NuttX/QURT). The call sites are identical in both
configurations.
The embedded path matters because a 64-bit atomic on Cortex-M has no
native instruction and lowers to a locked, interrupt-disabling
__atomic_* sequence. Measured on fmu-v6x (microbench microbench_perf):
perf_count 73.7 ns -> 171.8 ns
perf_begin+end 982.9 ns -> 1831.7 ns
~2x on instrumentation that wraps many module Run() loops. These are
single-writer diagnostic counters; the only effect of the plain path is
that a concurrent reader may observe an off-by-one torn read of a 64-bit
field, which is benign and self-correcting (the counter list itself is,
and remains, mutex-protected). The conditional version restores embedded
performance (76.6 ns / 1015 ns) while keeping SITL TSan-clean.
Single TSan/ASan-clean implementation that works on both POSIX and NuttX:
- size()/byteSize() take the AtomicTransaction lock so they don't race
with store() updating _next_slot/_n_slots.
- _grow() snapshots _n_slots under the lock for sizing the malloc, so
another thread's growth between the malloc and the memcpy can't make
the memcpy exceed the allocation. Both platforms use the unlock-
around-malloc + CAS-retry pattern: NuttX requires it (malloc can't
be called with IRQs disabled), and on POSIX it's safe because the
AtomicTransaction mutex still serializes everything else and the
brief unlock window doesn't touch shared state.
- ~DynamicSparseLayer() takes the lock, zeros _next_slot / _n_slots,
stores nullptr into _slots, then frees the buffer. This fixes the
heap-use-after-free TSan caught at process exit on POSIX, where the
static DynamicSparseLayer instances in parameters.cpp are torn down
by cxa_atexit while other threads (commander etc.) are still calling
param_get(). Any reader that acquires the lock post-destruction sees
_next_slot == 0 and falls through to the parent layer; the parent
chain in parameters.cpp is in reverse-declaration order, so the
parent is still alive when the child is destroyed.
Add DynamicSparseLayerTest with concurrent stress tests that reproduce
the races. ConcurrentMultipleWriters pre-allocates to TOTAL to avoid
the known ABA limitation in the CAS retry loop with concurrent _grow().
Drive-by: drop the unused void *p member from param_value_u — nothing
in the tree reads or writes it, and on 64-bit POSIX it was silently
doubling the size of every parameter slot.
Originally found while running the new SIH-based CI tests.
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.
* feat(boards): add support for AEDROXH7 flight controller
STM32H743-based FPV / racing flight controller from AEDROX.
Brings up:
- ICM-42688-P IMU on SPI2
- DPS310 baro on internal I2C2
- W25N NAND flash on SPI3 with littlefs at /fs/flash
- MAX7456 analog OSD on SPI1
- 8 motor outputs (TIM1 + TIM8) all bidirectional-DShot capable
- CAN1
- External I2C1 connector for compass / sensors
- 6 UARTs (TEL1, GPS1, RC, TEL2, ESC telemetry, debug console on UART8),
- Two user GPIOs on the M5-M8 connector, VTX power and camera-switch GPIOs,
buzzer on PA7 (active-buzzer transistor low-side switch), battery V/I sensing
via ADC1.
Board ID 1198 shared with ArduPilot's AP_HW_AEDROXH7 entry.
USB enumerates as 0x16D0:0x14FE with VENDORSTR "AEDROX".
Signed-off-by: Julian Oes <julian@oes.ch>
* docs(docs): Minor subedit
---------
Signed-off-by: Julian Oes <julian@oes.ch>
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
* feat(boards): add CBUnmanned H753-Stamp flight controller
* Update board_id to 1302
* docs: update CBUnmanned H753-Stamp board docs
Apply review feedback: correct the I2C bus count (add internal bus),
clarify SPI (IMUs on a dedicated internal bus), note the debug console
alongside the user UARTs, and describe how the board is powered from an
external 5 V supply with separate battery voltage sensing.
Add the pinout image and a PWM Outputs section.
Signed-off-by: Julian Oes <julian@oes.ch>
* refactor(boards): rename CBUnmanned H753-Stamp to H753-SOM
Rename the board directory, build target (cbunmanned_h753-som), USB
product strings, docs page, asset directory and pinout image, and the
bundled bootloader binary from -Stamp to -SOM. The hardware is a
System-on-Module, so SOM is the accurate name.
Signed-off-by: Julian Oes <julian@oes.ch>
---------
Signed-off-by: Julian Oes <julian@oes.ch>
Build all targets / Scan for Board Targets (push) Has been cancelled
Checks / Gate Checks [check_format] (push) Has been cancelled
Checks / Gate Checks [check_newlines] (push) Has been cancelled
Checks / Gate Checks [module_documentation] (push) Has been cancelled
Checks / Gate Checks [shellcheck_all] (push) Has been cancelled
Checks / Gate Checks [validate_module_configs] (push) Has been cancelled
Checks / Unit Tests (push) Has been cancelled
MacOS build / build (push) Has been cancelled
Ubuntu environment build / Build and Test (ubuntu:22.04) (push) Has been cancelled
Ubuntu environment build / Build and Test (ubuntu:24.04) (push) Has been cancelled
Container build / Set Tags and Variables (push) Has been cancelled
Docs - Orchestrator / T1: Detect Changes (push) Has been cancelled
Docs - Orchestrator / T2: Metadata Sync (push) Has been cancelled
Failsafe Simulator Build / build (failsafe_web) (push) Has been cancelled
FLASH usage analysis / Analyzing px4_fmu-v5x (push) Has been cancelled
FLASH usage analysis / Analyzing px4_fmu-v6x (push) Has been cancelled
ITCM check / Checking nxp_mr-tropic (push) Has been cancelled
ITCM check / Checking nxp_tropic-community (push) Has been cancelled
ITCM check / Checking px4_fmu-v5x (push) Has been cancelled
ITCM check / Checking px4_fmu-v6xrt (push) Has been cancelled
Python CI Checks / build (push) Has been cancelled
ROS Integration Tests / build (push) Has been cancelled
ROS Translation Node Tests / Build and test [humble] (push) Has been cancelled
ROS Translation Node Tests / Build and test [jazzy] (push) Has been cancelled
SITL Tests / Testing PX4 iris (push) Has been cancelled
Build all targets / Seed [${{ matrix.chip_family }}] (push) Has been cancelled
Build all targets / Build [${{ matrix.runner }}][${{ matrix.group }}] (push) Has been cancelled
Build all targets / Upload Artifacts (push) Has been cancelled
Container build / Build Container (amd64) (push) Has been cancelled
Container build / Build Container (arm64) (push) Has been cancelled
Container build / Deploy To Registry (push) Has been cancelled
Docs - Orchestrator / T2: PR Metadata (push) Has been cancelled
Docs - Orchestrator / T2: Link Check (push) Has been cancelled
Docs - Orchestrator / T3: Build Site (push) Has been cancelled
Docs - Orchestrator / T4: Deploy (push) Has been cancelled
FLASH usage analysis / Publish Results (push) Has been cancelled
Static Analysis / Clang-Tidy (push) Has been cancelled
Add support for the MAVLink FTP ListDirectoryWithTime command (opcode 16),
which returns directory listings with each file's last-modification time
appended as decimal seconds since the UNIX epoch (0 if unknown).
This lets a GCS retrieve file timestamps without downloading the files,
which is useful for log download. The plain ListDirectory output is
unchanged, and clients fall back to it when the server NAKs with
UnknownCommand.
Signed-off-by: Julian Oes <julian@oes.ch>
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>
The AirBrainH743 used Holybro's USB vendor ID (0x3162) by mistake.
This moves to 0x26ac like a few other boards do.
Signed-off-by: Julian Oes <julian@oes.ch>
The bootloader boot-delay feature has been mechanically broken on
every modern FMU board since the STM32F7/H7 transition. It has three
independent bugs that prevent it from ever working:
1. Offset mismatch: BOOT_DELAY_ADDRESS is hardcoded to 0x1a0, but the
NuttX vector table is 504 B (F76x) to 664 B (H743) long. The
linker places _bootdelay_signature at ALIGN(32) past end of
vectors (e.g. 0x2a0 on CubeOrange), never at 0x1a0. The bootloader
reads random exception_common pointers in place of the magic and
never matches BOOT_DELAY_SIGNATURE1/2. Verified on CubeOrange with
objdump of cubepilot_cubeorange_default.elf.
2. Flash cache never flushes: fc_write() stores arbitrary writes in
cache line 1 and only flushes on a very specific condition tied
to the sequential firmware upload flow. A standalone write during
PROTO_SET_DELAY is cached forever. fc_read() then returns the
cached value, so the post-write verify lies and the bootloader
reports success. Nothing ever reaches flash.
3. H7 write granularity: the STM32H7 flash controller requires a
full 32-byte program cycle per write. Single 32-bit writes from
flash_func_write_word() would not be accepted by the controller
even if they reached it.
The feature has been silently dead on every H7/F7 FMU board for
years and no one noticed, which is strong evidence nothing actually
depends on it. Rather than fix it (which would mean rewriting
PROTO_SET_DELAY, the flash cache path, and the H7 flash programming
path), remove it.
Changes:
- bl.c: PROTO_SET_DELAY case now immediately NACKs (goto cmd_bad)
so clients that still send the command get a clear rejection
instead of the previous silent fake-success. The opcode stays in
the protocol enum for backwards compatibility.
- bl.h: drop BOOT_DELAY_SIGNATURE1/2 and BOOT_DELAY_MAX.
- stm/stm32_common/main.c, nxp/imxrt_common/main.c: drop the
startup boot-delay sig check block.
- image_toc.c: decouple find_toc() from BOOT_DELAY_ADDRESS.
BOARD_IMAGE_TOC_OFFSET is now the required define when
BOOTLOADER_USE_TOC is enabled. The body is wrapped in #ifdef
BOOTLOADER_USE_TOC and falls back to a stub returning false when
the TOC is not in use (no upstream board currently enables it).
- Linker scripts: strip EXTERN(_bootdelay_signature) and the
FILL/. += 8 block from all 142 affected .ld files across boards/.
- hw_config.h: strip the #define BOOT_DELAY_ADDRESS and its comment
block entry from all 48 affected boards.
- Tools/px4_uploader.py, Tools/teensy_uploader.py: remove --boot-delay,
set_boot_delay(), and SET_BOOT_DELAY client-side counterpart.
Smoke-built on cubepilot_cubeorange_default and
cubepilot_cubeorange_bootloader; no link errors, no unresolved
symbols, flash usage unchanged.
Tested:
- New BL, new FW
- Old BL, old FW
- Old BL, new FW
- New BL, old FW
Integrators can declare read-only parameters in a per-board YAML file:
readonly_params.yaml.
There are two ways to define the read-only params:
- "block": default writable, explicitly list params to be locked
- "allow": default readonly, explicitly list params to be writable
Enforcement is activated by `param lock` in rcS after all startup
scripts have run, so board defaults and airframe scripts can still set
params during init.
The feedback via MAVLink uses the new
MAV_PARAM_ERROR_READ_ONLY as part of the PARAM_ERROR message.
The logger rework grows text slightly, pushing the already
flash-constrained zenoh variant over budget. Drop a handful of
optional features to recover the needed headroom:
- osd_msp_osd (MSP OSD, niche)
- actuator_test (bench diagnostic)
- fw_autotune_attitude_control / mc_autotune_attitude_control
(tuning can be done on a default build)
Replace the old SDLOG_DIRS_MAX=3 + larger-buffer override with the
new rotation-based tuning: SDLOG_MAX_SIZE=40 caps each log file at
40 MB so a handful of recent logs fit, and SDLOG_ROTATE=95 lets logs
fill up to 95% of the 128 MB volume.
Also set COM_ARM_SDCARD=0 (no SD on this board) and
SYS_DM_BACKEND=1 so missions are stored in RAM.
Replace the old SDLOG_DIRS_MAX=3 + larger-buffer override with the
new rotation-based tuning: SDLOG_MAX_SIZE=40 caps each log file at
40 MB so a handful of recent logs fit, and SDLOG_ROTATE=95 lets logs
fill up to 95% of the 128 MB volume.
Drop the BOARD_SMALL_FLASH_LOGGING define from board_config.h; the
logger no longer keys off a per-board compile flag.
The kakuteh7mini ships with a W25N01GV (1Gbit/128MB) SPI NAND flash on
SPI1, but the board init was treating SPI1 as an MMC/SD slot and the
W25N driver was not enabled. Enable the chip and use it for logging:
- spi.cpp: register the device as SPIDEV_FLASH(0) instead of SPIDEV_MMCSD(0)
- init.c: initialize the W25N MTD driver, register /dev/mtd0, mount
littlefs at /fs/flash with autoformat, and print the flash geometry
on boot for verification.
- nuttx defconfig: enable CONFIG_MTD_W25N, CONFIG_FS_LITTLEFS,
SPI1 DMA + DMAMUX1, drop the unused RAMTRON config.
- board_dma_map.h: define DMAMAP_SPI1_RX/TX for the SPI1 DMA channels.
- default.px4board: set CONFIG_BOARD_ROOT_PATH to /fs/flash.
- rc.board_defaults: drop the SDLOG_BACKEND=0 override that was
disabling logging entirely, drop COM_ARM_SDCARD (no SD card), and
tune SDLOG_MAX_SIZE=40 / SDLOG_ROTATE=95 to keep a few recent logs
in the 128 MB flash.
Add an NSH command to format a device with littlefs, analogous to
mkfatfs for FAT filesystems. The command unmounts the mount point,
then remounts with forceformat to format and mount in one step.
Enable the command on boards that use littlefs as primary storage
(airbrainh743 and kakuteh7v2), and document it in the airbrainh743
flight controller page as a recovery procedure for a corrupted
flash filesystem.
Usage: mklittlefs /dev/mtd0 /fs/flash
Make the logger practical on small flash storage (e.g. 128 MB W25N
NAND) while keeping the existing SD-card behaviour sensible.
- Move cleanup from boot to log start so logs can be downloaded via
MAVLink FTP before deletion.
- Delete individual .ulg files (oldest first) instead of whole
directories, prioritising the naming scheme not currently in use
(session dirs vs date dirs).
- Add SDLOG_MAX_SIZE (new, default 1024 MB): maximum size of a single
log file. When reached, the log rotates to a new file. The value is
also reserved as headroom in the cleanup threshold.
- Add SDLOG_ROTATE (new, default 90): maximum disk usage percentage.
Cleanup guarantees (100 - SDLOG_ROTATE)% free at all times, even
during writing of a new log file. 0 disables space-based cleanup,
100 allows filling the disk.
- Repurpose SDLOG_DIRS_MAX as an orthogonal directory-count cap
(default 0 = disabled). Useful for capping log usage by count
independent of disk size, e.g. in SITL.
- Drop the implicit ~300 MB free-space floor that used to fire even
with SDLOG_DIRS_MAX=0; space is now governed entirely by
SDLOG_ROTATE + SDLOG_MAX_SIZE.
- Split pure parsing helpers into util_parse.{h,cpp} and add unit
tests (loggerUtilTest.cpp).
- Simplify get_log_time to rely on clock_gettime (set by the GPS
driver); safer string handling in directory parsing.
- Reset _total_written in LogFileBuffer::reset() so callers reading
get_total_written() between stop and start don't see stale counts.
Sponsored by CubePilot.
fuseDirectStateMeasurement() ends with a call to constrainStateVariances(),
which iterates every state and, for any whose variance has grown past its
limit, calls fuseDirectStateMeasurement() again to apply a corrective
fuse. That inner call then runs its own constrainStateVariances(),
repeating the loop.
Call chain:
fuseDirectStateMeasurement ekf_helper.cpp:1030
`- constrainStateVariances ekf_helper.cpp:1085
`- constrainStateVar covariance.cpp:290
`- fuseDirectStateMeasurement covariance.cpp:302
`- constrainStateVariances (recursion)
`- ...
Under normal operation the P(i,i) > max branch is rarely taken and the
recursion stays shallow, however, while doing unrelated overnight testing
we saw a hard-fault with TCB corruption, presumably due to a stack overflow.
The trace pointed to fuseDirectStateMeasurement where Claude detected
the recursive call.
This fix breaks the recursion by gating the constrainStateVariances()
call on a new parameter, defaulting to true for existing callers.
constrainStateVar() passes false when it re-enters the fuse from the
constraint path, capping the worst-case recursion depth at 2.
These are generic flight controller boards (not drone-specific
products), so they should not come with a pre-selected airframe like
every other PX4 board. Drop the SYS_AUTOSTART default, matching the
convention used across the rest of the tree.
Drone-specific products (bitcraze/crazyflie, atl/mantis-edu) retain
their default airframe.
This board is not available for purchase online and the author confirmed
that it is "only used for research and educational purposes". Given
there are no docs about it, we should just remove it.
Set BOARD_FLASH_SECTORS to 13 so the bootloader does not erase the
parameter sectors (14 and 15) during firmware updates. Previously set
to 14 which allowed the bootloader to erase sector 14, potentially
wiping stored parameters.
* lightware_grf_serial: disable flow control
This might fix serial comms with the Lightware sensor.
* update sf45 to disable hw flow control
Signed-off-by: dirksavage88 <dirksavage88@gmail.com>
---------
Signed-off-by: dirksavage88 <dirksavage88@gmail.com>
Co-authored-by: dirksavage88 <dirksavage88@gmail.com>
It turns out ICM20602 and ICM20948 are not actually started/probed on
the px4/v6x platform. We should be able to remove them without
repercussions and save some flash.
Julian Oes
