From 24d42efdabc44374fece007ed2d80460eed9b745 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 12:36:10 +0200 Subject: [PATCH 01/15] Add issue: DataLost should wake the consumer via onDataAvailable The multi reader detects data loss but does not raise the callback gate, so the consumer is never notified and must run its own liveness timer (as the Sum Reader FB does). Documents the problem and a two-part fix: fire onDataAvailable on the DataLost transition, and arm the data-loss monitor at the start of monitoring so a missing first packet trips the same deadline as a producer that stops. Co-Authored-By: Claude Opus 4.8 --- .../11_issue_data_loss_callback.md | 152 ++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 multi-reader-rework-docs/11_issue_data_loss_callback.md diff --git a/multi-reader-rework-docs/11_issue_data_loss_callback.md b/multi-reader-rework-docs/11_issue_data_loss_callback.md new file mode 100644 index 0000000000..63d3bc5c99 --- /dev/null +++ b/multi-reader-rework-docs/11_issue_data_loss_callback.md @@ -0,0 +1,152 @@ +# Issue: `DataLost` does not wake the consumer — the reader computes the loss but never notifies + +Status: **Open** — proposed for the multi-reader rework. +Source: design review of the Sum Reader FB rework (`refactor/AI-refactor`), 2026-07-22. + +## Guiding principle + +The whole point of the multi reader is that **the consumer implements as little as possible**. The +reader owns synchronization, event ordering, data-loss detection, and recovery signalling; the +consumer should only have to (a) install one `onDataAvailable` callback and (b) `read()` when told +to, inspecting the returned status. Every timer, deadline, or liveness heuristic the consumer is +forced to run itself is a leak of reader responsibility into user code, and every such leak is a +pattern each future consumer has to re-implement (and get wrong). + +Measured against that principle, **data loss is currently a leak**: the reader detects it but does +not tell the consumer, so the consumer has to run its own clock to notice. + +## Problem + +A `DataLost` transition does **not** raise the `onDataAvailable` callback gate. The reader detects +the loss, updates its internal `ReaderState`, and stops there — the consumer is never woken and only +discovers the loss if it happens to `read()` for some other reason. + +### Evidence + +The callback gate never consults `ReaderState` (`core/opendaq/reader/docs/multi_reader.md:264`, +`core/opendaq/reader/src/multi_reader/notification_coordinator.cpp:158`): + +``` +shouldInvokeCallback = anyEvent() || allUsedReady() + = event.any() || (used.any() && (ready & used) == used) +``` + +Trace of a data-loss deadline crossing: + +1. `DataLossMonitor`'s waiter thread wakes at the deadline and fires its callback, which calls + `notificationCoordinator->requestEvaluation()` (`multi_reader_impl.cpp:140-146`). +2. The coalesced evaluation runs the full ladder and reaches the data-loss branch + (`multi_reader_impl.cpp:913-943`), which calls `invalidateSynchronizationLocked()` and + `setStateWithAffectedLocked(ReaderState::DataLost, …)`. +3. `setStateLocked` (`multi_reader_impl.cpp:392-397`) writes only `state`, `stateMessage`, and + `stateAffectedInputs`. It sets **no** event bit and **no** ready bit. +4. The dead input is still `used` but not `ready`, and no event bit is set anywhere, so + `shouldInvokeCallback()` evaluates to `false`. **No callback fires.** + +The reader has correctly computed `DataLost` (and a `read()` would return it), but nobody is told to +go read it. + +### Impact on consumers + +Because the reader stays silent, a consumer that must react to a stalled input has to run its own +liveness clock. The Sum Reader FB does exactly this, and it is pure duplication of logic the reader +already performs: + +- `SumReaderFbImpl::onPacketReceived` overrides the port notification purely to run a + data-loss/staleness timer off the healthy inputs' packet stream + (`sum_reader_fb_impl.cpp:333-340`), scheduling a deferred `read()` so the loss surfaces. +- `SumReaderFbImpl::maybeProbeLocked` additionally has to abandon a "stuck probe" — an input marked + used again whose producer is permanently silent — after one interval, because nothing ever tells + the FB that input is not delivering (`sum_reader_fb_impl.cpp:889-916`). + +Every future consumer of the multi reader would have to reinvent both mechanisms. That is precisely +the pattern the rework is meant to eliminate. + +## Proposed fix + +### Part 1 — a `DataLost` transition triggers the callback + +The timer is up; the consumer should be told. When the reader transitions a monitored input into +`DataLost`, it must raise the callback gate so `onDataAvailable` fires and the consumer's subsequent +`read()` returns the `DataLost` status (naming the affected inputs, exactly as a normal read would). + +This is nearly wired already — the `DataLossMonitor` waiter thread already drives a coalesced +evaluation on the deadline (`multi_reader_impl.cpp:140-146`). The only missing link is that the +evaluation, upon entering `DataLost`, does not raise the gate. The fix is to make the gate reflect a +state change that the consumer must observe, so the same waiter-thread-driven evaluation ends in a +fired callback rather than a silent state write. + +Suggested mechanism (implementer's choice of the two): + +- **Preferred — an explicit "attention" latch in `NotificationCoordinator`.** Add a latched signal + that `shouldInvokeCallback()` ORs in, set whenever the reader enters a state the consumer must be + told about that carries no event packet and no readiness (`DataLost`, and by the same argument + `SynchronizationFailed` / `Incompatible` / `Fail`), and cleared when the consumer reads that + status (or when the reader leaves the state). This keeps the existing `event` bit meaning strictly + "a returnable event packet exists" and cleanly separates "a state change you must see." +- **Minimal alternative — set the event bit on the affected slots** at the `DataLost` transition. + Smaller diff, but overloads the `event` bit (whose current contract is "returnable event packet"), + so the read path must tolerate an event-gated wake that yields a state rather than a packet. + +Either way the consumer contract becomes: *you are always woken when there is something to read — +data, an event, or a state change that demands action.* No consumer-side timer required. + +### Part 2 — arm the monitor at the start of monitoring, not on the first packet + +Today the monitor **arms on the first packet after a slot becomes monitored**: `onPacket` sets +`armed = true` (`data_loss_monitor.cpp:116-135`), while `setMonitored(slot, true)` deliberately does +not arm (`data_loss_monitor.cpp:137-153`), and `hasLostSlots()` requires `armed` +(`data_loss_monitor.cpp:101-114`). Consequently the monitor can only see *"was producing, then went +silent"* — it is blind to *"became used/connected/active but never produced a first packet."* + +That blind spot is the entire reason the FB needs its separate stuck-probe abandonment: a probed +input whose producer is permanently silent never arms, so no deadline ever trips. + +**Fix: arm at the start of monitoring.** In `setMonitored(slot, true)`, set `armed = true` and +`lastArrival = clock()`, so the slot has a deadline exactly one timeout into the future from the +moment it starts being monitored. A first packet that never comes is just as harmful as a producer +that stops — both should trip the same deadline. Packets still refresh `lastArrival` and clear the +loss on recovery, exactly as now. + +With Part 2 in place, the "input never produced" / "stuck probe" case is covered by the very same +`DataLost` path as "input died," and Part 1 makes both wake the consumer. The two parts together let +a consumer delete all of its own liveness timing. + +## Consequences / considerations for the implementer + +- **The data-loss timeout also becomes a first-data/establishment deadline.** Arming at start means + an input that connects but is slow to deliver its first packet (or its first descriptor) beyond the + timeout will be reported `DataLost`. This is the intended broadening (no data at the start is as + harmful as data stopping), but it couples "slow initial connection" and "steady-state loss" under + one timeout. If a different establishment budget is ever wanted, it can be a separate value that + defaults to the data-loss timeout; not required for this fix. +- **Monitoring is still gated on `used && connected && active`** (`multi_reader_impl.cpp:807-808`), + so parked/unused inputs and an inactive reader still do not arm — arming at start only affects + slots that are actually being monitored. A slot turned off clears its arming as it does today. +- **Fire-once semantics stay.** The monitor's `reported` flag already ensures one callback per + crossing (`data_loss_monitor.cpp:198-235`); the attention latch (Part 1) must likewise not + re-fire until cleared, to avoid a busy-wake loop while a loss is outstanding. +- **Consumer simplification is the acceptance signal.** After this change, `SumReaderFbImpl` should + be able to drop the staleness branch of `onPacketReceived` and the stuck-probe abandonment in + `maybeProbeLocked`, relying solely on `onDataAvailable` + the returned status. If it still needs a + timer, the fix is incomplete. + +## Affected files + +- `core/opendaq/reader/src/multi_reader/notification_coordinator.{h,cpp}` — attention latch + gate. +- `core/opendaq/reader/src/multi_reader_impl.cpp` — raise the gate on the `DataLost` transition + (and, if adopted, the other attention states); clear on read/state-exit. +- `core/opendaq/reader/src/multi_reader/data_loss_monitor.cpp` — arm in `setMonitored(_, true)`. +- `core/opendaq/reader/docs/multi_reader.md` — document that a state change raises the gate, and the + arm-at-start semantics (§9.5 Data loss, §the gate formula). +- Consumer cleanup (validation): `examples/modules/ref_fb_module/.../src/sum_reader_fb_impl.cpp`. + +## Acceptance criteria + +1. A monitored input that stops producing for longer than the timeout fires `onDataAvailable`; the + next `read()` returns a `DataLost` status naming that input, with no `read()`/timer on the + consumer side in between. +2. A monitored input that never produces a first packet within the timeout is reported identically. +3. Recovery is unchanged: a packet on the affected input clears the loss and resynchronizes. +4. No repeated wakes while a single loss is outstanding. +5. `SumReaderFbImpl` compiles and behaves correctly with its consumer-side liveness timing removed. From 1d5870d5a6c495ebfe20fcb3bfbac594e8ae6f96 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 12:39:27 +0200 Subject: [PATCH 02/15] Spec: data-loss recognition must trigger the onDataAvailable callback Document in the specification that a transition into DataLost raises the public callback so the consumer learns of a stalled input from the next read()'s status, without polling or a consumer-side liveness timer. Adds a stateChangeNotify term to the NotificationCoordinator gate (3.5) and states the callback-on-expiry requirement in the DataLossMonitor section (3.6). Co-Authored-By: Claude Opus 4.8 --- multi-reader-rework-docs/01_specification.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/multi-reader-rework-docs/01_specification.md b/multi-reader-rework-docs/01_specification.md index a7a06927d6..383e281284 100644 --- a/multi-reader-rework-docs/01_specification.md +++ b/multi-reader-rework-docs/01_specification.md @@ -139,16 +139,22 @@ One coordinator handles both paths; resampling is a per-input execution branch, Maintains `usedMask`, `readyMask`, `eventMask` bitsets. After any slot update: ``` -schedule one coalesced task iff (eventMask & usedMask).any() || (readyMask & usedMask) == usedMask +schedule one coalesced task iff (eventMask & usedMask).any() + || (readyMask & usedMask) == usedMask + || stateChangeNotify ``` -The scheduled task re-runs state evaluation and invokes the public `onDataAvailable` callback only if an event is returnable or one full block is readable. Callback is never invoked from `packetReceived` and never while any internal lock is held. The "ready" meaning is phase-dependent: first sample while synchronizing, one full block while synchronized. Blocked reads with a timeout are woken through the same path plus a condition variable. +The scheduled task re-runs state evaluation and invokes the public `onDataAvailable` callback when an event is returnable, one full block is readable, **or** the reader has just recognized a condition the consumer must act on that carries neither returnable data nor a returnable event — currently the transition into `DataLost`. + +Data-loss recognition **must** wake the consumer. The reader detects the loss on its own (scheduler-armed deadline; see §3.6) and the elapsed deadline is itself the notification: the consumer must not have to poll or run its own liveness timer to discover that an input has stalled. On the transition into `DataLost` the reader raises `onDataAvailable`, and the consumer reads the naming `DataLost` status on its next `read()`. This keeps the consumer implementation lean — a single `onDataAvailable` handler plus a `read()` is sufficient to observe data, events, and stalled inputs alike. + +`stateChangeNotify` is a latch set on entry to such a state and cleared once the consumer has been notified (the state is read or the reader leaves it), so a single loss wakes the consumer exactly once and does not busy-loop while the loss is outstanding. Callback is never invoked from `packetReceived` and never while any internal lock is held. The "ready" meaning is phase-dependent: first sample while synchronizing, one full block while synchronized. Blocked reads with a timeout are woken through the same path plus a condition variable. `getAvailableCount` and the read methods process pending inputs synchronously — correctness never depends on the scheduler having run the coalesced task. ### 3.6 `DataLossMonitor` (new) -`setDataLossTimeout(t)`; `0` disables (default). Armed per input after the first packet following connect or activation. On expiry (scheduler-armed deadline; fires even with no further packets), the affected inputs form the lost set → `DataLost` state listing every lost input. The next packet from an input clears only that input; the reader leaves `DataLost` when the set is empty. Inactive, unused, and disconnected slots are not monitored. +`setDataLossTimeout(t)`; `0` disables (default). Armed per input after the first packet following connect or activation. On expiry (scheduler-armed deadline; fires even with no further packets), the affected inputs form the lost set → `DataLost` state listing every lost input, and the reader raises the `onDataAvailable` callback (§3.5) so the consumer is notified of the loss without polling. The next packet from an input clears only that input; the reader leaves `DataLost` when the set is empty. Inactive, unused, and disconnected slots are not monitored. --- From 298ccbe33ee56122b7ca89f3725d835c31683e69 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 12:42:19 +0200 Subject: [PATCH 03/15] Spec: arm the data-loss monitor at the start of monitoring A slot arms when it becomes monitored (used + connected + active), with its deadline one full timeout ahead, rather than deferring until the first packet. A first packet that never arrives trips the same deadline as a producer that stops, so the timeout also bounds initial establishment: a used input that connects but delivers nothing within the timeout is reported DataLost (ladder step 9) instead of lingering in WaitingForDescriptors/WaitingForData. A slot re-arms from scratch each time it re-enters the monitored set. Co-Authored-By: Claude Opus 4.8 --- multi-reader-rework-docs/01_specification.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/multi-reader-rework-docs/01_specification.md b/multi-reader-rework-docs/01_specification.md index 383e281284..1a71c0ba74 100644 --- a/multi-reader-rework-docs/01_specification.md +++ b/multi-reader-rework-docs/01_specification.md @@ -154,7 +154,7 @@ Data-loss recognition **must** wake the consumer. The reader detects the loss on ### 3.6 `DataLossMonitor` (new) -`setDataLossTimeout(t)`; `0` disables (default). Armed per input after the first packet following connect or activation. On expiry (scheduler-armed deadline; fires even with no further packets), the affected inputs form the lost set → `DataLost` state listing every lost input, and the reader raises the `onDataAvailable` callback (§3.5) so the consumer is notified of the loss without polling. The next packet from an input clears only that input; the reader leaves `DataLost` when the set is empty. Inactive, unused, and disconnected slots are not monitored. +`setDataLossTimeout(t)`; `0` disables (default). A slot is **armed at the moment it becomes monitored** (used + connected + active), with its deadline set one full timeout ahead — arming is **not** deferred until the first packet. A first packet that never arrives is just as harmful as a producer that stops after delivering some, and both trip the same deadline: the timeout therefore also bounds initial establishment. A used input that connects but delivers no first packet (or no descriptors) within the timeout is reported `DataLost` at ladder step 9, rather than lingering in `WaitingForDescriptors`/`WaitingForData` indefinitely. Each arriving packet refreshes the slot's deadline to one timeout ahead. On expiry (scheduler-armed deadline; fires even with no further packets), the affected inputs form the lost set → `DataLost` state listing every lost input, and the reader raises the `onDataAvailable` callback (§3.5) so the consumer is notified of the loss without polling. The next packet from an input clears only that input; the reader leaves `DataLost` when the set is empty. Inactive, unused, and disconnected slots are not monitored; a slot re-arms from scratch each time it (re)enters the monitored set (e.g. after reconnect, reactivation, or `setInputUsed(true)`), so a stale pre-off arrival never counts toward a new deadline. --- From 5563e1dede3e5dce7cf218e6271754a7e8b2f496 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 12:51:11 +0200 Subject: [PATCH 04/15] Multi reader: DataLost recognition triggers the onDataAvailable callback Implements spec 3.5/3.6: a transition into DataLost now raises the public callback so the consumer learns of a stalled input from the next read()'s status, without polling or a consumer-side liveness timer. NotificationCoordinator gains a one-shot stateChangeNotify latch that shouldInvokeCallback() ORs in. The owner sets it in setStateLocked on the edge into DataLost (or when the lost set changes), and consumes it in onCoalescedEvaluation once the callback has fired - so a single loss wakes the consumer exactly once and healthy-input packets do not re-fire it while the loss persists. Co-Authored-By: Claude Opus 4.8 --- .../multi_reader/notification_coordinator.h | 18 +++++++++++++++--- .../multi_reader/notification_coordinator.cpp | 14 +++++++++++++- core/opendaq/reader/src/multi_reader_impl.cpp | 13 +++++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h b/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h index 252ddfb284..ffb96d5601 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h @@ -42,11 +42,14 @@ namespace multi_reader * task state outlives the coordinator and is checked under its own lock. * * 2. Used/ready/event masks deciding whether the public onDataAvailable callback fires: - * event.any() || (used.any() && (ready & used) == used). + * event.any() || (used.any() && (ready & used) == used) || stateChangeNotify. * Events on unused slots participate deliberately: they are the recovery * signal consumers react to with setInputUsed. The "ready" meaning is phase-dependent * (first sample while synchronizing, one full block while synchronized) - the owner - * sets the bits during its state evaluation. + * sets the bits during its state evaluation. stateChangeNotify is a one-shot latch for a + * state change that carries no returnable data or event (currently the transition into + * DataLost): the elapsed deadline is itself the notification, so the consumer is woken + * once and reads the naming status - it never has to poll or run its own liveness timer. * * Threading contract: requestEvaluation() and detach() are thread-safe. Everything else * (masks, callback queries) must be called with the owner's state lock held. The @@ -94,13 +97,21 @@ class NotificationCoordinator /// Clears ready and event bits (synchronization invalidated, topology changed, ...). void clearReadiness(); + /// One-shot latch: raise the callback gate for a state change that carries no returnable + /// data or event (currently the transition into DataLost). Set by the owner on the + /// transition; the owner consumes it (sets false) once the callback has fired, so a single + /// occurrence wakes the consumer exactly once and does not re-fire while it persists. + void setStateChangeNotify(bool notify); + bool getStateChangeNotify() const; + /// (event & used).any() bool anyUsedEvent() const; /// event.any() - unused slots included. bool anyEvent() const; /// used.any() && (ready & used) == used bool allUsedReady() const; - /// The callback gate: fires when there is any event, or when every used slot is ready. + /// The callback gate: fires when there is any event, when every used slot is ready, or when + /// a state-change notification is latched. bool shouldInvokeCallback() const; private: @@ -120,6 +131,7 @@ class NotificationCoordinator std::vector usedMask; std::vector readyMask; std::vector eventMask; + bool stateChangeNotifyFlag = false; }; } // namespace multi_reader diff --git a/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp b/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp index de06086d6d..837d71cf03 100644 --- a/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp +++ b/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp @@ -121,6 +121,16 @@ void NotificationCoordinator::clearReadiness() eventMask.assign(eventMask.size(), false); } +void NotificationCoordinator::setStateChangeNotify(bool notify) +{ + stateChangeNotifyFlag = notify; +} + +bool NotificationCoordinator::getStateChangeNotify() const +{ + return stateChangeNotifyFlag; +} + bool NotificationCoordinator::anyUsedEvent() const { for (SizeT i = 0; i < usedMask.size(); ++i) @@ -159,7 +169,9 @@ bool NotificationCoordinator::shouldInvokeCallback() const { // Events on unused inputs fire the callback too; that notification is the // recovery path (the consumer can re-include the input with setInputUsed). - return anyEvent() || allUsedReady(); + // stateChangeNotify covers a state change with neither data nor event to return + // (the DataLost deadline): the elapsed timer is itself the notification. + return anyEvent() || allUsedReady() || stateChangeNotifyFlag; } } // namespace multi_reader diff --git a/core/opendaq/reader/src/multi_reader_impl.cpp b/core/opendaq/reader/src/multi_reader_impl.cpp index 04b6fef8b1..b80abe1252 100644 --- a/core/opendaq/reader/src/multi_reader_impl.cpp +++ b/core/opendaq/reader/src/multi_reader_impl.cpp @@ -391,6 +391,14 @@ void MultiReaderImpl::applyDataLossTimeoutLocked() void MultiReaderImpl::setStateLocked(ReaderState newState, std::string message, std::vector affected) { + // Entering DataLost - or changing which inputs are lost while already in it - is a condition + // the consumer must act on that carries no returnable data or event. Latch a one-shot + // callback wake so the elapsed deadline itself notifies the consumer, which then reads the + // naming status (spec 3.5/3.6). Edge-triggered: an unchanged DataLost re-evaluation does not + // re-latch, so healthy-input packets cannot re-fire the callback while the loss persists. + if (newState == ReaderState::DataLost && (state != ReaderState::DataLost || affected != stateAffectedInputs)) + notificationCoordinator->setStateChangeNotify(true); + state = newState; stateMessage = std::move(message); stateAffectedInputs = std::move(affected); @@ -1098,6 +1106,11 @@ void MultiReaderImpl::onCoalescedEvaluation() updateCallbackStateLocked(); if (notificationCoordinator->shouldInvokeCallback()) callback = readCallback; + + // One-shot: consume a latched state-change wake (DataLost) once observed, so a single + // loss fires the callback exactly once rather than on every later evaluation. A blocked + // read is woken independently by notifyCondition below. + notificationCoordinator->setStateChangeNotify(false); } notifyCondition.notify_all(); From ff48c938c44e5898c26da1afef467c0a21513e55 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 12:57:19 +0200 Subject: [PATCH 05/15] Multi reader: arm the data-loss monitor at the start of monitoring Implements spec 3.6: a slot arms the moment it becomes monitored (used + connected + active), with a deadline one full timeout ahead, instead of deferring until its first packet. setMonitored arms on turn-on and disarms on turn-off; setTimeout re-arms monitored slots from now under the new timeout. Each packet still refreshes the deadline. Effect: a used input that is established (descriptors present) but never delivers data - notably a re-enabled input whose producer has gone quiet, whose queued data was dropped on re-enable - now trips the deadline and surfaces as DataLost (step 9, before WaitingForData) instead of hanging. This is what lets a consumer drop its own stuck-input timer. Tightens the 3.6 spec prose to match the verified ladder behavior: an input that has not yet delivered its initial descriptors precedes step 9 and remains WaitingForDescriptors (a never-established input is not "lost"); the initial descriptor event refreshes the deadline, so the data timeout is measured from establishment. Co-Authored-By: Claude Opus 4.8 --- .../opendaq/multi_reader/data_loss_monitor.h | 13 +++++--- .../src/multi_reader/data_loss_monitor.cpp | 31 +++++++++++++++---- multi-reader-rework-docs/01_specification.md | 2 +- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/core/opendaq/reader/include/opendaq/multi_reader/data_loss_monitor.h b/core/opendaq/reader/include/opendaq/multi_reader/data_loss_monitor.h index 80d2e1468c..6dfc772f11 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/data_loss_monitor.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/data_loss_monitor.h @@ -32,10 +32,12 @@ namespace multi_reader /** * @brief Per-input packet-liveness deadlines. * - * Monitoring arms per slot on the first packet after the slot becomes monitored - * (used + connected + reader active); a monitored, armed slot whose last arrival is older - * than the timeout is lost. A slot recovers when its next packet arrives; the owner leaves - * the DataLost state when no lost slots remain. Zero timeout disables monitoring (default). + * Monitoring arms a slot the moment it becomes monitored (used + connected + reader active), + * with a deadline one full timeout ahead - a missing first packet is treated exactly like a + * producer that stops after delivering some. A monitored, armed slot whose last arrival is older + * than the timeout is lost. Each packet refreshes the deadline; a slot recovers when its next + * packet arrives, and the owner leaves the DataLost state when no lost slots remain. Zero timeout + * disables monitoring (default). * * Deadlines fire without reads: a waiter thread wakes at the earliest unreported deadline * and invokes the deadline callback (once per crossing), which the owner routes into the @@ -80,7 +82,8 @@ class DataLossMonitor void onPacket(SizeT slot); /// Owner gate: monitoring applies only to used, connected slots of an active reader. - /// Turning a slot off clears its arming; it re-arms on the first packet after turning on. + /// Turning a slot off clears its arming; turning it on arms it immediately (deadline one + /// timeout ahead), and each packet thereafter refreshes the deadline. void setMonitored(SizeT slot, bool monitored); /// Monitored, armed slots whose deadline has expired, in slot order. diff --git a/core/opendaq/reader/src/multi_reader/data_loss_monitor.cpp b/core/opendaq/reader/src/multi_reader/data_loss_monitor.cpp index dda58dfa04..36ff3094f5 100644 --- a/core/opendaq/reader/src/multi_reader/data_loss_monitor.cpp +++ b/core/opendaq/reader/src/multi_reader/data_loss_monitor.cpp @@ -53,13 +53,22 @@ void DataLossMonitor::setTimeout(std::chrono::nanoseconds newTimeout) { std::unique_lock lock(mutex); timeout = newTimeout; - // Both directions clear the arming: disabling stops monitoring outright, and enabling - // must not count arrivals recorded before the deadline existed - each slot re-arms on - // its first packet under the new timeout + const auto now = clock(); + // Re-arm from now under the new timeout: a monitored slot gets a fresh full deadline (arming + // is not deferred to its next packet), and an arrival recorded before this timeout existed + // must not count toward it. Disabling (0) disarms every slot. for (auto& slot : slots) { - slot.armed = false; slot.reported = false; + if (slot.monitored && newTimeout.count() > 0) + { + slot.armed = true; + slot.lastArrival = now; + } + else + { + slot.armed = false; + } } ensureWaiterLocked(lock); cv.notify_all(); @@ -143,11 +152,21 @@ void DataLossMonitor::setMonitored(SizeT slot, bool monitored) if (slots[slot].monitored == monitored) return; slots[slot].monitored = monitored; - if (!monitored) + slots[slot].reported = false; + if (monitored && timeout.count() > 0) + { + // Arm at the start of monitoring: the deadline runs one full timeout from now, so a + // used + connected input of an active reader that never delivers a packet trips the + // same deadline as one whose producer stops after delivering some. onPacket refreshes + // the deadline on each arrival; turning monitoring off (below) disarms. + slots[slot].lastArrival = clock(); + slots[slot].armed = true; + } + else { slots[slot].armed = false; - slots[slot].reported = false; } + ensureWaiterLocked(lock); } cv.notify_all(); } diff --git a/multi-reader-rework-docs/01_specification.md b/multi-reader-rework-docs/01_specification.md index 1a71c0ba74..ff822ac75e 100644 --- a/multi-reader-rework-docs/01_specification.md +++ b/multi-reader-rework-docs/01_specification.md @@ -154,7 +154,7 @@ Data-loss recognition **must** wake the consumer. The reader detects the loss on ### 3.6 `DataLossMonitor` (new) -`setDataLossTimeout(t)`; `0` disables (default). A slot is **armed at the moment it becomes monitored** (used + connected + active), with its deadline set one full timeout ahead — arming is **not** deferred until the first packet. A first packet that never arrives is just as harmful as a producer that stops after delivering some, and both trip the same deadline: the timeout therefore also bounds initial establishment. A used input that connects but delivers no first packet (or no descriptors) within the timeout is reported `DataLost` at ladder step 9, rather than lingering in `WaitingForDescriptors`/`WaitingForData` indefinitely. Each arriving packet refreshes the slot's deadline to one timeout ahead. On expiry (scheduler-armed deadline; fires even with no further packets), the affected inputs form the lost set → `DataLost` state listing every lost input, and the reader raises the `onDataAvailable` callback (§3.5) so the consumer is notified of the loss without polling. The next packet from an input clears only that input; the reader leaves `DataLost` when the set is empty. Inactive, unused, and disconnected slots are not monitored; a slot re-arms from scratch each time it (re)enters the monitored set (e.g. after reconnect, reactivation, or `setInputUsed(true)`), so a stale pre-off arrival never counts toward a new deadline. +`setDataLossTimeout(t)`; `0` disables (default). A slot is **armed at the moment it becomes monitored** (used + connected + active), with its deadline set one full timeout ahead — arming is **not** deferred until the first packet. A first packet that never arrives is just as harmful as a producer that stops after delivering some, and both trip the same deadline. The timeout therefore also bounds a used input that is established (descriptors present) but delivers no data: it is reported `DataLost` at ladder step 9, rather than lingering in `WaitingForData` indefinitely. This is what lets a consumer's re-enabled-but-silent input — a probe of a producer that has gone quiet, whose queued data was dropped on re-enable — surface as a loss with no consumer-side timer. (An input that has not yet delivered even its initial descriptors precedes step 9 in the ladder and remains `WaitingForDescriptors`; a never-established input is not considered "lost". The initial descriptor event, once it arrives, refreshes the deadline, so the data timeout is measured from establishment.) Each arriving packet refreshes the slot's deadline to one timeout ahead. On expiry (scheduler-armed deadline; fires even with no further packets), the affected inputs form the lost set → `DataLost` state listing every lost input, and the reader raises the `onDataAvailable` callback (§3.5) so the consumer is notified of the loss without polling. The next packet from an input clears only that input; the reader leaves `DataLost` when the set is empty. Inactive, unused, and disconnected slots are not monitored; a slot re-arms from scratch each time it (re)enters the monitored set (e.g. after reconnect, reactivation, or `setInputUsed(true)`), so a stale pre-off arrival never counts toward a new deadline. --- From cb512e5e0de951a7b2a7ce74b471b589c35238fe Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 13:01:21 +0200 Subject: [PATCH 06/15] Sum reader FB: drop the onPacketReceived liveness checks The reader now notifies on data loss (onDataAvailable fires on the DataLost transition) and surfaces a re-enabled-but-silent input as DataLost (monitor armed at start), so the FB no longer needs its own packet-received hook to detect stalls, resolve probes, or pace recovery. Removes onPacketReceived and the deferred-check machinery (scheduleDeferredCheck/deferredCheck, deferredCheckScheduled, lastReaderCheck). The FB now reacts only to what the reader reports through onDataReceived -> processReaderLocked: a stalled or silent input arrives as an InputsFailed/DataLost status the existing handleStateLocked path already parks. Co-Authored-By: Claude Opus 4.8 --- .../ref_fb_module/sum_reader_fb_impl.h | 6 -- .../ref_fb_module/src/sum_reader_fb_impl.cpp | 83 ++----------------- 2 files changed, 6 insertions(+), 83 deletions(-) diff --git a/examples/modules/ref_fb_module/modules/ref_fb_module/include/ref_fb_module/sum_reader_fb_impl.h b/examples/modules/ref_fb_module/modules/ref_fb_module/include/ref_fb_module/sum_reader_fb_impl.h index 653043479f..aa39087a7e 100644 --- a/examples/modules/ref_fb_module/modules/ref_fb_module/include/ref_fb_module/sum_reader_fb_impl.h +++ b/examples/modules/ref_fb_module/modules/ref_fb_module/include/ref_fb_module/sum_reader_fb_impl.h @@ -23,7 +23,6 @@ #include #include -#include #include #include #include @@ -87,10 +86,7 @@ class SumReaderFbImpl final : public FunctionBlock void onConnected(const InputPortPtr& inputPort) override; void onDisconnected(const InputPortPtr& inputPort) override; - void onPacketReceived(const InputPortPtr& inputPort) override; void onDataReceived(); - void scheduleDeferredCheck(); - void deferredCheck(); void processReaderLocked(); void emitSumLocked(const std::vector& buffers, const std::vector& strides, SizeT commonCount, const MultiReaderStatusPtr& status); @@ -125,9 +121,7 @@ class SumReaderFbImpl final : public FunctionBlock // std::map: parked-port warnings enumerate in a deterministic order std::map parkedPorts; std::string probingPortId; - std::atomic deferredCheckScheduled{false}; std::chrono::steady_clock::time_point lastProbeTime{}; - std::chrono::steady_clock::time_point lastReaderCheck{}; bool readerErrored = false; SumMode mode = SumMode::EqualRates; diff --git a/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp b/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp index 7dc2c276a6..797bfddeb4 100644 --- a/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp +++ b/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include @@ -308,82 +307,14 @@ void SumReaderFbImpl::onDisconnected(const InputPortPtr& inputPort) configureValueDescriptorLocked(); } -void SumReaderFbImpl::onPacketReceived(const InputPortPtr& inputPort) -{ - // This notification can be delivered synchronously on a producer's stack - even from - // inside a connect still being constructed - so the reader must not be re-entered from - // here. Only decide whether anything needs evaluating and defer the work to a task. - bool schedule = false; - { - auto lock = this->getAcquisitionLock2(); - if (!reader.assigned() || readerErrored) - return; - - const auto now = std::chrono::steady_clock::now(); - - // Event-driven recovery of parked ports no longer needs this packet hook: unused-input - // events fire the reader's onDataAvailable callback (review Q5), and the status-driven - // probe (probeEventfulParkedLocked) reacts to them - if (!probingPortId.empty()) - { - // A pending probe resolves at status evaluation points; drive them from the - // packet stream, since a probe of a data-starved input produces no data callbacks - schedule = true; - } - else if (dataLossTimeoutSeconds > 0 && - now - lastReaderCheck >= std::chrono::duration(dataLossTimeoutSeconds)) - { - // Failure states that block data flow (a dead input never becomes ready) never - // invoke the data callback; stuck conditions are observed from the packet stream - // of the healthy inputs instead - schedule = true; - } - else if (!parkedPorts.empty() && recoveryRetryIntervalSeconds > 0 && - now - lastProbeTime >= std::chrono::duration(recoveryRetryIntervalSeconds)) - { - // Periodic fallback probing is due - schedule = true; - } - - if (schedule && deferredCheckScheduled.exchange(true)) - schedule = false; - } - - if (schedule) - scheduleDeferredCheck(); -} - -void SumReaderFbImpl::scheduleDeferredCheck() -{ - const auto scheduler = this->context.getScheduler(); - if (!scheduler.assigned()) - { - deferredCheckScheduled = false; - return; - } - - auto thisWeakRef = this->template getWeakRefInternal(); - scheduler.scheduleWork(Work( - [this, thisWeakRef = std::move(thisWeakRef)] - { - const auto thisFb = thisWeakRef.getRef(); - if (thisFb.assigned()) - this->deferredCheck(); - })); -} - -void SumReaderFbImpl::deferredCheck() -{ - auto lock = this->getAcquisitionLock2(); - deferredCheckScheduled = false; - if (!reader.assigned() || readerErrored) - return; - - processReaderLocked(); -} - void SumReaderFbImpl::onDataReceived() { + // The reader drives everything through this one callback. It fires when a block is ready, + // when an input has a returnable event (including an unused/parked input's recovery event, + // review Q5), and - since the reader's data-loss rework - when an input misses its packet + // deadline (DataLost). A stalled or never-delivering input therefore surfaces here as a + // status the read reports, so the FB needs no packet-received hook or liveness timer of its + // own: it reacts only to what the reader tells it. auto lock = this->getAcquisitionLock2(); processReaderLocked(); } @@ -393,8 +324,6 @@ void SumReaderFbImpl::processReaderLocked() if (!reader.assigned() || readerErrored) return; - lastReaderCheck = std::chrono::steady_clock::now(); - for (int iteration = 0; iteration < 64; ++iteration) { SizeT count = reader.getAvailableCount(); From 6326307dc5a4c4337fa8be2ad464459b75b59780 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 13:09:01 +0200 Subject: [PATCH 07/15] Tests: DataLost callback and arm-at-start data-loss monitoring Reader-level coverage for the data-loss rework: - notification_coordinator: stateChangeNotify opens the callback gate on its own and is a one-shot; clearReadiness leaves it intact. - data_loss_monitor: arming now happens at the start of monitoring, not on the first packet. Rewrites the two tests that encoded the old arm-on-first-packet contract (ArmsOnlyAfterFirstPacket, TurningMonitoringOffDisarms) and adds turn-on-arms-immediately cases. - multi_reader: DataLossFiresCallback proves the DataLost transition wakes onDataAvailable with nothing else driving it (Part 1); DataLossArmsAtStartForSilentReenabledInput proves a re-enabled but silent input surfaces as DataLost via arm-at-start (Part 2). Co-Authored-By: Claude Opus 4.8 --- .../reader/tests/test_data_loss_monitor.cpp | 46 ++++++-- .../reader/tests/test_multi_reader.cpp | 106 ++++++++++++++++++ .../tests/test_notification_coordinator.cpp | 33 ++++++ 3 files changed, 174 insertions(+), 11 deletions(-) diff --git a/core/opendaq/reader/tests/test_data_loss_monitor.cpp b/core/opendaq/reader/tests/test_data_loss_monitor.cpp index bdd7691e4d..5c2a815098 100644 --- a/core/opendaq/reader/tests/test_data_loss_monitor.cpp +++ b/core/opendaq/reader/tests/test_data_loss_monitor.cpp @@ -41,12 +41,34 @@ TEST_F(DataLossMonitorTest, ZeroTimeoutDisables) // DL-4 ASSERT_TRUE(monitor.lostSlots().empty()); } -TEST_F(DataLossMonitorTest, ArmsOnlyAfterFirstPacket) // DL-6 +TEST_F(DataLossMonitorTest, ArmsAtStartUnderTimeout) // DL-6 (arm at start of monitoring) { + // The slots are already monitored; enabling the timeout arms them from now. A first packet + // that never arrives trips the deadline just like a producer that stops after delivering some. monitor.setTimeout(duration_cast(100ms)); - advance(24h); - // No packet since the slots became monitored - nothing is armed, nothing is lost + + advance(99ms); ASSERT_TRUE(monitor.lostSlots().empty()); + + advance(2ms); + ASSERT_EQ(monitor.lostSlots(), (std::vector{0, 1})); +} + +TEST_F(DataLossMonitorTest, TurningMonitoringOnArmsImmediately) // arm at start of monitoring +{ + monitor.setTimeout(duration_cast(100ms)); + monitor.setMonitored(0, false); // disarm slot 0; slot 1 stays armed from setTimeout + + advance(200ms); + ASSERT_EQ(monitor.lostSlots(), (std::vector{1})); + + // Turning monitoring back on arms slot 0 from now, with no packet needed + monitor.setMonitored(0, true); + advance(99ms); + ASSERT_EQ(monitor.lostSlots(), (std::vector{1})); // slot 0 not yet expired + + advance(2ms); + ASSERT_EQ(monitor.lostSlots(), (std::vector{0, 1})); } TEST_F(DataLossMonitorTest, DeadlineExpiryReportsLoss) // DL-1 (deadline math) @@ -102,21 +124,23 @@ TEST_F(DataLossMonitorTest, UnmonitoredSlotNeverTrips) // DL-5 ASSERT_EQ(monitor.lostSlots(), (std::vector{0})); } -TEST_F(DataLossMonitorTest, TurningMonitoringOffDisarms) // DL-5/DL-6 +TEST_F(DataLossMonitorTest, TurningMonitoringOffDisarmsThenReArmsOnTurnOn) // DL-5/DL-6 { monitor.setTimeout(duration_cast(100ms)); monitor.onPacket(0); + // While off, slot 0 never trips no matter how stale (slot 1, armed from setTimeout, does) monitor.setMonitored(0, false); - monitor.setMonitored(0, true); - - // Re-enabling does not resurrect the old arrival - the slot re-arms on its next packet advance(1h); - ASSERT_TRUE(monitor.lostSlots().empty()); + ASSERT_EQ(monitor.lostSlots(), (std::vector{1})); - monitor.onPacket(0); - advance(150ms); - ASSERT_EQ(monitor.lostSlots(), (std::vector{0})); + // Turning monitoring back on re-arms slot 0 from now - not from the stale pre-off arrival + monitor.setMonitored(0, true); + advance(99ms); + ASSERT_EQ(monitor.lostSlots(), (std::vector{1})); // slot 0 fresh again + + advance(2ms); + ASSERT_EQ(monitor.lostSlots(), (std::vector{0, 1})); } TEST_F(DataLossMonitorTest, RealDeadlineFiresCallbackWithoutReads) // DL-1 (slow smoke test) diff --git a/core/opendaq/reader/tests/test_multi_reader.cpp b/core/opendaq/reader/tests/test_multi_reader.cpp index 25c9041074..1c639aed94 100644 --- a/core/opendaq/reader/tests/test_multi_reader.cpp +++ b/core/opendaq/reader/tests/test_multi_reader.cpp @@ -4397,3 +4397,109 @@ TEST_F(MultiReaderTest, DataLossDeadlineFiresWithoutReads) status = multi.read(nullptr, &count); ASSERT_EQ(status.getReadStatus(), ReadStatus::InputsFailed); } + +TEST_F(MultiReaderTest, DataLossFiresCallback) +{ + // Part 1 (spec 3.5): the transition into DataLost raises onDataAvailable on its own, so the + // consumer is woken and reads the loss with no polling. Real-time smoke test - only the + // reader's data-loss waiter thread drives the callback here. + readSignals.reserve(2); + addSignal(0, 10, createDomainSignal()); + addSignal(0, 10, createDomainSignal()); + + auto multi = MultiReaderBuilder() + .setInputPortNotificationMethod(PacketReadyNotification::SameThread) + .addSignals(signalsToList()) + .setDataLossTimeout(Ratio(1, 5)) // 200 ms + .build(); + + SizeT count{0}; + auto status = multi.read(nullptr, &count); + ASSERT_EQ(status.getReadStatus(), ReadStatus::Event); + sendPackets(0); + count = 0; + status = multi.read(nullptr, &count); + ASSERT_EQ(status.getReadStatus(), ReadStatus::Ok); + + // Drain the buffered pre-loss samples so the loss can surface once the queues run dry + { + double values0[10]{}; + double values1[10]{}; + void* buffers[2]{values0, values1}; + count = 10; + status = multi.read(buffers, &count); + ASSERT_EQ(count, 10u); + } + + // Arm the callback only now: no packet and no read drive it, so the wake we observe is the + // data-loss deadline itself + std::promise promise; + std::future future = promise.get_future(); + MultiReaderStatusPtr cbStatus; + multi.setOnDataAvailable( + [&] + { + multi.setOnDataAvailable(nullptr); // one-shot + SizeT c{0}; + cbStatus = multi.read(nullptr, &c); + promise.set_value(); + }); + + ASSERT_EQ(future.wait_for(std::chrono::seconds(5)), std::future_status::ready); + ASSERT_TRUE(cbStatus.assigned()); + ASSERT_EQ(cbStatus.getReadStatus(), ReadStatus::InputsFailed); + ASSERT_TRUE(cbStatus.getValid()); +} + +TEST_F(MultiReaderTest, DataLossArmsAtStartForSilentReenabledInput) +{ + // Part 2 (spec 3.6): a used input arms the moment it becomes monitored, so an input + // re-enabled onto a now-silent producer trips the deadline and surfaces as DataLost even + // though it never delivers a packet - the case that lets a consumer drop its own liveness timer. + readSignals.reserve(2); + auto& sig0 = addSignal(0, 10, createDomainSignal()); + auto& sig1 = addSignal(0, 10, createDomainSignal()); + + auto multi = MultiReaderBuilder() + .setInputPortNotificationMethod(PacketReadyNotification::SameThread) + .addSignals(signalsToList()) + .setDataLossTimeout(Ratio(10, 1)) // ten virtual seconds + .build(); + + auto* impl = dynamic_cast(multi.asPtr().getObject()); + ASSERT_NE(impl, nullptr); + auto virtualNow = std::chrono::steady_clock::now(); + impl->setDataLossClockForTest([&virtualNow] { return virtualNow; }); + + SizeT count{0}; + auto status = multi.read(nullptr, &count); + ASSERT_EQ(status.getReadStatus(), ReadStatus::Event); + sendPackets(0); + count = 0; + status = multi.read(nullptr, &count); + ASSERT_EQ(status.getReadStatus(), ReadStatus::Ok); + { + double values0[10]{}; + double values1[10]{}; + void* buffers[2]{values0, values1}; + count = 10; + status = multi.read(buffers, &count); + ASSERT_EQ(count, 10u); + } + + // Exclude input 1, then re-enable it onto a silent producer: it arms at the moment monitoring + // resumes, with no packet to refresh the deadline. + multi.setInputUsed(sig1.signal.getGlobalId(), false); + multi.setInputUsed(sig1.signal.getGlobalId(), true); + + // Input 0 keeps delivering so it stays fresh; input 1 never does and crosses its deadline. + virtualNow += std::chrono::seconds(11); + sig0.createAndSendPacket(1); + + count = 0; + status = multi.read(nullptr, &count); + ASSERT_EQ(status.getReadStatus(), ReadStatus::InputsFailed); + ASSERT_TRUE(status.getValid()); + ASSERT_EQ(static_cast(static_cast(status.getInputStates().get(sig1.signal.getGlobalId()))), + InputState::DataLost); +} diff --git a/core/opendaq/reader/tests/test_notification_coordinator.cpp b/core/opendaq/reader/tests/test_notification_coordinator.cpp index 5698dd2c61..3bf571531c 100644 --- a/core/opendaq/reader/tests/test_notification_coordinator.cpp +++ b/core/opendaq/reader/tests/test_notification_coordinator.cpp @@ -160,6 +160,39 @@ TEST_F(NotificationCoordinatorTest, ClearReadinessKeepsUsedMask) ASSERT_FALSE(coordinator.isUsed(1)); } +TEST_F(NotificationCoordinatorTest, StateChangeNotifyGatesCallback) +{ + // Part 1 (spec 3.5): a latched state-change notification (the DataLost deadline) opens the + // callback gate on its own, even with no events and no readiness, and is a one-shot. + NotificationCoordinator coordinator(manualExecutor(), loggerComponent); + coordinator.resize(2); + + // No events, no ready inputs -> gate closed + ASSERT_FALSE(coordinator.shouldInvokeCallback()); + + coordinator.setStateChangeNotify(true); + ASSERT_TRUE(coordinator.getStateChangeNotify()); + ASSERT_TRUE(coordinator.shouldInvokeCallback()); + + // Consuming the latch closes the gate again + coordinator.setStateChangeNotify(false); + ASSERT_FALSE(coordinator.getStateChangeNotify()); + ASSERT_FALSE(coordinator.shouldInvokeCallback()); +} + +TEST_F(NotificationCoordinatorTest, ClearReadinessLeavesStateChangeNotify) +{ + // clearReadiness drops ready/event bits (sync invalidated) but the state-change latch is a + // separate signal the owner consumes explicitly once the callback has fired. + NotificationCoordinator coordinator(manualExecutor(), loggerComponent); + coordinator.resize(2); + coordinator.setStateChangeNotify(true); + + coordinator.clearReadiness(); + ASSERT_TRUE(coordinator.getStateChangeNotify()); + ASSERT_TRUE(coordinator.shouldInvokeCallback()); +} + TEST_F(NotificationCoordinatorTest, ResizePreservesExistingBits) { NotificationCoordinator coordinator(manualExecutor(), loggerComponent); From 76b6781923f335296c0b2174ec26f0351f8806bc Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 13:27:36 +0200 Subject: [PATCH 08/15] Multi reader: wake the consumer on the whole InputsFailed family Extends the stateChangeNotify latch from DataLost to every InputsFailed transition (Incompatible / SynchronizationFailed / DataLost). Found while testing the Sum FB simplification: SumTest.ProbeDoesNotFlap stalled with no output. Re-probing a persistently incompatible input puts the reader into Incompatible, which - unlike an event or a ready block - fired no callback, because the failing descriptor was already cached (no new event) and no data is ready. With the FB's packet hook removed there was nothing to drive re-parking, so the healthy stream hung. Latching on the whole InputsFailed family makes the reader wake the consumer for a re-probe outcome the same way it does for a data-loss deadline. Edge-triggered on entry (or a change in affected inputs); consumed once observed, so a persistent failure does not busy-loop. Full reader suite (2080) and Sum FB suite (19) green. Co-Authored-By: Claude Opus 4.8 --- .../multi_reader/notification_coordinator.h | 14 ++++++----- .../multi_reader/notification_coordinator.cpp | 3 ++- core/opendaq/reader/src/multi_reader_impl.cpp | 25 ++++++++++++------- multi-reader-rework-docs/01_specification.md | 6 ++--- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h b/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h index ffb96d5601..8ace311275 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h @@ -47,9 +47,10 @@ namespace multi_reader * signal consumers react to with setInputUsed. The "ready" meaning is phase-dependent * (first sample while synchronizing, one full block while synchronized) - the owner * sets the bits during its state evaluation. stateChangeNotify is a one-shot latch for a - * state change that carries no returnable data or event (currently the transition into - * DataLost): the elapsed deadline is itself the notification, so the consumer is woken - * once and reads the naming status - it never has to poll or run its own liveness timer. + * state change that carries no returnable data or event - a transition into an InputsFailed + * state (Incompatible / SynchronizationFailed / DataLost) once the causing descriptors are + * cached, so no event fires and no data is ready. It wakes the consumer once to read the + * naming status, so the consumer never has to poll or run its own liveness timer. * * Threading contract: requestEvaluation() and detach() are thread-safe. Everything else * (masks, callback queries) must be called with the owner's state lock held. The @@ -98,9 +99,10 @@ class NotificationCoordinator void clearReadiness(); /// One-shot latch: raise the callback gate for a state change that carries no returnable - /// data or event (currently the transition into DataLost). Set by the owner on the - /// transition; the owner consumes it (sets false) once the callback has fired, so a single - /// occurrence wakes the consumer exactly once and does not re-fire while it persists. + /// data or event (a transition into an InputsFailed state - Incompatible / + /// SynchronizationFailed / DataLost). Set by the owner on the transition; the owner consumes + /// it (sets false) once the callback has fired, so a single occurrence wakes the consumer + /// exactly once and does not re-fire while it persists. void setStateChangeNotify(bool notify); bool getStateChangeNotify() const; diff --git a/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp b/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp index 837d71cf03..a5e2bd595b 100644 --- a/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp +++ b/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp @@ -170,7 +170,8 @@ bool NotificationCoordinator::shouldInvokeCallback() const // Events on unused inputs fire the callback too; that notification is the // recovery path (the consumer can re-include the input with setInputUsed). // stateChangeNotify covers a state change with neither data nor event to return - // (the DataLost deadline): the elapsed timer is itself the notification. + // (an InputsFailed transition: a data-loss deadline, or re-probing an input whose + // failing descriptor is already cached so no new event fires). return anyEvent() || allUsedReady() || stateChangeNotifyFlag; } diff --git a/core/opendaq/reader/src/multi_reader_impl.cpp b/core/opendaq/reader/src/multi_reader_impl.cpp index b80abe1252..a589fb7360 100644 --- a/core/opendaq/reader/src/multi_reader_impl.cpp +++ b/core/opendaq/reader/src/multi_reader_impl.cpp @@ -391,12 +391,19 @@ void MultiReaderImpl::applyDataLossTimeoutLocked() void MultiReaderImpl::setStateLocked(ReaderState newState, std::string message, std::vector affected) { - // Entering DataLost - or changing which inputs are lost while already in it - is a condition - // the consumer must act on that carries no returnable data or event. Latch a one-shot - // callback wake so the elapsed deadline itself notifies the consumer, which then reads the - // naming status (spec 3.5/3.6). Edge-triggered: an unchanged DataLost re-evaluation does not - // re-latch, so healthy-input packets cannot re-fire the callback while the loss persists. - if (newState == ReaderState::DataLost && (state != ReaderState::DataLost || affected != stateAffectedInputs)) + // Entering an InputsFailed-family state (Incompatible, SynchronizationFailed, DataLost) - or + // changing which inputs it affects while in one - is a condition the consumer must act on that + // carries no returnable data and, once the descriptors that caused it are cached, no + // returnable event either. The two cases that would otherwise wake nobody: a data-loss + // deadline (no packet at all), and re-probing a persistently incompatible / unsynchronizable + // input (its descriptor is already known, so no new event fires). Latch a one-shot callback + // wake so the reader itself notifies the consumer, which then reads the naming status + // (spec 3.5/3.6). Edge-triggered: an unchanged failure re-evaluation does not re-latch, so + // healthy-input packets cannot re-fire it while the failure persists. + const bool inputsFailed = newState == ReaderState::Incompatible || + newState == ReaderState::SynchronizationFailed || + newState == ReaderState::DataLost; + if (inputsFailed && (newState != state || affected != stateAffectedInputs)) notificationCoordinator->setStateChangeNotify(true); state = newState; @@ -1107,9 +1114,9 @@ void MultiReaderImpl::onCoalescedEvaluation() if (notificationCoordinator->shouldInvokeCallback()) callback = readCallback; - // One-shot: consume a latched state-change wake (DataLost) once observed, so a single - // loss fires the callback exactly once rather than on every later evaluation. A blocked - // read is woken independently by notifyCondition below. + // One-shot: consume a latched state-change wake (an InputsFailed transition) once + // observed, so a single failure fires the callback exactly once rather than on every + // later evaluation. A blocked read is woken independently by notifyCondition below. notificationCoordinator->setStateChangeNotify(false); } diff --git a/multi-reader-rework-docs/01_specification.md b/multi-reader-rework-docs/01_specification.md index ff822ac75e..41909edda9 100644 --- a/multi-reader-rework-docs/01_specification.md +++ b/multi-reader-rework-docs/01_specification.md @@ -144,11 +144,11 @@ schedule one coalesced task iff (eventMask & usedMask).any() || stateChangeNotify ``` -The scheduled task re-runs state evaluation and invokes the public `onDataAvailable` callback when an event is returnable, one full block is readable, **or** the reader has just recognized a condition the consumer must act on that carries neither returnable data nor a returnable event — currently the transition into `DataLost`. +The scheduled task re-runs state evaluation and invokes the public `onDataAvailable` callback when an event is returnable, one full block is readable, **or** the reader has just recognized a condition the consumer must act on that carries neither returnable data nor a returnable event. This last case is a transition into an `InputsFailed` state (`Incompatible` / `SynchronizationFailed` / `DataLost`): once the descriptors that caused the failure are cached, no new event fires, and no data is ready, so nothing else would wake the consumer. -Data-loss recognition **must** wake the consumer. The reader detects the loss on its own (scheduler-armed deadline; see §3.6) and the elapsed deadline is itself the notification: the consumer must not have to poll or run its own liveness timer to discover that an input has stalled. On the transition into `DataLost` the reader raises `onDataAvailable`, and the consumer reads the naming `DataLost` status on its next `read()`. This keeps the consumer implementation lean — a single `onDataAvailable` handler plus a `read()` is sufficient to observe data, events, and stalled inputs alike. +Failure recognition **must** wake the consumer. The reader detects it on its own — a data-loss deadline elapses (scheduler-armed; see §3.6), or a re-included input's cached descriptors are re-evaluated as incompatible / unsynchronizable — and the recognition is itself the notification: the consumer must not have to poll or run its own liveness timer to discover a stalled or failing input. On the transition into `InputsFailed` the reader raises `onDataAvailable`, and the consumer reads the naming status (with per-input states) on its next `read()`. This keeps the consumer implementation lean — a single `onDataAvailable` handler plus a `read()` is sufficient to observe data, events, and failing inputs alike, including re-probing a parked input to see whether it recovered. -`stateChangeNotify` is a latch set on entry to such a state and cleared once the consumer has been notified (the state is read or the reader leaves it), so a single loss wakes the consumer exactly once and does not busy-loop while the loss is outstanding. Callback is never invoked from `packetReceived` and never while any internal lock is held. The "ready" meaning is phase-dependent: first sample while synchronizing, one full block while synchronized. Blocked reads with a timeout are woken through the same path plus a condition variable. +`stateChangeNotify` is a latch set on entry to such a state (or when the set of affected inputs changes) and cleared once the consumer has been notified, so a single failure wakes the consumer exactly once and does not busy-loop while the failure is outstanding. Callback is never invoked from `packetReceived` and never while any internal lock is held. The "ready" meaning is phase-dependent: first sample while synchronizing, one full block while synchronized. Blocked reads with a timeout are woken through the same path plus a condition variable. `getAvailableCount` and the read methods process pending inputs synchronously — correctness never depends on the scheduler having run the coalesced task. From 702ed853a449ce52bfae887126ac5c24727f9463 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 14:58:48 +0200 Subject: [PATCH 09/15] Benchmark: report throughput in Hz + ns/sample, add maxrate scenario Throughput metrics now report absolute units - rate_Hz (common samples read per second) and ns_per_sample - instead of Msamp_s, across the inputs/packet/rates/stress/convert/events scenarios. Adds a 'maxrate' scenario measuring the maximum sustained multi-read rate with packet production excluded from the timed region (only read() is timed; the backlog is refilled outside the clock). Sweeps inputs {8,16,32} x packet {64,128,256}; the 16x128 point is the headline. Co-Authored-By: Claude Opus 4.8 --- .../reader/tests/bench_multi_reader.cpp | 68 +++++++++++++++++-- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/core/opendaq/reader/tests/bench_multi_reader.cpp b/core/opendaq/reader/tests/bench_multi_reader.cpp index 884f1e4742..354ce06bb8 100644 --- a/core/opendaq/reader/tests/bench_multi_reader.cpp +++ b/core/opendaq/reader/tests/bench_multi_reader.cpp @@ -6,6 +6,9 @@ * Output: CSV lines "scenario,param,metric,value" on stdout. Build the `bench_multi_reader` * target in Release and run with no arguments (runs all scenarios). * + * Throughput metrics are reported in absolute units: `rate_Hz` (common samples read per second) + * and `ns_per_sample` (nanoseconds of wall time per common sample). rate_Hz == 1e9 / ns_per_sample. + * * Scenarios: * inputs - throughput vs number of inputs (equal rate), incl. extreme 64/128 * packet - throughput vs packet size, incl. extreme-small 1/2/4/8 @@ -13,6 +16,8 @@ * events - throughput vs descriptor-event rate (resync every K packets) * event_inputs - event throughput vs input count (fixed event rate, many inputs) * stress - combined worst corner: many inputs x small packets + * maxrate - maximum sustained read rate (read timed, packet production excluded from the + * clock); grid of inputs x packet size, incl. the 16-input / 128-sample point * resync - time per resync when every packet batch forces a re-synchronization * convert - typed-read conversion throughput (native copy vs type conversion) */ @@ -167,11 +172,13 @@ struct Bench } }; -double megaSamplesPerSec(SizeT commonSamples, double seconds) +// Common samples read per second (Hz). This is the "multi-reading rate": how many aligned +// common-domain samples the reader delivers per second. +double rateHz(SizeT commonSamples, double seconds) { if (seconds <= 0.0) return 0.0; - return (static_cast(commonSamples) / seconds) / 1e6; + return static_cast(commonSamples) / seconds; } void emit(const char* scenario, const std::string& param, const char* metric, double value) @@ -195,8 +202,8 @@ void throughput(const char* scenario, const std::string& param, Bench& b, SizeT total += b.drain(); } const double secs = std::chrono::duration(Clock::now() - t0).count(); - emit(scenario, param, "Msamp_s", megaSamplesPerSec(total, secs)); - emit(scenario, param, "ns_per_common_sample", total ? (secs * 1e9 / total) : 0.0); + emit(scenario, param, "rate_Hz", rateHz(total, secs)); + emit(scenario, param, "ns_per_sample", total ? (secs * 1e9 / total) : 0.0); } void scenarioInputs() @@ -272,7 +279,7 @@ void scenarioEvents() total += b.drain(); } const double secs = std::chrono::duration(Clock::now() - t0).count(); - emit("events", "every_" + std::to_string(k), "Msamp_s", megaSamplesPerSec(total, secs)); + emit("events", "every_" + std::to_string(k), "rate_Hz", rateHz(total, secs)); } } @@ -300,7 +307,7 @@ void scenarioEventInputs() total += b.drain(); } const double secs = std::chrono::duration(Clock::now() - t0).count(); - emit("event_inputs", std::to_string(n), "Msamp_s", megaSamplesPerSec(total, secs)); + emit("event_inputs", std::to_string(n), "rate_Hz", rateHz(total, secs)); } } @@ -320,6 +327,54 @@ void scenarioStress() } } +// Maximum sustained multi-read rate. Only the read() call is timed; packet production is refilled +// OUTSIDE the clock (a real producer runs on its own thread), so this isolates the reader's own +// ceiling. Each read consumes exactly one packet-sized block, so the per-read orchestration cost +// is amortised over the packet - which is why the packet size is the key parameter here. +// Reported as rate_Hz (common samples/s) and ns_per_sample. The 16x128 row is the headline point. +void scenarioMaxRate() +{ + for (SizeT n : {8u, 16u, 32u}) + { + for (SizeT p : {64u, 128u, 256u}) + { + Bench b; + b.build(n, SampleType::Float64, {1}); + + // Warm up to synchronized steady state, then keep a backlog so reads never starve + for (int i = 0; i < 64; ++i) + b.sendAll(p); + b.reader.getAvailableCount(); // adopt + synchronize once + + std::vector> bufs(n, std::vector(p)); + std::vector ptrs(n); + for (SizeT i = 0; i < n; ++i) + ptrs[i] = bufs[i].data(); + + double readNs = 0.0; + SizeT samples = 0; + const SizeT reads = 20000; + for (SizeT r = 0; r < reads; ++r) + { + // Refill happens outside the timed region, so packet send never counts + if (b.reader.getAvailableCount() < p) + for (int i = 0; i < 64; ++i) + b.sendAll(p); + + SizeT cnt = p; + const auto r0 = Clock::now(); + b.reader.read(ptrs.data(), &cnt); + readNs += std::chrono::duration(Clock::now() - r0).count(); + samples += cnt; + } + + const std::string param = std::to_string(n) + "x" + std::to_string(p); + emit("maxrate", param, "ns_per_sample", samples ? readNs / samples : 0.0); + emit("maxrate", param, "rate_Hz", (samples && readNs > 0.0) ? (samples / (readNs / 1e9)) : 0.0); + } + } +} + void scenarioResync() { // Worst case: force a resync on EVERY batch (descriptor change on one input each time), @@ -503,6 +558,7 @@ int main(int argc, char** argv) if (only.empty() || only == "events") scenarioEvents(); if (only.empty() || only == "event_inputs") scenarioEventInputs(); if (only.empty() || only == "stress") scenarioStress(); + if (only.empty() || only == "maxrate") scenarioMaxRate(); if (only.empty() || only == "resync") scenarioResync(); if (only.empty() || only == "convert") scenarioConvert(); if (only.empty() || only == "micro") scenarioMicro(); From 5f958d4f5e643e87dff8ee18f0984566c2bf0e0d Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Wed, 22 Jul 2026 15:11:39 +0200 Subject: [PATCH 10/15] Reader: silence C4244 at the intentional sample-type conversions typed_reading_utils.cpp failed the Release build under newer MSVC (VS 2026): the value-copy (readData) and domain-compare (findDomainValue) loops convert between packet and read sample types by design, but MSVC emits C4244 on the converting constructor's argument when the read type is a class type (complex / range). The outer static_cast expresses intent yet cannot silence an inner argument conversion, so /WX turned it into C2220 and broke the build. Wrap both conversion sites in the same guarded #pragma warning(disable : 4244) the old typed_reader.cpp path already used. Compile-time only - the static_cast expressions are unchanged, so there is no runtime effect. opendaq Release now builds clean with OPENDAQ_RELEASE_WARNINGS_AS_ERRORS on. Co-Authored-By: Claude Opus 4.8 --- .../reader/src/typed_reading_utils.cpp | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/core/opendaq/reader/src/typed_reading_utils.cpp b/core/opendaq/reader/src/typed_reading_utils.cpp index 457de56a86..d821400be5 100644 --- a/core/opendaq/reader/src/typed_reading_utils.cpp +++ b/core/opendaq/reader/src/typed_reading_utils.cpp @@ -265,22 +265,33 @@ ErrCode readData(const ReadLayout& readLayout, return OPENDAQ_SUCCESS; } + // The typed reader converts between the packet and read sample types by design, so a + // narrowing conversion here is intended, not a defect. The static_cast expresses intent + // but cannot silence C4244 when OutputT is a class type (e.g. complex/range): the + // narrowing is on the converting constructor's argument, so suppress it at the site. +#if defined(_MSC_VER) +# pragma warning(push) +# pragma warning(disable : 4244) +#endif // If the type of samples is the same, then just copy if constexpr (std::is_same_v) { // Returns the pointer to the value after the last copied one - *outputBuffer = std::copy_n(dataStart, valuesPerSample * toRead, dataOut); // C4244 - possible data loss due to conversion + *outputBuffer = std::copy_n(dataStart, valuesPerSample * toRead, dataOut); } else { for (std::size_t i = 0; i < toRead * valuesPerSample; ++i) { - dataOut[i] = static_cast(dataStart[i]); // C4244 - possible data loss due to conversion + dataOut[i] = static_cast(dataStart[i]); } // Set the pointer to the value after the last copied one *outputBuffer = &dataOut[toRead]; } +#if defined(_MSC_VER) +# pragma warning(pop) +#endif return OPENDAQ_SUCCESS; } @@ -479,7 +490,16 @@ SizeT findDomainValue(const ReadLayout& readLayout, for (std::size_t i = 0; i < size * readLayout.valuesPerSample; ++i) { - OutputT value = static_cast(domainBuffer[i]); // C4244 - possible data loss due to conversion + // Intentional domain sample-type conversion; C4244 is on the converting + // constructor's argument, which the static_cast cannot silence (see readData). +#if defined(_MSC_VER) +# pragma warning(push) +# pragma warning(disable : 4244) +#endif + OutputT value = static_cast(domainBuffer[i]); +#if defined(_MSC_VER) +# pragma warning(pop) +#endif bool greaterEqual = false; if constexpr (IsTemplateOf::value) From 6f24939701a9bda759d65ced4fa6e30fbd4e7a18 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Fri, 24 Jul 2026 14:05:54 +0200 Subject: [PATCH 11/15] Reader: replace RatioPtr with plain TickResolution in DomainInfo DomainInfo::resolution is now a plain two-Int struct (num/den) instead of a refcounted RatioPtr, so the hot domain-value arithmetic (epochOffsetTicks, tickMultiplier, to/fromDomain, toAbsoluteTime) carries no object references. {0, 0} is the unassigned sentinel; a converting constructor from RatioPtr keeps descriptor ingestion and test literals unchanged. toSysTime now has a two-integer overload (resolutionNum/resolutionDen); the RatioPtr overload delegates to it, and all DomainInfo-driven call sites (domain_value.h, typed_reading_utils.cpp) use the integer form. The RatioPtr type remains only at the public API boundaries: descriptor ingestion converts once, and getTickResolution / the common domain descriptor construct a Ratio from the plain values. Tests: mechanical accessor updates (getNumerator/getDenominator -> num/den) and the null-resolution literal becomes TickResolution{}. Full reader suite green (2080). Co-Authored-By: Claude Opus 4.8 --- .../reader/include/opendaq/domain_value.h | 77 +++++++++++++------ .../multi_reader/synchronization_manager.h | 5 +- .../reader/include/opendaq/reader_utils.h | 16 ++-- .../reader/src/multi_reader/queue_reader.cpp | 11 ++- .../multi_reader/synchronization_manager.cpp | 5 +- core/opendaq/reader/src/multi_reader_impl.cpp | 6 +- .../reader/src/typed_reading_utils.cpp | 9 ++- .../reader/tests/test_domain_value.cpp | 4 +- .../tests/test_synchronization_manager.cpp | 12 +-- 9 files changed, 94 insertions(+), 51 deletions(-) diff --git a/core/opendaq/reader/include/opendaq/domain_value.h b/core/opendaq/reader/include/opendaq/domain_value.h index 064811472d..e2fcc0f4a4 100644 --- a/core/opendaq/reader/include/opendaq/domain_value.h +++ b/core/opendaq/reader/include/opendaq/domain_value.h @@ -25,10 +25,46 @@ BEGIN_NAMESPACE_OPENDAQ +/// Plain-value tick resolution (num/den); replaces RatioPtr in the domain-value arithmetic so +/// the hot conversion paths carry no refcounted object. {0, 0} means "not assigned". +struct TickResolution +{ + Int num = 0; + Int den = 0; + + TickResolution() = default; + + TickResolution(Int num, Int den) + : num(num) + , den(den) + { + } + + // Convenience conversion from the descriptor's RatioPtr; an unassigned ratio yields {0, 0}. + TickResolution(const RatioPtr& ratio) + { + if (ratio.assigned()) + { + num = ratio.getNumerator(); + den = ratio.getDenominator(); + } + } + + friend bool operator==(const TickResolution& lhs, const TickResolution& rhs) + { + return lhs.num == rhs.num && lhs.den == rhs.den; + } + + friend bool operator!=(const TickResolution& lhs, const TickResolution& rhs) + { + return !(lhs == rhs); + } +}; + struct DomainInfo { std::chrono::system_clock::time_point epoch; - RatioPtr resolution; + TickResolution resolution; static DomainInfo fromDescriptor(const DataDescriptorPtr& descriptor) { @@ -43,16 +79,11 @@ struct DomainInfo friend bool operator==(const DomainInfo& lhs, const DomainInfo& rhs) { - if (!lhs.resolution.assigned() || !rhs.resolution.assigned()) + // {0, 0} is the unassigned sentinel (see TickResolution) + if (lhs.resolution == TickResolution{} || rhs.resolution == TickResolution{}) DAQ_THROW_EXCEPTION(InvalidParameterException, "DomainInfo::resolution must be assigned."); - if (!(lhs.epoch == rhs.epoch)) - return false; - if (!(lhs.resolution.getNumerator() == rhs.resolution.getNumerator())) - return false; - if (!(lhs.resolution.getDenominator() == rhs.resolution.getDenominator())) - return false; - return true; + return lhs.epoch == rhs.epoch && lhs.resolution == rhs.resolution; } friend bool operator!=(const DomainInfo& lhs, const DomainInfo& rhs) @@ -64,8 +95,8 @@ struct DomainInfo inline std::ostream& operator<<(std::ostream& os, const DomainInfo& info) { os << "DomainInfo{" - << "epoch=" << info.epoch.time_since_epoch().count() << ", resolution=" << info.resolution.getNumerator() << "/" - << info.resolution.getDenominator() << "}"; + << "epoch=" << info.epoch.time_since_epoch().count() << ", resolution=" << info.resolution.num << "/" + << info.resolution.den << "}"; return os; } @@ -76,12 +107,12 @@ namespace domain_conversion /// zero (the sub-tick remainder of an epoch is not representable on the tick grid). inline Int epochOffsetTicks(const std::chrono::system_clock::time_point& from, const std::chrono::system_clock::time_point& to, - const RatioPtr& resolution) + const TickResolution& resolution) { using SysPeriod = std::chrono::system_clock::period; const Int epochDiff = from.time_since_epoch().count() - to.time_since_epoch().count(); - const Int scaleNumerator = SysPeriod::num * resolution.getDenominator(); - const Int scaleDenominator = SysPeriod::den * resolution.getNumerator(); + const Int scaleNumerator = SysPeriod::num * resolution.den; + const Int scaleDenominator = SysPeriod::den * resolution.num; return epochDiff * scaleNumerator / scaleDenominator; } @@ -92,10 +123,10 @@ namespace domain_conversion Int denominator; }; - inline TickMultiplier tickMultiplier(const RatioPtr& sourceResolution, const RatioPtr& targetResolution) + inline TickMultiplier tickMultiplier(const TickResolution& sourceResolution, const TickResolution& targetResolution) { - return {sourceResolution.getNumerator() * targetResolution.getDenominator(), - sourceResolution.getDenominator() * targetResolution.getNumerator()}; + return {sourceResolution.num * targetResolution.den, + sourceResolution.den * targetResolution.num}; } } // namespace domain_conversion @@ -231,8 +262,8 @@ class DomainValueImpl : public DomainValue void roundUpOnDomainInterval(const RatioPtr& interval) override { - auto num = domain.resolution.getNumerator() * interval.getDenominator(); - auto den = domain.resolution.getDenominator() * interval.getNumerator(); + auto num = domain.resolution.num * interval.getDenominator(); + auto den = domain.resolution.den * interval.getNumerator(); const Int gcd = std::gcd(num, den); num /= gcd; @@ -251,7 +282,7 @@ class DomainValueImpl : public DomainValue std::chrono::system_clock::time_point toAbsoluteTime() const override { - return reader::toSysTime(value, domain.epoch, domain.resolution); + return reader::toSysTime(value, domain.epoch, domain.resolution.num, domain.resolution.den); } Type getValue() const @@ -265,7 +296,7 @@ class DomainValueImpl : public DomainValue using namespace reader; std::stringstream ss; - ss << toSysTime(value, domain.epoch, domain.resolution); + ss << toSysTime(value, domain.epoch, domain.resolution.num, domain.resolution.den); return ss.str(); } @@ -352,7 +383,7 @@ class DomainValueImpl final : public DomainValue std::chrono::system_clock::time_point toAbsoluteTime() const override { - return reader::toSysTime(value.start, domain.epoch, domain.resolution); + return reader::toSysTime(value.start, domain.epoch, domain.resolution.num, domain.resolution.den); } RangeType64 getValue() const @@ -366,7 +397,7 @@ class DomainValueImpl final : public DomainValue using namespace reader; std::stringstream ss; - ss << toSysTime(value.start, domain.epoch, domain.resolution); + ss << toSysTime(value.start, domain.epoch, domain.resolution.num, domain.resolution.den); return ss.str(); } diff --git a/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h b/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h index db0dbe84ee..38e59cbac1 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h @@ -51,9 +51,10 @@ struct CommonModel /// common resolution folds in 1/commonSampleRate. Zero when the model is not usable. std::int64_t ticksPerCommonSample() const { - if (!commonDomain.resolution.assigned() || commonSampleRate <= 0) + // {0, 0} is TickResolution's unassigned sentinel + if (commonDomain.resolution.den == 0 || commonSampleRate <= 0) return 0; - return commonDomain.resolution.getDenominator() / (commonDomain.resolution.getNumerator() * commonSampleRate); + return commonDomain.resolution.den / (commonDomain.resolution.num * commonSampleRate); } }; diff --git a/core/opendaq/reader/include/opendaq/reader_utils.h b/core/opendaq/reader/include/opendaq/reader_utils.h index 99e2d244bb..5924cb123b 100644 --- a/core/opendaq/reader/include/opendaq/reader_utils.h +++ b/core/opendaq/reader/include/opendaq/reader_utils.h @@ -171,12 +171,12 @@ namespace reader struct SysTime { template - static auto ToSysTime(T value, std::chrono::system_clock::time_point epoch, const RatioPtr& resolution) + static auto ToSysTime(T value, std::chrono::system_clock::time_point epoch, Int resolutionNum, Int resolutionDen) { using namespace std::chrono; using Seconds = duration; - auto offset = Seconds((resolution.getNumerator() * value) / static_cast(resolution.getDenominator())); + auto offset = Seconds((resolutionNum * value) / static_cast(resolutionDen)); return round(epoch + offset); } }; @@ -185,7 +185,7 @@ namespace reader struct SysTime::value>> { template - static auto ToSysTime(T /*value*/, std::chrono::system_clock::time_point /*epoch*/, const RatioPtr& /*resolution*/) + static auto ToSysTime(T /*value*/, std::chrono::system_clock::time_point /*epoch*/, Int /*resolutionNum*/, Int /*resolutionDen*/) { return std::chrono::system_clock::time_point{}; } @@ -195,17 +195,23 @@ namespace reader struct SysTime::value>> { template - static auto ToSysTime(T /*value*/, std::chrono::system_clock::time_point /*epoch*/, const RatioPtr& /*resolution*/) + static auto ToSysTime(T /*value*/, std::chrono::system_clock::time_point /*epoch*/, Int /*resolutionNum*/, Int /*resolutionDen*/) { return std::chrono::system_clock::time_point{}; } }; } + template + auto toSysTime(T value, std::chrono::system_clock::time_point epoch, Int resolutionNum, Int resolutionDen) + { + return detail::SysTime:: template ToSysTime(value, epoch, resolutionNum, resolutionDen); + } + template auto toSysTime(T value, std::chrono::system_clock::time_point epoch, const RatioPtr& resolution) { - return detail::SysTime:: template ToSysTime(value, epoch, resolution); + return toSysTime(value, epoch, resolution.getNumerator(), resolution.getDenominator()); } inline std::int64_t getSampleRate(const DataDescriptorPtr& dataDescriptor) diff --git a/core/opendaq/reader/src/multi_reader/queue_reader.cpp b/core/opendaq/reader/src/multi_reader/queue_reader.cpp index da4212be62..1c4779cb2c 100644 --- a/core/opendaq/reader/src/multi_reader/queue_reader.cpp +++ b/core/opendaq/reader/src/multi_reader/queue_reader.cpp @@ -803,7 +803,7 @@ void QueueReader::parseDomainDescriptor() // END Type Conversion // Resolution and origin - auto newResolution = descriptor.getTickResolution(); + const TickResolution newResolution(descriptor.getTickResolution()); if (typeCtx.domainInfo.resolution != newResolution) { typeCtx.domainInfo.resolution = newResolution; @@ -838,16 +838,15 @@ void QueueReader::parseDomainDescriptor() } const bool resolutionValid = - typeCtx.domainInfo.resolution.assigned() && - typeCtx.domainInfo.resolution.getNumerator() > 0 && - typeCtx.domainInfo.resolution.getDenominator() > 0; + typeCtx.domainInfo.resolution.num > 0 && + typeCtx.domainInfo.resolution.den > 0; const bool deltaPositive = delta.getFloatValue() > 0.0; double sr = 0.0; if (resolutionValid && deltaPositive) { - sr = static_cast(typeCtx.domainInfo.resolution.getDenominator()) / - (static_cast(typeCtx.domainInfo.resolution.getNumerator()) * delta.getFloatValue()); + sr = static_cast(typeCtx.domainInfo.resolution.den) / + (static_cast(typeCtx.domainInfo.resolution.num) * delta.getFloatValue()); } const bool deltaIsInteger = (delta.getFloatValue() == static_cast(delta.getIntValue())); diff --git a/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp b/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp index 86a2ee777a..1ef25c17f3 100644 --- a/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp +++ b/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp @@ -154,7 +154,8 @@ SyncSetupResult SynchronizationManager::buildCommonModelImpl(const std::vector missing; for (SizeT i = 0; i < count; ++i) { - if (!inputs[i]->getDomainDescriptor().assigned() || !inputs[i]->getDomainInfo().resolution.assigned()) + // resolution {0, 0} is TickResolution's unassigned sentinel + if (!inputs[i]->getDomainDescriptor().assigned() || inputs[i]->getDomainInfo().resolution.den == 0) missing.push_back(slotIndices[i]); } if (!missing.empty()) @@ -241,7 +242,7 @@ SyncSetupResult SynchronizationManager::buildCommonModelImpl(const std::vectorgetDomainInfo(); commonEpoch = std::min(commonEpoch, domainInfo.epoch); - resolutions.push_back(domainInfo.resolution); + resolutions.push_back(Ratio(domainInfo.resolution.num, domainInfo.resolution.den)); } resolutions.push_back(Ratio(1, commonRate)); diff --git a/core/opendaq/reader/src/multi_reader_impl.cpp b/core/opendaq/reader/src/multi_reader_impl.cpp index a589fb7360..bd078c4b98 100644 --- a/core/opendaq/reader/src/multi_reader_impl.cpp +++ b/core/opendaq/reader/src/multi_reader_impl.cpp @@ -1230,7 +1230,7 @@ EventPacketPtr MultiReaderImpl::mainDescriptorPacketLocked() { cachedCommonDomainDescriptor = DataDescriptorBuilderCopy(mainDomainDescriptor) .setOrigin(reader::isoEpochString(model.commonDomain.epoch)) - .setTickResolution(model.commonDomain.resolution) + .setTickResolution(Ratio(model.commonDomain.resolution.num, model.commonDomain.resolution.den)) .setRule(LinearDataRule(static_cast(model.ticksPerCommonSample()), 0)) .build(); } @@ -1843,7 +1843,9 @@ ErrCode MultiReaderImpl::getTickResolution(IRatio** resolution) return OPENDAQ_IGNORED; } - *resolution = syncManager->getModel().commonDomain.resolution.addRefAndReturn(); + // The public API reports the resolution as a Ratio object; the model stores plain values + const auto& res = syncManager->getModel().commonDomain.resolution; + *resolution = Ratio(res.num, res.den).detach(); return OPENDAQ_SUCCESS; } diff --git a/core/opendaq/reader/src/typed_reading_utils.cpp b/core/opendaq/reader/src/typed_reading_utils.cpp index d821400be5..f4a549f185 100644 --- a/core/opendaq/reader/src/typed_reading_utils.cpp +++ b/core/opendaq/reader/src/typed_reading_utils.cpp @@ -456,7 +456,8 @@ SizeT findDomainValueLinear(const DataPacketPtr& domainPacket, { // Tick corresponding to index in signal's resolution ticks. OutputT tick = startTick + static_cast(index) * ruleDelta; - auto readValueSysTime = reader::toSysTime(tick, target->getDomain().epoch, target->getDomain().resolution); + auto readValueSysTime = + reader::toSysTime(tick, target->getDomain().epoch, target->getDomain().resolution.num, target->getDomain().resolution.den); *absoluteTimestamp = readValueSysTime.time_since_epoch().count(); } } @@ -508,7 +509,8 @@ SizeT findDomainValue(const ReadLayout& readLayout, { if (absoluteTimestamp) { - auto readValueSysTime = reader::toSysTime(value.start, target->getDomain().epoch, target->getDomain().resolution); + auto readValueSysTime = reader::toSysTime( + value.start, target->getDomain().epoch, target->getDomain().resolution.num, target->getDomain().resolution.den); *absoluteTimestamp = readValueSysTime.time_since_epoch().count(); } greaterEqual = true; @@ -520,7 +522,8 @@ SizeT findDomainValue(const ReadLayout& readLayout, { if (absoluteTimestamp) { - auto readValueSysTime = reader::toSysTime(value, target->getDomain().epoch, target->getDomain().resolution); + auto readValueSysTime = reader::toSysTime( + value, target->getDomain().epoch, target->getDomain().resolution.num, target->getDomain().resolution.den); *absoluteTimestamp = readValueSysTime.time_since_epoch().count(); } greaterEqual = true; diff --git a/core/opendaq/reader/tests/test_domain_value.cpp b/core/opendaq/reader/tests/test_domain_value.cpp index 761c87abc6..d1172a777f 100644 --- a/core/opendaq/reader/tests/test_domain_value.cpp +++ b/core/opendaq/reader/tests/test_domain_value.cpp @@ -31,8 +31,8 @@ TEST_F(DomainValueTest, DomainInfoComparison) daq::DomainInfo info6 = {daq::reader::parseEpoch("1999-01-01T00:00:00+00:00"), daq::Ratio(5, 2000)}; ASSERT_FALSE(info6 == info1); - // Unassigned resolution - daq::DomainInfo info7 = {daq::reader::parseEpoch("1999-01-01T00:00:00+00:00"), nullptr}; + // Unassigned resolution ({0, 0} sentinel, also what a null RatioPtr converts to) + daq::DomainInfo info7 = {daq::reader::parseEpoch("1999-01-01T00:00:00+00:00"), daq::TickResolution{}}; ASSERT_THROW((void) (info7 == info1), daq::InvalidParameterException); } diff --git a/core/opendaq/reader/tests/test_synchronization_manager.cpp b/core/opendaq/reader/tests/test_synchronization_manager.cpp index c7fa319661..c63aea3c0a 100644 --- a/core/opendaq/reader/tests/test_synchronization_manager.cpp +++ b/core/opendaq/reader/tests/test_synchronization_manager.cpp @@ -168,8 +168,8 @@ TEST_F(SyncManagerTest, ModelEqualRates) ASSERT_EQ(model.commonSampleRate, 1000); ASSERT_EQ(model.sampleRateDividers, (std::vector{1, 1})); ASSERT_EQ(model.blockLcm, 1u); - ASSERT_EQ(model.commonDomain.resolution.getNumerator(), 1); - ASSERT_EQ(model.commonDomain.resolution.getDenominator(), 1000); + ASSERT_EQ(model.commonDomain.resolution.num, 1); + ASSERT_EQ(model.commonDomain.resolution.den, 1000); ASSERT_EQ(inputs[0]->reader->getSampleRateDivider(), 1u); ASSERT_EQ(inputs[1]->reader->getSampleRateDivider(), 1u); } @@ -187,8 +187,8 @@ TEST_F(SyncManagerTest, ModelMixedResolutions) ASSERT_EQ(model.commonSampleRate, 30); ASSERT_EQ(model.sampleRateDividers, (std::vector{3, 2})); ASSERT_EQ(model.blockLcm, 6u); - ASSERT_EQ(model.commonDomain.resolution.getNumerator(), 1); - ASSERT_EQ(model.commonDomain.resolution.getDenominator(), 30); + ASSERT_EQ(model.commonDomain.resolution.num, 1); + ASSERT_EQ(model.commonDomain.resolution.den, 30); } TEST_F(SyncManagerTest, ModelDeltaBasedDividers) @@ -232,8 +232,8 @@ TEST_F(SyncManagerTest, ModelRequiredRateRefinesResolution) ASSERT_EQ(model.commonSampleRate, 2000); ASSERT_EQ(model.sampleRateDividers, (std::vector{2, 4})); ASSERT_EQ(model.blockLcm, 4u); - ASSERT_EQ(model.commonDomain.resolution.getNumerator(), 1); - ASSERT_EQ(model.commonDomain.resolution.getDenominator(), 2000); + ASSERT_EQ(model.commonDomain.resolution.num, 1); + ASSERT_EQ(model.commonDomain.resolution.den, 2000); } TEST_F(SyncManagerTest, ModelRequiredRateNotDivisible) From 34828a46a262c5db660d2bf87c3ecb9b1afbf7d3 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Fri, 24 Jul 2026 14:35:47 +0200 Subject: [PATCH 12/15] Reader: SignalEvent carries descriptor changes as tri-state descriptors SignalEvent no longer tracks separate changed flags; the descriptor pointers themselves encode the change: unassigned = unchanged (parameter absent), the explicit NullDataDescriptor marker (sample type Null) = descriptor unset, anything else = changed to it. The new unpackDataDescriptorEventPacket helper returns this raw encoding, and toEventPacket round-trips it unchanged. QueueReader gains ValueDescriptorNull / DomainDescriptorNull issues: the descriptor parses flag them first and return immediately when the cached descriptor is missing or explicitly unset, so a change-to-null makes the reader invalid (Incompatible) instead of silently clearing state, and a Null descriptor never reaches DomainInfo::fromDescriptor. The issues are raised from construction - a virgin reader without descriptors reads as invalid instead of valid-while-empty; the state ladder still reports it as WaitingForDescriptors (step 6 precedes the validity check). NoChange events (a descriptor-changed packet with both parameters absent) are no longer enqueued as pending events - they carry no information and previously surfaced as a blocking no-op event when the queue was empty. Full reader suite (2080) and Sum FB suite (19) green. Co-Authored-By: Claude Opus 4.8 --- .../opendaq/multi_reader/queue_reader.h | 8 ++- .../reader/src/multi_reader/queue_reader.cpp | 51 +++++++++++-------- .../include/opendaq/event_packet_utils.h | 15 ++++++ 3 files changed, 47 insertions(+), 27 deletions(-) diff --git a/core/opendaq/reader/include/opendaq/multi_reader/queue_reader.h b/core/opendaq/reader/include/opendaq/multi_reader/queue_reader.h index ff06d2a54f..2c1b3d4cb7 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/queue_reader.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/queue_reader.h @@ -61,10 +61,6 @@ class SignalEvent SignalEventType eventType; DataDescriptorPtr domainDescriptor; DataDescriptorPtr valueDescriptor; - // A change may carry a null descriptor (descriptor removed); explicit flags keep - // "changed to null" distinguishable from "unchanged". - bool domainDescriptorChanged = false; - bool valueDescriptorChanged = false; Int gapDiff; }; @@ -97,7 +93,9 @@ enum class QueueReaderIssue : uint32_t UnsupportedDomainRule = 1 << 2, OriginParsingFailed = 1 << 3, DomainUnitInvalid = 1 << 4, - DomainNotScalar = 1 << 5 // domain descriptor has dimensions - a vector timestamp has no meaning + DomainNotScalar = 1 << 5, // domain descriptor has dimensions - a vector timestamp has no meaning + ValueDescriptorNull = 1 << 6, // descriptor explicitly unset (the NullDataDescriptor marker, sample type Null) + DomainDescriptorNull = 1 << 7 }; class QueueReader diff --git a/core/opendaq/reader/src/multi_reader/queue_reader.cpp b/core/opendaq/reader/src/multi_reader/queue_reader.cpp index 1c4779cb2c..6adf4e127b 100644 --- a/core/opendaq/reader/src/multi_reader/queue_reader.cpp +++ b/core/opendaq/reader/src/multi_reader/queue_reader.cpp @@ -25,11 +25,11 @@ SignalEvent::SignalEvent(const EventPacketPtr& packet) } else { - // The parse distinguishes "changed to null" (explicit null marker) from "unchanged" - // (parameter absent) - a removed descriptor must not be mistaken for no change. - const auto [valueDescChanged, domainDescChanged, newValueDescriptor, newDomainDescriptor] = parseDataDescriptorEventPacket(packet); - valueDescriptorChanged = valueDescChanged; - domainDescriptorChanged = domainDescChanged; + // Tri-state per descriptor, carried by the descriptor itself: unassigned = unchanged + // (parameter absent), the explicit NullDataDescriptor marker (sample type Null) = + // descriptor unset, anything else = changed to it. A removed descriptor is therefore + // never mistaken for no change without needing separate flags. + const auto [newValueDescriptor, newDomainDescriptor] = unpackDataDescriptorEventPacket(packet); domainDescriptor = newDomainDescriptor; valueDescriptor = newValueDescriptor; updateType(); @@ -41,15 +41,16 @@ void SignalEvent::updateType() if (eventType == SignalEventType::Gap) return; - if (domainDescriptorChanged && valueDescriptorChanged) + // Assigned = changed (the NullDataDescriptor "unset" marker counts as a change) + if (domainDescriptor.assigned() && valueDescriptor.assigned()) { eventType = SignalEventType::DomainAndValueChanged; } - else if (domainDescriptorChanged) + else if (domainDescriptor.assigned()) { eventType = SignalEventType::DomainChanged; } - else if (valueDescriptorChanged) + else if (valueDescriptor.assigned()) { eventType = SignalEventType::ValueChanged; } @@ -65,16 +66,11 @@ bool SignalEvent::merge(const SignalEvent& other) if (this->eventType == SignalEventType::Gap || other.eventType == SignalEventType::Gap) return false; - if (other.domainDescriptorChanged) - { - domainDescriptorChanged = true; + // Newest change wins per descriptor; an unassigned (unchanged) side never overwrites + if (other.domainDescriptor.assigned()) domainDescriptor = other.domainDescriptor; - } - if (other.valueDescriptorChanged) - { - valueDescriptorChanged = true; + if (other.valueDescriptor.assigned()) valueDescriptor = other.valueDescriptor; - } updateType(); return true; } @@ -104,9 +100,7 @@ EventPacketPtr SignalEvent::toEventPacket() const { // Unchanged descriptors stay absent (null parameter); a changed descriptor uses the // explicit null marker when removed, so consumers can tell "removed" from "unchanged". - return DataDescriptorChangedEventPacket( - valueDescriptorChanged ? descriptorToEventPacketParam(valueDescriptor) : nullptr, - domainDescriptorChanged ? descriptorToEventPacketParam(domainDescriptor) : nullptr); + return DataDescriptorChangedEventPacket(valueDescriptor, domainDescriptor); } } @@ -127,6 +121,10 @@ QueueReader::QueueReader(const InputPortConfigPtr& port, typeCtx.valueIn = SampleType::Undefined; typeCtx.valueOut = mode == ReadMode::RawValue ? SampleType::Undefined : valueReadType; refreshConnectionInternal(); + + // Start with issues set - without descriptors the queue reader cannot be valid. + parseDomainDescriptor(); + parseValueDescriptor(); } void QueueReader::refreshConnectionInternal() @@ -752,7 +750,8 @@ SignalEventType QueueReader::addEncounteredEvent(const EventPacketPtr& packet) break; } parseCachedDescriptors(); - addToEventQueue(std::move(event)); + if (event.getType() != SignalEventType::NoChange) + addToEventQueue(std::move(event)); return eventType; } @@ -771,7 +770,11 @@ void QueueReader::addToEventQueue(SignalEvent&& event) void QueueReader::parseDomainDescriptor() { auto& descriptor = typeCtx.domainLayout.descriptor; - if (!descriptor.assigned()) + + // Either unset (NullDescriptor) or not assigned is an issue for the queue reader. + const bool descriptorNull = !descriptor.assigned() || descriptor.getSampleType() == SampleType::Null; + issues.set(QueueReaderIssue::DomainDescriptorNull, descriptorNull); + if (descriptorNull) return; // Type conversion @@ -903,7 +906,11 @@ void QueueReader::parseDomainDescriptor() void QueueReader::parseValueDescriptor() { auto& descriptor = typeCtx.valueLayout.descriptor; - if (!descriptor.assigned()) + + // See parseDomainDescriptor: the NullDataDescriptor "unset" marker is flagged, not parsed, same for unassigned + const bool descriptorNull = !descriptor.assigned() || descriptor.getSampleType() == SampleType::Null; + issues.set(QueueReaderIssue::ValueDescriptorNull, descriptorNull); + if (descriptorNull) return; auto postScaling = descriptor.getPostScaling(); diff --git a/core/opendaq/signal/include/opendaq/event_packet_utils.h b/core/opendaq/signal/include/opendaq/event_packet_utils.h index 5c1b07725d..226cbf21e2 100644 --- a/core/opendaq/signal/include/opendaq/event_packet_utils.h +++ b/core/opendaq/signal/include/opendaq/event_packet_utils.h @@ -41,6 +41,21 @@ inline std::tuple parseDataDes return std::make_tuple(valueDescriptorChanged, domainDescriptorChanged, newValueDescriptor, newDomainDescriptor); } +inline std::tuple unpackDataDescriptorEventPacket(const EventPacketPtr& eventPacket) +{ + if (!eventPacket.assigned()) + DAQ_THROW_EXCEPTION(ArgumentNullException, "Event packet not assigned"); + + if (!(eventPacket.getEventId() == event_packet_id::DATA_DESCRIPTOR_CHANGED)) + DAQ_THROW_EXCEPTION(InvalidParameterException, R"(Invalid event packet id: {})", eventPacket.getEventId()); + + const auto params = eventPacket.getParameters(); + const DataDescriptorPtr valueDescriptorParam = params[event_packet_param::DATA_DESCRIPTOR]; + const DataDescriptorPtr domainDescriptorParam = params[event_packet_param::DOMAIN_DATA_DESCRIPTOR]; + + return std::make_tuple(valueDescriptorParam, domainDescriptorParam); +} + inline DataDescriptorPtr descriptorToEventPacketParam(const DataDescriptorPtr& dataDescriptor) { return dataDescriptor.assigned() ? dataDescriptor : NullDataDescriptor(); From 3b06577d6fc3f99f9475bb5a54f0249bb88306f7 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Fri, 24 Jul 2026 17:57:58 +0200 Subject: [PATCH 13/15] Multi reader: lock-free callback gate on the producer path Replace the NotificationCoordinator bit masks with a shared, lock-free CallbackGate. The producer path (Input::packetReceived) now updates its gate flags from an O(1) connection introspection and schedules a coalesced evaluation only when the callback gate is open (or when it must force one: non-steady state, or an untrusted snapshot). In the steady synchronized state a data packet that does not complete a readable block for every input costs one atomic flag update and no scheduler round-trip. - callback_gate.h: CallbackGate (used/ready/event counters, one-shot state-change latch, owner-pass epoch) + SlotGateFlags (per-slot packed atomic word). Producers raise only the ready flag; events force an evaluation and are set exclusively by the owner under the state lock. - NotificationCoordinator: drops the mask API, keeps coalesced scheduling, exposes the shared gate and the PassGuard epoch. - MultiReaderImpl: owner-side gate helpers; publishProducerGateLocked is the single funnel every evaluateStateLocked exit passes through; every sample-moving section is bracketed by a PassGuard; slot removal disarms the slot's gate contribution atomically. - ReadCoordinator::effectiveMinimum made public: the gate ready threshold now matches the availability/discard minimum exactly. A producer raise is advisory - it can cost at most one spurious evaluation (reconciled before any user callback fires) and can never cause or suppress an onDataAvailable. Tests: add test_callback_gate.cpp, producer-gate tests in test_multi_reader_input.cpp, rewrite test_notification_coordinator.cpp for the new API. Full reader suite passes (2088 tests). Also fixes three pre-existing warning-as-error issues in unrelated reader test files (uninitialized/unused locals) that block the test target from building under the stricter toolchain once a new test source forces a full recompile. Co-Authored-By: Claude Fable 5 --- core/opendaq/reader/docs/multi_reader.md | 50 +++- .../opendaq/multi_reader/callback_gate.h | 234 ++++++++++++++++ .../include/opendaq/multi_reader/input.h | 75 ++++- .../multi_reader/notification_coordinator.h | 90 +++--- .../opendaq/multi_reader/read_coordinator.h | 7 +- .../include/opendaq/multi_reader_impl.h | 43 ++- core/opendaq/reader/src/CMakeLists.txt | 2 + .../opendaq/reader/src/multi_reader/input.cpp | 88 +++++- .../multi_reader/notification_coordinator.cpp | 129 ++------- core/opendaq/reader/src/multi_reader_impl.cpp | 264 ++++++++++++++---- core/opendaq/reader/tests/CMakeLists.txt | 1 + .../reader/tests/test_callback_gate.cpp | 148 ++++++++++ .../reader/tests/test_domain_value.cpp | 2 +- .../reader/tests/test_multi_reader_input.cpp | 120 +++++++- .../tests/test_notification_coordinator.cpp | 128 ++------- .../reader/tests/test_queue_reader.cpp | 8 +- .../reader/tests/test_typed_reading.cpp | 2 +- 17 files changed, 1028 insertions(+), 363 deletions(-) create mode 100644 core/opendaq/reader/include/opendaq/multi_reader/callback_gate.h create mode 100644 core/opendaq/reader/tests/test_callback_gate.cpp diff --git a/core/opendaq/reader/docs/multi_reader.md b/core/opendaq/reader/docs/multi_reader.md index fe46a7c540..90ff6bb657 100644 --- a/core/opendaq/reader/docs/multi_reader.md +++ b/core/opendaq/reader/docs/multi_reader.md @@ -143,7 +143,8 @@ The facade owns one instance of each component and is the only thing that holds | **`QueueReader`** | `multi_reader/queue_reader.*` | Per-input queue over one connection: adopts packets (`drain`), tracks value/domain descriptors and events, computes available samples, and executes the actual value/domain copy on `read`/`skip`. | | **`SynchronizationManager`** | `multi_reader/synchronization_manager.*` | All cross-input math: builds the `CommonModel` (common sample rate, per-input dividers, `blockLcm`, tick resolution) and aligns every input to a common start tick. | | **`ReadCoordinator`** | `multi_reader/read_coordinator.*` | Availability, planning, and committing a read/skip under one set of alignment rules. Produces a `ReadPlan` and executes it across all inputs; a partial commit is impossible by construction. | -| **`NotificationCoordinator`** | `multi_reader/notification_coordinator.*` | Coalesces producer wake-ups into at most one scheduled evaluation, and holds the used/ready/event bit masks that decide whether `onDataAvailable` fires. | +| **`NotificationCoordinator`** | `multi_reader/notification_coordinator.*` | Coalesces producer wake-ups into at most one scheduled evaluation, and owns the shared `CallbackGate`. | +| **`CallbackGate` / `SlotGateFlags`** | `multi_reader/callback_gate.h` | Lock-free gate state shared between the owner and its producer-side slots: per-slot ready/event flags (packed atomic words) feeding shared counters, an owner-maintained used count, a one-shot state-change latch, and the owner-pass epoch. Producers query `isSatisfied()` and raise ready flags directly on the producer thread. | | **`DataLossMonitor`** | `multi_reader/data_loss_monitor.*` | Per-input packet deadlines. Its timer thread requests an evaluation when a deadline is crossed; the facade decides when a crossed deadline becomes the `DataLost` state. | The facade constructor wires these together; naming convention: a method suffixed `...Locked` @@ -209,7 +210,7 @@ READ is the single authority that transitions state and surfaces events. **Why READ still owns events without its own escalation code.** The producer sets `dataPlaneDirty` on every packet, and NOTIFY re-arms it whenever it records an event it did not process. So the read-side refresh always does a full pass (and escalates) while an event is pending. The callback -gate is independent of `ReaderState` — it reads only the bit masks — so deferring the state +gate is independent of `ReaderState` — it reads only the gate counters — so deferring the state transition does not affect when the callback fires. **`getAvailableCount` event guard.** Because the query no longer transitions to `EventPending`, it @@ -226,8 +227,20 @@ guard — the count naturally stops at them. ### 9.1 Producer path and `clear-then-drain` Packet delivery is **lock-free**: `Input::packetReceived` (producer thread) sets the input's -`packetPending` atomic and then sets `dataPlaneDirty`, and schedules the coalesced evaluation. It -never takes the state mutex. +`packetPending` atomic and `dataPlaneDirty`, updates the input's gate flags from a minimal O(1) +connection introspection (its published basis plus the connection's own until-event / has-event +counters), and schedules the coalesced evaluation **only when the callback gate is open** — or +unconditionally when the reader is not in the steady synchronized state or the snapshot could not be +trusted (`forceEvaluation`). It takes no mutex at all. This is the core of the "don't schedule until +we know we want the callback" design: in steady state, a data packet that does not complete a +readable block for every input costs one atomic flag update and no scheduler round-trip. + +Producers only ever **raise** flags, and only the **ready** flag (the common data-packet case). +Events are rare and always leave the steady state, so any event indication forces a full evaluation +instead; event flags are set exclusively by the owner under the state lock. A producer raise is +therefore advisory: it can cost at most one spurious evaluation (which reconciles against ground +truth before any user callback fires) and can never cause a spurious `onDataAvailable`, nor suppress +one that is due. Consumers adopt queued packets by **clearing `packetPending` before draining**, never after. A packet that arrives after the clear re-arms the flag and is caught on the next pass (at-least-once); @@ -256,18 +269,33 @@ already reset — that was the "availability undercount" race and is why the ord Event surfacing and `discardLeftoverSegments` happen only on the READ path (or the full ladder). -### 9.4 Readiness / event bits and the callback gate +### 9.4 Readiness / event flags and the callback gate -`NotificationCoordinator` holds three per-slot bit masks — `used`, `ready`, `event`. The gate is: +The `CallbackGate` holds three atomic counters — `used`, `ready`, `event` — plus a one-shot +`stateChangeNotify` latch. Each slot's contribution lives in a `SlotGateFlags` word (an armed bit +plus ready/event bits) whose every transition adjusts the matching counter exactly once. The gate is: ``` -shouldInvokeCallback = event.any() || (used.any() && (ready & used) == used) +isSatisfied = event > 0 || stateChangeNotify || (used > 0 && ready >= used) ``` -i.e. fire when any input has an event (used or unused — the recovery signal) or when every used -input has a readable block. Readiness while synchronized means "a full aligned block before the next -event"; while synchronizing it means "the first sample." The owner sets these bits during the -data-plane pass; the gate never reads `ReaderState`. +i.e. fire when any input has an event (used or unused — the recovery signal), when a state-change +wake is latched (an `InputsFailed` transition that carries no data or event), or when every used +input has a readable block. `ready >= used` (rather than `==`) tolerates a transient straggler flag +on a slot leaving the used set; the scheduled evaluation reconciles to ground truth before the user +callback fires. Readiness while synchronized means "the smallest servable aligned request +(`effectiveMinimum`) before the next event"; while establishing it means "the first sample." The gate +never reads `ReaderState`. + +**Who writes the flags.** The owner sets both flags authoritatively during every full evaluation +(`publishProducerGateLocked`, the single funnel every `evaluateStateLocked` exit passes through) and +during the light callback/read passes. Producers additionally raise the **ready** flag on the packet +path (lock-free), self-gating steady-state data packets. To keep producer raises trustworthy the +owner brackets every state-lock section that moves or consumes samples with a `CallbackGate::PassGuard` +(an odd/even **epoch**); a producer that observes a non-quiet or changed epoch does not trust its +arithmetic and forces an evaluation instead. Slot removal calls `SlotGateFlags::disarm()`, which +atomically retires that slot's counter contributions and turns every later producer raise into a +no-op, so a packet racing a removal can never leave the counters drifted. ### 9.5 Data loss (in-band) diff --git a/core/opendaq/reader/include/opendaq/multi_reader/callback_gate.h b/core/opendaq/reader/include/opendaq/multi_reader/callback_gate.h new file mode 100644 index 0000000000..7b6e392b8b --- /dev/null +++ b/core/opendaq/reader/include/opendaq/multi_reader/callback_gate.h @@ -0,0 +1,234 @@ +/* + * Copyright 2022-2026 openDAQ d.o.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once +#include + +#include +#include +#include + +BEGIN_NAMESPACE_OPENDAQ + +namespace multi_reader +{ + +/** + * @brief Lock-free callback-gate state shared between the multi reader (owner) and its + * per-input slots (producer threads). + * + * The gate decides whether the public onDataAvailable callback is worth scheduling: + * + * event > 0 || (used > 0 && ready >= used) || stateChangeNotify + * + * `ready`/`event` are counters over the per-slot flag words (SlotGateFlags below); `used` is + * the number of used slots, owner-maintained. Events on unused slots participate deliberately: + * they are the recovery signal consumers answer with setInputUsed. `ready >= used` (not ==) + * tolerates a transient straggler flag on a slot leaving the used set; the scheduled + * evaluation re-verifies against ground truth before the user callback fires, so a stale + * counter can only cost a spurious task, never a missed callback. + * + * The pass epoch is the producers' consistency guard: the owner brackets every state-lock + * section that can move samples (adopt, read, drop) with a PassGuard, making the epoch odd + * for its duration. A producer computing flag raises from the published per-slot basis plus + * the connection counters samples an even epoch before and the same value after; anything + * else means the basis may be torn and the producer must fall back to requesting an + * evaluation instead of trusting its arithmetic. + * + * Threading: everything here is atomic; raises can arrive from any producer thread while + * the owner reconciles under its state lock. Producers only ever RAISE flags - a stale raise + * costs one spurious evaluation (which reconciles), while lowering is reserved to the owner, + * so a producer can never suppress a wake-up it should have caused. + */ +class CallbackGate +{ +public: + bool isSatisfied() const + { + if (event.load() > 0) + return true; + if (stateChangeNotify.load()) + return true; + const SizeT usedCount = used.load(); + return usedCount > 0 && ready.load() >= usedCount; + } + + /// Owner-maintained used-slot count (state lock held by the caller). + void adjustUsed(std::int64_t delta) + { + if (delta > 0) + used.fetch_add(static_cast(delta)); + else if (delta < 0) + used.fetch_sub(static_cast(-delta)); + } + + /// One-shot latch for a state change with no returnable data or event (InputsFailed family). + void setStateChangeNotify(bool notify) + { + stateChangeNotify.store(notify); + } + + bool getStateChangeNotify() const + { + return stateChangeNotify.load(); + } + + std::uint64_t passEpoch() const + { + return epoch.load(); + } + + static bool epochQuiet(std::uint64_t value) + { + return (value & 1u) == 0; + } + + /** + * @brief RAII marker for an owner pass (state lock held) that may move samples between a + * connection and its QueueReader or consume them. Producers observing an odd or changed + * epoch do not trust their availability arithmetic and conservatively request an evaluation. + */ + class PassGuard + { + public: + explicit PassGuard(CallbackGate& gate) + : gate(&gate) + { + gate.epoch.fetch_add(1); + } + + PassGuard(PassGuard&& other) noexcept + : gate(other.gate) + { + other.gate = nullptr; + } + + PassGuard(const PassGuard&) = delete; + PassGuard& operator=(const PassGuard&) = delete; + PassGuard& operator=(PassGuard&&) = delete; + + ~PassGuard() + { + if (gate) + gate->epoch.fetch_add(1); + } + + private: + CallbackGate* gate; + }; + +private: + friend class SlotGateFlags; + + std::atomic used{0}; + std::atomic ready{0}; + std::atomic event{0}; + std::atomic_bool stateChangeNotify{false}; + /// Odd while an owner pass is in flight; incremented on entry and exit. + std::atomic epoch{0}; +}; + +/** + * @brief One slot's ready/event gate flags, packed with an armed bit into a single atomic word + * so flag transitions and the shared counters can never diverge: every observed transition + * adjusts the matching CallbackGate counter exactly once, and disarm() atomically retires the + * slot's contribution - a producer raise racing the owner's removal either lands before the + * disarm (and is subtracted by it) or loses the CAS and sees the slot disarmed. + * + * Producers use raiseReady/raiseEvent only (flags only ever go up on the producer path); + * the owner sets flags in either direction while holding its state lock. + */ +class SlotGateFlags +{ +public: + explicit SlotGateFlags(std::shared_ptr sharedGate) + : gate(std::move(sharedGate)) + { + } + + bool ready() const + { + return (word.load() & ReadyBit) != 0; + } + + bool event() const + { + return (word.load() & EventBit) != 0; + } + + /// @return true if the flag transitioned (and the shared counter was bumped). + bool raiseReady() + { + return setBit(ReadyBit, true, gate->ready); + } + + bool raiseEvent() + { + return setBit(EventBit, true, gate->event); + } + + bool setReady(bool value) + { + return setBit(ReadyBit, value, gate->ready); + } + + bool setEvent(bool value) + { + return setBit(EventBit, value, gate->event); + } + + /// Owner removal: retire this slot's counter contributions; all later raises are no-ops. + void disarm() + { + const std::uint32_t old = word.exchange(0); + if (old & ReadyBit) + gate->ready.fetch_sub(1); + if (old & EventBit) + gate->event.fetch_sub(1); + } + +private: + static constexpr std::uint32_t ArmedBit = 1; + static constexpr std::uint32_t ReadyBit = 2; + static constexpr std::uint32_t EventBit = 4; + + bool setBit(std::uint32_t bit, bool value, std::atomic& counter) + { + std::uint32_t current = word.load(); + for (;;) + { + if ((current & ArmedBit) == 0) + return false; + const std::uint32_t next = value ? (current | bit) : (current & ~bit); + if (next == current) + return false; + if (word.compare_exchange_weak(current, next)) + { + if (value) + counter.fetch_add(1); + else + counter.fetch_sub(1); + return true; + } + } + } + + std::shared_ptr gate; + std::atomic word{ArmedBit}; +}; + +} // namespace multi_reader + +END_NAMESPACE_OPENDAQ diff --git a/core/opendaq/reader/include/opendaq/multi_reader/input.h b/core/opendaq/reader/include/opendaq/multi_reader/input.h index 495f4bd4ec..19be350c2b 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/input.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/input.h @@ -18,11 +18,14 @@ #include #include #include +#include #include #include #include #include +#include +#include BEGIN_NAMESPACE_OPENDAQ @@ -49,20 +52,35 @@ struct IInputListener /// The signal was disconnected from the slot's port. virtual void slotDisconnected(SizeT slotIndex) = 0; /** - * @brief Every packet arrival (bounded producer path). Coalescing is the owner's - * job (NotificationCoordinator); the slot's packetPending bit stays set until - * clearPacketPending() for cheap "anything new since last evaluation" queries. + * @brief Every packet arrival (bounded producer path). The slot has already updated its + * gate flags from a minimal connection introspection; the listener decides whether the + * shared gate warrants scheduling an evaluation. forceEvaluation is the conservative + * escape hatch: the slot could not trust its snapshot (owner pass in flight, connection + * mid-rebind) or the reader is in a state where every packet must re-enter the state + * machine (anything but steady Synchronized) - the listener then schedules + * unconditionally, restoring the classic packet-per-evaluation behavior. */ - virtual void slotPacketReceived(SizeT slotIndex) = 0; + virtual void slotPacketReceived(SizeT slotIndex, bool forceEvaluation) = 0; }; /** * @brief One input of the multi reader: owns the port reference and the per-input QueueReader, - * implements IInputPortNotifications for that port, and holds the used/connected/pending flags. + * implements IInputPortNotifications for that port, and holds the used/connected/pending flags + * plus this slot's producer-facing callback-gate state. + * + * Gate state (all atomics, producer-readable): + * - gate flags (SlotGateFlags): this slot's ready/event contribution to the shared CallbackGate. + * - basis: the adopted queue's availability-until-event (native samples) and whether any event + * packet is adopted - published by the owner after every pass that moves or consumes samples. + * The producer adds the connection's own O(1) counters on top to get the current truth. + * - readyThresholdNative: the effective minimum (native samples) at which this slot becomes + * ready; NeverReady disables producer ready-raises (no model, unused, unconnected). + * - wakeOnAnyPacket: every packet forces an evaluation (any state but steady Synchronized). * * Threading contract: - * - The IInputPortNotifications entry points are bounded: they update atomics and forward one - * semantic notification; no dequeue, no descriptor parsing, no locks, no user callbacks. + * - The IInputPortNotifications entry points are bounded: they update atomics, read two O(1) + * connection counters and forward one semantic notification; no dequeue, no descriptor + * parsing, no reader-state locks, no user callbacks. * - Everything under "owner-side API" must be called with the owner's state lock held; the * QueueReader has no lock of its own. * - The port holds only a weak reference to this object (its listener), so the owner's strong @@ -73,6 +91,9 @@ class Input final : public ImplementationOfWeak public: using SteadyClock = std::chrono::steady_clock; + /// Sentinel threshold: the producer never raises the ready flag. + static constexpr SizeT NeverReady = std::numeric_limits::max(); + explicit Input(SizeT index, const InputPortConfigPtr& port, SampleType valueReadType, @@ -80,7 +101,8 @@ class Input final : public ImplementationOfWeak ReadMode mode, const LoggerComponentPtr& logger, IInputListener* listener, - bool globalIdFromSignal); + bool globalIdFromSignal, + std::shared_ptr gate); // IInputPortNotifications (producer/connection threads) ErrCode INTERFACE_FUNC acceptsSignal(IInputPort* inputPort, ISignal* signal, Bool* accept) override; @@ -118,7 +140,7 @@ class Input final : public ImplementationOfWeak bool syncConnection(); /** - * @brief Used flag only - excluding the slot from masks, compatibility, synchronization and + * @brief Used flag only - excluding the slot from the gate, compatibility, synchronization and * availability is the owner's responsibility, as is deactivating the port (setPortActive) * and resetting/revalidating on re-enable. */ @@ -132,11 +154,37 @@ class Input final : public ImplementationOfWeak void setPortActive(bool active); + // --- Gate maintenance (owner state lock held unless noted) --- + + /// This slot's ready/event contribution to the shared gate. Producer-safe for raises; + /// owner-only for lowering. + SlotGateFlags& gateFlags(); + + /** + * @brief Publish the adopted queue's producer-visible basis: availability until the next + * event (native samples) and whether any event packet (leading or buried) is adopted. + * Owner-called after every pass that adopts or consumes samples on this slot. + */ + void publishGateBasis(SizeT availableNativeUntilEvent, bool hasEventPackets); + + /// Native-sample threshold at which the producer raises the ready flag; NeverReady disables. + void setReadyThresholdNative(SizeT thresholdNative); + + /// True in every state but steady Synchronized: each packet forces an evaluation. + void setWakeOnAnyPacket(bool wake); + /// Owner teardown: no listener notifications are forwarded after this returns. void detachListener(); private: IInputListener* getListener() const; + /** + * @brief Producer-side gate maintenance: raise this slot's ready/event flags from the + * published basis plus the connection's O(1) counters, guarded by the owner-pass epoch. + * @return false when the snapshot cannot be trusted (owner pass in flight, epoch moved, + * connection unassigned) - the caller then forces an evaluation instead. + */ + bool tryRaiseGateFlags(); std::atomic index; const bool globalIdFromSignal; @@ -153,6 +201,15 @@ class Input final : public ImplementationOfWeak std::atomic_bool packetPending{false}; std::atomic lastPacketArrival{SteadyClock::time_point{}}; + std::shared_ptr callbackGate; + SlotGateFlags flags; + std::atomic basisAvailableNative{0}; + std::atomic_bool basisHasEventPackets{false}; + std::atomic readyThresholdNative{NeverReady}; + /// Defaults to true: until the first full evaluation publishes a steady Synchronized + /// state, every packet re-enters the state machine (classic behavior). + std::atomic_bool wakeOnAnyPacket{true}; + LoggerComponentPtr loggerComponent; }; diff --git a/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h b/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h index 8ace311275..208da1615f 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/notification_coordinator.h @@ -16,6 +16,7 @@ #pragma once #include #include +#include #include #include @@ -23,7 +24,6 @@ #include #include #include -#include BEGIN_NAMESPACE_OPENDAQ @@ -31,32 +31,29 @@ namespace multi_reader { /** - * @brief Readiness tracking and callback coalescing for the multi reader. + * @brief Evaluation-task scheduling and the shared callback gate for the multi reader. * - * Two independent responsibilities: + * Two responsibilities: * - * 1. Coalesced evaluation scheduling. requestEvaluation() is the bounded producer-thread - * entry point: it schedules at most one evaluation task on the scheduler. The task - * clears its scheduled flag before running, so updates arriving during an evaluation - * schedule exactly one follow-up. The task never runs after detach() - the shared - * task state outlives the coordinator and is checked under its own lock. + * 1. Coalesced evaluation scheduling. requestEvaluation() is the bounded, lock-free entry + * point: it schedules at most one evaluation task on the scheduler. The task clears its + * scheduled flag before running, so updates arriving during an evaluation schedule exactly + * one follow-up. The task never runs after detach() - the shared task state outlives the + * coordinator and is checked under its own lock (taken only inside the task, never on the + * request path). * - * 2. Used/ready/event masks deciding whether the public onDataAvailable callback fires: - * event.any() || (used.any() && (ready & used) == used) || stateChangeNotify. - * Events on unused slots participate deliberately: they are the recovery - * signal consumers react to with setInputUsed. The "ready" meaning is phase-dependent - * (first sample while synchronizing, one full block while synchronized) - the owner - * sets the bits during its state evaluation. stateChangeNotify is a one-shot latch for a - * state change that carries no returnable data or event - a transition into an InputsFailed - * state (Incompatible / SynchronizationFailed / DataLost) once the causing descriptors are - * cached, so no event fires and no data is ready. It wakes the consumer once to read the - * naming status, so the consumer never has to poll or run its own liveness timer. + * 2. The shared CallbackGate (see callback_gate.h). Producers raise per-slot flags and query + * gateSatisfied() locklessly; a task is scheduled from the packet path only when the gate + * is open (or a producer could not trust its snapshot). The owner reconciles the flags to + * ground truth under its state lock before letting the gate fire the user callback, so + * producer raises are advisory: they can cause a spurious task but never a spurious user + * callback, and they can never suppress one. * - * Threading contract: requestEvaluation() and detach() are thread-safe. Everything else - * (masks, callback queries) must be called with the owner's state lock held. The - * evaluation callback itself runs on a scheduler thread without any coordinator lock - * held - the owner takes its own lock inside and must invoke user callbacks only after - * releasing it. detach() must be called without holding locks the evaluation takes. + * Threading contract: requestEvaluation(), detach() and every gate query are thread-safe and + * lock-free on the caller's side. The evaluation callback runs on a scheduler thread without + * any coordinator lock held - the owner takes its own lock inside and must invoke user + * callbacks only after releasing it. detach() must be called without holding locks the + * evaluation takes. */ class NotificationCoordinator { @@ -72,31 +69,22 @@ class NotificationCoordinator /// The owner's coalesced evaluation entry point. Set once during construction of the owner. void setEvaluationCallback(EvaluationCallback callback); - /// Producer-thread safe; schedules at most one coalesced evaluation task. + /// Producer-thread safe and lock-free; schedules at most one coalesced evaluation task. void requestEvaluation(); - /// No evaluation callback runs after this returns-except one already in flight on + /// No evaluation callback runs after this returns - except one already in flight on /// another thread, which detach() waits out via the task-state lock. void detach(); - // --- Masks (owner state lock held) --- - void resize(SizeT slotCount); - /// Drops one slot's bits, shifting the following slots down by one. - void erase(SizeT index); - SizeT getSlotCount() const; - - void setUsed(SizeT index, bool used); - void setReady(SizeT index, bool ready); - void setEvent(SizeT index, bool hasEvent); - bool isUsed(SizeT index) const; - /// Current ready/event bit for one slot. The callback pass uses these to skip a slot that - /// already satisfies the gate: a ready/event slot cannot stop satisfying it until a read - /// consumes it (the read path lowers the bit), so the callback never needs to re-touch it. - bool getReady(SizeT index) const; - bool getEvent(SizeT index) const; - - /// Clears ready and event bits (synchronization invalidated, topology changed, ...). - void clearReadiness(); + /// Shared gate state; each Input holds a reference so producer raises and owner + /// reconciliation adjust the same counters. + const std::shared_ptr& gate() const; + + /// Lock-free: the callback gate (see CallbackGate::isSatisfied). + bool gateSatisfied() const; + + /// Mark the current thread as an owner pass for producers' consistency checks. + CallbackGate::PassGuard beginOwnerPass(); /// One-shot latch: raise the callback gate for a state change that carries no returnable /// data or event (a transition into an InputsFailed state - Incompatible / @@ -106,16 +94,6 @@ class NotificationCoordinator void setStateChangeNotify(bool notify); bool getStateChangeNotify() const; - /// (event & used).any() - bool anyUsedEvent() const; - /// event.any() - unused slots included. - bool anyEvent() const; - /// used.any() && (ready & used) == used - bool allUsedReady() const; - /// The callback gate: fires when there is any event, when every used slot is ready, or when - /// a state-change notification is latched. - bool shouldInvokeCallback() const; - private: struct TaskState { @@ -127,13 +105,9 @@ class NotificationCoordinator void scheduleTask(); std::shared_ptr taskState; + std::shared_ptr gateState; WorkExecutor executor; LoggerComponentPtr loggerComponent; - - std::vector usedMask; - std::vector readyMask; - std::vector eventMask; - bool stateChangeNotifyFlag = false; }; } // namespace multi_reader diff --git a/core/opendaq/reader/include/opendaq/multi_reader/read_coordinator.h b/core/opendaq/reader/include/opendaq/multi_reader/read_coordinator.h index af29b8c288..b11c0925b9 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/read_coordinator.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/read_coordinator.h @@ -128,9 +128,14 @@ class ReadCoordinator */ std::vector discardLeftoverSegments(const std::vector& inputs, const CommonModel& model, SizeT minReadCount); -private: + /** + * @brief The smallest servable aligned request (common-rate equivalent): max(blockLcm, + * minReadCount) rounded up to whole blocks. The same minimum gates availability, the + * leftover-segment discard and the callback-gate ready threshold. + */ static SizeT effectiveMinimum(const CommonModel& model, SizeT minReadCount); +private: bool configured = false; LoggerComponentPtr loggerComponent; }; diff --git a/core/opendaq/reader/include/opendaq/multi_reader_impl.h b/core/opendaq/reader/include/opendaq/multi_reader_impl.h index 05a3dcf719..a78a56a228 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader_impl.h +++ b/core/opendaq/reader/include/opendaq/multi_reader_impl.h @@ -63,9 +63,10 @@ enum class ReaderState * SynchronizationManager, all queue work in the per-slot QueueReaders, read planning in * the ReadCoordinator and callback coalescing in the NotificationCoordinator. * - * Locking: one state mutex; producer threads never take it - * (Input::packetReceived only touches atomics and schedules the coalesced - * evaluation); user callbacks are invoked with no lock held. + * Locking: one state mutex; producer threads never take it - and take no other mutex either: + * Input::packetReceived touches atomics plus two O(1) connection counters, and an evaluation + * task is scheduled only once the callback gate is open (or the snapshot cannot be trusted). + * User callbacks are invoked with no lock held. * * Naming convention: a "...Locked" suffix means "the caller must already hold `mutex`" - * such a method never takes the lock itself and must only be called from code that does. @@ -153,14 +154,15 @@ class MultiReaderImpl : public ImplementationOfWeak // Why this second listener surface exists: a port can have exactly one listener, and that // listener is the slot (it owns the port's QueueReader pairing). This private interface is // the slot's channel back up to the facade - it carries the slot index, keeps the producer - // path bounded (slotPacketReceived touches atomics and schedules the coalesced evaluation, - // taking no facade lock), and serializes external-listener forwarding so user callbacks - // never run under the state mutex. The facade itself is deliberately NOT an - // IInputPortNotifications: ports never see the reader directly. + // path bounded (slotPacketReceived touches atomics and schedules the coalesced evaluation + // only when the callback gate is open, taking no facade lock and no other mutex), and + // serializes external-listener forwarding so user callbacks never run under the state + // mutex. The facade itself is deliberately NOT an IInputPortNotifications: ports never see + // the reader directly. bool slotAcceptsSignal(SizeT slotIndex, const SignalPtr& signal) override; void slotConnected(SizeT slotIndex) override; void slotDisconnected(SizeT slotIndex) override; - void slotPacketReceived(SizeT slotIndex) override; + void slotPacketReceived(SizeT slotIndex, bool forceEvaluation) override; // --- Construction --- /// Source normalization (construction and addInput): validates the list (assigned, @@ -173,8 +175,19 @@ class MultiReaderImpl : public ImplementationOfWeak // --- State machine (state mutex held) --- /// Full state evaluation - the transition handler run by the paths that change state - /// (connect/disconnect, used/active changes, topology, events, deadlines). + /// (connect/disconnect, used/active changes, topology, events, deadlines). Runs the + /// ladder, then publishes the producer-facing gate state (publishProducerGateLocked). void evaluateStateLocked(); + /// The evaluation ladder itself; only evaluateStateLocked calls this. + void evaluateStateLadderLocked(); + /** + * @brief Publish the producer-facing callback-gate state after a full evaluation: per-slot + * basis (adopted availability-until-event + adopted events), the ready threshold, the + * wake-on-any-packet mode, and - while synchronized - the ground-truth ready/event flags. + * Outside the steady Synchronized state every packet forces an evaluation, so only the + * flags the ladder maintains matter there. + */ + void publishProducerGateLocked(); /// Data-plane pass for the read and query paths: while synchronized, drains the slots that /// received packets, publishes the availability cache, and maintains the readiness bits. With /// escalateOnEvent (the read path) it escalates to evaluateStateLocked when an event surfaces so @@ -236,6 +249,18 @@ class MultiReaderImpl : public ImplementationOfWeak void reindexSlotsLocked(); void setPortsActiveLocked(bool active); + // --- Callback-gate maintenance (state mutex held) --- + // The per-slot flags and the shared counters live in CallbackGate/SlotGateFlags; these + // helpers are the owner-side write path (the flag word adjusts the counters itself). + void setSlotReadyLocked(multi_reader::Input* slot, bool ready); + void setSlotEventLocked(multi_reader::Input* slot, bool event); + /// Used-flag change with gate accounting: adjusts the used count and drops a stale ready flag. + void applySlotUsedLocked(multi_reader::Input* slot, bool used); + /// Publish one slot's adopted basis (availability-until-event + adopted events) for producers. + void publishSlotBasisLocked(multi_reader::Input* slot); + /// Lowers every slot's ready/event flag (synchronization invalidated, reader deactivated). + void clearGateReadinessLocked(); + /// Slot index of the explicitly selected main input; notFound when the default /// (first used input) applies or the selection is dangling. SizeT mainSlotIndexLocked() const; diff --git a/core/opendaq/reader/src/CMakeLists.txt b/core/opendaq/reader/src/CMakeLists.txt index b4c2bce4dc..c8176a9966 100644 --- a/core/opendaq/reader/src/CMakeLists.txt +++ b/core/opendaq/reader/src/CMakeLists.txt @@ -140,6 +140,7 @@ function(create_component_source_groups_${BASE_NAME}) ${SDK_HEADERS_DIR}/multi_reader/data_loss_monitor.h ${SDK_HEADERS_DIR}/multi_reader/input.h ${SDK_HEADERS_DIR}/multi_reader/synchronization_manager.h + ${SDK_HEADERS_DIR}/multi_reader/callback_gate.h ${SDK_HEADERS_DIR}/multi_reader/notification_coordinator.h ${SDK_HEADERS_DIR}/multi_reader/read_coordinator.h ${SDK_HEADERS_DIR}/multi_reader_impl.h @@ -186,6 +187,7 @@ set(SRC_PrivateHeaders_Component multi_reader/data_loss_monitor.h multi_reader/input.h multi_reader/synchronization_manager.h + multi_reader/callback_gate.h multi_reader/notification_coordinator.h multi_reader/read_coordinator.h reader_status_impl.h diff --git a/core/opendaq/reader/src/multi_reader/input.cpp b/core/opendaq/reader/src/multi_reader/input.cpp index b9ffefbe90..15d1104002 100644 --- a/core/opendaq/reader/src/multi_reader/input.cpp +++ b/core/opendaq/reader/src/multi_reader/input.cpp @@ -1,5 +1,7 @@ #include +#include + BEGIN_NAMESPACE_OPENDAQ namespace multi_reader @@ -12,12 +14,15 @@ Input::Input(SizeT index, ReadMode mode, const LoggerComponentPtr& logger, IInputListener* listener, - bool globalIdFromSignal) + bool globalIdFromSignal, + std::shared_ptr gate) : index(index) , globalIdFromSignal(globalIdFromSignal) , port(port) , queueReader(port, valueReadType, domainReadType, mode, logger, globalIdFromSignal) , listener(listener) + , callbackGate(std::move(gate)) + , flags(callbackGate) , loggerComponent(logger) { connectedState = port.getConnection().assigned(); @@ -67,12 +72,70 @@ ErrCode Input::packetReceived(IInputPort* /*inputPort*/) { lastPacketArrival.store(SteadyClock::now()); packetPending = true; + + // Steady Synchronized state: raise the gate flags from a minimal introspection and let + // the listener schedule only when the gate is open. Any other state (or an untrusted + // snapshot) forces the evaluation - the classic packet-per-evaluation behavior. + bool force = wakeOnAnyPacket.load(); + if (!force) + force = !tryRaiseGateFlags(); + if (auto* const target = getListener()) - target->slotPacketReceived(index); + target->slotPacketReceived(index, force); return OPENDAQ_SUCCESS; }); } +bool Input::tryRaiseGateFlags() +{ + // Epoch guard (see CallbackGate): an owner pass can move samples from the connection into + // the adopted queue between our reads, making basis + connection undercount. A raise can + // never be wrong for long (the evaluation reconciles), but a SKIPPED raise could silence + // the gate forever - so anything inconsistent returns false and the caller forces an + // evaluation instead. + const auto epochBefore = callbackGate->passEpoch(); + if (!CallbackGate::epochQuiet(epochBefore)) + return false; + + // A set event flag already holds the gate open; the caller schedules via gateSatisfied. + if (flags.event()) + return true; + + // Events are rare and always transition the reader out of the steady synchronized state. + // Producers never touch the event counter (that would race the owner and risk a stale flag + // the owner's gate-skip logic would perpetuate); instead any event indication - adopted + // (basis) or still on the connection - forces a full evaluation, which sets event flags + // authoritatively under the state lock. Only readiness, the common data-packet case, is + // self-gated here. + if (basisHasEventPackets.load()) + return false; + + const auto connection = port.getConnection(); + if (!connection.assigned()) + return false; // mid-(dis)connect: let the evaluation sort it out + + // Both connection queries are O(1) counter reads under the connection's own lock, which the + // enqueue that triggered this notification has already released. + if (connection.hasEventPacket()) + return false; + + if (!flags.ready()) + { + const SizeT threshold = readyThresholdNative.load(); + if (threshold != NeverReady) + { + const SizeT available = basisAvailableNative.load() + static_cast(connection.getSamplesUntilNextEventPacket()); + if (available >= threshold) + { + if (callbackGate->passEpoch() != epochBefore) + return false; // an owner pass ran under us; its end-of-pass truth wins + flags.raiseReady(); + } + } + } + return true; +} + SizeT Input::getIndex() const { return index; @@ -165,6 +228,27 @@ void Input::setPortActive(bool active) port.setActive(active); } +SlotGateFlags& Input::gateFlags() +{ + return flags; +} + +void Input::publishGateBasis(SizeT availableNativeUntilEvent, bool hasEventPackets) +{ + basisAvailableNative.store(availableNativeUntilEvent); + basisHasEventPackets.store(hasEventPackets); +} + +void Input::setReadyThresholdNative(SizeT thresholdNative) +{ + readyThresholdNative.store(thresholdNative); +} + +void Input::setWakeOnAnyPacket(bool wake) +{ + wakeOnAnyPacket.store(wake); +} + void Input::detachListener() { listener = nullptr; diff --git a/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp b/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp index a5e2bd595b..39da04a4a6 100644 --- a/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp +++ b/core/opendaq/reader/src/multi_reader/notification_coordinator.cpp @@ -7,6 +7,7 @@ namespace multi_reader NotificationCoordinator::NotificationCoordinator(const SchedulerPtr& scheduler, const LoggerComponentPtr& logger) : taskState(std::make_shared()) + , gateState(std::make_shared()) , loggerComponent(logger) { executor = [scheduler](std::function work) @@ -20,6 +21,7 @@ NotificationCoordinator::NotificationCoordinator(const SchedulerPtr& scheduler, NotificationCoordinator::NotificationCoordinator(WorkExecutor executor, const LoggerComponentPtr& logger) : taskState(std::make_shared()) + , gateState(std::make_shared()) , executor(std::move(executor)) , loggerComponent(logger) { @@ -48,131 +50,46 @@ void NotificationCoordinator::detach() taskState->callback = nullptr; } -void NotificationCoordinator::scheduleTask() -{ - // The lambda holds the task state alive; a coordinator destroyed with a task still - // queued leaves a harmless no-op behind (detach cleared the callback). - executor( - [state = taskState] - { - // Cleared before running: updates arriving during the evaluation schedule - // exactly one follow-up task instead of being lost. - state->scheduled = false; - - std::lock_guard lock(state->mutex); - if (state->callback) - state->callback(); - }); -} - -void NotificationCoordinator::resize(SizeT slotCount) -{ - usedMask.resize(slotCount, true); - readyMask.resize(slotCount, false); - eventMask.resize(slotCount, false); -} - -void NotificationCoordinator::erase(SizeT index) -{ - // Removing one input must not disturb the remaining inputs' bits. - usedMask.erase(usedMask.begin() + index); - readyMask.erase(readyMask.begin() + index); - eventMask.erase(eventMask.begin() + index); -} - -SizeT NotificationCoordinator::getSlotCount() const -{ - return usedMask.size(); -} - -void NotificationCoordinator::setUsed(SizeT index, bool used) -{ - usedMask.at(index) = used; -} - -void NotificationCoordinator::setReady(SizeT index, bool ready) -{ - readyMask.at(index) = ready; -} - -void NotificationCoordinator::setEvent(SizeT index, bool hasEvent) -{ - eventMask.at(index) = hasEvent; -} - -bool NotificationCoordinator::isUsed(SizeT index) const +const std::shared_ptr& NotificationCoordinator::gate() const { - return usedMask.at(index); + return gateState; } -bool NotificationCoordinator::getReady(SizeT index) const +bool NotificationCoordinator::gateSatisfied() const { - return readyMask.at(index); + return gateState->isSatisfied(); } -bool NotificationCoordinator::getEvent(SizeT index) const +CallbackGate::PassGuard NotificationCoordinator::beginOwnerPass() { - return eventMask.at(index); -} - -void NotificationCoordinator::clearReadiness() -{ - readyMask.assign(readyMask.size(), false); - eventMask.assign(eventMask.size(), false); + return CallbackGate::PassGuard(*gateState); } void NotificationCoordinator::setStateChangeNotify(bool notify) { - stateChangeNotifyFlag = notify; + gateState->setStateChangeNotify(notify); } bool NotificationCoordinator::getStateChangeNotify() const { - return stateChangeNotifyFlag; -} - -bool NotificationCoordinator::anyUsedEvent() const -{ - for (SizeT i = 0; i < usedMask.size(); ++i) - { - if (usedMask[i] && eventMask[i]) - return true; - } - return false; -} - -bool NotificationCoordinator::anyEvent() const -{ - for (SizeT i = 0; i < eventMask.size(); ++i) - { - if (eventMask[i]) - return true; - } - return false; + return gateState->getStateChangeNotify(); } -bool NotificationCoordinator::allUsedReady() const +void NotificationCoordinator::scheduleTask() { - bool anyUsed = false; - for (SizeT i = 0; i < usedMask.size(); ++i) - { - if (!usedMask[i]) - continue; - anyUsed = true; - if (!readyMask[i]) - return false; - } - return anyUsed; -} + // The lambda holds the task state alive; a coordinator destroyed with a task still + // queued leaves a harmless no-op behind (detach cleared the callback). + executor( + [state = taskState] + { + // Cleared before running: updates arriving during the evaluation schedule + // exactly one follow-up task instead of being lost. + state->scheduled = false; -bool NotificationCoordinator::shouldInvokeCallback() const -{ - // Events on unused inputs fire the callback too; that notification is the - // recovery path (the consumer can re-include the input with setInputUsed). - // stateChangeNotify covers a state change with neither data nor event to return - // (an InputsFailed transition: a data-loss deadline, or re-probing an input whose - // failing descriptor is already cached so no new event fires). - return anyEvent() || allUsedReady() || stateChangeNotifyFlag; + std::lock_guard lock(state->mutex); + if (state->callback) + state->callback(); + }); } } // namespace multi_reader diff --git a/core/opendaq/reader/src/multi_reader_impl.cpp b/core/opendaq/reader/src/multi_reader_impl.cpp index bd078c4b98..6fe0e34944 100644 --- a/core/opendaq/reader/src/multi_reader_impl.cpp +++ b/core/opendaq/reader/src/multi_reader_impl.cpp @@ -149,10 +149,10 @@ MultiReaderImpl::MultiReaderImpl(MultiReaderImpl* old, SampleType valueReadType, createSlots(ports); std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); for (SizeT i = 0; i < usedFlags.size() && i < slots.size(); ++i) { - slots[i]->setUsed(usedFlags[i]); - notificationCoordinator->setUsed(i, usedFlags[i]); + applySlotUsedLocked(slots[i], usedFlags[i]); slots[i]->getQueueReader().seedDescriptors(oldValueDescriptors[i], oldDomainDescriptors[i]); } applyDataLossTimeoutLocked(); @@ -218,6 +218,7 @@ MultiReaderImpl::MultiReaderImpl(const MultiReaderBuilderPtr& builder) createSlots(ports); std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); if (mainInputId.assigned() && findSlotByIdLocked(mainInputId) == notFound) DAQ_THROW_EXCEPTION(NotFoundException, "The selected main input does not match any source component"); // Adopted ports may arrive deactivated (a previous owner parked them via @@ -347,8 +348,11 @@ void MultiReaderImpl::createSlots(const ListPtr& inputPorts) readMode, loggerComponent, static_cast(this), - typeOfInputs == InputType::Signals); + typeOfInputs == InputType::Signals, + notificationCoordinator->gate()); auto* slot = static_cast(slotObject.getObject()); + // Slots default to used; the gate's used count follows the slot set + notificationCoordinator->gate()->adjustUsed(1); port.setListener(slotObject); slotObjects.push_back(std::move(slotObject)); @@ -356,7 +360,6 @@ void MultiReaderImpl::createSlots(const ListPtr& inputPorts) ++position; } - notificationCoordinator->resize(slots.size()); dataLossMonitor->resize(slots.size()); } @@ -404,7 +407,17 @@ void MultiReaderImpl::setStateLocked(ReaderState newState, std::string message, newState == ReaderState::SynchronizationFailed || newState == ReaderState::DataLost; if (inputsFailed && (newState != state || affected != stateAffectedInputs)) + { notificationCoordinator->setStateChangeNotify(true); + // No scheduling here: setStateLocked runs under the state mutex, and the inline + // (no-scheduler) executor would re-enter onCoalescedEvaluation and deadlock. The wake + // is delivered without it: an InputsFailed state is non-Synchronized, so the reader sets + // wakeOnAnyPacket for every slot (publishProducerGateLocked) and the next packet forces + // an evaluation that observes the latch; the pure data-loss deadline (no packet at all) + // schedules from the monitor's own callback, outside any lock. When the transition + // happens inside a coalesced evaluation (an escalated ladder), that same + // onCoalescedEvaluation observes the latch after the ladder returns. + } state = newState; stateMessage = std::move(message); @@ -541,14 +554,16 @@ void MultiReaderImpl::drainUnusedSlotsLocked() slot->syncConnection(); if (!slot->isConnected()) { - notificationCoordinator->setEvent(slot->getIndex(), false); + slot->publishGateBasis(0, false); + setSlotEventLocked(slot, false); continue; } slot->clearPacketPending(); auto& reader = slot->getQueueReader(); reader.drain(); - notificationCoordinator->setEvent(slot->getIndex(), reader.hasPendingEvents()); + publishSlotBasisLocked(slot); + setSlotEventLocked(slot, reader.hasPendingEvents()); } } @@ -584,7 +599,10 @@ void MultiReaderImpl::refreshDataPlaneLocked(bool escalateOnEvent) dataPlaneSlotAvailable.assign(slots.size(), 0); const bool haveModel = syncManager->hasModel(); - const SizeT block = haveModel ? syncManager->getModel().blockLcm : 0; + // The gate's ready threshold is the smallest servable aligned request - the same minimum + // the availability alignment and the leftover-segment discard enforce - so an open gate + // always means a read can actually return samples. + const SizeT gateMinimum = haveModel ? ReadCoordinator::effectiveMinimum(syncManager->getModel(), minReadCount) : 0; bool escalate = false; bool anyEvent = false; @@ -604,7 +622,8 @@ void MultiReaderImpl::refreshDataPlaneLocked(bool escalateOnEvent) { auto& reader = slot->getQueueReader(); reader.drain(); - notificationCoordinator->setEvent(slot->getIndex(), reader.hasPendingEvents()); + publishSlotBasisLocked(slot); + setSlotEventLocked(slot, reader.hasPendingEvents()); } } continue; @@ -625,6 +644,7 @@ void MultiReaderImpl::refreshDataPlaneLocked(bool escalateOnEvent) // new packet arriving, so gating this on packetsArrived would miss it. Both queries // are O(1) (empty-check / sticky adoption flag), so per-cycle is cheap. const bool hasEvent = reader.hasPendingEvents() || reader.hasQueuedEventPackets(); + publishSlotBasisLocked(slot); if (hasEvent) { // The read/query path escalates so the full ladder transitions to EventPending and @@ -635,7 +655,7 @@ void MultiReaderImpl::refreshDataPlaneLocked(bool escalateOnEvent) escalate = true; else { - notificationCoordinator->setEvent(slot->getIndex(), true); + setSlotEventLocked(slot, true); anyEvent = true; } continue; @@ -644,19 +664,19 @@ void MultiReaderImpl::refreshDataPlaneLocked(bool escalateOnEvent) // The read/query path leaves event bits to evaluateStateLocked; the callback path owns // them here, so clear a stale bit once the slot's events have drained away. if (!escalateOnEvent) - notificationCoordinator->setEvent(slot->getIndex(), false); + setSlotEventLocked(slot, false); // Availability is O(1) here (the queue reader maintains it incrementally across drains // and reads), so recomputing it for every used slot each real pass is cheap - and it is // exactly the count createPlan needs, so caching it removes createPlan's separate walk. - // Readiness is derived from the same value: a slot is ready with a full aligned block - // buffered before its next event. + // Readiness is derived from the same value: a slot is ready with the smallest servable + // aligned request buffered before its next event. if (haveModel) { const SizeT avail = reader.getAvailableSamplesUntilEvent(); dataPlaneSlotAvailable[i] = avail; availableCommon = std::min(availableCommon, avail); - notificationCoordinator->setReady(slot->getIndex(), avail >= block); + setSlotReadyLocked(slot, avail >= gateMinimum); } } @@ -691,6 +711,108 @@ void MultiReaderImpl::refreshDataPlaneLocked(bool escalateOnEvent) } void MultiReaderImpl::evaluateStateLocked() +{ + evaluateStateLadderLocked(); + publishProducerGateLocked(); +} + +void MultiReaderImpl::setSlotReadyLocked(Input* slot, bool ready) +{ + slot->gateFlags().setReady(ready); +} + +void MultiReaderImpl::setSlotEventLocked(Input* slot, bool event) +{ + slot->gateFlags().setEvent(event); +} + +void MultiReaderImpl::applySlotUsedLocked(Input* slot, bool used) +{ + if (slot->isUsed() == used) + return; + slot->setUsed(used); + notificationCoordinator->gate()->adjustUsed(used ? 1 : -1); + // An unused slot contributes only events to the gate (the recovery signal); a stale ready + // flag would let `ready >= used` open the gate on data the read path will never touch. + if (!used) + setSlotReadyLocked(slot, false); +} + +void MultiReaderImpl::publishSlotBasisLocked(Input* slot) +{ + auto& reader = slot->getQueueReader(); + const SizeT divider = reader.getSampleRateDivider() > 0 ? reader.getSampleRateDivider() : 1; + const bool hasEventPackets = reader.hasPendingEvents() || reader.hasQueuedEventPackets(); + slot->publishGateBasis(reader.getAvailableSamplesUntilEvent() / divider, hasEventPackets); +} + +void MultiReaderImpl::clearGateReadinessLocked() +{ + for (auto* slot : slots) + { + setSlotReadyLocked(slot, false); + setSlotEventLocked(slot, false); + } +} + +void MultiReaderImpl::publishProducerGateLocked() +{ + // The steady Synchronized state is the only one where producers gate their own scheduling; + // everywhere else every packet forces an evaluation (wakeOnAnyPacket), which preserves the + // classic liveness of the establishment, failure and recovery paths - a DataLost slot's + // reviving packet or a Synchronizing slot's alignment progress never waits on the gate. + const bool steady = state == ReaderState::Synchronized && syncManager->hasModel(); + const SizeT gateMinimum = steady ? ReadCoordinator::effectiveMinimum(syncManager->getModel(), minReadCount) : 0; + + for (auto* slot : slots) + { + slot->setWakeOnAnyPacket(!steady); + + if (!slot->isConnected()) + { + slot->publishGateBasis(0, false); + slot->setReadyThresholdNative(Input::NeverReady); + setSlotReadyLocked(slot, false); + setSlotEventLocked(slot, false); + continue; + } + + auto& reader = slot->getQueueReader(); + const SizeT divider = reader.getSampleRateDivider() > 0 ? reader.getSampleRateDivider() : 1; + const bool hasEventPackets = reader.hasPendingEvents() || reader.hasQueuedEventPackets(); + const SizeT untilEventCommon = reader.getAvailableSamplesUntilEvent(); + slot->publishGateBasis(untilEventCommon / divider, hasEventPackets); + + if (!slot->isUsed()) + { + // Data is dropped at the inactive port, so only the event flag matters; the ladder + // (drainUnusedSlotsLocked) maintains it and a producer can still raise it. + slot->setReadyThresholdNative(Input::NeverReady); + setSlotReadyLocked(slot, false); + continue; + } + + if (steady) + { + // Ground truth while synchronized: ready with the smallest servable request buffered + // before the next event, event buried-inclusive (a sub-block residual in front of a + // buried event must still open the gate so a read can surface it). + slot->setReadyThresholdNative(gateMinimum / divider); + setSlotReadyLocked(slot, untilEventCommon >= gateMinimum); + setSlotEventLocked(slot, hasEventPackets); + } + else + { + // Establishment semantics: the first sample marks the slot ready (the gate then + // wakes the consumer as the last input starts delivering); event flags stay + // exactly as the ladder decided for the current state. + slot->setReadyThresholdNative(1); + setSlotReadyLocked(slot, reader.getAvailableSamples() > 0); + } + } +} + +void MultiReaderImpl::evaluateStateLadderLocked() { // The full ladder can drain, drop segments or change state, so any availability the last // fast pass cached is no longer authoritative. Clearing it here (the single funnel every full @@ -736,12 +858,12 @@ void MultiReaderImpl::evaluateStateLocked() slots[slotIndex]->syncConnection(); if (!slots[slotIndex]->isConnected()) { - notificationCoordinator->setEvent(slotIndex, false); + setSlotEventLocked(slots[slotIndex], false); continue; } const bool hasEvents = inactiveReaders[position]->hasPendingEvents(); - notificationCoordinator->setEvent(slotIndex, hasEvents); + setSlotEventLocked(slots[slotIndex], hasEvents); if (hasEvents) inactiveEventInputs.push_back(slotIndex); } @@ -807,7 +929,7 @@ void MultiReaderImpl::evaluateStateLocked() // returnable, so the callback must not fire on the // events already queued on the connected inputs for (const auto index : slotIndices) - notificationCoordinator->setEvent(index, false); + setSlotEventLocked(slots[index], false); invalidateModelLocked(); setStateWithAffectedLocked(ReaderState::WaitingForConnections, "Inputs", " have no signal connected", std::move(unconnected)); @@ -840,7 +962,7 @@ void MultiReaderImpl::evaluateStateLocked() const bool hasEvents = usedReaders[position]->hasPendingEvents(); if (hasEvents) eventInputs.push_back(slotIndices[position]); - notificationCoordinator->setEvent(slotIndices[position], hasEvents); + setSlotEventLocked(slots[slotIndices[position]], hasEvents); // A connected input with neither descriptors nor events is still completing its // connect handshake: the signal's initial descriptor event has not been enqueued @@ -859,7 +981,7 @@ void MultiReaderImpl::evaluateStateLocked() if (handshakeInFlight) { for (const auto index : slotIndices) - notificationCoordinator->setEvent(index, false); + setSlotEventLocked(slots[index], false); } else if (!eventInputs.empty()) { @@ -915,7 +1037,7 @@ void MultiReaderImpl::evaluateStateLocked() if (exposeBuriedEventsLocked(invalidInputs)) { for (const auto index : invalidInputs) - notificationCoordinator->setEvent(index, slots[index]->getQueueReader().hasPendingEvents()); + setSlotEventLocked(slots[index], slots[index]->getQueueReader().hasPendingEvents()); setStateWithAffectedLocked(ReaderState::EventPending, "Events pending on inputs", "", std::move(invalidInputs)); return; } @@ -972,7 +1094,7 @@ void MultiReaderImpl::evaluateStateLocked() if (exposeBuriedEventsLocked(setup.affectedInputs)) { for (const auto index : setup.affectedInputs) - notificationCoordinator->setEvent(index, slots[index]->getQueueReader().hasPendingEvents()); + setSlotEventLocked(slots[index], slots[index]->getQueueReader().hasPendingEvents()); setStateWithAffectedLocked(ReaderState::EventPending, "Events pending on inputs", "", std::move(setup.affectedInputs)); return; } @@ -1009,11 +1131,15 @@ void MultiReaderImpl::evaluateStateLocked() setStateLocked(ReaderState::Synchronizing, std::move(result.message), std::move(result.affectedInputs)); break; case SyncOutcome::EventPending: + { + // setStateLocked moved affectedInputs into the state; read them back from there invalidateSynchronizationLocked(); + auto affected = result.affectedInputs; setStateLocked(ReaderState::EventPending, std::move(result.message), std::move(result.affectedInputs)); - for (const auto index : result.affectedInputs) - notificationCoordinator->setEvent(index, true); + for (const auto index : affected) + setSlotEventLocked(slots[index], true); break; + } case SyncOutcome::Failed: // Synchronization failure no longer deactivates the reader. // Unlike the Incompatible paths, we do NOT drop buffered data to surface a @@ -1027,17 +1153,9 @@ void MultiReaderImpl::evaluateStateLocked() } } - // 13. Readiness for the callback gate: a full aligned block while synchronized, - // the first sample while still synchronizing - for (SizeT position = 0; position < usedReaders.size(); ++position) - { - bool ready = false; - if (state == ReaderState::Synchronized && syncManager->hasModel()) - ready = usedReaders[position]->getAvailableSamplesUntilEvent() >= syncManager->getModel().blockLcm; - else - ready = usedReaders[position]->getAvailableSamples() > 0; - notificationCoordinator->setReady(slotIndices[position], ready); - } + // 13. Readiness for the callback gate - the smallest servable request while synchronized, + // the first sample while still establishing - is published by publishProducerGateLocked, + // which every evaluateStateLocked exit path funnels through. } void MultiReaderImpl::updateCallbackStateLocked() @@ -1051,21 +1169,21 @@ void MultiReaderImpl::updateCallbackStateLocked() } const bool haveModel = syncManager->hasModel(); - const SizeT block = haveModel ? syncManager->getModel().blockLcm : 0; + const SizeT gateMinimum = haveModel ? ReadCoordinator::effectiveMinimum(syncManager->getModel(), minReadCount) : 0; for (SizeT i = 0; i < slots.size(); ++i) { auto* slot = slots[i]; - const SizeT index = slot->getIndex(); const bool used = slot->isUsed(); + auto& gateFlags = slot->gateFlags(); // A slot that already satisfies the callback gate cannot stop satisfying it until a read - // consumes it (the read path lowers the bit then), so the callback pass never needs to + // consumes it (the read path lowers the flag then), so the callback pass never needs to // re-touch it. Readiness only participates in the gate for used inputs; for an unused input - // only its event participates (the recovery signal), so a stale ready bit must not skip it. + // only its event participates (the recovery signal), so a stale ready flag must not skip it. // Skipping also leaves packetPending set, so the read/query path still adopts data queued // behind the slot. - if (notificationCoordinator->getEvent(index) || (used && notificationCoordinator->getReady(index))) + if (gateFlags.event() || (used && gateFlags.ready())) continue; // Nothing new here: a slot that does not already satisfy the gate and received no packet @@ -1082,20 +1200,22 @@ void MultiReaderImpl::updateCallbackStateLocked() { auto& reader = slot->getQueueReader(); reader.drain(); - notificationCoordinator->setEvent(index, reader.hasPendingEvents()); + publishSlotBasisLocked(slot); + setSlotEventLocked(slot, reader.hasPendingEvents()); } continue; } auto& reader = slot->getQueueReader(); reader.drain(); + publishSlotBasisLocked(slot); // Buried-inclusive: a sub-block residual before a buried event still fires the callback so // the consumer reads and the read path surfaces the event. const bool hasEvent = reader.hasPendingEvents() || reader.hasQueuedEventPackets(); - notificationCoordinator->setEvent(index, hasEvent); + setSlotEventLocked(slot, hasEvent); if (!hasEvent && haveModel) - notificationCoordinator->setReady(index, reader.getAvailableSamplesUntilEvent() >= block); + setSlotReadyLocked(slot, reader.getAvailableSamplesUntilEvent() >= gateMinimum); } } @@ -1104,14 +1224,16 @@ void MultiReaderImpl::onCoalescedEvaluation() ProcedurePtr callback; { std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); if (invalid) return; // The coalesced task only decides whether onDataAvailable should fire; it maintains the - // gate bits without running the state ladder for events (deferred to the read/query path) - // and without walking slots that already satisfy the gate. + // gate flags without running the state ladder for events (deferred to the read/query path) + // and without walking slots that already satisfy the gate. Producer raises are advisory; + // this reconciliation is what stands between a stale raise and a spurious user callback. updateCallbackStateLocked(); - if (notificationCoordinator->shouldInvokeCallback()) + if (notificationCoordinator->gateSatisfied()) callback = readCallback; // One-shot: consume a latched state-change wake (an InputsFailed transition) once @@ -1147,6 +1269,7 @@ void MultiReaderImpl::slotConnected(SizeT slotIndex) { { std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); if (slotIndex < slots.size()) { slots[slotIndex]->rebindConnection(); @@ -1172,6 +1295,7 @@ void MultiReaderImpl::slotDisconnected(SizeT slotIndex) { { std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); if (slotIndex < slots.size()) { // Disarm immediately - the state evaluation may return before its monitor @@ -1192,13 +1316,22 @@ void MultiReaderImpl::slotDisconnected(SizeT slotIndex) } } -void MultiReaderImpl::slotPacketReceived(SizeT slotIndex) +void MultiReaderImpl::slotPacketReceived(SizeT slotIndex, bool forceEvaluation) { - // Bounded producer path: no state mutex, no queue access + // Bounded producer path: no state mutex, no queue access, no mutex at all - the slot has + // already raised its gate flags from a minimal connection introspection. // Mark the data plane changed before the notify below, so a consumer woken by it sees it. dataPlaneDirty.store(true, std::memory_order_release); dataLossMonitor->onPacket(slotIndex); - notificationCoordinator->requestEvaluation(); + + // An evaluation task is scheduled only when the callback gate is open - any event flag, + // every used slot ready, or a latched state-change wake - or when the slot could not trust + // its snapshot / the reader is not in the steady Synchronized state (forceEvaluation). + // A closed gate means this packet provably cannot fire onDataAvailable, so scheduling + // would only burn a scheduler round-trip; blocked reads are woken by the notify below and + // re-check availability themselves. + if (forceEvaluation || notificationCoordinator->gateSatisfied()) + notificationCoordinator->requestEvaluation(); notifyCondition.notify_all(); if (externalListener.assigned()) @@ -1444,7 +1577,7 @@ MultiReaderStatusPtr MultiReaderImpl::readEventsLocked() if (packet.assigned()) events.set(slot->getInputId(), packet); - notificationCoordinator->setEvent(slot->getIndex(), reader.hasPendingEvents()); + setSlotEventLocked(slot, reader.hasPendingEvents()); } // Every returned event invalidates synchronization; descriptor changes may have @@ -1467,6 +1600,9 @@ ErrCode MultiReaderImpl::readInternal(void** valueBuffers, bool skip) { std::unique_lock lock(mutex); + // Spans the whole read, including the timed waits (their predicate refreshes the data + // plane): producers treat the entire read as an owner pass and schedule conservatively. + const auto ownerPass = notificationCoordinator->beginOwnerPass(); if (invalid) { @@ -1619,13 +1755,18 @@ ErrCode MultiReaderImpl::readInternal(void** valueBuffers, // directly. This is the "fall on read" half of readiness maintenance. if (plan.commonCount > 0) { - const auto block = model.blockLcm; + const SizeT gateMinimum = ReadCoordinator::effectiveMinimum(model, minReadCount); for (SizeT position = 0; position < used.size(); ++position) { const SizeT remaining = availableCached ? dataPlaneSlotAvailable[slotIndices[position]] - plan.commonCount : used[position]->getAvailableSamplesUntilEvent(); - notificationCoordinator->setReady(slotIndices[position], remaining >= block); + auto* slot = slots[slotIndices[position]]; + // Refresh the full producer-visible basis (availability AND adopted-event state) from + // the consumed frontier, so a late async packetReceived never self-gates against a + // stale event basis and raises a phantom ready/forces a needless evaluation. + publishSlotBasisLocked(slot); + setSlotReadyLocked(slot, remaining >= gateMinimum); } // The read advanced the frontier, so the cached counts are now stale and a previously @@ -1699,13 +1840,14 @@ ErrCode MultiReaderImpl::getAvailableCount(SizeT* count) OPENDAQ_PARAM_NOT_NULL(count); std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); *count = 0; if (invalid) return OPENDAQ_SUCCESS; // The query does not run the state ladder for events (escalateOnEvent = false); it drains, - // maintains the callback bits, and lets the read path surface any event. + // maintains the callback flags, and lets the read path surface any event. refreshDataPlaneLocked(false); if (state == ReaderState::Synchronized) { @@ -1759,6 +1901,7 @@ ErrCode MultiReaderImpl::getEmpty(Bool* empty) OPENDAQ_PARAM_NOT_NULL(empty); std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); bool allHaveData = !slots.empty(); for (auto* slot : slots) @@ -1916,6 +2059,7 @@ ErrCode MultiReaderImpl::setActive(Bool isActive) ProcedurePtr callback; { std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); const bool changed = this->isActive != static_cast(isActive); this->isActive = isActive; @@ -1924,7 +2068,7 @@ ErrCode MultiReaderImpl::setActive(Bool isActive) { setPortsActiveLocked(isActive); invalidateSynchronizationLocked(); - notificationCoordinator->clearReadiness(); + clearGateReadinessLocked(); // Deactivation suspends the data flow: queued data and gap events are dropped // (they are meaningless once the stream pauses), while descriptor changes stay @@ -1988,6 +2132,7 @@ ErrCode MultiReaderImpl::addInput(IComponent* input) list.pushBack(input); std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); normalizeSources(list); auto ports = createOrAdoptPorts(list); @@ -2009,6 +2154,7 @@ ErrCode MultiReaderImpl::removeInput(IString* id) OPENDAQ_PARAM_NOT_NULL(id); std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); const auto position = findSlotByIdLocked(StringPtr::Borrow(id)); if (position == notFound) @@ -2022,6 +2168,12 @@ ErrCode MultiReaderImpl::removeInput(IString* id) auto* slot = slots[position]; slot->detachListener(); + // Retire the slot's gate contribution atomically: disarm() subtracts whatever flags are + // set and makes any in-flight producer raise a no-op, so the shared counters can never + // drift when a packet races the removal. + if (slot->isUsed()) + notificationCoordinator->gate()->adjustUsed(-1); + slot->gateFlags().disarm(); if (!portBinder.assigned()) slot->getPort().remove(); @@ -2030,8 +2182,7 @@ ErrCode MultiReaderImpl::removeInput(IString* id) reindexSlotsLocked(); // Only the removed input's per-slot state goes; the remaining - // inputs keep their readiness/event bits and armed data-loss deadlines - notificationCoordinator->erase(position); + // inputs keep their readiness/event flags and armed data-loss deadlines dataLossMonitor->erase(position); invalidateModelLocked(); @@ -2044,14 +2195,14 @@ ErrCode MultiReaderImpl::setInputUsed(IString* id, Bool isUsed) OPENDAQ_PARAM_NOT_NULL(id); std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); const auto position = findSlotByIdLocked(StringPtr::Borrow(id)); if (position == notFound) return OPENDAQ_ERR_NOTFOUND; auto* slot = slots[position]; - slot->setUsed(isUsed); - notificationCoordinator->setUsed(position, isUsed); + applySlotUsedLocked(slot, isUsed); if (!isUsed) dataLossMonitor->setMonitored(position, false); @@ -2093,6 +2244,7 @@ ErrCode MultiReaderImpl::setMainInput(IString* id) { { std::lock_guard lock(mutex); + const auto ownerPass = notificationCoordinator->beginOwnerPass(); StringPtr newId = StringPtr::Borrow(id); if (newId.assigned() && newId.getLength() == 0) diff --git a/core/opendaq/reader/tests/CMakeLists.txt b/core/opendaq/reader/tests/CMakeLists.txt index 956fc7d984..a435ccf171 100644 --- a/core/opendaq/reader/tests/CMakeLists.txt +++ b/core/opendaq/reader/tests/CMakeLists.txt @@ -18,6 +18,7 @@ set(TEST_SOURCES test_factories.cpp test_queue_reader.cpp test_multi_reader_input.cpp test_synchronization_manager.cpp + test_callback_gate.cpp test_notification_coordinator.cpp test_read_coordinator.cpp test_data_loss_monitor.cpp diff --git a/core/opendaq/reader/tests/test_callback_gate.cpp b/core/opendaq/reader/tests/test_callback_gate.cpp new file mode 100644 index 0000000000..3c8cd1fe7f --- /dev/null +++ b/core/opendaq/reader/tests/test_callback_gate.cpp @@ -0,0 +1,148 @@ +#include + +#include +#include "reader_common.h" + +#include +#include + +using namespace daq; +using namespace daq::multi_reader; + +class CallbackGateTest : public ReaderTest<> +{ +protected: + // A gate with `count` slots, all armed and marked used - the state a freshly constructed + // multi reader publishes before any evaluation. + void makeSlots(SizeT count) + { + gate = std::make_shared(); + gate->adjustUsed(static_cast(count)); + flags.clear(); + for (SizeT i = 0; i < count; ++i) + flags.push_back(std::make_unique(gate)); + } + + std::shared_ptr gate; + std::vector> flags; +}; + +TEST_F(CallbackGateTest, EmptyGateClosed) +{ + gate = std::make_shared(); + ASSERT_FALSE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, AnyEventOpensGate) +{ + makeSlots(3); + ASSERT_FALSE(gate->isSatisfied()); + + flags[1]->setEvent(true); + ASSERT_TRUE(gate->isSatisfied()); + + // Even an event on an unused slot keeps the gate open (the recovery signal) + gate->adjustUsed(-1); + ASSERT_TRUE(gate->isSatisfied()); + + flags[1]->setEvent(false); + ASSERT_FALSE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, AllUsedReadyOpensGate) +{ + makeSlots(3); + flags[0]->setReady(true); + flags[1]->setReady(true); + ASSERT_FALSE(gate->isSatisfied()); // slot 2 not ready + + flags[2]->setReady(true); + ASSERT_TRUE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, ReadyToleratesStragglerOnLeavingUsedSet) +{ + // ready >= used, not ==: a slot leaving the used set with its ready flag still up is + // tolerated (a scheduled evaluation reconciles it), so the gate never misses a wake. + makeSlots(2); + flags[0]->setReady(true); + flags[1]->setReady(true); + ASSERT_TRUE(gate->isSatisfied()); + + // Slot 1 becomes unused but its ready flag has not been lowered yet: ready(2) >= used(1) + gate->adjustUsed(-1); + ASSERT_TRUE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, NoUsedSlotsMeansNotReady) +{ + makeSlots(2); + flags[0]->setReady(true); + flags[1]->setReady(true); + ASSERT_TRUE(gate->isSatisfied()); + + // With no used slots the readiness term cannot open the gate (usedCount > 0 is required), + // even while the ready flags are still up - readiness of nothing is not a reason to wake. + gate->adjustUsed(-2); + ASSERT_FALSE(gate->isSatisfied()); + + // An event, however, still opens it (unused-slot events are the recovery signal). + flags[0]->setEvent(true); + ASSERT_TRUE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, StateChangeNotifyOpensGate) +{ + makeSlots(2); + ASSERT_FALSE(gate->isSatisfied()); + + gate->setStateChangeNotify(true); + ASSERT_TRUE(gate->isSatisfied()); + + gate->setStateChangeNotify(false); + ASSERT_FALSE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, RaiseIsIdempotentOnCounters) +{ + makeSlots(2); + // Repeated raises must bump the shared counter exactly once (transition, not level) + ASSERT_TRUE(flags[0]->raiseReady()); + ASSERT_FALSE(flags[0]->raiseReady()); + ASSERT_TRUE(flags[1]->raiseReady()); + ASSERT_TRUE(gate->isSatisfied()); // both ready -> gate open, counter == 2 + + // Lowering one drops the counter so the gate closes again + ASSERT_TRUE(flags[0]->setReady(false)); + ASSERT_FALSE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, DisarmRetiresContributions) +{ + makeSlots(2); + flags[0]->setReady(true); + flags[0]->setEvent(true); + ASSERT_TRUE(gate->isSatisfied()); + + // Removing the slot: disarm subtracts its ready and event contributions atomically + flags[0]->disarm(); + gate->adjustUsed(-1); // owner also drops the used count for the removed slot + ASSERT_FALSE(gate->isSatisfied()); + + // A late producer raise on the disarmed slot is a no-op and cannot reopen the gate + ASSERT_FALSE(flags[0]->raiseEvent()); + ASSERT_FALSE(gate->isSatisfied()); +} + +TEST_F(CallbackGateTest, PassGuardTogglesEpochParity) +{ + gate = std::make_shared(); + const auto before = gate->passEpoch(); + ASSERT_TRUE(CallbackGate::epochQuiet(before)); + { + CallbackGate::PassGuard guard(*gate); + ASSERT_FALSE(CallbackGate::epochQuiet(gate->passEpoch())); + } + ASSERT_TRUE(CallbackGate::epochQuiet(gate->passEpoch())); + ASSERT_NE(gate->passEpoch(), before); // a full pass advanced the epoch +} diff --git a/core/opendaq/reader/tests/test_domain_value.cpp b/core/opendaq/reader/tests/test_domain_value.cpp index d1172a777f..8defbc9bf2 100644 --- a/core/opendaq/reader/tests/test_domain_value.cpp +++ b/core/opendaq/reader/tests/test_domain_value.cpp @@ -172,7 +172,7 @@ TEST_F(DomainValueTest, SameDomainSameType) ASSERT_THROW((void) (*value1P < *value1InCommonDomainP), daq::InvalidParameterException); auto value2 = std::make_unique>(domain1, 1213000); - daq::DomainValue* value2P = value2.get(); + [[maybe_unused]] daq::DomainValue* value2P = value2.get(); ASSERT_THROW((void) (*value1 < *value2), daq::InvalidParameterException); } diff --git a/core/opendaq/reader/tests/test_multi_reader_input.cpp b/core/opendaq/reader/tests/test_multi_reader_input.cpp index 9e3dbca539..f7e4d5f5e3 100644 --- a/core/opendaq/reader/tests/test_multi_reader_input.cpp +++ b/core/opendaq/reader/tests/test_multi_reader_input.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include "reader_common.h" @@ -8,6 +9,7 @@ using namespace daq::multi_reader; #include +#include // Records the semantic notifications an Input forwards to its owner struct RecordingSlotListener final : daq::multi_reader::IInputListener @@ -16,6 +18,7 @@ struct RecordingSlotListener final : daq::multi_reader::IInputListener std::atomic connectedCount{0}; std::atomic disconnectedCount{0}; std::atomic packetPendingCount{0}; + std::atomic forcedCount{0}; std::atomic lastIndex{static_cast(-1)}; bool acceptSignals = true; @@ -38,9 +41,11 @@ struct RecordingSlotListener final : daq::multi_reader::IInputListener lastIndex = slotIndex; } - void slotPacketReceived(daq::SizeT slotIndex) override + void slotPacketReceived(daq::SizeT slotIndex, bool forceEvaluation) override { ++packetPendingCount; + if (forceEvaluation) + ++forcedCount; lastIndex = slotIndex; } }; @@ -79,7 +84,7 @@ class MultiReaderInputTest : public ReaderTest<> void createSlot(SizeT index, const InputPortConfigPtr& port, IInputListener* listener, bool globalIdFromSignal = false) { slotObj = createWithImplementation( - index, port, SampleType::Float64, SampleType::Int64, ReadMode::Scaled, loggerComponent, listener, globalIdFromSignal); + index, port, SampleType::Float64, SampleType::Int64, ReadMode::Scaled, loggerComponent, listener, globalIdFromSignal, gate); slot = static_cast(slotObj.getObject()); port.setListener(slotObj); } @@ -97,6 +102,7 @@ class MultiReaderInputTest : public ReaderTest<> protected: SignalConfigPtr domainSignal; + std::shared_ptr gate{std::make_shared()}; ObjectPtr slotObj; Input* slot{}; }; @@ -260,6 +266,116 @@ TEST_F(MultiReaderInputTest, RebindConnectionDrainsQueue) ASSERT_EQ(queueReader.getAvailableSamples(), 5u); } +TEST_F(MultiReaderInputTest, ProducerForcesEvaluationInNonSteadyState) +{ + // Default (wakeOnAnyPacket == true, the pre-first-evaluation state): every packet forces + // an evaluation regardless of the gate, preserving classic establishment liveness. + RecordingSlotListener listener; + auto port = createPort(); + createSlot(0, port, &listener); + port.connect(signal); + + slot->clearPacketPending(); + listener.packetPendingCount = 0; + listener.forcedCount = 0; + + sendDataPacket(5, 100); + ASSERT_EQ(listener.packetPendingCount, 1); + ASSERT_EQ(listener.forcedCount, 1); // forced because the owner has not gone steady yet +} + +TEST_F(MultiReaderInputTest, ProducerRaisesReadyFromConnectionCountersWhenSteady) +{ + RecordingSlotListener listener; + auto port = createPort(); + createSlot(0, port, &listener); + port.connect(signal); + + // Adopt the initial descriptor event so subsequent packets are pure data + slot->rebindConnection(); + auto& reader = slot->getQueueReader(); + if (reader.hasPendingEvents()) + reader.popFrontEvent(); + + // Simulate the owner publishing a steady Synchronized gate: no wake-on-any, ready at 10 + // native samples, empty adopted basis. + slot->setWakeOnAnyPacket(false); + slot->setReadyThresholdNative(10); + slot->publishGateBasis(0, false); + slot->gateFlags().setReady(false); + slot->gateFlags().setEvent(false); + + slot->clearPacketPending(); + listener.forcedCount = 0; + + // Below threshold: 5 native samples on the connection, not adopted, ready must stay down + sendDataPacket(5, 200); + ASSERT_FALSE(slot->gateFlags().ready()); + ASSERT_EQ(listener.forcedCount, 0); // steady state, gate closed -> not forced + + // Crossing the threshold: the producer raises ready from basis + connection counters + sendDataPacket(5, 205); + ASSERT_TRUE(slot->gateFlags().ready()); +} + +TEST_F(MultiReaderInputTest, ProducerForcesEvaluationOnConnectionEventPacket) +{ + RecordingSlotListener listener; + auto port = createPort(); + createSlot(0, port, &listener); + port.connect(signal); + + slot->rebindConnection(); + auto& reader = slot->getQueueReader(); + if (reader.hasPendingEvents()) + reader.popFrontEvent(); + + slot->setWakeOnAnyPacket(false); + slot->setReadyThresholdNative(10); + slot->publishGateBasis(0, false); + slot->gateFlags().setReady(false); + slot->gateFlags().setEvent(false); + + listener.forcedCount = 0; + + // Events are owner-managed: a producer that sees an event packet on the connection (via the + // O(1) hasEventPacket counter) does NOT raise the event flag itself - that would race the + // owner and risk a stale flag. It forces a full evaluation, which sets event flags under the + // state lock. So the slot's own event flag stays down; the listener is forced instead. + signal.setDescriptor(DataDescriptorBuilder().setSampleType(SampleType::Int32).build()); + + ASSERT_FALSE(slot->gateFlags().event()); + ASSERT_GE(listener.forcedCount.load(), 1); +} + +TEST_F(MultiReaderInputTest, ProducerFallsBackToForceDuringOwnerPass) +{ + RecordingSlotListener listener; + auto port = createPort(); + createSlot(0, port, &listener); + port.connect(signal); + + slot->rebindConnection(); + auto& reader = slot->getQueueReader(); + if (reader.hasPendingEvents()) + reader.popFrontEvent(); + + slot->setWakeOnAnyPacket(false); + slot->setReadyThresholdNative(10); + slot->publishGateBasis(0, false); + + slot->clearPacketPending(); + listener.forcedCount = 0; + + // An owner pass in flight makes the epoch noisy: the producer cannot trust its snapshot + // and forces an evaluation instead of raising flags. + { + CallbackGate::PassGuard pass(*gate); + sendDataPacket(5, 300); + ASSERT_EQ(listener.forcedCount, 1); + } +} + TEST_F(MultiReaderInputTest, UsedFlagAndPortActive) { RecordingSlotListener listener; diff --git a/core/opendaq/reader/tests/test_notification_coordinator.cpp b/core/opendaq/reader/tests/test_notification_coordinator.cpp index 3bf571531c..38185bca46 100644 --- a/core/opendaq/reader/tests/test_notification_coordinator.cpp +++ b/core/opendaq/reader/tests/test_notification_coordinator.cpp @@ -97,124 +97,46 @@ TEST_F(NotificationCoordinatorTest, QueuedTaskOutlivesCoordinator) ASSERT_EQ(evaluations, 0); } -TEST_F(NotificationCoordinatorTest, EventOnAnyInputGatesCallback) +TEST_F(NotificationCoordinatorTest, NoSchedulerRunsInline) { - NotificationCoordinator coordinator(manualExecutor(), loggerComponent); - coordinator.resize(3); - - ASSERT_FALSE(coordinator.shouldInvokeCallback()); - - coordinator.setEvent(1, true); - ASSERT_TRUE(coordinator.anyUsedEvent()); - ASSERT_TRUE(coordinator.shouldInvokeCallback()); - - // Events on unused inputs fire the callback too (review Q5): the notification is the - // recovery API for consumers that parked the input - coordinator.setUsed(1, false); - ASSERT_FALSE(coordinator.anyUsedEvent()); - ASSERT_TRUE(coordinator.anyEvent()); - ASSERT_TRUE(coordinator.shouldInvokeCallback()); - - // Consuming the event clears the gate - coordinator.setEvent(1, false); - ASSERT_FALSE(coordinator.shouldInvokeCallback()); -} + NotificationCoordinator coordinator(SchedulerPtr(nullptr), loggerComponent); + int evaluations = 0; + coordinator.setEvaluationCallback([&] { ++evaluations; }); -TEST_F(NotificationCoordinatorTest, AllUsedReadyGatesCallback) -{ - NotificationCoordinator coordinator(manualExecutor(), loggerComponent); - coordinator.resize(3); - - coordinator.setReady(0, true); - coordinator.setReady(1, true); - ASSERT_FALSE(coordinator.allUsedReady()); // input 2 not ready - - coordinator.setReady(2, true); - ASSERT_TRUE(coordinator.allUsedReady()); - ASSERT_TRUE(coordinator.shouldInvokeCallback()); - - // An unused input is excluded from the readiness requirement - coordinator.setReady(2, false); - coordinator.setUsed(2, false); - ASSERT_TRUE(coordinator.allUsedReady()); - - // No used inputs at all means nothing is ready - coordinator.setUsed(0, false); - coordinator.setUsed(1, false); - ASSERT_FALSE(coordinator.allUsedReady()); - ASSERT_FALSE(coordinator.shouldInvokeCallback()); -} + coordinator.requestEvaluation(); + ASSERT_EQ(evaluations, 1); -TEST_F(NotificationCoordinatorTest, ClearReadinessKeepsUsedMask) -{ - NotificationCoordinator coordinator(manualExecutor(), loggerComponent); - coordinator.resize(2); - coordinator.setUsed(1, false); - coordinator.setReady(0, true); - coordinator.setEvent(0, true); - ASSERT_TRUE(coordinator.shouldInvokeCallback()); - - coordinator.clearReadiness(); - ASSERT_FALSE(coordinator.shouldInvokeCallback()); - ASSERT_TRUE(coordinator.isUsed(0)); - ASSERT_FALSE(coordinator.isUsed(1)); + coordinator.requestEvaluation(); + ASSERT_EQ(evaluations, 2); } -TEST_F(NotificationCoordinatorTest, StateChangeNotifyGatesCallback) +// The coordinator exposes the shared gate; the gate's own semantics are covered in +// test_callback_gate.cpp. Here we only check the coordinator wires the gate through so a +// producer query and the owner's reconciliation see the same state. +TEST_F(NotificationCoordinatorTest, GateSharedAndStateChangeNotify) { - // Part 1 (spec 3.5): a latched state-change notification (the DataLost deadline) opens the - // callback gate on its own, even with no events and no readiness, and is a one-shot. NotificationCoordinator coordinator(manualExecutor(), loggerComponent); - coordinator.resize(2); - - // No events, no ready inputs -> gate closed - ASSERT_FALSE(coordinator.shouldInvokeCallback()); + ASSERT_TRUE(coordinator.gate() != nullptr); + ASSERT_FALSE(coordinator.gateSatisfied()); + // A state-change latch opens the gate through the coordinator surface too coordinator.setStateChangeNotify(true); ASSERT_TRUE(coordinator.getStateChangeNotify()); - ASSERT_TRUE(coordinator.shouldInvokeCallback()); + ASSERT_TRUE(coordinator.gateSatisfied()); - // Consuming the latch closes the gate again coordinator.setStateChangeNotify(false); - ASSERT_FALSE(coordinator.getStateChangeNotify()); - ASSERT_FALSE(coordinator.shouldInvokeCallback()); + ASSERT_FALSE(coordinator.gateSatisfied()); } -TEST_F(NotificationCoordinatorTest, ClearReadinessLeavesStateChangeNotify) +TEST_F(NotificationCoordinatorTest, BeginOwnerPassMakesEpochNoisy) { - // clearReadiness drops ready/event bits (sync invalidated) but the state-change latch is a - // separate signal the owner consumes explicitly once the callback has fired. NotificationCoordinator coordinator(manualExecutor(), loggerComponent); - coordinator.resize(2); - coordinator.setStateChangeNotify(true); + const auto& gate = coordinator.gate(); - coordinator.clearReadiness(); - ASSERT_TRUE(coordinator.getStateChangeNotify()); - ASSERT_TRUE(coordinator.shouldInvokeCallback()); -} - -TEST_F(NotificationCoordinatorTest, ResizePreservesExistingBits) -{ - NotificationCoordinator coordinator(manualExecutor(), loggerComponent); - coordinator.resize(2); - coordinator.setUsed(1, false); - - coordinator.resize(4); - ASSERT_EQ(coordinator.getSlotCount(), 4u); - ASSERT_FALSE(coordinator.isUsed(1)); - ASSERT_TRUE(coordinator.isUsed(2)); // new slots default to used - ASSERT_FALSE(coordinator.shouldInvokeCallback()); // and to not-ready -} - -TEST_F(NotificationCoordinatorTest, NoSchedulerRunsInline) -{ - NotificationCoordinator coordinator(SchedulerPtr(nullptr), loggerComponent); - int evaluations = 0; - coordinator.setEvaluationCallback([&] { ++evaluations; }); - - coordinator.requestEvaluation(); - ASSERT_EQ(evaluations, 1); - - coordinator.requestEvaluation(); - ASSERT_EQ(evaluations, 2); + ASSERT_TRUE(CallbackGate::epochQuiet(gate->passEpoch())); + { + auto pass = coordinator.beginOwnerPass(); + ASSERT_FALSE(CallbackGate::epochQuiet(gate->passEpoch())); + } + ASSERT_TRUE(CallbackGate::epochQuiet(gate->passEpoch())); } diff --git a/core/opendaq/reader/tests/test_queue_reader.cpp b/core/opendaq/reader/tests/test_queue_reader.cpp index 28b8fdf1f4..af332373d7 100644 --- a/core/opendaq/reader/tests/test_queue_reader.cpp +++ b/core/opendaq/reader/tests/test_queue_reader.cpp @@ -307,7 +307,7 @@ TEST_F(QueueReaderTest, CreateBeforeConnection) std::unique_ptr domainValue = std::make_unique>(DomainInfo{std::chrono::system_clock::time_point{}, Ratio(1, 1000)}, 512); - bool valid; + bool valid = false; ASSERT_NO_THROW(valid = reader.isValid()); ASSERT_FALSE(valid); @@ -355,14 +355,14 @@ TEST_F(QueueReaderTest, CreateBeforeConnectionRecovery) std::unique_ptr domainValue = std::make_unique>(DomainInfo{std::chrono::system_clock::time_point{}, Ratio(1, 1000)}, 512); - bool valid; + bool valid = false; ASSERT_NO_THROW(valid = reader.isValid()); ASSERT_TRUE(valid); ASSERT_NO_THROW(reader.getDomainInfo()); ASSERT_NO_THROW(reader.getFirstSampleDomainValue()); ASSERT_NO_THROW(reader.advanceToDomainValue(domainValue.get())); - Int sr; + Int sr = 0; ASSERT_NO_THROW(sr = reader.getSampleRate()); ASSERT_EQ(sr, sampleRate); ASSERT_NO_THROW(reader.dropOutdatedPacketSegments()); @@ -400,7 +400,7 @@ TEST_F(QueueReaderTest, InvalidDomainAndBack) inputPort.connect(signal); reader.updateConnection(); - bool valid; + bool valid = false; ASSERT_NO_THROW(valid = reader.isValid()); ASSERT_FALSE(valid); diff --git a/core/opendaq/reader/tests/test_typed_reading.cpp b/core/opendaq/reader/tests/test_typed_reading.cpp index b2c9b90f43..c5c8584c02 100644 --- a/core/opendaq/reader/tests/test_typed_reading.cpp +++ b/core/opendaq/reader/tests/test_typed_reading.cpp @@ -148,7 +148,7 @@ TEST_F(TypedReadingTest, ExplicitRuleReadData) { void* bufferP = buffer.data(); daq::SizeT count = packetSize - i; - daq::ErrCode err = daq::TypedReadingUtils::readData( + [[maybe_unused]] daq::ErrCode err = daq::TypedReadingUtils::readData( daq::SampleType::UInt64, daq::SampleType::UInt64, true, readLayout, domainPacket.getRawData(), i, &bufferP, count); for (daq::SizeT j = 0; j < count; ++j) From 63bf33a82c93bd29ab2b620cd3fd582b8e5ba6c6 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Mon, 27 Jul 2026 09:26:57 +0200 Subject: [PATCH 14/15] fix clang build --- .../modules/ref_fb_module/src/sum_reader_fb_impl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp b/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp index 797bfddeb4..39a61a5ed8 100644 --- a/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp +++ b/examples/modules/ref_fb_module/modules/ref_fb_module/src/sum_reader_fb_impl.cpp @@ -710,7 +710,7 @@ bool SumReaderFbImpl::ensureRateModelLocked() return false; const auto commonRule = commonDomainDescriptor.getRule(); - const NumberPtr ruleStart = commonRule.assigned() ? commonRule.getParameters().get("start") : NumberPtr(0); + const NumberPtr ruleStart = commonRule.assigned() ? commonRule.getParameters().get("start").asPtr() : NumberPtr(0); sumDomainDataDescriptor = DataDescriptorBuilderCopy(commonDomainDescriptor) .setRule(LinearDataRule(static_cast(ticksPerCommonSample * static_cast(blockLcm)), ruleStart)) From 69e5faf6b1819ce635cdfc81c0270760c0dffc36 Mon Sep 17 00:00:00 2001 From: tomaz-cvetko Date: Tue, 28 Jul 2026 07:41:19 +0200 Subject: [PATCH 15/15] Reader: TickResolution in rationalGcd, copy the sync start candidate Continues the DomainInfo change (6f249397): rationalGcd now takes and returns TickResolution rather than RatioPtr, so buildCommonModelImpl can pass DomainInfo::resolution straight through instead of rebuilding a RatioPtr the callee immediately decomposes again. The unassigned-pointer guard becomes a zero-denominator guard. pickStartCandidate takes firstSamples by const reference and builds its candidate with toDomain instead of moving the element out of the caller's vector. The move was correct, but only because every read of firstSamples happened to precede it and because synchronize() rebuilds the vector on each retry round - an ordering constraint nothing enforced, and one that anything added to the tick search would have silently broken. It also left the caller holding a null slot on the TargetNotRepresentable and NoCommonTick paths, where the stolen value is destroyed rather than returned. The elements are already in the common domain, so the conversion is the identity; it costs one allocation on a path that runs once per sync round. RationalGcd now covers the default-constructed {0, 0} resolution, the value analogue of the unassigned RatioPtr the old guard rejected. Full reader suite passes (2089 tests); MultiReaderTest.OffsetToLinear remains flaky for the pre-existing reason, unrelated to either change. Co-Authored-By: Claude Opus 5 (1M context) --- .../multi_reader/synchronization_manager.h | 9 ++++--- .../multi_reader/synchronization_manager.cpp | 27 ++++++++++--------- .../tests/test_synchronization_manager.cpp | 22 ++++++++------- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h b/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h index 38e59cbac1..31991da738 100644 --- a/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h +++ b/core/opendaq/reader/include/opendaq/multi_reader/synchronization_manager.h @@ -135,7 +135,7 @@ class SynchronizationManager * the coarsest resolution every input resolution is an integer multiple of * (1/10 and 1/15 yield 1/30). nullopt on overflow or invalid ratio. */ - static std::optional rationalGcd(const std::vector& ratios); + static std::optional rationalGcd(const std::vector& ratios); /** * @brief Cross-input checks and (re)construction of the CommonModel. @@ -210,9 +210,10 @@ class SynchronizationManager std::optional checkSynchronizationDistance(const std::vector>& firstSamples, const std::vector& slotIndices) const; - /// synchronize() step 3: choose the tick every input should start on. The candidate is - /// moved out of @p firstSamples; the search itself is documented at the definition. - CandidatePick pickStartCandidate(std::vector>& firstSamples, + /// synchronize() step 3: choose the tick every input should start on. The candidate is an + /// independent copy, so @p firstSamples stays intact on every path (including the failure + /// ones); the search itself is documented at the definition. + CandidatePick pickStartCandidate(const std::vector>& firstSamples, const std::vector& slotIndices) const; /// synchronize() step 4: advance every input's cursor to the candidate and classify the diff --git a/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp b/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp index 1ef25c17f3..1024f97b2f 100644 --- a/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp +++ b/core/opendaq/reader/src/multi_reader/synchronization_manager.cpp @@ -93,7 +93,7 @@ std::optional SynchronizationManager::checkedLcm(std::int64_t a, s return checkedMultiply(a / gcd, b); } -std::optional SynchronizationManager::rationalGcd(const std::vector& ratios) +std::optional SynchronizationManager::rationalGcd(const std::vector& ratios) { if (ratios.empty()) return std::nullopt; @@ -104,11 +104,8 @@ std::optional SynchronizationManager::rationalGcd(const std::vector SynchronizationManager::rationalGcd(const std::vector& inputs, @@ -236,15 +233,15 @@ SyncSetupResult SynchronizationManager::buildCommonModelImpl(const std::vectorgetDomainInfo().epoch; - std::vector resolutions; + std::vector resolutions; resolutions.reserve(count + 1); for (SizeT i = 0; i < count; ++i) { const auto& domainInfo = inputs[i]->getDomainInfo(); commonEpoch = std::min(commonEpoch, domainInfo.epoch); - resolutions.push_back(Ratio(domainInfo.resolution.num, domainInfo.resolution.den)); + resolutions.push_back(domainInfo.resolution); } - resolutions.push_back(Ratio(1, commonRate)); + resolutions.push_back(TickResolution{1, commonRate}); const auto commonResolution = rationalGcd(resolutions); if (!commonResolution) @@ -321,7 +318,7 @@ std::optional SynchronizationManager::checkSynchronizationDistance( } SynchronizationManager::CandidatePick SynchronizationManager::pickStartCandidate( - std::vector>& firstSamples, const std::vector& slotIndices) const + const std::vector>& firstSamples, const std::vector& slotIndices) const { const SizeT count = firstSamples.size(); @@ -353,7 +350,13 @@ SynchronizationManager::CandidatePick SynchronizationManager::pickStartCandidate } } - auto candidate = std::move(firstSamples[latestIndex]); + // An independent copy, not the element itself: the candidate is mutated below + // (roundUpOnDomainInterval / shiftTicks) and is destroyed outright on the failure paths, so + // owning it separately keeps firstSamples valid for the caller and removes the ordering + // hazard that a stolen element would impose on anything added after this point. The elements + // are already in the common domain (collectFirstSamples converted them), so this conversion + // is the identity and costs one small allocation on a path that runs once per sync round. + auto candidate = firstSamples[latestIndex]->toDomain(model.commonDomain); if (!ticksKnown) { // Fallback: tick values unavailable (unusual domain read type) or a full-unit start diff --git a/core/opendaq/reader/tests/test_synchronization_manager.cpp b/core/opendaq/reader/tests/test_synchronization_manager.cpp index c63aea3c0a..352b61207d 100644 --- a/core/opendaq/reader/tests/test_synchronization_manager.cpp +++ b/core/opendaq/reader/tests/test_synchronization_manager.cpp @@ -134,24 +134,26 @@ TEST_F(SyncManagerTest, CheckedArithmetic) TEST_F(SyncManagerTest, RationalGcd) { - const auto gcd1 = SynchronizationManager::rationalGcd({Ratio(1, 10), Ratio(1, 15)}); + const auto gcd1 = SynchronizationManager::rationalGcd({TickResolution{1, 10}, TickResolution{1, 15}}); ASSERT_TRUE(gcd1.has_value()); - ASSERT_EQ((*gcd1).getNumerator(), 1); - ASSERT_EQ((*gcd1).getDenominator(), 30); + ASSERT_EQ(gcd1->num, 1); + ASSERT_EQ(gcd1->den, 30); - const auto gcd2 = SynchronizationManager::rationalGcd({Ratio(1, 1000), Ratio(1, 1000)}); + const auto gcd2 = SynchronizationManager::rationalGcd({TickResolution{1, 1000}, TickResolution{1, 1000}}); ASSERT_TRUE(gcd2.has_value()); - ASSERT_EQ((*gcd2).getNumerator(), 1); - ASSERT_EQ((*gcd2).getDenominator(), 1000); + ASSERT_EQ(gcd2->num, 1); + ASSERT_EQ(gcd2->den, 1000); // Unreduced input: 2/10 reduces to 1/5, gcd(1/5, 1/15) = 1/15 - const auto gcd3 = SynchronizationManager::rationalGcd({Ratio(2, 10), Ratio(1, 15)}); + const auto gcd3 = SynchronizationManager::rationalGcd({TickResolution{2, 10}, TickResolution{1, 15}}); ASSERT_TRUE(gcd3.has_value()); - ASSERT_EQ((*gcd3).getNumerator(), 1); - ASSERT_EQ((*gcd3).getDenominator(), 15); + ASSERT_EQ(gcd3->num, 1); + ASSERT_EQ(gcd3->den, 15); ASSERT_EQ(SynchronizationManager::rationalGcd({}), std::nullopt); - ASSERT_EQ(SynchronizationManager::rationalGcd({Ratio(0, 10)}), std::nullopt); + ASSERT_EQ(SynchronizationManager::rationalGcd({TickResolution{0, 10}}), std::nullopt); + // Default-constructed {0, 0}: a resolution that was never set must not be treated as valid + ASSERT_EQ(SynchronizationManager::rationalGcd({TickResolution{}}), std::nullopt); } // --- Common model construction (spec section 4) ---