-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Attempt to Fix Windows Serial Link #13932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -107,6 +107,18 @@ void LinkManager::createConnectedLink(const LinkConfiguration *config) | |||||||
|
|
||||||||
| bool LinkManager::createConnectedLink(SharedLinkConfigurationPtr &config) | ||||||||
| { | ||||||||
| #ifndef QGC_NO_SERIAL_LINK | ||||||||
| // If we're creating a serial link, disconnect any existing autoconnect link on the same port first. | ||||||||
| // This prevents "Access denied" errors on Windows when the user manually connects to a port | ||||||||
| // that autoconnect has already opened. | ||||||||
| if (config->type() == LinkConfiguration::TypeSerial) { | ||||||||
| const SerialConfiguration *newSerialConfig = qobject_cast<const SerialConfiguration*>(config.get()); | ||||||||
| if (newSerialConfig && !config->isAutoConnect()) { | ||||||||
| _disconnectAutoConnectLink(newSerialConfig->portName()); | ||||||||
| } | ||||||||
| } | ||||||||
| #endif | ||||||||
|
|
||||||||
| SharedLinkInterfacePtr link = nullptr; | ||||||||
|
|
||||||||
| switch(config->type()) { | ||||||||
|
|
@@ -915,6 +927,38 @@ void LinkManager::_addSerialAutoConnectLink() | |||||||
| continue; | ||||||||
| } | ||||||||
| if (_portAlreadyConnected(portInfo.systemLocation()) || (_autoConnectRTKPort == portInfo.systemLocation())) { | ||||||||
| // Check for zombie autoconnect links: port is open but no MAVLink data received | ||||||||
| if (_autoConnectRTKPort != portInfo.systemLocation()) { | ||||||||
| 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(); | ||||||||
| continue; | ||||||||
| } | ||||||||
| } else { | ||||||||
| // Link is communicating, clear any timeout counter | ||||||||
| _autoconnectNoMavlinkCount.remove(portInfo.systemLocation()); | ||||||||
| } | ||||||||
| } | ||||||||
| qCDebug(LinkManagerVerboseLog) << "Skipping existing autoconnect" << portInfo.systemLocation(); | ||||||||
| } else if (!_autoconnectPortWaitList.contains(portInfo.systemLocation())) { | ||||||||
| // We don't connect to the port the first time we see it. The ability to correctly detect whether we | ||||||||
|
|
@@ -999,6 +1043,32 @@ bool LinkManager::_allowAutoConnectToBoard(QGCSerialPortInfo::BoardType_t boardT | |||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
| void LinkManager::_disconnectAutoConnectLink(const QString &portName) | ||||||||
| { | ||||||||
| const QString searchPort = portName.trimmed(); | ||||||||
| SharedLinkInterfacePtr linkToDisconnect; | ||||||||
|
|
||||||||
| { | ||||||||
| QMutexLocker locker(&_linksMutex); | ||||||||
| for (const SharedLinkInterfacePtr &linkInterface : _rgLinks) { | ||||||||
| const SharedLinkConfigurationPtr linkConfig = linkInterface->linkConfiguration(); | ||||||||
| if (!linkConfig || !linkConfig->isAutoConnect()) { | ||||||||
| continue; | ||||||||
| } | ||||||||
| const SerialConfiguration *serialConfig = qobject_cast<const SerialConfiguration*>(linkConfig.get()); | ||||||||
| if (serialConfig && (serialConfig->portName() == searchPort)) { | ||||||||
| linkToDisconnect = linkInterface; | ||||||||
| break; | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| if (linkToDisconnect) { | ||||||||
| 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"; | |
| qCDebug(LinkManagerLog) << "Disconnecting existing autoconnect link on port" << searchPort << "to allow manual connection"; | |
| _autoconnectNoMavlinkCount.remove(searchPort); |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -161,6 +161,8 @@ private slots: | |||||
| #else | ||||||
| static constexpr int _autoconnectConnectDelayMSecs = 1000; | ||||||
| #endif | ||||||
| // If an autoconnect serial link hasn't received any MAVLink data within this time, disconnect and retry | ||||||
| static constexpr int _autoconnectNoMavlinkTimeoutMSecs = 10000; | ||||||
|
|
||||||
| #ifndef QGC_NO_SERIAL_LINK | ||||||
| private: | ||||||
|
|
@@ -183,10 +185,12 @@ private slots: | |||||
| bool _allowAutoConnectToBoard(QGCSerialPortInfo::BoardType_t boardType) const; | ||||||
| void _addSerialAutoConnectLink(); | ||||||
| bool _portAlreadyConnected(const QString &portName); | ||||||
| void _disconnectAutoConnectLink(const QString &portName); | ||||||
| void _filterCompositePorts(QList<QGCSerialPortInfo> &portList); | ||||||
|
|
||||||
| 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 | ||||||
|
||||||
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.