fix: serial PTT — Win32 WaitCommEvent path for FTDI DSR detection#2147
fix: serial PTT — Win32 WaitCommEvent path for FTDI DSR detection#2147ten9876 merged 6 commits intoten9876:mainfrom
Conversation
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice work, @chibondking. The FTDI WaitCommEvent workaround is well-researched and the platform split is clean. A few observations:
Looks good
- Win32
WaitCommEventloop: Correct pattern — synchronous blocking call on a dedicated thread,SetCommMask(0)to abort on close,QueuedConnectionto marshal back. The non-overlapped approach is the right call for FTDI VCP compatibility. - MainWindow destructor fix:
BlockingQueuedConnectionto close the port on the worker thread beforequit()is the correct ordering to avoid the cross-thread assert.moveToThread(currentThread)before delete is also correct. - Spurious PTT on open fix: Only deasserting pins with assigned functions (leaving unassigned pins HIGH for foot-switch source voltage) is a thoughtful change that matches real wiring.
- Polarity re-sample: Re-sampling after a polarity flip in
loadSettings()prevents the false-edge bug cleanly. - Uses
AppSettingsthroughout (notQSettings).
Items to consider
-
staticlocals inpollInputPins()(non-Windows path): The added diagnostic logging usesstatic QSerialPort::PinoutSignals lastRaw{}andstatic bool lastDebounceOk{}— these are process-global, not per-instance. There's only oneSerialPortControllertoday, so it works, but if a second instance were ever created (e.g. multi-radio), they'd share state. Consider making thesem_members instead, or at minimum adding a comment noting the single-instance assumption. -
Win32
GetCommState/SetCommStatefailures are silent: IfSetCommStatefails afterGetCommStatesucceeds, the handle is closed but noerrorOccurredsignal is emitted and no diagnostic is logged. Adding aqCWarning+emit errorOccurred(...)there (similar to theCreateFileWfailure path) would help users diagnose misconfigured baud/framing. -
HAVE_SERIALPORTon Windows: The PR correctly assumesHAVE_SERIALPORTis still defined on Windows builds (since Qt6::SerialPort ships on Windows), making the MainWindow#ifdef HAVE_SERIALPORTguards around the new destructor code and theserialSettingsChangedconnection correct. Worth a brief comment inSerialPortController.hnoting that on Windows,HAVE_SERIALPORTis expected to be defined but the QSerialPort code path is not used — just so a future reader doesn't wonder whether the ifdefs are wrong. -
RadioSetupDialogsignal: The newserialSettingsChanged()signal is not guarded by#ifdef HAVE_SERIALPORT, which is fine (unused signals are harmless), but it does diverge from the existing pattern wherebuildSerialTab()is guarded. No action needed — just noting for consistency awareness.
None of the above are blockers. This is solid platform work with good error handling and thorough testing notes. Thanks for tracking down the FTDI GetCommModemStatus staleness issue — that's the kind of driver-level knowledge that saves people hours of debugging.
QSerialPort::pinoutSignals() wraps GetCommModemStatus(), which returns stale zeros on FTDI VCP drivers. The FTDI driver only refreshes modem status in the context of a WaitCommEvent completion — not from a background poll. Adding EV_DSR to Qt's comm mask does not help because Qt owns the port handle exclusively and its own WaitCommEvent loop consumes the completions. Fix: on Windows, bypass QSerialPort entirely. Open the COM port with CreateFile (synchronous, non-overlapped) and dedicate a QThread to a blocking WaitCommEvent loop. When EV_DSR or EV_CTS fires, read GetCommModemStatus immediately in that completion context (where FTDI guarantees fresh data) and marshal the result back to the controller thread via QueuedConnection. The Linux/macOS QSerialPort polling path is unchanged. Also fix MainWindow destructor: call SerialPortController::close() on the ExtControllers thread via BlockingQueuedConnection before quitting the thread, then moveToThread back to main before deletion. Prevents the "Cannot send events to objects owned by a different thread" fatal assert seen on app exit. Also fix spurious PTT on open: only deassert DTR/RTS pins that have an assigned PTT/CW function. Unassigned pins stay HIGH (OS default), providing the source voltage for foot switch circuits that loop an output pin back to DSR/CTS. Also fix phantom PTT edge on polarity change: in-place settings update now re-samples pin state after polarity flip so the next event/poll doesn't see a false edge. Tested: FTDI-based foot switch on COM17, DSR active-high. Clean press and release edges with contact-bounce debouncing at 20 ms.
- Promote static locals lastRaw/lastDebounceOk in pollInputPins() to m_lastRawPins/m_lastDebounceLogged members — static locals are process-global and would share state across instances - Add qCWarning + emit errorOccurred() on GetCommState/SetCommState failures in Win32 open() — previously silent, now consistent with the CreateFileW failure path - Add comment in SerialPortController.h clarifying that HAVE_SERIALPORT is defined on Windows but the QSerialPort code path is bypassed by the Win32 WaitCommEvent path Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
chibondking
force-pushed
the
fix/serial-ptt-stuck-tx
branch
from
474ec64 to
3622cab
Compare
|
@AetherClaude changes applied based on feedback |
|
Thanks @chibondking — I've read through the updated diff. The overall approach is solid: the Win32 A couple of things I noticed: 1. Stale queued event after In the Win32 path, Easy fix — add a guard at the top of void SerialPortController::processWinPinChange(bool dsrRaw, bool ctsRaw)
{
if (!isOpen()) return;
// ...
}2. Diagnostic logging volume in the polling path The new per-poll logging block in Everything else looks good. The watcher thread lifecycle is clean (atomic flag + |
A QueuedConnection invocation of processWinPinChange could be delivered after close() — after m_hWin is nulled and pin state fields are reset — because Qt defers cross-thread signals until the next event-loop spin. Without the guard the stale delivery would re-set m_lastCtsActive/m_lastDsrActive and emit a phantom externalPttChanged edge. Add an isOpen() early-return to make any post-close delivery a no-op.
Adds a clarifying comment near the Qt6::SerialPort find_package call explaining that the Windows path bypasses Qt6::SerialPort for raw Win32 WaitCommEvent (per the SerialPortController fix in this PR). Side-effect: touching CMakeLists.txt activates the path filter that gates check-windows in .github/workflows/ci.yml so this otherwise src/-only PR actually exercises a Windows compile. The PR is near-entirely Windows-specific code, so a clean MSVC build is the test we most want before merge. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
2 participants
QSerialPort::pinoutSignals() wraps GetCommModemStatus(), which returns
stale zeros on FTDI VCP drivers. The FTDI driver only refreshes modem
status in the context of a WaitCommEvent completion — not from a
background poll. Adding EV_DSR to Qt's comm mask does not help because
Qt owns the port handle exclusively and its own WaitCommEvent loop
consumes the completions.
Fix: on Windows, bypass QSerialPort entirely. Open the COM port with
CreateFile (synchronous, non-overlapped) and dedicate a QThread to a
blocking WaitCommEvent loop. When EV_DSR or EV_CTS fires, read
GetCommModemStatus immediately in that completion context (where FTDI
guarantees fresh data) and marshal the result back to the controller
thread via QueuedConnection.
The Linux/macOS QSerialPort polling path is unchanged.
Also fix MainWindow destructor: call SerialPortController::close() on
the ExtControllers thread via BlockingQueuedConnection before quitting
the thread, then moveToThread back to main before deletion. Prevents
the "Cannot send events to objects owned by a different thread" fatal
assert seen on app exit.
Also fix spurious PTT on open: only deassert DTR/RTS pins that have an
assigned PTT/CW function. Unassigned pins stay HIGH (OS default),
assert seen on app exit.
Also fix spurious PTT on open: only deassert DTR/RTS pins that have an
assigned PTT/CW function. Unassigned pins stay HIGH (OS default),
providing the source voltage for foot switch circuits that loop an
output pin back to DSR/CTS.
Also fix phantom PTT edge on polarity change: in-place settings update
now re-samples pin state after polarity flip so the next event/poll
doesn't see a false edge.
Tested: FTDI-based foot switch on COM17, DSR active-high. Clean press
and release edges with contact-bounce debouncing at 20 ms.