refactor(uORB): use a dedicated SubscriptionIntervalAtomic for callbacks

_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).

boards/nxp/mr-tropic/nuttx-config/scripts/itcm_functions_includes.ld

@@ -69,7 +69,8 @@
 *(.text._ZN15OutputPredictor19correctOutputStatesEyRKN6matrix10QuaternionIfEERKNS0_7Vector3IfEERK9LatLonAltS8_S8_)
 *(.text._ZN3Ekf12predictStateERKN9estimator9imuSampleE)
 *(.text._ZN3px46logger6Logger3runEv)
-*(.text._ZN4uORB20SubscriptionInterval7updatedEv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb0EE7updatedEv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb1EE7updatedEv)
 *(.text._ZN24MavlinkStreamCommandLong4sendEv)
 *(.text._ZN9Commander3runEv)
 *(.text._ZN3Ekf17predictCovarianceERKN9estimator9imuSampleE)
@@ -237,7 +238,8 @@
 *(.text._ZN3Ekf16controlMagFusionERKN9estimator9imuSampleE)
 *(.text.pthread_sem_give)
 *(.text._ZN7sensors10VehicleIMU16ParametersUpdateEb)
-*(.text._ZN4uORB20SubscriptionInterval4copyEPv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb0EE4copyEPv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb1EE4copyEPv)
 *(.text._ZN12I2CSPIDriverI9ICM42688PE3RunEv)
 *(.text.imxrt_epcomplete.constprop.0)
 *(.text._ZNK6matrix6MatrixIfLj3ELj1EEmiERKS1_)

boards/nxp/tropic-community/nuttx-config/scripts/itcm_functions_includes.ld

@@ -69,7 +69,8 @@
 *(.text._ZN15OutputPredictor19correctOutputStatesEyRKN6matrix10QuaternionIfEERKNS0_7Vector3IfEERK9LatLonAltS8_S8_)
 *(.text._ZN3Ekf12predictStateERKN9estimator9imuSampleE)
 *(.text._ZN3px46logger6Logger3runEv)
-*(.text._ZN4uORB20SubscriptionInterval7updatedEv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb0EE7updatedEv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb1EE7updatedEv)
 *(.text._ZN24MavlinkStreamCommandLong4sendEv)
 *(.text._ZN9Commander3runEv)
 *(.text._ZN3Ekf17predictCovarianceERKN9estimator9imuSampleE)
@@ -237,7 +238,8 @@
 *(.text._ZN3Ekf16controlMagFusionERKN9estimator9imuSampleE)
 *(.text.pthread_sem_give)
 *(.text._ZN7sensors10VehicleIMU16ParametersUpdateEb)
-*(.text._ZN4uORB20SubscriptionInterval4copyEPv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb0EE4copyEPv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb1EE4copyEPv)
 *(.text._ZN12I2CSPIDriverI9ICM42688PE3RunEv)
 *(.text.imxrt_epcomplete.constprop.0)
 *(.text._ZNK6matrix6MatrixIfLj3ELj1EEmiERKS1_)

boards/px4/fmu-v6xrt/nuttx-config/scripts/itcm_functions_includes.ld

@@ -69,7 +69,8 @@
 *(.text._ZN15OutputPredictor19correctOutputStatesEyRKN6matrix10QuaternionIfEERKNS0_7Vector3IfEERK9LatLonAltS8_S8_)
 *(.text._ZN3Ekf12predictStateERKN9estimator9imuSampleE)
 *(.text._ZN3px46logger6Logger3runEv)
-*(.text._ZN4uORB20SubscriptionInterval7updatedEv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb0EE7updatedEv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb1EE7updatedEv)
 *(.text._ZN24MavlinkStreamCommandLong4sendEv)
 *(.text._ZN9Commander3runEv)
 *(.text._ZN3Ekf17predictCovarianceERKN9estimator9imuSampleE)
@@ -240,7 +241,8 @@
 *(.text._ZN3Ekf16controlMagFusionERKN9estimator9imuSampleE)
 *(.text.pthread_sem_give)
 *(.text._ZN7sensors10VehicleIMU16ParametersUpdateEb)
-*(.text._ZN4uORB20SubscriptionInterval4copyEPv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb0EE4copyEPv)
+*(.text._ZN4uORB24SubscriptionIntervalBaseILb1EE4copyEPv)
 *(.text._ZN12I2CSPIDriverI9ICM42688PE3RunEv)
 *(.text.imxrt_epcomplete.constprop.0)
 *(.text._ZNK6matrix6MatrixIfLj3ELj1EEmiERKS1_)

platforms/common/uORB/Subscription.hpp

@@ -41,7 +41,6 @@
 #include <uORB/uORB.h>
 #include <uORB/topics/uORBTopics.hpp>
 
-#include <px4_platform_common/atomic.h>
 #include <px4_platform_common/defines.h>
 #include <lib/mathlib/mathlib.h>
 

platforms/common/uORB/SubscriptionBlocking.hpp

@@ -68,8 +68,9 @@ public:
 	{
 		// signal immediately if no interval, otherwise only if interval has elapsed
 		hrt_abstime last_update = _last_update.load();
+		const uint32_t interval_us = _interval_us.load();
 
-		if ((_interval_us == 0) || (hrt_elapsed_time(&last_update) >= _interval_us)) {
+		if ((interval_us == 0) || (hrt_elapsed_time(&last_update) >= interval_us)) {
 			pthread_cond_signal(&_cv);
 		}
 	}

platforms/common/uORB/SubscriptionCallback.hpp

@@ -47,7 +47,11 @@ namespace uORB
 {
 
 // Subscription wrapper class with callbacks on new publications
-class SubscriptionCallback : public SubscriptionInterval, public ListNode<SubscriptionCallback *>
+//
+// Uses SubscriptionIntervalAtomic because call() runs on the publishing thread
+// (synchronously from orb_publish) while the subscriber thread updates the
+// interval / last-update fields.
+class SubscriptionCallback : public SubscriptionIntervalAtomic, public ListNode<SubscriptionCallback *>
 {
 public:
 	/**
@@ -58,7 +62,7 @@ public:
 	 * @param instance The instance for multi sub.
 	 */
 	SubscriptionCallback(const orb_metadata *meta, uint32_t interval_us = 0, uint8_t instance = 0) :
-		SubscriptionInterval(meta, interval_us, instance)
+		SubscriptionIntervalAtomic(meta, interval_us, instance)
 	{
 	}
 
@@ -147,8 +151,9 @@ public:
 
 		if ((generation - _last_scheduled_generation) >= req) {
 			const hrt_abstime last_update = _last_update.load();
+			const uint32_t interval_us = _interval_us.load();
 
-			if ((_interval_us == 0) || (hrt_elapsed_time(&last_update) >= _interval_us)) {
+			if ((interval_us == 0) || (hrt_elapsed_time(&last_update) >= interval_us)) {
 				_last_scheduled_generation = generation;
 				_work_item->ScheduleNow();
 			}

platforms/common/uORB/SubscriptionInterval.cpp

@@ -33,21 +33,29 @@
 
 #include "SubscriptionInterval.hpp"
 
+#include <drivers/drv_hrt.h>
+
 namespace uORB
 {
 
-bool SubscriptionInterval::updated()
+// The bodies are defined out-of-line and explicitly instantiated below (rather
+// than inline in the header) to keep them from being emitted at every call site
+// - they are shared by many subscribers, so the flash cost matters on target.
+
+template <bool ATOMIC>
+bool SubscriptionIntervalBase<ATOMIC>::updated()
 {
 	hrt_abstime last_update = _last_update.load();
 
-	if (advertised() && (hrt_elapsed_time(&last_update) >= _interval_us)) {
+	if (advertised() && (hrt_elapsed_time(&last_update) >= _interval_us.load())) {
 		return _subscription.updated();
 	}
 
 	return false;
 }
 
-bool SubscriptionInterval::update(void *dst)
+template <bool ATOMIC>
+bool SubscriptionIntervalBase<ATOMIC>::update(void *dst)
 {
 	if (updated()) {
 		return copy(dst);
@@ -56,15 +64,17 @@ bool SubscriptionInterval::update(void *dst)
 	return false;
 }
 
-bool SubscriptionInterval::copy(void *dst)
+template <bool ATOMIC>
+bool SubscriptionIntervalBase<ATOMIC>::copy(void *dst)
 {
 	if (_subscription.copy(dst)) {
 		const hrt_abstime now = hrt_absolute_time();
+		const uint32_t interval_us = _interval_us.load();
 
-		// make sure we don't set a timestamp before the timer started counting (now - _interval_us would wrap because it's unsigned)
-		if (now > _interval_us) {
+		// make sure we don't set a timestamp before the timer started counting (now - interval_us would wrap because it's unsigned)
+		if (now > interval_us) {
 			// shift last update time forward, but don't let it get further behind than the interval
-			_last_update.store(math::constrain(_last_update.load() + _interval_us, now - _interval_us, now));
+			_last_update.store(math::constrain(_last_update.load() + interval_us, now - interval_us, now));
 
 		} else {
 			_last_update.store(now);
@@ -76,4 +86,13 @@ bool SubscriptionInterval::copy(void *dst)
 	return false;
 }
 
+// Only these two variants exist, so instantiate them here once each.
+template bool SubscriptionIntervalBase<false>::updated();
+template bool SubscriptionIntervalBase<false>::update(void *dst);
+template bool SubscriptionIntervalBase<false>::copy(void *dst);
+
+template bool SubscriptionIntervalBase<true>::updated();
+template bool SubscriptionIntervalBase<true>::update(void *dst);
+template bool SubscriptionIntervalBase<true>::copy(void *dst);
+
 } // namespace uORB

platforms/common/uORB/SubscriptionInterval.hpp

@@ -53,8 +53,44 @@
 namespace uORB
 {
 
+namespace subscription_interval_detail
+{
+
+// Drop-in, non-atomic stand-in for px4::atomic. It mirrors the load()/store()
+// interface so the shared SubscriptionIntervalBase body works unchanged, but
+// compiles down to plain member access with no synchronization overhead.
+template <typename T>
+class NonAtomic
+{
+public:
+	NonAtomic() = default;
+	explicit NonAtomic(T value) : _value(value) {}
+	T load() const { return _value; }
+	void store(T value) { _value = value; }
+private:
+	T _value{};
+};
+
+// Minimal compile-time type selector (std::conditional is unavailable: the STL is
+// not used in firmware headers that also build for NuttX).
+template <bool ATOMIC, typename T> struct storage { using type = px4::atomic<T>; };
+template <typename T> struct storage<false, T> { using type = NonAtomic<T>; };
+
+template <bool ATOMIC, typename T>
+using storage_t = typename storage<ATOMIC, T>::type;
+
+} // namespace subscription_interval_detail
+
 // Base subscription wrapper class
-class SubscriptionInterval
+//
+// ATOMIC selects whether the interval and last-update fields use atomic storage.
+// They only need to be atomic for callback subscriptions (SubscriptionCallback and
+// derived), where call() runs on the publishing thread while the subscriber thread
+// updates them. Plain subscriptions are single-threaded, so the default keeps the
+// fields as plain members. Use the SubscriptionInterval / SubscriptionIntervalAtomic
+// aliases below rather than this template directly.
+template <bool ATOMIC = false>
+class SubscriptionIntervalBase
 {
 public:
 
@@ -65,7 +101,7 @@ public:
 	 * @param interval The requested maximum update interval in microseconds.
 	 * @param instance The instance for multi sub.
 	 */
-	SubscriptionInterval(ORB_ID id, uint32_t interval_us = 0, uint8_t instance = 0) :
+	SubscriptionIntervalBase(ORB_ID id, uint32_t interval_us = 0, uint8_t instance = 0) :
 		_subscription{id, instance},
 		_interval_us(interval_us)
 	{}
@@ -77,14 +113,14 @@ public:
 	 * @param interval The requested maximum update interval in microseconds.
 	 * @param instance The instance for multi sub.
 	 */
-	SubscriptionInterval(const orb_metadata *meta, uint32_t interval_us = 0, uint8_t instance = 0) :
+	SubscriptionIntervalBase(const orb_metadata *meta, uint32_t interval_us = 0, uint8_t instance = 0) :
 		_subscription{meta, instance},
 		_interval_us(interval_us)
 	{}
 
-	SubscriptionInterval() : _subscription{nullptr} {}
+	SubscriptionIntervalBase() : _subscription{nullptr} {}
 
-	~SubscriptionInterval() = default;
+	~SubscriptionIntervalBase() = default;
 
 	bool subscribe() { return _subscription.subscribe(); }
 	void unsubscribe() { _subscription.unsubscribe(); }
@@ -113,7 +149,7 @@ public:
 	bool		valid() const { return _subscription.valid(); }
 
 	uint8_t		get_instance() const { return _subscription.get_instance(); }
-	uint32_t        get_interval_us() const { return _interval_us; }
+	uint32_t        get_interval_us() const { return _interval_us.load(); }
 	unsigned	get_last_generation() const { return _subscription.get_last_generation(); }
 	orb_id_t	get_topic() const { return _subscription.get_topic(); }
 
@@ -121,13 +157,13 @@ public:
 	 * Set the interval in microseconds
 	 * @param interval The interval in microseconds.
 	 */
-	void		set_interval_us(uint32_t interval) { _interval_us = interval; }
+	void		set_interval_us(uint32_t interval) { _interval_us.store(interval); }
 
 	/**
 	 * Set the interval in milliseconds
 	 * @param interval The interval in milliseconds.
 	 */
-	void		set_interval_ms(uint32_t interval) { _interval_us = interval * 1000; }
+	void		set_interval_ms(uint32_t interval) { _interval_us.store(interval * 1000); }
 
 	/**
 	 * Set the last data update
@@ -137,9 +173,27 @@ public:
 protected:
 
 	Subscription	_subscription;
-	px4::atomic<uint64_t>	_last_update{0};	// last subscription update in microseconds
-	uint32_t	_interval_us{0};	// maximum update interval in microseconds
+	subscription_interval_detail::storage_t<ATOMIC, uint64_t>	_last_update{0};	// last subscription update in microseconds
+	subscription_interval_detail::storage_t<ATOMIC, uint32_t>	_interval_us{0};	// maximum update interval in us
 
 };
 
+// Single-threaded subscription with a minimum update interval (the common case).
+using SubscriptionInterval = SubscriptionIntervalBase<false>;
+
+// Same, but with atomic interval / last-update fields for the callback subscriptions,
+// where call() runs on the publishing thread concurrently with the subscriber.
+using SubscriptionIntervalAtomic = SubscriptionIntervalBase<true>;
+
+// The out-of-line methods are explicitly instantiated in SubscriptionInterval.cpp.
+// Declaring them extern here keeps other translation units from emitting their own
+// weak copies, so the single strong definition can be pinned into ITCM by name on
+// the boards that do so (see boards/*/nuttx-config/scripts/itcm_*.ld).
+extern template bool SubscriptionIntervalBase<false>::updated();
+extern template bool SubscriptionIntervalBase<false>::update(void *dst);
+extern template bool SubscriptionIntervalBase<false>::copy(void *dst);
+extern template bool SubscriptionIntervalBase<true>::updated();
+extern template bool SubscriptionIntervalBase<true>::update(void *dst);
+extern template bool SubscriptionIntervalBase<true>::copy(void *dst);
+
 } // namespace uORB