Attempt to Fix Windows Serial Link#13932
Conversation
…MAVLink data and add timeout handling
There was a problem hiding this comment.
Pull request overview
This pull request addresses USB serial connection issues on Windows by implementing a comprehensive fix for "Access denied" errors that occur when serial ports are opened but not receiving MAVLink data.
Changes:
- Auto-disconnects existing autoconnect links before manual connection attempts to prevent port conflicts
- Implements zombie link detection that disconnects and retries autoconnect links after 10 seconds of no MAVLink data
- Allows autoconnect retry for ports that are in the link list but not yet connected (async connection in progress)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Comms/LinkManager.h | Added timeout constant, zombie detection map, and helper method declaration for disconnecting autoconnect links |
| src/Comms/LinkManager.cc | Implemented manual connection pre-check, zombie link detection logic, autoconnect link disconnection helper, and connection state verification for retry logic |
| SharedLinkInterfacePtr existingLink; | ||
| { | ||
| QMutexLocker locker(&_linksMutex); | ||
| const QString searchPort = portInfo.systemLocation().trimmed(); | ||
| for (const SharedLinkInterfacePtr &linkInterface : _rgLinks) { | ||
| const SharedLinkConfigurationPtr linkConfig = linkInterface->linkConfiguration(); | ||
| if (linkConfig && linkConfig->isAutoConnect()) { | ||
| const SerialConfiguration *serialConfig = qobject_cast<const SerialConfiguration*>(linkConfig.get()); | ||
| if (serialConfig && (serialConfig->portName() == searchPort)) { | ||
| existingLink = linkInterface; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (existingLink && !existingLink->decodedFirstMavlinkPacket()) { | ||
| _autoconnectNoMavlinkCount[portInfo.systemLocation()] += _autoconnectUpdateTimerMSecs; | ||
| if (_autoconnectNoMavlinkCount[portInfo.systemLocation()] > _autoconnectNoMavlinkTimeoutMSecs) { | ||
| qCWarning(LinkManagerLog) << "Autoconnect link on" << portInfo.systemLocation() | ||
| << "has not received MAVLink data - disconnecting to retry"; | ||
| _autoconnectNoMavlinkCount.remove(portInfo.systemLocation()); | ||
| existingLink->disconnect(); |
There was a problem hiding this comment.
Potential race condition: The existingLink shared pointer is obtained inside a mutex lock but then used outside the lock. While SharedLinkInterfacePtr holds a reference count that prevents object deletion, the link could be removed from _rgLinks by another thread between line 946 and line 948. This could lead to disconnecting a link that was already processed for removal by another thread. While disconnect() is documented as safe to call multiple times, consider adding a check to verify the link is still in _rgLinks before calling disconnect(), or hold the mutex for the entire zombie detection and disconnect operation to ensure atomicity.
|
|
||
| UdpIODevice *_nmeaSocket = nullptr; | ||
| QMap<QString, int> _autoconnectPortWaitList; ///< key: QGCSerialPortInfo::systemLocation, value: wait count | ||
| QMap<QString, int> _autoconnectNoMavlinkCount; ///< key: port systemLocation, value: seconds since autoconnect with no MAVLink data |
There was a problem hiding this comment.
The comment states the value is in "seconds since autoconnect", but the code actually increments this by _autoconnectUpdateTimerMSecs (which is in milliseconds) at line 949. The value stored is actually in milliseconds, not seconds. The comment should be corrected to say "milliseconds since autoconnect with no MAVLink data" to match the implementation.
| QMap<QString, int> _autoconnectNoMavlinkCount; ///< key: port systemLocation, value: seconds since autoconnect with no MAVLink data | |
| QMap<QString, int> _autoconnectNoMavlinkCount; ///< key: port systemLocation, value: milliseconds since autoconnect with no MAVLink data |
| } | ||
|
|
||
| if (linkToDisconnect) { | ||
| qCDebug(LinkManagerLog) << "Disconnecting existing autoconnect link on port" << searchPort << "to allow manual connection"; |
There was a problem hiding this comment.
When disconnecting an autoconnect link to allow manual connection, the _autoconnectNoMavlinkCount entry for the port is not cleared. If the manual connection later fails and autoconnect tries again, it will resume the timeout counter from where it left off, potentially causing the new autoconnect attempt to timeout prematurely.
Consider adding _autoconnectNoMavlinkCount.remove(portName) after successfully finding and disconnecting an autoconnect link to reset the timeout counter when the user manually intervenes.
| qCDebug(LinkManagerLog) << "Disconnecting existing autoconnect link on port" << searchPort << "to allow manual connection"; | |
| qCDebug(LinkManagerLog) << "Disconnecting existing autoconnect link on port" << searchPort << "to allow manual connection"; | |
| _autoconnectNoMavlinkCount.remove(searchPort); |
Build ResultsPlatform Status
All builds passed. Test Resultslinux_gcc_64: 0 passed, 0 skipped Total: 0 passed, 0 skipped Artifact Sizes
No baseline available for comparisonUpdated: 2026-02-05 23:43:50 UTC • Triggered by: Android |
|
I don't see disconnecting an AutoConnected link if you have a Serial Link at the same port automatically as being a good thing. That's pretty magic stuff. Much better to give the user better information as to what is going on. Instead of just some random port failure tell them they can't connect the manual link because of autoconnect. You could even given them the option right there in the UI to turn autoconnect off for serial. I also don't think you should automatically disconnect serial links which don't have traffic on them. That's how SiK radios work isn't it? You plug in your SiK radio and it sits there waiting for a vehicle to show up. That's the whole concept of autoconnect. If you are using manual links the best thing to do is to turn off autoconnect not try to be smart about it somehow. I've looked at the Windows USB hang up thing ages ago. My conclusion was that USB over serial in general on Windows sucks. And it just happens randomly without much you can do about it in code. Unless your case differs from mine. That said I never runs Windows any more so things could have change. My only access to Windows is through a VM which tends to screw up USB connections in different ways. |
|
The issue is that I sit with my FC connected over USB and QGC doesn't always connect. Even if its connected and I initiate a reboot from QGC, it won't reconnect. The app log just continuously spits out this.
|
|
But then if you reboot QGC it will autoconnect? The case I was looking at the USB port would just get hung up. Needed to reboot OS to clear. |
|
A start of QGC doesn't always fix it. A reboot of windows might. |
Yup. That matches what I looked at like 10 years ago! I never found a way to fix this from the QGC side of things. Nor did I ever find a way to clear the port without rebooting windows. Doesn't mean it isn't QGC's fault though. @HTRamsey Holden knows way more about hardware type stuff than I do. He might have ideas... Do you have Claude CoWork? If so, when the port is hung up and QGC reboot still doesn't connect. Ask Claude why the port is out of whack. I had a strange hung usb problem on my Mac and Claude at least gave me a way to clear it without rebooting. |
On recent daily builds of QGC I have been having issues with USB connections.