refactor(uORB): pass generation to SubscriptionCallback::call() to drop the _last_generation atomic

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.
This commit is contained in:
Julian Oes
2026-06-18 15:39:42 +12:00
committed by Ramon Roche
parent bffa69a554
commit 0aa25de0dd
6 changed files with 32 additions and 20 deletions

View File

@@ -55,7 +55,7 @@ bool Subscription::subscribe()
if (node) {
_node = node;
_last_generation.store(initial_generation);
_last_generation = initial_generation;
return true;
}
}
@@ -70,15 +70,15 @@ void Subscription::unsubscribe()
}
_node = nullptr;
_last_generation.store(0);
_last_generation = 0;
}
bool Subscription::update(void *dst)
{
if (subscribe()) {
unsigned gen = _last_generation.load();
unsigned gen = _last_generation;
bool ret = Manager::orb_data_copy(_node, dst, gen, true);
_last_generation.store(gen);
_last_generation = gen;
return ret;
}
@@ -88,9 +88,9 @@ bool Subscription::update(void *dst)
bool Subscription::copy(void *dst)
{
if (subscribe()) {
unsigned gen = _last_generation.load();
unsigned gen = _last_generation;
bool ret = Manager::orb_data_copy(_node, dst, gen, false);
_last_generation.store(gen);
_last_generation = gen;
return ret;
}
@@ -172,7 +172,7 @@ bool Subscription::advertised()
bool Subscription::updated()
{
if (subscribe()) {
return Manager::updates_available(_node, _last_generation.load());
return Manager::updates_available(_node, _last_generation);
}
return false;

View File

@@ -118,7 +118,7 @@ public:
bool ChangeInstance(uint8_t instance);
uint8_t get_instance() const { return _instance; }
unsigned get_last_generation() const { return _last_generation.load(); }
unsigned get_last_generation() const { return _last_generation; }
orb_id_t get_topic() const { return get_orb_meta(_orb_id); }
ORB_ID orb_id() const { return _orb_id; }
@@ -132,7 +132,7 @@ protected:
void *_node{nullptr};
px4::atomic<unsigned> _last_generation{0}; /**< last generation the subscriber has seen */
unsigned _last_generation{0}; /**< last generation the subscriber has seen */
ORB_ID _orb_id{ORB_ID::INVALID};
uint8_t _instance{0};

View File

@@ -64,7 +64,7 @@ public:
pthread_cond_destroy(&_cv);
}
void call() override
void call(unsigned generation) override
{
// signal immediately if no interval, otherwise only if interval has elapsed
hrt_abstime last_update = _last_update.load();

View File

@@ -41,6 +41,7 @@
#include <uORB/SubscriptionInterval.hpp>
#include <containers/List.hpp>
#include <px4_platform_common/px4_work_queue/WorkItem.hpp>
#include <drivers/drv_hrt.h>
namespace uORB
{
@@ -101,14 +102,19 @@ public:
return ret;
}
virtual void call() = 0;
virtual void call(unsigned generation) = 0;
bool registered() const { return _registered; }
protected:
bool _registered{false};
// Node generation at our last ScheduleNow(), used by the count gate. Only ever
// touched from call() (which runs under the publishing node's lock, so it is
// serialized) - do NOT write it from the subscriber thread (e.g. in
// registerCallback()), or it becomes a cross-thread race. It self-initializes:
// the first call() sees a large delta and schedules once, then tracks normally.
unsigned _last_scheduled_generation{0};
};
// Subscription with callback that schedules a WorkItem
@@ -130,14 +136,20 @@ public:
virtual ~SubscriptionCallbackWorkItem() = default;
void call() override
void call(unsigned generation) override
{
// schedule immediately if updated (queue depth or subscription interval)
uint8_t req = _required_updates.load();
// 'generation' is the publishing node's generation, handed in by the
// publisher - so unlike before we never read the subscriber's own cursor
// (_last_generation, mutated on the subscriber's thread) from here.
// Schedule once enough new samples have accrued since our last schedule
// (count gate), respecting the optional interval (interval gate).
const uint8_t req = _required_updates.load();
if ((req == 0)
|| (Manager::updates_available(_subscription.get_node(), _subscription.get_last_generation()) >= req)) {
if (updated()) {
if ((generation - _last_scheduled_generation) >= req) {
const hrt_abstime last_update = _last_update.load();
if ((_interval_us == 0) || (hrt_elapsed_time(&last_update) >= _interval_us)) {
_last_scheduled_generation = generation;
_work_item->ScheduleNow();
}
}

View File

@@ -196,7 +196,7 @@ uORB::DeviceNode::write(cdev::file_t *filp, const char *buffer, size_t buflen)
// callbacks
for (auto item : _callbacks) {
item->call();
item->call(generation + 1);
}
/* Mark at least one data has been published */

View File

@@ -45,7 +45,7 @@ public:
CallbackHandler(orb_id_t id) : uORB::SubscriptionCallback(id) {}
virtual ~CallbackHandler() {}
void call() override
void call(unsigned generation) override
{
px4::msg::GpioIn new_input;