Simplify the FB interface - #1
Open
tomaz-cvetko wants to merge 15 commits into
Open
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Continues the DomainInfo change (6f24939): 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brief
(Concise one-line description of what changed and what was added/removed, maybe even why)
Description
(Individual high-level changes)
Usage example
In C++ use:
In terminal use:
cd folderAPI changes
Note
Modifying, removing, or adding a function to an interface inherited by another, breaks binary compatibility of module shared libraries
(An overview of changes on the interface level (abstract structs with pure virtual functions), if any, one function per line)
Required application changes
(Changes required in openDAQ applications/executables)
Instead of:
foo();Do:
bar();Required module changes
(Changes required in openDAQ shared libraries/modules)
Instead of:
foo();Do:
bar();