feat(simulator): add local socket backend for aux serial ports#7285
feat(simulator): add local socket backend for aux serial ports#7285raphaelcoeffic wants to merge 3 commits into
Conversation
raphaelcoeffic
changed the title
|
Very cool, this was long on my whish-list. Did not do any testing but it sounds very good. |
raphaelcoeffic
force-pushed
the
feat-simulator-socket-aux
branch
from
0c92db5 to
3242687
Compare
Phase 1 of pluggable host serial backends. Introduces an abstract HostSerialBackend QObject interface and extracts the existing QSerialPort handling into a QSerialPortBackend implementation. HostSerialConnector becomes a thin per-port owner of a backend pointer. Per-port encoding and baudrate are cached on the connector so they survive backend swaps and reconnects. Error reporting moves out of the connector: backends emit errorOccurred, the connector forwards via a backendError signal, and SimulatorMainWindow shows the message box. No behaviour change. Sets up the next phase, which adds a QLocalSocketBackend so user scripts can stand in for hardware on simulator aux ports. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds QLocalSocketBackend so user scripts can stand in for hardware on simulator aux serial ports. Backed by QLocalServer, which uses AF_UNIX on macOS / Linux and named pipes on Windows — no #ifdefs on our side. Single-client semantics: a second connection while one is active is rejected. The Configure Serial Ports dialog grows a per-port type selector (Not assigned / Serial port / Local socket) with a context- dependent input widget — combo for serial, line edit prefilled with edgetx-sim-aux<N> for sockets. While a socket is listening the dialog shows the resolved native path so users know where to point their script. HostSerialConnector::connectSerialPort is replaced with a generic connectBackend(index, kind, spec). Per-port encoding / baudrate are still cached on the connector and re-applied across backend swaps; setEncoding/setBaudrate are no-ops on the socket backend. Qt Network is now an explicit find_package COMPONENTS entry and linked into the common library. It was previously pulled in transitively by QNetworkAccessManager in updates/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QLocalServer's default TempLocation on macOS is a per-user $TMPDIR like /var/folders/.../T/, which is awkward to find when the other end of the socket is a Python script driven from a shell. Resolve bare names to /tmp/<name> on Unix so the path is predictable and matches Linux. Absolute paths supplied by the user pass through unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pfeerick
force-pushed
the
feat-simulator-socket-aux
branch
from
3242687 to
d31b9c8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR refactors simulator auxiliary serial port handling from direct ChangesBackend Abstraction & Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/src/simulation/serialportsdialog.cpp (1)
169-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve configured serial ports even when they are currently unavailable.
If
currentPortNameis missing fromavailablePorts(), this helper falls back to"Not Assigned". Reopening the dialog or pressing Refresh can then silently clear an existing AUX mapping on OK. Please keep the current value as an explicit"unavailable"entry so the spec round-trips until the user changes it.Suggested fix
void SerialPortsDialog::populateSerialPortCombo(QComboBox * cb, const QString & currentPortName) { cb->clear(); cb->addItem(tr("Not Assigned"), ""); - if (currentPortName.isEmpty()) + bool foundCurrent = currentPortName.isEmpty(); + if (foundCurrent) cb->setCurrentIndex(0); const auto serialPortInfos = QSerialPortInfo::availablePorts(); for (int i = 0; i < serialPortInfos.size(); i++) { const auto portInfo = serialPortInfos[i]; cb->addItem(portInfo.portName(), portInfo.portName()); - if (portInfo.portName() == currentPortName) + if (portInfo.portName() == currentPortName) { + foundCurrent = true; cb->setCurrentIndex(i + 1); + } } + + if (!currentPortName.isEmpty() && !foundCurrent) { + cb->addItem(tr("%1 (unavailable)").arg(currentPortName), currentPortName); + cb->setCurrentIndex(cb->count() - 1); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/simulation/serialportsdialog.cpp` around lines 169 - 182, populateSerialPortCombo currently drops the configured currentPortName if it's not returned by QSerialPortInfo::availablePorts(), causing the dialog to revert to "Not Assigned"; modify populateSerialPortCombo to detect when currentPortName is non-empty and not present in serialPortInfos and, in that case, add an explicit entry (e.g., port name plus a visual marker like " (unavailable)" or use the raw name as the item text with a data role indicating unavailable) before adding the available ports, and set the QComboBox current index to that new entry so the configured value round-trips until the user changes it; reference the function populateSerialPortCombo, the QComboBox pointer cb, and currentPortName to locate where to insert this logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@companion/src/simulation/hostserialbackend_localsocket.cpp`:
- Around line 65-74: The code unconditionally calls
QLocalServer::removeServer(listenName) before attempting
server->listen(listenName), which can unlink a live socket; change the logic to
first call server->listen(listenName) and only if that fails with an error
indicating an orphaned/stale socket (e.g., server->error() or
server->errorString() showing "Address already in use"/equivalent) call
QLocalServer::removeServer(listenName) and retry listen once; update the error
handling around emit errorOccurred(...) to report the final failure if the retry
also fails and ensure you reference listenName, server->listen, and
QLocalServer::removeServer in the fix.
In `@companion/src/simulation/hostserialbackend_serialport.cpp`:
- Around line 28-35: The constructor currently only connects readyRead; also
connect the QSerialPort::errorOccurred signal in QSerialPortBackend (the same
place where readyRead is wired) to a lambda that forwards runtime errors to the
base class by calling emit errorOccurred(...) using port->errorString() (and
optionally include the QSerialPort::SerialPortError value for more detail);
ensure this connection uses the same QObject context (this) so
unplug/disconnect, parity and other runtime errors are reported to the UI as
required by HostSerialBackend.
In `@companion/src/simulation/hostserialconnector.cpp`:
- Around line 51-58: The functions that accept a signed int index (e.g.,
HostSerialConnector::getBackendKind, and the other index-based entry points in
this file such as the getters/setters around lines 60-86 and 117-125 that access
hostAuxBackends/hostAuxBackendKinds) do not reject negative indices; add a guard
that returns a safe failure value (e.g., BackendNone for getBackendKind, nullptr
or equivalent for backend getters, and no-op or false for setters) when index <
0 or index >= MAX_HOST_SERIAL, and keep the existing QMutexLocker usage around
the validated access to hostAuxBackends and hostAuxBackendKinds to prevent
out-of-bounds reads/writes.
In `@companion/src/simulation/simulatormainwindow.cpp`:
- Around line 177-180: The QMessageBox::warning call that displays the backend
serial error (the call with tr("Host Serial Error") and message format "AUX%1:
%2", using the local index variable) uses QMessageBox::Cancel as its button
which is misleading; change the button role to QMessageBox::Ok (e.g., replace
the two QMessageBox::Cancel args with QMessageBox::Ok or the single Ok overload)
so the popup uses an acknowledgment "Ok" button instead of "Cancel".
---
Outside diff comments:
In `@companion/src/simulation/serialportsdialog.cpp`:
- Around line 169-182: populateSerialPortCombo currently drops the configured
currentPortName if it's not returned by QSerialPortInfo::availablePorts(),
causing the dialog to revert to "Not Assigned"; modify populateSerialPortCombo
to detect when currentPortName is non-empty and not present in serialPortInfos
and, in that case, add an explicit entry (e.g., port name plus a visual marker
like " (unavailable)" or use the raw name as the item text with a data role
indicating unavailable) before adding the available ports, and set the QComboBox
current index to that new entry so the configured value round-trips until the
user changes it; reference the function populateSerialPortCombo, the QComboBox
pointer cb, and currentPortName to locate where to insert this logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d38892b3-ee42-4c66-a82a-233c9a662e34
📒 Files selected for processing (14)
cmake/QtDefs.cmakecompanion/src/CMakeLists.txtcompanion/src/simulation/CMakeLists.txtcompanion/src/simulation/hostserialbackend.hcompanion/src/simulation/hostserialbackend_localsocket.cppcompanion/src/simulation/hostserialbackend_localsocket.hcompanion/src/simulation/hostserialbackend_serialport.cppcompanion/src/simulation/hostserialbackend_serialport.hcompanion/src/simulation/hostserialconnector.cppcompanion/src/simulation/hostserialconnector.hcompanion/src/simulation/serialportsdialog.cppcompanion/src/simulation/serialportsdialog.hcompanion/src/simulation/serialportsdialog.uicompanion/src/simulation/simulatormainwindow.cpp
| // A previous run (or another process) may have left a stale socket | ||
| // file behind on Unix; QLocalServer::removeServer is the documented | ||
| // way to clear it before re-listening. | ||
| QLocalServer::removeServer(listenName); | ||
|
|
||
| if (!server->listen(listenName)) { | ||
| emit errorOccurred(tr("Failed to listen on local socket \"%1\": %2") | ||
| .arg(listenName, server->errorString())); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Qt QLocalServer removeServer documentation semantics behavior
💡 Result:
QLocalServer::removeServer(const QString &name) is a static function that removes any lingering server instance for the given name to allow a new listen call to succeed, primarily for crash recovery when the previous server wasn't cleaned up. It returns true on success, false otherwise.[1][2][3] Semantics and behavior: - On Unix-like systems, it deletes the Unix domain socket file associated with the name.[1][2][3] - On Windows, it does nothing, as local servers use named pipes or similar without removable files.[1][2][3] - Warning: Avoid calling on sockets of currently running server instances, as it could disrupt them.[1][2][3] - Introduced in Qt 4.5.[2][4] This is consistently documented across Qt 5 and 6 versions, with no significant changes noted.[1][2][3][5]
Citations:
- 1: https://doc.qt.io/qt-6.8/qlocalserver.html
- 2: https://doc-snapshots.qt.io/qt5-5.15/qlocalserver.html
- 3: https://doc.qt.io/qt-6/qlocalserver.html
- 4: https://stuff.mit.edu/afs/athena/software/texmaker_v5.0.2/qt57/doc/qtnetwork/qlocalserver.html
- 5: https://codebrowser.dev/qt5/qtbase/src/network/socket/qlocalserver.cpp.html
Don't remove the socket path before proving it's stale.
Lines 68-70 call QLocalServer::removeServer() unconditionally before attempting to listen. According to Qt documentation, this function deletes the Unix domain socket file on Unix-like systems, but the documentation explicitly warns against calling it on sockets of currently running server instances, as it could disrupt them. This code can therefore unlink a live socket owned by another process on the same system. Instead, attempt the initial listen() call first, and only remove and retry after establishing the endpoint is stale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@companion/src/simulation/hostserialbackend_localsocket.cpp` around lines 65 -
74, The code unconditionally calls QLocalServer::removeServer(listenName) before
attempting server->listen(listenName), which can unlink a live socket; change
the logic to first call server->listen(listenName) and only if that fails with
an error indicating an orphaned/stale socket (e.g., server->error() or
server->errorString() showing "Address already in use"/equivalent) call
QLocalServer::removeServer(listenName) and retry listen once; update the error
handling around emit errorOccurred(...) to report the final failure if the retry
also fails and ensure you reference listenName, server->listen, and
QLocalServer::removeServer in the fix.
| QSerialPortBackend::QSerialPortBackend(const QString & portName, QObject * parent) | ||
| : HostSerialBackend(parent), | ||
| port(new QSerialPort(portName, this)) | ||
| { | ||
| connect(port, &QSerialPort::readyRead, this, [this]() { | ||
| emit dataReceived(port->readAll()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n companion/src/simulation/hostserialbackend_serialport.cpp | head -50Repository: EdgeTX/edgetx
Length of output: 1810
🏁 Script executed:
fd -t f "hostserialbackend.h" --type fRepository: EdgeTX/edgetx
Length of output: 102
🏁 Script executed:
cat -n companion/src/simulation/hostserialbackend.hRepository: EdgeTX/edgetx
Length of output: 2827
🏁 Script executed:
cat -n companion/src/simulation/hostserialbackend_serialport.cpp | tail -50Repository: EdgeTX/edgetx
Length of output: 1444
🏁 Script executed:
rg "QSerialPort::errorOccurred" --type cpp --type hRepository: EdgeTX/edgetx
Length of output: 39
🏁 Script executed:
rg "QSerialPort::" --type cpp --type h -A 2 -B 2 | head -100Repository: EdgeTX/edgetx
Length of output: 1593
🌐 Web query:
Qt QSerialPort errorOccurred signal documentation
💡 Result:
Qt QSerialPort::errorOccurred(QSerialPort::SerialPortError error) documentation summary (Qt Serial Port) - Signature: void errorOccurred(QSerialPort::SerialPortError error) [1] - When it is emitted: QSerialPort emits errorOccurred “when an error occurs in the serial port.” [1] - What the parameter means: the provided error value describes the type of error that occurred. [1] - Relationship to QSerialPort::error: the docs also note you can call error to find the type of the error that occurred last. [1] - Important historical note: for backward compatibility, after a successful open the errorOccurred signal is emitted with the NoError error code. [1] SerialPortError enum (how to interpret the signal’s argument) - QSerialPort::SerialPortError enumerates possible error codes such as NoError, DeviceNotFoundError, PermissionError, OpenError, NotOpenError, ResourceError, UnsupportedOperationError, TimeoutError, and UnknownError. [1][2] - Example from the docs: NoError is documented as “No error occurred.” [2] Code usage pattern (Qt/C++) - Connect to the signal and switch on the enum: connect(&serialPort, &QSerialPort::errorOccurred, this, [](QSerialPort::SerialPortError e) { // handle e }); Notes: - The docs do not claim this signal itself provides a human-readable message; it only provides the error enum. (For human-readable text, you typically use QIODevice::errorString/QSerialPort::errorString, which are separate from the signal.) [3] Sources: [1] [2]
Citations:
- 1: https://doc.qt.io/qt-6/qserialport.html
- 2: https://doc.qt.io/qt-5/qserialport.html
- 3: https://doc.qt.io/qt-6/qserialport-members.html
Forward runtime QSerialPort errors to UI.
Lines 32-34 only wire readyRead, so the constructor doesn't listen for runtime serial errors. After the port successfully opens, unplugging the device, disconnections, parity errors, or other runtime failures won't be reported to the UI. This violates the base class contract documented in HostSerialBackend, which requires errorOccurred to be emitted "when something goes wrong opening or operating the transport."
Suggested fix
QSerialPortBackend::QSerialPortBackend(const QString & portName, QObject * parent)
: HostSerialBackend(parent),
port(new QSerialPort(portName, this))
{
connect(port, &QSerialPort::readyRead, this, [this]() {
emit dataReceived(port->readAll());
});
+ connect(port, &QSerialPort::errorOccurred, this,
+ [this](QSerialPort::SerialPortError error) {
+ if (error != QSerialPort::NoError)
+ emit errorOccurred(port->errorString());
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QSerialPortBackend::QSerialPortBackend(const QString & portName, QObject * parent) | |
| : HostSerialBackend(parent), | |
| port(new QSerialPort(portName, this)) | |
| { | |
| connect(port, &QSerialPort::readyRead, this, [this]() { | |
| emit dataReceived(port->readAll()); | |
| }); | |
| } | |
| QSerialPortBackend::QSerialPortBackend(const QString & portName, QObject * parent) | |
| : HostSerialBackend(parent), | |
| port(new QSerialPort(portName, this)) | |
| { | |
| connect(port, &QSerialPort::readyRead, this, [this]() { | |
| emit dataReceived(port->readAll()); | |
| }); | |
| connect(port, &QSerialPort::errorOccurred, this, | |
| [this](QSerialPort::SerialPortError error) { | |
| if (error != QSerialPort::NoError) | |
| emit errorOccurred(port->errorString()); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@companion/src/simulation/hostserialbackend_serialport.cpp` around lines 28 -
35, The constructor currently only connects readyRead; also connect the
QSerialPort::errorOccurred signal in QSerialPortBackend (the same place where
readyRead is wired) to a lambda that forwards runtime errors to the base class
by calling emit errorOccurred(...) using port->errorString() (and optionally
include the QSerialPort::SerialPortError value for more detail); ensure this
connection uses the same QObject context (this) so unplug/disconnect, parity and
other runtime errors are reported to the UI as required by HostSerialBackend.
| HostSerialConnector::BackendKind HostSerialConnector::getBackendKind(int index) const | ||
| { | ||
| if (index >= MAX_HOST_SERIAL) | ||
| return BackendNone; | ||
|
|
||
| QMutexLocker locker(&hostAuxPortsMutex); | ||
| return hostAuxBackendKinds[index]; | ||
| } |
There was a problem hiding this comment.
Reject negative AUX indices too.
These entry points accept signed int index, but the guard only checks index >= MAX_HOST_SERIAL. A value like -1 will still index before hostAuxBackends / hostAuxBackendKinds, which is an out-of-bounds read/write.
Suggested fix
HostSerialConnector::BackendKind HostSerialConnector::getBackendKind(int index) const
{
- if (index >= MAX_HOST_SERIAL)
+ if (index < 0 || index >= MAX_HOST_SERIAL)
return BackendNone;
...
}
QString HostSerialConnector::getBackendSpec(int index) const
{
- if (index >= MAX_HOST_SERIAL)
+ if (index < 0 || index >= MAX_HOST_SERIAL)
return QString();
...
}
QString HostSerialConnector::getLocalSocketFullName(int index) const
{
- if (index >= MAX_HOST_SERIAL)
+ if (index < 0 || index >= MAX_HOST_SERIAL)
return QString();
...
}
void HostSerialConnector::connectBackend(int index, int kind, const QString & spec)
{
- if (index >= MAX_HOST_SERIAL)
+ if (index < 0 || index >= MAX_HOST_SERIAL)
return;
...
}Also applies to: 60-86, 117-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@companion/src/simulation/hostserialconnector.cpp` around lines 51 - 58, The
functions that accept a signed int index (e.g.,
HostSerialConnector::getBackendKind, and the other index-based entry points in
this file such as the getters/setters around lines 60-86 and 117-125 that access
hostAuxBackends/hostAuxBackendKinds) do not reject negative indices; add a guard
that returns a safe failure value (e.g., BackendNone for getBackendKind, nullptr
or equivalent for backend getters, and no-op or false for setters) when index <
0 or index >= MAX_HOST_SERIAL, and keep the existing QMutexLocker usage around
the validated access to hostAuxBackends and hostAuxBackendKinds to prevent
out-of-bounds reads/writes.
| QMessageBox::warning(this, tr("Host Serial Error"), | ||
| tr("AUX%1: %2").arg(index + 1).arg(message), | ||
| QMessageBox::Cancel, QMessageBox::Cancel); | ||
| }); |
There was a problem hiding this comment.
Use an acknowledgment button for backend error popups
Line 177–180 shows an informational warning, but the only action is Cancel, which suggests a reversible operation. Use Ok instead.
Proposed fix
- QMessageBox::warning(this, tr("Host Serial Error"),
- tr("AUX%1: %2").arg(index + 1).arg(message),
- QMessageBox::Cancel, QMessageBox::Cancel);
+ QMessageBox::warning(this, tr("Host Serial Error"),
+ tr("AUX%1: %2").arg(index + 1).arg(message),
+ QMessageBox::Ok, QMessageBox::Ok);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QMessageBox::warning(this, tr("Host Serial Error"), | |
| tr("AUX%1: %2").arg(index + 1).arg(message), | |
| QMessageBox::Cancel, QMessageBox::Cancel); | |
| }); | |
| QMessageBox::warning(this, tr("Host Serial Error"), | |
| tr("AUX%1: %2").arg(index + 1).arg(message), | |
| QMessageBox::Ok, QMessageBox::Ok); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@companion/src/simulation/simulatormainwindow.cpp` around lines 177 - 180, The
QMessageBox::warning call that displays the backend serial error (the call with
tr("Host Serial Error") and message format "AUX%1: %2", using the local index
variable) uses QMessageBox::Cancel as its button which is misleading; change the
button role to QMessageBox::Ok (e.g., replace the two QMessageBox::Cancel args
with QMessageBox::Ok or the single Ok overload) so the popup uses an
acknowledgment "Ok" button instead of "Cancel".
3 participants
Summary
Lets simulator aux serial ports talk to a local socket (
QLocalServer— AF_UNIX on macOS / Linux, named pipes on Windows), so user scripts can stand in for hardware without a USB-serial adapter. Backends are now pluggable behind aHostSerialBackendinterface; the existingQSerialPortpath is the other implementation. Configure via Settings → Configure Serial Ports; the dialog gains a per-port type selector and shows the resolved socket path.Stacked on #7284 — review that first.
Test plan
AF_UNIXclient at/tmp/edgetx-sim-aux1.Summary by CodeRabbit
New Features
Bug Fixes