Reader review branch by JakaMohorko · Pull Request #1 · JakaMohorko/openDAQ

JakaMohorko · 2026-07-17T06:15:45Z

Review PR

JakaMohorko

Will continue from the queue reader onwards in a separate review.

JakaMohorko · 2026-07-17T06:26:20Z

+struct DomainInfo
+{
+    std::chrono::system_clock::time_point epoch;
+    RatioPtr resolution;


I wonder if we could use std::ratio internally. The openDAQ ratio object isn't really the most ideal for calculations.

We could but this would mean that we duplicate some functionality that already exist and overload it for std::ratio (seemed like it's not ideal so I haven't commited to that yet).

JakaMohorko · 2026-07-17T06:29:33Z

+        using SysPeriod = std::chrono::system_clock::period;
+        Int scaleNumerator = SysPeriod::num * commonDomain.resolution.getDenominator();
+        Int scaleDenominator = SysPeriod::den * commonDomain.resolution.getNumerator();
+        Int offsetFromCommon = epochOffset * scaleNumerator / scaleDenominator;


This calculation should be done somewhat carefully to not result in integer overflow. Also, when dividing by scaleDenominator, can this result in a non-integer number?

Very unlikely. I think it is reasonable to expect that the later epoch is representible in ticks of common resolution. For example, epochs that are whole seconds satisfy this for all resolutions that are divisors of a unit.

JakaMohorko · 2026-07-17T06:32:06Z

+        using SysPeriod = std::chrono::system_clock::period;
+        Int scaleNumerator = SysPeriod::num * commonDomain.resolution.getDenominator();
+        Int scaleDenominator = SysPeriod::den * commonDomain.resolution.getNumerator();
+        Int offsetFromCommon = epochOffset * scaleNumerator / scaleDenominator;
+        Type valueScaledToCommon = valueInCommon - offsetFromCommon;
+
+        Int multiplierNumerator = regularDomain.resolution.getNumerator() * commonDomain.resolution.getDenominator();
+        Int multiplierDenominator = regularDomain.resolution.getDenominator() * commonDomain.resolution.getNumerator();


This calculation is duplicated from above if I'm not mistaken. Could be merged into a helper method. It seems like the bottom code is the same.

Yes, we could make a helper out of this

JakaMohorko · 2026-07-17T06:34:11Z

+        if (den % num != 0)  // 1 = k * num/den, the resolution is a fractional divider of a unit
+            DAQ_THROW_EXCEPTION(NotSupportedException, "Resolution must be aligned on full unit of domain");


The exception here seems like it'll be hard to catch. It might be prudent to at least define new error codes here. If the user receives this error, it'll be hard to debug, especially as this is not thrown in case of bugs, but in case of incompatible signal connections.

It might be worth re-thinking a system for error handling in case of incompatibilities. Throwing exceptions feels a bit overkill.

JakaMohorko · 2026-07-17T06:35:42Z

+        // Offset of current domain in common domain ticks
+        Int epochOffset = domain.epoch.time_since_epoch().count() - commonDomain.epoch.time_since_epoch().count();
+
+        using SysPeriod = std::chrono::system_clock::period;
+        Int scaleNumerator = SysPeriod::num * commonDomain.resolution.getDenominator();
+        Int scaleDenominator = SysPeriod::den * commonDomain.resolution.getNumerator();
+        RangeValue offsetFromCommon = static_cast<RangeValue>(epochOffset * scaleNumerator / scaleDenominator);
+
+        // tick_common = tick * multiplier
+        Int multiplierNumerator = domain.resolution.getNumerator() * commonDomain.resolution.getDenominator();
+        Int multiplierDenominator = domain.resolution.getDenominator() * commonDomain.resolution.getNumerator();


Feels like a really similar calculation as above again.

JakaMohorko · 2026-07-17T07:24:04Z

+
+BEGIN_NAMESPACE_OPENDAQ
+
+SignalEvent::SignalEvent(const EventPacketPtr& packet)


I think merging events is a good approach. I would potentially revisit the reader status API to see if there's a better way of portraying the events instead of forwarding event packets.

Would be cool, but with the object having to be an API interface implementation and event packets being canonical, the reason for change would have to be strong.

JakaMohorko · 2026-07-17T07:37:59Z

+    typeCtx.domainIn = SampleType::Undefined;
+    typeCtx.domainOut = domainReadType;
+    typeCtx.valueIn = SampleType::Undefined;
+    typeCtx.valueOut = mode == ReadMode::RawValue ? SampleType::Undefined : valueReadType;


As mentioned above, we need to clarify how the read mode and value read type interact (and that Unscaled actually does nothing).

JakaMohorko · 2026-07-17T07:41:04Z

+    // TODO: Add first timestamp mechanism for sync tolerance checking
+    checkConnection();
+    drainConnection();


Could this be a separate step that's potentially called externally? It seems that this needs to be called before any read operation.

The flow will likely be:

User API call

Check "are readers ready?"

Read data with queue readers

Between 2 and 3, we could update the reader state, then read whatever's there. It might even be better that the connection state is not updated within a "read" or "getAvailable" call.

JakaMohorko · 2026-07-17T07:50:02Z

+void QueueReader::checkConnection() const
+{
+    if (!connection.assigned())
+        DAQ_THROW_EXCEPTION(InvalidOperationException, "Connection must be assigned for this operation.");
+}


This is probably a placeholder, but the design should be done so that this can not be triggered.

JakaMohorko · 2026-07-17T07:53:40Z

+SizeT QueueReader::getAvailableSamples()
+{
+    return getAvailableSamplesNative() * sampleRateDivider;
+}


As mentioned above, I think we're somewhat mixing these calculations and doing them both internally in the queue reader and externally.

The user has to allocate buffers in "native" domain. To do this, they will have to use dividers either provided by the reader or calculated manually - this is inconvenient. The reader is only capable of communicating the common domain and a single available count. The only choice here is whether QueueReader does this or the ReadCoordinator will do this for all signals. Being aware of a single divider seems less messy than bookkeeping all dividers to me.

JakaMohorko commented Jul 17, 2026

View reviewed changes

JakaMohorko force-pushed the reader-review-branch branch 2 times, most recently from 1b6c21b to 267a93a Compare July 17, 2026 15:57

Reader-rework

353d63e

JakaMohorko force-pushed the reader-review-branch branch from 267a93a to 353d63e Compare July 17, 2026 15:58

		if (den % num != 0) // 1 = k * num/den, the resolution is a fractional divider of a unit
		DAQ_THROW_EXCEPTION(NotSupportedException, "Resolution must be aligned on full unit of domain");


		BEGIN_NAMESPACE_OPENDAQ

		SignalEvent::SignalEvent(const EventPacketPtr& packet)

Conversation

JakaMohorko commented Jul 17, 2026

Uh oh!

JakaMohorko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants