fix(devices): ulanzi-start/stop work on Windows and Linux, and a query that cannot be answered says so

[nigelfenton]

Closes #5242.

devices ulanzi-start / ulanzi-stop returned ok: true on Windows and Linux while doing
nothing at all — the handler body was wrapped in #ifdef Q_OS_MAC and the #else branch
was a cheerful stub. A caller checking ok, the obvious field, read success from a call
that never reached the backend; only the separate supported: false said otherwise.

Two problems with different answers

Lifecycle is cross-platform. start() and stop() exist identically on all three
backends — EvdevEncoderManager, UlanziDialWindowsManager, UlanziDialMacOSManager —
and on the no-op fallback used for Windows builds without hidapi. Gating them on Q_OS_MAC
withheld working control from the two platforms the dial is most used on, while the verb's
own help has always advertised "diagnostics and lifecycle control" on every platform.
They now run everywhere.

The snapshot is not. diagnostics() really is macOS-only, so a bare devices ulanzi
query still cannot be answered on Linux/Windows. That now returns ok: false with an error
naming what is supported here, rather than reporting success for a question the platform
cannot answer.

The calls are queued, not direct

On Linux and Windows the backend is moved to the ExtControllers thread
(MainWindow_Controllers.cpp), so calling start()/stop() straight from the bridge's
thread would drive hidapi and evdev from the wrong one. Every other call site in the app
already uses QMetaObject::invokeMethod(..., Qt::QueuedConnection); the macOS-only
original could call directly only because macOS keeps the backend on the main thread for
its CFRunLoop.

Because the call is queued it has not run when the reply is built, so the reply says
queued: true rather than reporting an isConnected() describing the state before the
request. Reporting pre-call state would read as a failed command and would recreate, in a
new place, exactly the ok-means-nothing confusion this change removes.

The null-backend check now also applies on Linux and Windows; previously only the macOS
branch made it.

Verification, and its limits

Verified live on Windows 11 (MSVC, Qt 6.10.3), through the automation bridge against a
running app:

call	before	after
devices ulanzi	ok: true	ok: false + error naming what is supported
devices ulanzi-start	ok: true, nothing ran	ok: true, queued: true, dispatched
devices ulanzi-stop	ok: true, nothing ran	ok: true, queued: true, dispatched

Full app builds and links on Windows; I confirmed the binary postdates the edit rather than
trusting the exit code.

Not verified: that start()/stop() visibly change scanning state on hardware. Being
explicit about why, because it is not for want of trying.

The only dial here is a BLE-paired D100H, which is the unsupported OEM variant from
#5212/#3485: hid_enumerate() reports it as KEHWIN / Dial_Lite across five collections,
and kProductMatch (L"Ulanzi Dial") matches none of them, so connectionChanged never
fires and the mapper's status label cannot move — with or without this change, and with
Ulanzi Studio running. Per the recon in #5212 the KEHWIN firmware cannot be driven over
this backend at all.

Compiled on Linux/aarch64 — Raspberry Pi 5, Debian 13, g++ 14.2.0, Qt 6. Zero errors,
no warnings on the changed file, and UlanziDialBackend resolves to EvdevEncoderManager
there, so the real backend was compiled against rather than the no-op fallback. Worth doing
separately from CI because this #ifdef branch has never been built with start()/stop()
outside it, and the queued-invocation change is specifically for the two platforms that move
the backend to the ExtControllers thread.

I also tried to exercise the evdev path live on that Pi, and could not: the dial does not
enumerate over USB there on either port, with a known-good cable and zero enumeration
errors in the kernel log — the port presents no USB data device, consistent with the unit
being a BLE peripheral.

So: the response contract is proven, the dispatch is proven, and the scanning behaviour
behind it is not. A reviewer with a genuine Ulanzi Dial — or a Linux box with one
attached — can close that gap in a minute. It would be easy to imply more coverage than
that, and the whole point of this issue is a call that claimed success it had not earned.

Not in scope

No diagnostics() for the Linux/Windows backends (item 3 of the issue). It needs a
decision about what a common snapshot should contain, and #5212 — still open — adds the
unsupported-variant state that belongs in it. Doing it here would conflict with that PR and
pre-empt the design question.

[aethersdr-agent]

Issue fit

Partially. This is #5242, and it implements the issue's suggested fix (1) + (2) + (3) faithfully: ok:false for the unanswerable query, start/stop hoisted out of #ifdef Q_OS_MAC, lifecycle reporting ok:true with the operation. Item (4) — a reduced cross-platform diagnostics() — is explicitly deferred, which the issue framed as a separate design call, so leaving it out is right.

What it also does, which the issue did not ask for: it converts the macOS lifecycle call from direct to Qt::QueuedConnection. That is where the problem is (Blocker 1). The Linux/Windows half of the change looks correct to me and is the part the issue was actually about.

Scope

File / group	What it changes	Claimed by title/body?	Verdict
src/gui/MainWindow_Session.cpp — new #include "core/UlanziDialBackend.h"	complete type + alias on every platform	implied by the fix	In scope. Needed: the file only had core/UlanziDialMacOSManager.h under #if defined(Q_OS_MAC) (line 43-45), and MainWindow.h:177 only forward-declares.
same file — m_dialBackend null guard hoisted above the #ifdef	guard now applies on all platforms	yes	In scope. Side effect worth noting in the body: on Linux/Windows a devices ulanzi query with no backend now answers "Ulanzi backend unavailable" instead of the macOS-only message. Fine, but it's a third response shape.
same file — lifecycle hoisted + queued	the headline fix	yes	In scope; see Blocker 1 for the macOS half.
same file — non-mac response reshaped (ok:false for query; operation/supported/queued/enabled for lifecycle)	yes	In scope, but this is new/changed public bridge surface — see Blocker 2.
docs/automation-bridge.md	not touched	—	Absence: the documented contract at docs/automation-bridge.md:2363-2372 is now wrong. See Blocker 2.

No unrelated files, no CHANGELOG entry (correct — it must not have one), no build/CI churn, no UI or default changes, so §6 preference check is clean: this is a fix pointing at a filed issue with a repro, not a smuggled preference.

Blockers

1. On macOS the snapshot is now taken before the queued start/stop runs, so devices ulanzi-stop reports pre-stop state. (inline at src/gui/MainWindow_Session.cpp:2654, consequence at :2670)

AutomationServer is constructed on the main thread (MainWindow_Session.cpp:2563) and never moved — its only entry points are QLocalServer::newConnection (AutomationServer.cpp:2253) and QLocalSocket::readyRead (:2520), with no thread pool or QtConcurrent in the file — so m_deviceDiagnosticsHandler runs on the main thread. On macOS the backend deliberately stays on the main thread (MainWindow_Controllers.cpp:2791-2796 skips moveToThread under Q_OS_MAC). Caller and receiver are therefore the same thread, and Qt::QueuedConnection posts the call to the event loop rather than running it — so m_dialBackend->diagnostics() at line 2670 executes first and returns the state before the operation.

That breaks a contract the docs state outright, docs/automation-bridge.md:2366-2367:

A successful stop reports restorationStatus:"success", systemEventsSuppressed:false, and eventSystemClientRetained:false.

After this change those fields describe the still-running backend. It also matters for #5212, whose review blocker was "stop() clears the variant state and the advisory is withdrawn" — verified through this very snapshot.

The PR's own comment at 2681-2685 identifies exactly this hazard ("Reporting isConnected() here would describe the state BEFORE the request") and guards the non-mac branch against it — the macOS branch was missed.

Qt::AutoConnection fixes it in one word and keeps everything the PR wants: main→main on macOS resolves to direct (old semantics, fresh snapshot); main→ExtControllers on Linux/Windows resolves to queued (the thread-safety the comment correctly argues for). Suggestion fence inline.

2. The non-macOS wire contract changed and docs/automation-bridge.md still documents the old one — and the ok flip is a compatibility break for existing callers. (see the scope table; the doc file is not in the diff, so this is a body finding rather than an inline anchor)

docs/automation-bridge.md:2368-2372 currently reads:

The read-only diagnostic is available in Observe only mode; none of these actions keys the transmitter. On non-macOS platforms it returns supported:false because those backends do not use the affected IOKit claim path.

After this PR, on Linux/Windows: devices ulanzi returns ok:false, the message key is gone and replaced by error, lifecycle calls gain operation, supported, enabled, and a brand-new queued field, and the stated reason for supported:false is no longer the IOKit claim path but the absence of diagnostics(). docs/automation-bridge.md:3815 (the verb table) is unaffected.

Two things follow. First, the docs need updating in this PR — the bridge is a third-party-facing protocol and this file is its contract. Second, flipping ok from true to false on a verb that has shipped is a deliberate break of any script that checks ok; I think it is the right break (it is the whole point of the issue, and CONTRIBUTING/#5242 both argue for loud refusal over a cheerful stub), but "new field queued + changed ok semantics on a shipped verb" is a maintainer call per §3, not something a reviewer waves through. Flagging it as one rather than as a defect.

Nits

• No test covers the changed code. tests/automation_device_diagnostics_test.cpp installs a stub handler and exercises only AutomationServer::doDeviceDiagnostics dispatch — it would pass identically against unfixed main, and against this PR, and against a version where the #else branch returns ok:true again. That is not a criticism of the test (it was written for the dispatcher), just an observation that this fix ships with zero regression pinning. The lambda lives in MainWindow so a direct test is awkward; if the eventual (4) work extracts the response-shaping into a free function, that would be the moment to pin it.
• The comment block at 2637-2653 runs two distinct arguments (cross-platform lifecycle; queued-vs-direct) together with no blank line between them, and the second argument is the one that turns out to be over-broad. Worth a blank line and a "except on macOS, where caller and receiver share a thread" clause once Blocker 1 is addressed.
• enabled is now computed before both branches even though the query branch on non-mac returns early without it. Harmless (one settings read), just slightly wasted on the refusal path.

What I tried to break

• The "start()/stop() exist identically on all three backends" claim — confirmed against the head checkout: EvdevEncoderManager.h, UlanziDialWindowsManager.h, UlanziDialMacOSManager.h, plus the inline no-op fallback class in UlanziDialBackend.h. diagnostics() greps to exactly one hit, UlanziDialMacOSManager.h:34. The claim holds.
• The "every other call site in the app uses this form" claim — holds: MainWindow.cpp:3305/:3308 and MainWindow_Controllers.cpp:2947 all use invokeMethod(m_dialBackend, &UlanziDialBackend::start, Qt::QueuedConnection). It is precedent — but all three of those are fire-and-forget with no immediately-following read of the result, which is what makes copying the form onto a path that reads diagnostics() two lines later the defect in Blocker 1.
• The alias/type mismatch — MainWindow.h:1259-1265 declares m_dialBackend as the concrete per-platform type, not the alias, so &UlanziDialBackend::start could in principle be a different member. It isn't: the alias resolves to that same concrete class on each platform. No finding, and CI compiling on all three platforms corroborates it.
• The #else/no-hidapi fallback backend — its start()/stop() are plain inline non-slots, but the pointer-to-member invokeMethod overload goes through the functor path, so no Q_INVOKABLE is required. check-windows is green, which is where that configuration would surface.
• Teardown / null-backend reentrancy — m_dialBackend is deleted only in the shutdown path (MainWindow.cpp:2758-2764), after m_extCtrlThread has joined; the automation bridge is torn down in the same sequence. A queued start outliving the object would need the bridge to service a request after the thread join, which I could not construct from the code. The hoisted null guard covers the ordinary case.
• Bypassing UlanziDialEnabled — devices ulanzi-start starts scanning regardless of the setting (and on macOS trips the Input Monitoring TCC prompt #3257 guards against). Not a finding: that was equally true before this PR on macOS, and the verb is gated behind write-mode in AutomationServer.cpp:11816.

CI is green on all four checks for d63d92f (build, check-windows, check-macos, Static checks) — worth noting that per the project's -R-filtered ctest, none of those runs a test that touches this handler, so green here means "it compiles everywhere", which is genuinely the thing I most wanted to know about the new include and the PMF overload at the Qt floor.

All findings above are reasoned from reading the code in the head checkout. I am headless: no build, no ctest, no bridge to drive, and no Ulanzi Dial. In particular I did not observe the stale macOS snapshot in Blocker 1 — I derived it from the thread affinities at MainWindow_Controllers.cpp:2791 and MainWindow_Session.cpp:2563. If you disagree that the handler runs on the main thread, that single premise is what settles it.

Recommendation

Request changes, on Blocker 1 only — it is a one-word change (Qt::QueuedConnection → Qt::AutoConnection) that costs nothing and preserves every property the PR argues for, while restoring the macOS snapshot freshness the docs promise. Blocker 2 splits: the docs update belongs in this PR, and the ok-flip/queued-field question belongs to a maintainer, though I'd expect them to agree with the change since #5242 argues for it directly.

Genuinely good work — the diagnosis in #5242 was precise, the fix matches it, and the comments carry the reasoning rather than restating the code. The macOS branch is the one place the new reasoning didn't get re-applied to the old path. Thanks for taking this one on.

---

_{🤖 aethersdr-agent · cost: $6.1992 · model: claude-opus-5}

[aethersdr-agent]

Blocker: this makes the macOS lifecycle call asynchronous, so the snapshot returned two lines below describes the state before the operation.

AutomationServer is created on the main thread (MainWindow_Session.cpp:2563), never moveToThread'd, and its only entry points are QLocalServer::newConnection / QLocalSocket::readyRead — so this handler runs on the main thread. On macOS the backend also stays on the main thread (MainWindow_Controllers.cpp:2791-2796 skips moveToThread under Q_OS_MAC, for the CFRunLoop reason your comment cites). Same thread + Qt::QueuedConnection = the call is posted to the event loop and has not run when m_dialBackend->diagnostics() executes at line 2670.

That breaks what docs/automation-bridge.md:2366-2367 promises: "A successful stop reports restorationStatus:"success", systemEventsSuppressed:false, and eventSystemClientRetained:false." It also matters for #5212, whose review blocker is verified through exactly this snapshot.

Your comment at 2681-2685 spots this hazard for the non-mac branch and guards it; the macOS branch just needs the same care. Qt::AutoConnection gives you both: main→main on macOS resolves to a direct call (old semantics, fresh snapshot), main→ExtControllers on Linux/Windows resolves to queued (the thread-safety you correctly want). It also keeps the code honest if the macOS thread policy ever changes.

Suggested change

	if (diagnostic == QLatin1String("ulanzi-start")) {
	m_dialBackend->start();
	QMetaObject::invokeMethod(m_dialBackend, &UlanziDialBackend::start,
	Qt::QueuedConnection);
	} else if (diagnostic == QLatin1String("ulanzi-stop")) {
	m_dialBackend->stop();
	QMetaObject::invokeMethod(m_dialBackend, &UlanziDialBackend::stop,
	Qt::QueuedConnection);
	}
	if (diagnostic == QLatin1String("ulanzi-start")) {
	// AUTO, NOT QUEUED. On Linux and Windows the backend lives on the
	// ExtControllers thread (MainWindow_Controllers.cpp), so this
	// resolves to a queued call and hidapi/evdev stay on their own
	// thread. On macOS the backend stays on the main thread for its
	// CFRunLoop and this handler runs there too, so it resolves to a
	// direct call -- which the diagnostics() snapshot below depends on:
	// forcing Queued there would snapshot the state BEFORE the
	// operation ran.
	QMetaObject::invokeMethod(m_dialBackend, &UlanziDialBackend::start,
	Qt::AutoConnection);
	} else if (diagnostic == QLatin1String("ulanzi-stop")) {
	QMetaObject::invokeMethod(m_dialBackend, &UlanziDialBackend::stop,
	Qt::AutoConnection);
	}

[aethersdr-agent]

This is the read that Blocker 1 (line 2654) invalidates on macOS: with the start/stop above posted rather than executed, diagnostics() returns the pre-operation state, and lines 2671-2672 then label it with operation: "ulanzi-stop". The result reads as "the stop ran and here is the after state" when it is the before state — the precise confusion the non-mac branch's comment at 2681-2685 exists to avoid. Switching line 2654/2658 to Qt::AutoConnection restores this.

[aethersdr-agent]

Nit, non-blocking: queued is a new field on a shipped bridge verb, and just above, devices ulanzi on this platform flips from ok:true to ok:false with message replaced by error. I think both are the right calls (they are what #5242 asks for), but they are protocol surface changes, so per the project's scope rules they want a maintainer's sign-off rather than mine — and they want docs/automation-bridge.md:2363-2372 updated in this PR, since that section still documents the old shape and still gives the IOKit claim path as the reason for supported:false.

[aethersdr-agent]

Antares Detector — candidate vulnerable file(s)

• src/core/UlanziDialWindowsManager.cpp

The code at line 214 in src/core/UlanziDialWindowsManager.cpp uses memcpy(pkt.data() + 28, opus.constData(), opus.size()) to copy an incoming audio packet into a fixed-size buffer without checking that opus.size() <= remaining capacity (64 - 28 = 36 bytes). This is an out-of-bounds write vulnerability.

_{Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting.}

---

_{🤖 aethersdr-agent · cost: $6.8065 · model: claude-opus-5}

[nigelfenton]

Both blockers are real. Thanks — the first one is a regression I introduced, and your thread analysis is exactly right.

Blocker 1: confirmed, and it was mine

I verified your chain against the head checkout rather than taking it on trust:

• MainWindow_Controllers.cpp:2791 guards the moveToThread with #ifndef Q_OS_MAC, with a comment saying macOS keeps the backend on the main thread for the CFRunLoop.
• AutomationServer is make_unique'd at MainWindow_Session.cpp:2558, never moved, and greps clean for moveToThread / QThreadPool / QtConcurrent.
• So on macOS caller and receiver are the same thread, Qt::QueuedConnection posts instead of running, and diagnostics() returns pre-call state.

What makes it clearly a regression rather than a pre-existing wart: before this PR macOS called start()/stop() directly and the snapshot was correct. Hoisting the lifecycle out of the #ifdef broke it. You are also right that my own comment at 2681-2685 identifies precisely this hazard and guards the non-mac branch — I wrote the argument down and then missed the platform it actually applied to.

Fixed in 953d795f with Qt::AutoConnection, as you suggested: direct main→main on macOS, queued main→ExtControllers on Linux/Windows.

I also stopped hardcoding queued: true, which your finding made obviously wrong — on the direct path it would be a lie. It is now derived:

result[QStringLiteral("queued")] =
    m_dialBackend->thread() != QThread::currentThread();

so it reports what actually happened, and stays honest if the threading decision in MainWindow_Controllers.cpp ever changes.

Your note that this also matters for #5212 is well taken — that review's blocker ("stop() clears the variant state and the advisory is withdrawn") is verified through this very snapshot, so a stale snapshot would have quietly undermined the evidence there too.

Blocker 2: docs updated

docs/automation-bridge.md now documents the split — that only the snapshot is macOS-specific, that a bare devices ulanzi is a refusal carrying error rather than message, that lifecycle calls return ok:true with operation/enabled/queued, and that supported:false refers to the snapshot and not the operation. I also documented what queued means on each platform, since it is a new field.

On the ok flip being a maintainer call — agreed, and I am not treating your read as approval

Flagging it as §3 rather than as a defect is the right call.

@jeremymturner — the explicit question for you: this changes devices ulanzi on Linux/Windows from ok:true, supported:false to ok:false + error, and adds a new queued field. That is a deliberate break of any script keying on ok, which is the point of #5242, but it is your call and not mine. Happy to split the response-shape change out and land the threading fix alone if you would rather take them separately.

Nits

• The comment block is now split with the two arguments separated, and carries the macOS caller/receiver clause.
• enabled became a lambda in f3216e69 so the refusal path no longer does a settings read it discards.
• On the missing test: you are right that automation_device_diagnostics_test.cpp would pass against unfixed main — it pins the dispatcher, not this lambda. I have not added one, because pinning it means extracting the response shaping out of the MainWindow lambda, and that is the item (4) refactor devices ulanzi-start/ulanzi-stop return ok:true on Windows/Linux but do nothing (#ifdef Q_OS_MAC stub) #5242 deferred. Worth doing there rather than reshaping the code to suit a test in this PR.

Verification

Builds and links on MSVC / Qt 6.10.3 with hidapi enabled, so the real UlanziDialWindowsManager compiles rather than the no-op fallback.

I confirmed the real linker exit code rather than the wrapper's — a first attempt reported success while AetherSDR.exe had in fact failed to link, on an unrelated missing fftw3f.lib. Worth stating because "green" from a build wrapper is not evidence on its own.

Still not exercised. No physical Ulanzi dial here, so the Windows lifecycle path is not hardware-verified. More to the point: the macOS branch does not compile on this box at all, so the very #ifdef split that caused this bug means my build cannot prove the fix on the platform it fixes. CI's macOS job builds it, but building is not the same as exercising the snapshot ordering — that wants a macOS reviewer with a dial.

73, Nigel G0JKN

[nigelfenton]

Follow-up: I said above that my build "cannot prove the fix on the platform it fixes". I got access to a macOS box, so that caveat is now partly retired — with one part deliberately left standing.

The Q_OS_MAC branch compiles, and I proved it rather than assuming it

macOS 26.6.2, Apple clang 21, Qt 6.11.0, hidapi 0.15.0 (so the real UlanziDialMacOSManager compiles, not the no-op fallback). Full AetherSDR target: 877/877, zero errors.

A clean build does not by itself prove the macOS-only branch was the one compiled, so I break-tested it. Injecting a sentinel next to the diagnostics() call inside the #ifdef Q_OS_MAC block:

MainWindow_Session.cpp:2681:62: error: use of undeclared identifier 'DELIBERATE_BREAK_MAC_BRANCH'

The compiler reached line 2681, which is inside the guarded branch — so that code is genuinely being compiled here, and the Qt::AutoConnection fix is in the binary. Sentinel removed, tree back to clean, relinked.

As a cross-check in the other direction, the non-mac refusal string ("...ulanzi-start and ulanzi-stop are supported here") greps to 0 occurrences in the macOS binary, which is what it should be if the #else branch was excluded.

Tests

automation_device_diagnostics_test on macOS: 6/6 PASS, including devices ulanzi-stop dispatches lifecycle control when writable.

Stating the obvious limitation, because it is the same one you raised: this test installs a stub provider and pins the dispatcher, so it passes against unfixed main too. It is evidence the change breaks nothing, not evidence the ordering bug is fixed. I am not presenting it as the latter.

What is still not proven, and why

The snapshot ordering itself is still unverified at runtime. There is no physical Ulanzi dial on this machine, so I cannot drive devices ulanzi-stop against a live backend and observe that restorationStatus/systemEventsSuppressed now describe the post-stop state.

What I can say precisely:

• Before: on macOS, caller and receiver share a thread, so Qt::QueuedConnection posted and diagnostics() ran first. Verified by reading the source, and consistent with your analysis.
• Now: Qt::AutoConnection resolves same-thread to a direct call, so start()/stop() completes before diagnostics() is reached. This is Qt-documented behaviour rather than something I measured.
• Unmeasured: the actual field values from a real dial before and after.

So the honest status is that the fix is correct by construction and now proven to compile on the affected platform, but the end-to-end behavioural claim still wants a macOS reviewer with hardware. I would rather say that than let a compile pass and a stub test read as more than they are.

CI is green on f3216e69 across all four checks (build, Static checks, check-windows, check-macos).

73, Nigel G0JKN

[jeremymturner]

I'm confused, why am I being tagged in this PR?

[nigelfenton]

@jeremymturner — apologies, that was my mistake and you can disregard this PR entirely.

I was trying to escalate a compatibility decision to the project maintainer, and I tagged the wrong Jeremy. GOVERNANCE.md names the maintainer as Jeremy Fielder / KK7GWY, which is @ten9876 — not you. Sorry for the noise.

@ten9876 — the question was meant for you. A reviewer flagged one part of this PR as a §3 maintainer call rather than a defect, and I agree with that framing, so I did not want to proceed on my own judgement.

The decision: on Linux/Windows this changes the shipped devices ulanzi bridge verb from ok:true, supported:false to ok:false plus an error, and adds a new queued field. That is a deliberate break of any script that keys on ok.

The argument for it is that it is the entire point of #5242 — the verb was reporting success for a call that did nothing at all on those platforms, and only the separate supported:false said otherwise. Loud refusal over a cheerful stub. But it is a shipped, third-party-facing protocol, so the call is yours rather than mine.

Happy to split the response-shape change out and land just the threading fix if you would prefer to take them separately, or to drop the shape change altogether.

The rest of the PR is not in question: the threading regression it fixes is real, I verified it against the source, and it is now break-tested as compiling on macOS. All four CI checks are green on f3216e69.

73, Nigel G0JKN