Both were defined inline in the header, so the List add/remove bodies
were duplicated into the constructor and destructor of every one of the
150+ ModuleParams-derived classes. Construction and destruction are cold
paths; a call is smaller at each site.
Two consumers reach ModuleParams without px4_platform on the link line
and now need the definitions explicitly: health_and_arming_checks (the
functional-ModeManagement test only pulls it in transitively through
modules__commander, after px4_platform) and the failsafe_web emscripten
build, which compiles module_params.cpp directly.
The smaller constructors let the compiler fully inline the defaulted
FlightTaskDescend constructor on the ITCM boards, so its entry is
dropped from their linker scripts.
Saves 3968 B of .text on px4_fmu-v6x_default.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
Co-authored-by: Balduin <balduin@auterion.com>
board_determine_hw_info() inits LPADC2 and consumes the function-static
once flag, so board_adc's later LPADC1 init is a no-op. CFG stays at
reset (PWREN=0, no settling). Match the STM32 once-per-base pattern.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
board_query_manifest walked the list for mft[ndx].id == id then
returned &mft[id]. On a sparse list that is the wrong row, and for
PX4_MFT_T100_ETH (id 7) it is out of bounds.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
macos.sh --sim-tools installs the unversioned Homebrew opencv formula, which
is now 5.0.0, and PX4-OpticalFlow does not build against it:
klt_feature_tracker/src/trackFeatures.cpp:43:10:
fatal error: 'opencv2/core/types_c.h' file not found
PX4-OpticalFlow/src/flow_opencv.cpp:110:7:
fatal error: no member named 'undistortPoints' in namespace 'cv'
types_c.h was removed in OpenCV 5, and undistortPoints moved when calib3d was
split into 3d/calib. The failure lands about a thousand targets into
make px4_sitl and points at submodule sources rather than at the dependency.
ubuntu.sh takes libopencv-dev from apt, which is still 4.x, so CI never sees
this.
Install opencv@4 instead. It is keg-only, so find_package still resolves to
5.0.0 without a prefix hint. Add one alongside the qt@5 hint from 9c2e634325,
which is keg-only for the same reason.
Assisted-by: Claude:claude-opus-5[1m]
Signed-off-by: Jackson Korba <jackson.korba@gmail.com>
Run the ELF import checker after linking the QURT shared object when a
board registers provider symbol files or forbidden import prefixes.
Include the linked C++ runtime shared objects as providers when
validating against a target system image.
Assisted-by: OpenAI:Codex
* fix(cdcacm_autostart): start MAVLink without holding the USB port
SYS_USB_AUTO=2 opened /dev/ttyACM0 O_RDONLY and kept it for the life of
the link, then treated a successful mavlink spawn as permanent success.
If mavlink later failed its UART open retries and exited, the driver
stayed "connected" and never restarted — listen-first hosts (production
USB MAVLink benches) saw no heartbeats until a VBUS cycle.
Start mavlink without a probe open, track its PID and restart if it
dies while VBUS is present, close the autodetect fd before handing the
device to a protocol, and drop the unused legacy cdc_acm_check path.
Assisted-by: Grok:grok-4.5
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* fix(cdcacm_autostart): initialise actuator_armed before the armed gate
copy() leaves the destination untouched when a topic has never been advertised,
so with commander not running `report` is whatever was on the stack. When that
garbage has armed set, run_state_machine() takes the "do not reconfigure USB
while flying" branch every cycle and never advances the state machine, so
MAVLink is never started on the USB CDC — on a vehicle that is definitionally
not flying, because commander is not running.
Seen on an ARK FMU v6X production test bench: a `commander stop` early in the
sequence leaves `listener actuator_armed` reporting "never published", and from
then on `mavlink status` shows no ttyACM instance at all while a host that opens
the port waits out its timeout against silence.
Zero-initialising restores the correct default for that case: not armed, so the
state machine runs.
* Update src/drivers/cdcacm_autostart/cdcacm_autostart.cpp
* Update src/drivers/cdcacm_autostart/cdcacm_autostart.cpp
* Update src/drivers/cdcacm_autostart/cdcacm_autostart.cpp
* Update src/drivers/cdcacm_autostart/cdcacm_autostart.cpp
---------
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
Make px4::atomic's value constructor and ModuleBase::Descriptor
constexpr so the ~40 per-module 'desc' statics are constant-initialised
instead of each emitting static-init code, and apply the same to the
mixer_module FunctionProvider table.
Signed-off-by: Balduin <balduin@auterion.com>
* refactor(platforms/nuttx): remove dead PX4_I2C_BUS_MTD board define
The macro's value has been unused since the MTD manifest refactor
(68ab736b1 "Refactor mtd to make available to board startup"), which made
the board manifest the source of truth for the EEPROM bus and address:
at24xxx_attach() derives the bus from PX4_I2C_DEVID_BUS(instance.devid).
All that survived was a presence check that silently degraded a former
compile-time #error into a runtime failure.
Because the value stopped mattering, it drifted freely. Eleven boards
carry "4,5", which is not even a usable bus number, and several boards
disagree with their own manifest (fmu-v5x declares 4,5 against actual
buses 3 and 4; fmu-v6xrt declares 1 against 3 and 6; 7-nano declares 1
against 4). crazyflie21 defines it with no MTD manifest at all.
Guard at24xxx_attach() on CONFIG_I2C instead, which is the condition the
code actually depends on and matches how the neighbouring ramtron_attach()
guards on CONFIG_MTD_RAMTRON.
Assisted-by: Claude:claude-opus-5[1m]
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* fix(platforms/nuttx): guard at24xxx_attach on CONFIG_MTD
CONFIG_I2C is too broad: nearly every board enables I2C, including those
without MTD. Compiling the real at24xxx_attach body kept the mtd_partition
/ register_mtddriver / ftl_initialize path live, so boards without
CONFIG_MTD failed to link.
CONFIG_MTD is the NuttX flag that actually enables that infrastructure
(and is set on every board with an I2C EEPROM manifest). CONFIG_MTD_AT24XX
is not suitable: only crazyflie sets it; FMU boards use px4_24xxxx_mtd.c
and deliberately omit the NuttX AT24 driver.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
---------
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* boards: enable CONFIG_FS_LARGEFILE for NuttX boards with MMC/SD
Without large file support, fsblkcnt_t/off_t are 32-bit and statfs
consumers (procfs /fs/usage, df) compute the volume size as
f_bsize * f_blocks in 32 bits, which wraps at 4GB. Boards with an
SD card or eMMC larger than 4GB report the wrong capacity, e.g. a
32GB SD card on FMU-V6X:
nsh> ls -l /dev/mmcsd0
brw-rw-rw-1850212352 /dev/mmcsd0 (= 31914983424 mod 2^32)
nsh> df -h
vfat 1749M 240K 1749M /fs/microsd
With CONFIG_FS_LARGEFILE=y the same card reports the full 29G.
This enables the option on the 77 boards that build the MMC/SD block
driver (CONFIG_MMCSD=y, SDIO or SPI). Boards without removable storage
and constrained-flash targets (CONFIG_BOARD_CONSTRAINED_FLASH=y) are
left unchanged.
Note: on the px4_firmware_nuttx-10.3.0+ branch the 64-bit file types
are additionally gated on CONFIG_HAVE_LONG_LONG, which is only defined
via nuttx/compiler.h include order, making the option unreliable. That
gating is fixed by the companion NuttX backport PR
(PX4/NuttX, branch backport-fs-largefile, upstream commit 92b2f1bd3d3).
Tested on a custom STM32H743 board with 8GB eMMC and on FMU-V6X with a
32GB SD card.
* fix(boards): enable CONFIG_FS_LARGEFILE for MMC/SD variant configs
The test/console/stackcheck/socketcan/sysview/cyphal/debug/cryptotest
variant configs of boards covered by the previous commit were missed,
leaving them with 32-bit off_t. fmu-v5 protected is intentionally
excluded: protected-mode syscall proxies keep 32-bit widths.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* chore(nuttx): bump submodule for FS_LARGEFILE backport
Picks up the backport of upstream NuttX 92b2f1bd3d3, which gates the
64-bit file types on CONFIG_FS_LARGEFILE alone instead of the
include-order-dependent CONFIG_HAVE_LONG_LONG.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
---------
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
Co-authored-by: Jacob Dahl <dahl.jakejacob@gmail.com>
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>
