Give PRs generated via the skill a fixed, skimmable structure instead of an ad-hoc leading paragraph. Keeps the existing no-attribution, no-test-plan, and terse rules.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
wait_until_ready() polls MAVSDK's is_armable, but a known hysteresis gap
(and estimator heading-reference flicker at startup) means the flag can
read armable while PX4 then denies the arm command with 'no heading
reference'. This made arm() fail intermittently. Retry the arm command
until the autopilot accepts it, so the transient is absorbed while real
failures still time out.
Signed-off-by: Julian Oes <julian@oes.ch>
The four older VTOL RTL tests waited only 120s for disarm while running
the same climb-to-550m-and-land missions as the other RTL tests, which
already use 150s. 'RTL direct Mission Land' occasionally tipped just
over 120s of sim time (observed 123.76s), failing wait_until_disarmed.
Align all of them to 150s for margin.
Signed-off-by: Julian Oes <julian@oes.ch>
Switching the SITL tests to the SIH simulator exercises sih.cpp, whose
gcov data contains a spurious negative counter (line 510) that newer
lcov treats as a fatal error, failing the coverage step. Pass
--ignore-errors negative so lcov zeroes the bad count and continues, as
the tool itself suggests.
Signed-off-by: Julian Oes <julian@oes.ch>
The 0.01m (1cm) tolerance was not reachable within the 15s sim-time
window for the standard_vtol, causing the takeoff-and-hold test to time
out. Use 0.1m, which still verifies altitude hold but is physically
achievable, and stays below the 0.15m hold-monitoring tolerance.
Signed-off-by: Julian Oes <julian@oes.ch>
MAVSDK's OffboardImpl::process_heartbeat() resets the setpoint state to
NotActive when a heartbeat without offboard mode arrives more than 3 s
after _last_started. _last_started is only ever written by start(), so
before the first start() it is epoch zero and the guard is always true:
any heartbeat that slips in between set_*() and start() makes start()
fail with NoSetpointSet.
At speed factor 30 PX4's 1 Hz (sim time) heartbeat arrives every ~33 ms
of wall time, which made 'Offboard attitude control' fail roughly one
run in three. Re-send the setpoint and retry start() in a small loop at
both offboard start sites. Failed 1/3 before, 16/16 after.
This is arguably a MAVSDK bug (_last_started should be initialized when
setpoint streaming begins); a retry keeps the tests robust either way.
Signed-off-by: Julian Oes <julian@oes.ch>
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.
poll_condition_with_timeout polls the autopilot's monotonic time, which is
already simulation time. The previous code divided a sim-time budget by the
speed factor to compute a wall-clock timeout, but this is unnecessary and
adds a dependency on the speed-factor query.
Symmetric with the existing COM_DL_LOSS_T / COM_RC_LOSS_T /
COM_OF_LOSS_T / COM_OBC_LOSS_T scalings just above. At high simulation
speed the default 10s preflight disarm is reached much sooner in
real wall time than tests expect, so scale it the same way.
Replace 5 functions that used std::future::wait_for() (wall-clock time)
with poll_condition_with_timeout() (simulation time). This prevents
timeouts from expiring prematurely when running at high speed factors
under lockstep.
Affected functions:
- wait_until_altitude()
- wait_for_landed_state()
- wait_for_flight_mode()
- start_and_wait_for_mission_sequence()
- wait_until_speed_lower_than()
Also remove debug printf statements from start_checking_altitude().
The DO_LAND_START item carried loiter params (heading required, radius,
and a stray 1 in param3) hand-copied from the adjacent LOITER_TO_ALT
item, and LOITER_TO_ALT itself had param3 set, which the spec defines
as empty. PX4's mission item param validation now rejects such items.
Signed-off-by: Julian Oes <julian@oes.ch>
The param validation table rejected mission items that QGC-generated
plans legitimately contain:
- NAV_VTOL_LAND param3 is the approach altitude per the MAVLink spec.
QGC writes it in every VTOL landing pattern; PX4's mission logic
ignores it, but that is no reason to reject the upload.
- DO_LAND_START param5-7 are the optional lat/lon/alt marker position
per the spec, which QGC also writes.
Both made any QGC VTOL landing plan fail to upload with
MAV_MISSION_INVALID_PARAMx, surfaced by the SIH SITL VTOL tests.
Signed-off-by: Julian Oes <julian@oes.ch>
The ExternalProject_Add(mavsdk_tests ...) lived at the bottom of
sitl_targets_gazebo-classic.cmake, outside the if(gazebo_FOUND) gate
and with a comment noting it doesn't depend on sitl_gazebo. Historical
artifact: mavsdk_tests is a black-box MAVLink test driver that talks
to a running PX4 over UDP, and works against any simulator backend
(sitl_gazebo-classic, gz, sih, jmavsim, ...).
With CI moving from classic Gazebo to SIH, the misplacement becomes
actively misleading. Pull the block into a new top-level
cmake/mavsdk_tests.cmake and include it from CMakeLists.txt under the
existing posix-only block, so the dependency graph reflects what is
actually true.
Behavior is unchanged: target is EXCLUDE_FROM_ALL, only built on
demand via `make mavsdk_tests` or `make tests_integration`.
Reverts the hotfix from #27487. The navigator now guarantees the course
field is NaN unless Course mode explicitly commands it, so
PX4_ISFINITE(course) is again a sufficient trigger for course guidance.
Signed-off-by: Ramon Roche <mrpollo@gmail.com>
The reposition triplet was zero-initialized at boot and memset after
consumption, and mission_item_to_position_setpoint() never wrote the
course field, so a finite course (0.0, or stale after leaving Course
mode) could leak into published position setpoints. Reset the
reposition triplet with reset_position_setpoint() and clear course when
converting mission items, so the field is only finite while Course mode
commands it.
Replaces the consumer-side clear from #27537 with a fix at the source.
Signed-off-by: Ramon Roche <mrpollo@gmail.com>
* fix(drivers/uavcannode): make CANNODE_NODE_ID the sole node ID policy
The startup bootloader_app_shared_write() never reached the bootloader:
it only accepts App-signed data and invalidates the shared region on
every boot. The only consumer was uavcannode itself on a restart
without a reset (nsh stop/start), where the persisted bus_speed=0 made
the CAN driver init fail and left the node off the bus. The bootloader
learns our node ID exclusively through the firmware update handoff in
cb_beginfirmware_update, which already carries the static ID.
CANNODE_NODE_ID now overrides the bootloader handoff only when nonzero,
so the default (0) reuses the ID the bootloader already allocated
instead of running allocation a second time. The handoff is ignored
unless it carries a valid bitrate, healing regions poisoned by firmware
that wrote bus_speed=0. Max is 125 because 126/127 are reserved for
debug tools by DroneCAN convention, matching UAVCAN_NODE_ID.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* fix(drivers/uavcannode): harden bootloader handoff validation
Invalidate the shared region whenever it was read valid, not only when
the handoff is accepted, so stale data cannot linger on boards whose
bootloader never invalidates it. Require a plausible bitrate (the
CANNODE_BITRATE minimum) rather than nonzero: the canbootloader alt-app
update path can hand off bus_speed = CAN_UNDEFINED (999). Reword the
invalid-param error, which claimed dynamic allocation while keeping the
bootloader handoff.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
---------
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
CONFIG_GPS_SPARTN landed in #27919; call out the opt-in framing path and that keys stay on the receiver.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* feat(gps): inject SPARTN corrections alongside RTCM
Add a SPARTN transport-layer framer and feed gps_inject_data through both
RTCM3 and SPARTN parsers so PointPerfect-style SPARTN streams can be
reassembled and written to the receiver the same way RTCM already is.
Gated by CONFIG_GPS_SPARTN (default on). Disabled on px4_fmu-v6x where
flash is already at the limit; enabled on ark_can-rtk-gps.
Depends on PX4-GPSDrivers for automatic u-blox SPARTN input enable.
Signed-off-by: alexklimaj <alex@arkelectron.com>
* fix(gnss): avoid undefined shift in SPARTN CRC-32
Use uint64_t for the CRC working register so n==32 does not perform
1u << 32 (clang-analyzer BitwiseShift).
Signed-off-by: alexklimaj <alex@arkelectron.com>
* make format
* feat(gps): enable SPARTN support in board configurations
* feat(gps): enhance SPARTN support with additional frame tracking and status reporting
* fix(gps): frame RTCM3 and SPARTN from a single buffer
Feeding every inject chunk to an independent framer per protocol let each
one resync inside the other's payloads. That is not symmetric: RTCM3 is
covered by CRC-24Q, but SPARTN's header carries no usable integrity check
(TF006 is 4 bits over a non-byte-aligned field) and TF005 permits an 8-bit
message CRC, so a stray 0x73 in an RTCM3 payload is framed as SPARTN at
roughly 1 in 1024.
RTCM3-only is what every board actually runs, and over 50 MB of it the two
framers produced 211 bogus SPARTN frames (210 declaring CRC-8), each
re-injecting up to 1 kB of the stream back into the receiver. The reverse
direction produced none.
Frame both protocols from one buffer instead: whichever preamble comes
first is framed, and a valid frame consumes its own payload, so bytes
inside one protocol's frame never start the other's. The same 50 MB now
yields zero. Frames are also injected in arrival order rather than all
RTCM3 then all SPARTN, and one buffer replaces two (2248 B/instance,
down from ~4350 B).
Also reject TF002 message types 5-119, which SPARTN reserves, as the one
header field with a checkable range.
CONFIG_GPS_SPARTN was default y, so it built into every target with a GPS
including px4_fmu-v6x, which the flash report showed gaining the framer
despite the intent to keep it off. Default it to n and enable it explicitly
where it is wanted; the ark GPS boards already opt in, and SITL opts in so
the framing tests keep running in CI.
Rtcm3Parser and SpartnParser are replaced by CorrectionFramer; their tests
carry over to it. RtcmStress fed "garbage" drawn from 0x01-0xD2 to avoid a
preamble, which includes 0x73, and RtcmBustedSender ended its stream on a
candidate the framer was still waiting to complete; both now avoid every
preamble and flush respectively.
* feat(gps): enable SPARTN on ARK flight controllers
Covers receivers attached over UART rather than CAN. All four targets
link with margin; fmu-v6xrt has no px4board on this branch yet.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
---------
Signed-off-by: alexklimaj <alex@arkelectron.com>
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
Co-authored-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* fix(gps): split RTCM corrections and moving-baseline uORB topics
The single gps_inject_data topic served two unrelated purposes:
external fixed-base RTCM corrections (from MAVLink GPS_RTCM_DATA or
UAVCAN RTCMStream) and moving-base-to-rover RTCM 4072. In a
dual-GPS-with-moving-base plus fixed-base setup, the two streams
collided on the same queue and the FMU UAVCAN bridge mirrored
fixed-base RTCM onto the MovingBaselineData CAN message, breaking
rover heading or RTK fix (see PX4/PX4-Autopilot#27088).
Split by role:
- rtcm_corrections (renamed from gps_inject_data): external RTCM
flowing into the vehicle; producers are
MAVLink, UAVCAN RTCMStream, and GPS drivers in
dump mode.
- rtcm_moving_baseline (new): moving-base GPS output intended for a
rover; single producer per vehicle
(MAX_INSTANCES = 1).
The GPS driver routes its own RTCM output to the right topic via
GPSHelper::isMovingBase(), and gates the two inbound streams per role
using new GPSHelper virtuals (PX4-GPSDrivers#212):
shouldInjectRTCMCorrections() is true for any configured receiver, so a
UART2 moving-base rover still accepts fixed-base corrections over its
main link; shouldInjectMovingBaseline() is true only for a UART1/CAN
heading rover, since a UART2 rover gets the baseline directly in
hardware and a moving base produces rather than consumes it. That
submodule PR also renames the ambiguous UBXMode fields to name their
UART explicitly (RoverWithMovingBase -> RoverWithMovingBaseUART2,
MovingBase -> MovingBaseUART2).
Septentrio's publish_rtcm_corrections() always publishes to
rtcm_moving_baseline (only the Secondary moving base calls it).
Rover-side consumers (gps, septentrio, uavcan bridge) drain both topics
independently; each topic gets its own stale-link switchover timer so
corrections failover is not suppressed by moving-baseline traffic, or
vice versa.
FMU UAVCAN bridge: two independent drain loops, one per topic. No
more dual-publish of a single uORB message onto both RTCMStream and
MovingBaselineData CAN streams.
CANnode MovingBaselineDataPub subscribes to rtcm_moving_baseline. The
bus_type == UAVCAN check is kept, now purely as a loop guard so a node
with both CANNODE_PUB_MBD and CANNODE_SUB_MBD does not rebroadcast a
peer's moving-baseline data back onto the bus. CANnode RTCMStream
subscriber maps each CAN source node ID to its own rtcm_corrections
instance (one PublicationMulti per source, capped at MAX_INSTANCES) so
multiple CAN RTCM sources (e.g. dual rovers outputting MSM7 for logging
plus a fixed-base feed) land on independent uORB instances instead of
interleaving on one, which would otherwise defeat the consumer's
per-instance stale-link selection.
Depends on PX4-GPSDrivers#212 (submodule bump included).
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* fix(gps): use separate RTCM parsers for corrections and moving baseline
On a rover injecting both fixed-base corrections and moving-baseline RTCM, feeding both streams through a single parser allowed a fragmented frame from one source to be corrupted by bytes interleaved from the other. Reassemble each stream in its own Rtcm3Parser so frames are recovered independently.
Also collapse the two near-identical topics into a single RtcmData.msg published under both rtcm_corrections and rtcm_moving_baseline (the SensorGps pattern), track corrections and moving-baseline injection on separate perf counters so the reported corrections rate is no longer inflated by moving-baseline traffic, and zero-initialize the CAN DeviceId unions before populating their fields.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* fix(septentrio): avoid bugprone sizeof division on RTCM byte buffer
moving_baseline.data is a uint8_t array, so sizeof(data)/sizeof(data[0]) divides by 1; clang-tidy's bugprone-sizeof-expression flags this as a suspicious sizeof(T)/sizeof(T). Use sizeof(data) directly - the capacity value is unchanged.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* refactor(gps): use dedicated per-stream RTCM drain functions
rtcm_moving_baseline has a single publisher (instance 0), so its
consumers are now a plain uORB::Subscription instead of a 4-instance
SubscriptionMultiArray, and the per-stream selected-instance and
stale-link timer members it no longer needs are removed.
With each RTCM stream now a fixed type with a single caller, the
templated drain helpers (drain_rtcm_subscriptions, the overloaded
drain_rtcm_to_can) bought nothing, so replace them with dedicated
functions: drainRtcmCorrections()/drainMovingBaseline() in the GPS
driver and the UAVCAN bridge, drain_rtcm_corrections()/
drain_moving_baseline() in Septentrio. The UAVCAN bridge calls
PublishRTCMStream/PublishMovingBaselineData directly instead of through
Forward lambdas.
Rename SeptentrioDriver::publish_rtcm_corrections() to
publish_moving_baseline(): it is only reached from the Secondary
moving-base decode path and only ever emits moving-baseline RTCM.
Corrections-path behavior (instance selection, generation-gap warning,
burst cap, self-injection filter) is unchanged.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* refactor(gps): rename RTCM inject gate to receiverReady and simplify chunk helper
Rename GPSHelper::shouldInjectRTCMCorrections() to receiverReady(). The
virtual gates injection of both RTCM corrections and moving-baseline, and
for UBX it simply reports whether the receiver is configured, so the name
now describes what it actually gates rather than implying it only concerns
corrections. Bumps the GPS-drivers submodule to the matching rename.
Drop the vestigial message-type template parameter from publish_rtcm_chunks:
both topics share rtcm_data_s, so only the publication type needs templating.
* fix(septentrio): log dropped RTCM uORB generations
Match the gps driver and warn when the RTCM corrections or moving-baseline
subscription skips a uORB generation, so dropped injection data is visible.
* docs(docs): Docs only update to the RtcmData msg
* chore(gps): pin GPSDrivers to merged main
Contains #212 (RTCM/moving-baseline gating virtuals), #213 (X20 CFG-ODO
NAK tolerance), and #215 (SPARTN input enable, best-effort VALSET).
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* fix(septentrio): reassemble RTCM frames per stream before injecting
Both drains wrote raw uORB chunks to the receiver, so a fragmented frame
on one stream could get the other stream's bytes spliced in mid-frame and
corrupt both. Reassemble each stream in its own parser and only write
complete frames, mirroring the gps driver. Injection stats now count
frames instead of uORB chunks.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
* refactor(mavlink)!: remove GPS_RTCM_DATA output stream
GPS_RTCM_DATA is a GCS-to-vehicle correction transport; echoing
rtcm_corrections back out over MAVLink had no consumer and the echo was
lossy anyway (uint8 len and 180-byte payload truncate 300-byte uORB
chunks). Receiving GPS_RTCM_DATA is unchanged.
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
---------
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
We should not publish gimbal things if we don't actually have a gimbal
model in Gz. Otherwise this confuses any other gimbal setups that one
might try.
* fix(navigator): remove generic reset of navigator triplets on mode change
- make each navigation mode decide if it needs the reset and what data it
wants to capture before the reset
- fix RTL climbing so that it remains on an established loiter when RTL
is being activated -> avoids weird trajectories and potential geofence breaches
Signed-off-by: RomanBapst <bapstroman@gmail.com>
* feat(mavsdk_tests): added various integration tests for loitering behavior:
- make sure RTL climb remains on established loiter
- make sure new loiter at current location is set when HOLD is engaged while
vehicle is transiting to another loiter
- make sure altitude is locked to current altitude when vehicle is currently
flying an established loiter but is transiting to a higher altitude
- make sure vehicle resets from figure of 8 to a loiter when HOLD is engaged
while vehicle is established on a figure of 8
Signed-off-by: RomanBapst <bapstroman@gmail.com>
* navigator: improve wording
Signed-off-by: RomanBapst <bapstroman@gmail.com>
* rtl_mission_direct_land: avoid loitering on a previously set figure of 8
Signed-off-by: RomanBapst <bapstroman@gmail.com>
* fix(navigator): harden geofence position checks and preserve loiter on breach
The GF_SOURCE_GPS path previously gated the breach check on the global
position timestamp even though it evaluated the fence against raw GPS
coordinates. Validate each source against the data it actually uses:
the fused global position on freshness (<1s), and raw GPS on freshness
(<2s) plus a valid fix.
For the LOITER breach action, only issue the reposition when the fused
global position is valid, since the setpoint is flown on that estimate,
and source all reposition coordinates from it. When the vehicle is
already established on a circular (orbit) loiter, keep that loiter's
center instead of re-centering on the current position to avoid a jump.
Signed-off-by: RomanBapst <bapstroman@gmail.com>
* fixup
Signed-off-by: RomanBapst <bapstroman@gmail.com>
* make methods const
Signed-off-by: RomanBapst <bapstroman@gmail.com>
---------
Signed-off-by: RomanBapst <bapstroman@gmail.com>
