Repository navigation
Generic client: single sampling scheduler and monitored item fixes - #36
Open
viacheslauK wants to merge 13 commits into
Open
viacheslauK wants to merge 13 commits into
viacheslauK wants to merge 13 commits into
Conversation
…rence and usage steps
viacheslauK
changed the title
…ructor to prevent use after destruction
viacheslauK
force-pushed
the
improvements-for-generic-client
branch
from
August 28, 2026 19:10
8e76917 to
c3752de
Compare
…ependent, drop macOS skips
viacheslauK
marked this pull request as ready for review
August 31, 2026 05:56
viacheslauK
requested review from
JakaMohorko,
NikolaiShipilov and
denise-opendaq
August 31, 2026 05:57
denise-opendaq
approved these changes
Sep 14, 2026
…#41) The device now holds only defaults for new blocks: DefaultTimestampMode and DefaultSamplingInterval. Each MonitoredItem has its own TimestampMode and SamplingInterval properties, which start from those defaults. None of these properties is part of a config. They exist only as properties after the object is created.
JakaMohorko
reviewed
Oct 2, 2026
| sampler.stop(); | ||
| } | ||
|
|
||
| PropertyObjectPtr OpcuaGenericClientDeviceImpl::createDefaultConfig() |
Contributor
There was a problem hiding this comment.
The removal of any property (even in config) should be explicitly stated in the PR description, as it's a breaking change.
Comment on lines
+242
to
+243
| server's own sampling and publishing settings do not apply. Every successful read publishes a sample, | ||
| even when the value has not changed — there is no deadband or change filter. |
Contributor
There was a problem hiding this comment.
Does this apply even if the timestamps are equal? We should probably avoid sending packets with repeated timestamps.
Contributor
Author
There was a problem hiding this comment.
Yes, even if the timestamps are equal
|
|
||
| const auto samplingInterval = | ||
| readProperty<Int, IInteger>(objPtr, PROPERTY_NAME_OPCUA_DEFAULT_SAMPLING_INTERVAL, DEFAULT_OPCUA_MIFB_SAMPLING_INTERVAL); | ||
| if (samplingInterval <= 0 || samplingInterval > static_cast<Int>(std::numeric_limits<uint32_t>::max())) |
Contributor
There was a problem hiding this comment.
This should be added as a min/max attribute of the SamplingInterval property.
| { | ||
| std::unique_lock lock(mutex); | ||
| items.erase(std::remove_if(items.begin(), items.end(), [item](const Entry& e) { return e.item == item; }), items.end()); | ||
| inFlightCv.wait(lock, [this, item] { return inFlight != item; }); |
Contributor
There was a problem hiding this comment.
This can deadlock during removed() if reconnection happens at the same time as destruction.
| .setUnit(Unit("s", -1, "seconds", "time")) | ||
| .setTickResolution(Ratio(1, 1'000'000)) | ||
| .setOrigin("1970-01-01T00:00:00Z") | ||
| .setName("Time") |
Contributor
There was a problem hiding this comment.
We should not set the name of data descriptors that are not struct types.
| @@ -418,7 +465,7 @@ void OpcUaMonitoredItemFbImpl::createSignal() | |||
|
|
|||
| void OpcUaMonitoredItemFbImpl::reconfigureSignal(const FbConfig& prevConfig) | |||
Contributor
There was a problem hiding this comment.
Seems like prevConfig is now an usused parameter.
Comment on lines
+37
to
+40
| | `DeviceNodeIDType` | Selection | `1` — `String` | | ||
| | `DeviceNodeIDString` | String | `""` | | ||
| | `DeviceNodeIDNumeric` | Int | `0` | | ||
| | `DeviceNamespaceIndex` | Int | `0` | |
Contributor
There was a problem hiding this comment.
As discussed, these should be a fallback in case the DeviceSet is not found / has no child objects of DeviceType.
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
3 participants
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.
Summary
Replaces the reader thread that each
OpcUaMonitoredItemFbImplused to own with oneSamplingSchedulerper device, and fixes several monitored-item defects found along the way.Changes
SamplingScheduler(new) drives every monitored item of a device from one thread, each item keeping its own deadline.OpcUaDataValue::isDateTime(); DateTime values now build a packet and are read asUA_DateTimeinstead ofreadScalar<UA_Int64>, including theUtcTimesubtype.SamplingIntervalrejects negative and above-uint32_tvalues; reads reject a null value, report a failed packet build, and catchstd::exception/...beyondOpcUaException.test_sampling_scheduler.cpp; device and monitored-item suites extended; test server publishes.dtand.utcnodes;USAGE.mdadds a property reference and walkthrough.