Reference from devices pane - #24
Conversation
The frame rate compatibility warning was dead code until now, so it starts firing where it previously never did.
There was a problem hiding this comment.
Reviewed against the settings and device subsystems and the engine's runner/plugin threading, since this change makes something outside the graph drive node state. The Firmware Info fix and the settings-callback hardening are correct, and the devices pane is the right home for the reference. Two items need addressing: the reference listener mutates node state from a non-runner thread, and the frame rate warning fires permanently on Free Run. Two more have no line to anchor to: WaitVBLNode.cpp:73 still says "Check reference source property", which no longer exists, and the Firmware Info commit has an empty body for a bug that is not visible in the diff.
| void SubscribeToReference() | ||
| { | ||
| if (!Device || ReferenceListenerId) | ||
| return; | ||
| ReferenceListenerId = Device->AddReferenceSourceListener([this](NTV2ReferenceSource) { UpdateReferenceSource(); }); | ||
| UpdateReferenceSource(); |
There was a problem hiding this comment.
The listener runs on the thread that called SetReference, which for a devices pane change is the plugin manager thread (OnMessageFromEditor -> UpdateSettingsCallback -> SetReference), while node state is owned by the runner thread (PathRunner.cpp:537,557). UpdateReferenceSource() therefore writes Channel::StatusMessages concurrently with Channel::Open/Close on the runner, and UpdateStatus() iterates that same map. A lock is the wrong tool: the callback does driver register reads, and the runner reaches the same path from IncrementDropCount on the per-frame drop path. UpdateDeviceProperties can publish the reference on the device instead, and nosEngine.SetNodeDirty(NodeId) returns the per-node warning to the runner via EnqueueOrCallNodeDirtied (RunnerRouter.cpp:68-98).
| CurrentChannel.SetStatus(aja::Channel::StatusType::Reference, fb::NodeStatusMessageType::INFO, | ||
| "Reference: " + Device->ReferenceSourceToString(curRef), "", 0, false); | ||
|
|
||
| if (IsInput) | ||
| return; | ||
|
|
||
| if (GetFrameRateFamily(refFrameRate) != GetFrameRateFamily(FrameRate)) |
There was a problem hiding this comment.
GetReferenceAndFrameRate has no case NTV2_REFERENCE_FREERUN, so refFrameRate stays NTV2_FRAMERATE_UNKNOWN and GetFrameRateFamily returns NTV2_FRAMERATE_INVALID, which never matches a real family. Every output channel on a Free Run device will show "Reference incompatible with frame rate" permanently. IsExternallySynced (WaitVBLNode.cpp:336) already treats FREERUN and an unknown reference rate as their own case rather than a mismatch. This also warns spuriously at load, since SubscribeToReference runs from the Device pin watcher while FrameRate is still INVALID.
| return; | ||
| device->RemoveReferenceSourceListener(*ReferenceListenerId); | ||
| ReferenceListenerId.reset(); | ||
| CurrentChannel.ClearStatus(Channel::StatusType::Reference); |
There was a problem hiding this comment.
ReferenceInvalid is not cleared here. Setting the Device pin to None unsubscribes and leaves Device null, so nothing re-evaluates the warning and it stays on the node with no device attached.
| channelPin.is_interlaced = !IsProgressivePicture(format); | ||
| CurrentChannel.Update(std::move(channelPin), true); | ||
| // The frame rate may have changed, so re-check it against the reference. | ||
| UpdateReferenceSource(); |
There was a problem hiding this comment.
TryUpdateChannel returns early when !ShouldOpen and when the format is UNKNOWN, so neither the reference line nor the warning is refreshed on those paths and the previous status persists.
| // Remember the selection first, so that a later ClearState() restores it instead of the default. | ||
| SelectedReference = referenceSource; | ||
|
|
||
| NTV2ReferenceSource curRef{}; | ||
| if ((!GetReference(curRef) || curRef != referenceSource) && !SetReference(referenceSource)) | ||
| { | ||
| nosEngine.LogE("Device %s: Failed to set reference source to '%s'", GetDisplayName().c_str(), referenceValue.c_str()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
SelectedReference is assigned before the card accepts the value, so a refused reference is still stored and re-applied by the next ClearState(); assigning after SetReference succeeds avoids that. Separately, when the card already holds the requested reference SetReference is skipped and listeners are not notified, which makes the notification non-idempotent.
| // Reporting failure makes the settings subsystem keep the default value instead of persisting | ||
| // a reference the card never accepted. | ||
| if (!device->second->UpdateReferenceSource(nos::InterpretObjectData<const char>(itemValue), false)) | ||
| return NOS_RESULT_FAILED; |
There was a problem hiding this comment.
The behaviour is correct: OnMessageFromEditor skips UpdateEntry and logs "last value remains" when the callback fails (EntryPoint.cpp:120). The comment says default, but what the subsystem keeps is the last value.
| ReferenceListeners.Map.erase(id); | ||
| } | ||
|
|
||
| bool AJADevice::SetReference(const NTV2ReferenceSource inRefSource, const bool inKeepFramePulseSelect) |
There was a problem hiding this comment.
Dispatching under the lock is load-bearing: RemoveReferenceSourceListener takes the same mutex, so ~ChannelNode on the runner blocks until an in-flight callback finishes, which is what keeps the captured this alive. Worth stating in a comment so the dispatch is not moved out of the lock later.
| // Must outlive the RegisterDevice call below: driverProp.Value is a borrowed pointer into it. | ||
| std::string driverPropMessage; | ||
| nosDeviceProperty driverProp{}; | ||
| bool isFirmwareValid = true; | ||
| if (!CheckFirmware(firmwareMsg, firmwareMsgDetails)) { | ||
| std::string driverPropMessage = firmwareMsg + "\n Details: " + firmwareMsgDetails; | ||
| driverPropMessage = firmwareMsg + "\n Details: " + firmwareMsgDetails; | ||
| driverProp = {.Name = nos::Name("Firmware Info"), .Value = driverPropMessage.c_str()}; |
There was a problem hiding this comment.
Correct fix. DeviceManager::RegisterDevice copies Value into a std::string (sys-device/Source/DeviceSubsystem.cpp:93), so the previous block-scoped string was destroyed before the subsystem read it.
| NTV2ReferenceSource parsed{}; | ||
| if (device->ParseReferenceSource(refValue, parsed)) | ||
| device->UpdateReferenceSource(refValue, true); |
There was a problem hiding this comment.
Skipping "None" here avoids an error log for a value that never named a reference. The value is parsed twice though, once for the guard and once inside UpdateReferenceSource; a non-logging variant would avoid the second pass.
2 participants
No description provided.