Link connect/disconnect only through LinkManager

Centralize all connecting and disconnecting of links through
LinkManager::[Connect|Disconnect]Link api. Connect/Disconnect no longer
allowed through LinkInterface. This allows creation of new
LinkManager::setConnections[Suspended|Allowed] methods to disallow
connections as needed.

src/comm/LinkInterface.h

@@ -38,6 +38,8 @@ along with PIXHAWK. If not, see <http://www.gnu.org/licenses/>.
 #include <QMutexLocker>
 #include <QMetaType>
 
+class LinkManager;
+
 /**
 * The link interface defines the interface for all links used to communicate
 * with the groundstation application.
@@ -46,6 +48,10 @@ along with PIXHAWK. If not, see <http://www.gnu.org/licenses/>.
 class LinkInterface : public QThread
 {
     Q_OBJECT
+    
+    // Only LinkManager is allowed to _connect or _disconnect a link
+    friend class LinkManager;
+    
 public:
     LinkInterface() :
         QThread(0)
@@ -133,20 +139,6 @@ public:
         return getCurrentDataRate(outDataIndex, outDataWriteTimes, outDataWriteAmounts);
     }
 
-    /**
-     * @brief Connect this interface logically
-     *
-     * @return True if connection could be established, false otherwise
-     **/
-    virtual bool connect() = 0;
-
-    /**
-     * @brief Disconnect this interface logically
-     *
-     * @return True if connection could be terminated, false otherwise
-     **/
-    virtual bool disconnect() = 0;
-
 public slots:
 
     /**
@@ -323,6 +315,20 @@ protected slots:
      **/
     virtual void readBytes() = 0;
 
+private:
+    /**
+     * @brief Connect this interface logically
+     *
+     * @return True if connection could be established, false otherwise
+     **/
+    virtual bool _connect(void) = 0;
+    
+    /**
+     * @brief Disconnect this interface logically
+     *
+     * @return True if connection could be terminated, false otherwise
+     **/
+    virtual bool _disconnect(void) = 0;
 };
 
 #endif // _LINKINTERFACE_H_

src/comm/LinkManager.cc

@@ -31,11 +31,12 @@ This file is part of the QGROUNDCONTROL project
 
 #include <QList>
 #include <QApplication>
-#include "LinkManager.h"
-#include <iostream>
-
+#include <QMessageBox>
 #include <QDebug>
 
+#include "LinkManager.h"
+#include "MainWindow.h"
+
 LinkManager* LinkManager::instance()
 {
     static LinkManager* _instance = 0;
@@ -54,46 +55,52 @@ LinkManager* LinkManager::instance()
  *
  * This class implements the singleton design pattern and has therefore only a private constructor.
  **/
-LinkManager::LinkManager()
+LinkManager::LinkManager() :
+    _connectionsSuspended(false)
 {
-    links = QList<LinkInterface*>();
-    protocolLinks = QMap<ProtocolInterface*, LinkInterface*>();
+    _links = QList<LinkInterface*>();
+    _protocolLinks = QMap<ProtocolInterface*, LinkInterface*>();
 }
 
 LinkManager::~LinkManager()
 {
     disconnectAll();
-    dataMutex.lock();
-    foreach (LinkInterface* link, links)
-    {
-        if(link) link->deleteLater();
+    
+    _dataMutex.lock();
+    foreach (LinkInterface* link, _links) {
+        Q_ASSERT(link);
+        link->deleteLater();
     }
-    dataMutex.unlock();
+    _links.clear();
+    _dataMutex.unlock();
 }
 
 void LinkManager::add(LinkInterface* link)
 {
-    dataMutex.lock();
-    if (!links.contains(link))
+    Q_ASSERT(link);
+    
+    _dataMutex.lock();
+    
+    if (!_links.contains(link))
     {
-        if(!link) return;
         connect(link, SIGNAL(destroyed(QObject*)), this, SLOT(removeObj(QObject*)));
-        links.append(link);
-        dataMutex.unlock();
+        _links.append(link);
+        _dataMutex.unlock();
         emit newLink(link);
     } else {
-        dataMutex.unlock();
+        _dataMutex.unlock();
     }
 }
 
 void LinkManager::addProtocol(LinkInterface* link, ProtocolInterface* protocol)
 {
+    Q_ASSERT(link);
+    Q_ASSERT(protocol);
+    
     // Connect link to protocol
     // the protocol will receive new bytes from the link
-    if (!link || !protocol) return;
-
-    dataMutex.lock();
-    QList<LinkInterface*> linkList = protocolLinks.values(protocol);
+    _dataMutex.lock();
+    QList<LinkInterface*> linkList = _protocolLinks.values(protocol);
 
     // If protocol has not been added before (list length == 0)
     // OR if link has not been added to protocol, add
@@ -104,43 +111,48 @@ void LinkManager::addProtocol(LinkInterface* link, ProtocolInterface* protocol)
         // Add status
         connect(link, SIGNAL(connected(bool)), protocol, SLOT(linkStatusChanged(bool)));
         // Store the connection information in the protocol links map
-        protocolLinks.insertMulti(protocol, link);
-        dataMutex.unlock();
+        _protocolLinks.insertMulti(protocol, link);
+        _dataMutex.unlock();
         // Make sure the protocol clears its metadata for this link.
         protocol->resetMetadataForLink(link);
     } else {
-        dataMutex.unlock();
+        _dataMutex.unlock();
     }
-    //qDebug() << __FILE__ << __LINE__ << "ADDED LINK TO PROTOCOL" << link->getName() << protocol->getName() << "NEW SIZE OF LINK LIST:" << protocolLinks.size();
+    //qDebug() << __FILE__ << __LINE__ << "ADDED LINK TO PROTOCOL" << link->getName() << protocol->getName() << "NEW SIZE OF LINK LIST:" << _protocolLinks.size();
 }
 
 QList<LinkInterface*> LinkManager::getLinksForProtocol(ProtocolInterface* protocol)
 {
-    dataMutex.lock();
-    QList<LinkInterface*> links = protocolLinks.values(protocol);
-    dataMutex.unlock();
+    _dataMutex.lock();
+    QList<LinkInterface*> links = _protocolLinks.values(protocol);
+    _dataMutex.unlock();
     return links;
 }
 
 ProtocolInterface* LinkManager::getProtocolForLink(LinkInterface* link)
 {
-    dataMutex.lock();
-    ProtocolInterface* interface = protocolLinks.key(link);
-    dataMutex.unlock();
+    _dataMutex.lock();
+    ProtocolInterface* interface = _protocolLinks.key(link);
+    _dataMutex.unlock();
     return interface;
 }
 
 bool LinkManager::connectAll()
 {
+    if (_connectionsSuspendedMsg()) {
+        return false;
+    }
+    
     bool allConnected = true;
 
-    dataMutex.lock();
-    foreach (LinkInterface* link, links)
-    {
-        if(!link) {}
-        else if(!link->connect()) allConnected = false;
+    _dataMutex.lock();
+    foreach (LinkInterface* link, _links) {
+        Q_ASSERT(link);
+        if (!link->_connect()) {
+            allConnected = false;
+        }
     }
-    dataMutex.unlock();
+    _dataMutex.unlock();
 
     return allConnected;
 }
@@ -149,58 +161,62 @@ bool LinkManager::disconnectAll()
 {
     bool allDisconnected = true;
 
-    dataMutex.lock();
-    foreach (LinkInterface* link, links)
+    _dataMutex.lock();
+    foreach (LinkInterface* link, _links)
     {
-        //static int i=0;
-        if(!link) {}
-        else if(!link->disconnect()) allDisconnected = false;
+        Q_ASSERT(link);
+        if (!link->disconnect()) {
+            allDisconnected = false;
+        }
     }
-    dataMutex.unlock();
+    _dataMutex.unlock();
 
     return allDisconnected;
 }
 
 bool LinkManager::connectLink(LinkInterface* link)
 {
-    if(!link) return false;
-    return link->connect();
+    Q_ASSERT(link);
+    
+    if (_connectionsSuspendedMsg()) {
+        return false;
+    }
+
+    return link->_connect();
 }
 
 bool LinkManager::disconnectLink(LinkInterface* link)
 {
-    if(!link) return false;
-    return link->disconnect();
+    Q_ASSERT(link);
+    return link->_disconnect();
 }
 
 void LinkManager::removeObj(QObject* link)
 {
-    LinkInterface* linkInterface = dynamic_cast<LinkInterface*>(link);
-    if (linkInterface)
-    {
-        removeLink(linkInterface);
-    }
+    // Be careful of the fact that by the time this signal makes it through the queue
+    // the link object has already been destructed.
+    removeLink((LinkInterface*)link);
 }
 
 bool LinkManager::removeLink(LinkInterface* link)
 {
     if(link)
     {
-        dataMutex.lock();
-        for (int i=0; i < QList<LinkInterface*>(links).size(); i++)
+        _dataMutex.lock();
+        for (int i=0; i < _links.size(); i++)
         {
-            if(link==links.at(i))
+            if(link==_links.at(i))
             {
-                links.removeAt(i); //remove from link list
+                _links.removeAt(i); //remove from link list
             }
         }
         // Remove link from protocol map
-        QList<ProtocolInterface* > protocols = protocolLinks.keys(link);
+        QList<ProtocolInterface* > protocols = _protocolLinks.keys(link);
         foreach (ProtocolInterface* proto, protocols)
         {
-            protocolLinks.remove(proto, link);
+            _protocolLinks.remove(proto, link);
         }
-        dataMutex.unlock();
+        _dataMutex.unlock();
 
         // Emit removal of link
         emit linkRemoved(link);
@@ -218,16 +234,18 @@ bool LinkManager::removeLink(LinkInterface* link)
  */
 LinkInterface* LinkManager::getLinkForId(int id)
 {
-    dataMutex.lock();
+    _dataMutex.lock();
     LinkInterface* linkret = NULL;
-    foreach (LinkInterface* link, links)
+    foreach (LinkInterface* link, _links)
     {
+        Q_ASSERT(link);
+        
         if (link->getId() == id)
         {
             linkret = link;
         }
     }
-    dataMutex.unlock();
+    _dataMutex.unlock();
     return linkret;
 }
 
@@ -236,25 +254,49 @@ LinkInterface* LinkManager::getLinkForId(int id)
  */
 const QList<LinkInterface*> LinkManager::getLinks()
 {
-    dataMutex.lock();
-    QList<LinkInterface*> ret(links);
-    dataMutex.unlock();
+    _dataMutex.lock();
+    QList<LinkInterface*> ret(_links);
+    _dataMutex.unlock();
     return ret;
 }
 
 const QList<SerialLink*> LinkManager::getSerialLinks()
 {
-    dataMutex.lock();
+    _dataMutex.lock();
     QList<SerialLink*> s;
 
-    foreach (LinkInterface* i, links)
+    foreach (LinkInterface* link, _links)
     {
-        SerialLink* link = qobject_cast<SerialLink*>(i);
+        Q_ASSERT(link);
+        
+        SerialLink* serialLink = qobject_cast<SerialLink*>(link);
 
-        if (link)
-            s.append(link);
+        if (serialLink)
+            s.append(serialLink);
     }
-    dataMutex.unlock();
+    _dataMutex.unlock();
 
     return s;
 }
+
+/// @brief If all new connections should be suspended a message is displayed to the user and true
+///         is returned;
+bool LinkManager::_connectionsSuspendedMsg(void)
+{
+    if (_connectionsSuspended) {
+        QMessageBox::information(MainWindow::instance(),
+                                 tr("Connect not allowed"),
+                                 tr("Connect not allowed: %1").arg(_connectionsSuspendedReason));
+        return true;
+    } else {
+        return false;
+    }
+}
+
+void LinkManager::setConnectionsSuspended(QString reason)
+{
+    _connectionsSuspended = true;
+    _connectionsSuspendedReason = reason;
+    Q_ASSERT(!reason.isEmpty());
+}
+

src/comm/LinkManager.h

@@ -71,8 +71,15 @@ public:
 
     /** @brief Get a list of all protocols */
     const QList<ProtocolInterface*> getProtocols() {
-        return protocolLinks.uniqueKeys();
+        return _protocolLinks.uniqueKeys();
     }
+    
+    /// @brief Sets the lag to suspend the all new connections
+    ///     @param reason User visible reason to suspend connections
+    void setConnectionsSuspended(QString reason);
+    
+    /// @brief Sets the flag to allow new connections to be made
+    void setConnectionsAllowed(void) { _connectionsSuspended = false; }
 
 public slots:
 
@@ -88,19 +95,22 @@ public slots:
     bool disconnectAll();
     bool disconnectLink(LinkInterface* link);
 
-protected:
-    LinkManager();
-    QList<LinkInterface*> links;
-    QMultiMap<ProtocolInterface*,LinkInterface*> protocolLinks;
-    QMutex dataMutex;
-
-private:
-    static LinkManager* _instance;
-
 signals:
     void newLink(LinkInterface* link);
     void linkRemoved(LinkInterface* link);
-
+    
+private:
+    LinkManager(void);
+    
+    QList<LinkInterface*> _links;
+    QMultiMap<ProtocolInterface*,LinkInterface*> _protocolLinks;
+    QMutex _dataMutex;
+    
+    bool _connectionsSuspendedMsg(void);
+    static LinkManager* _instance;
+    
+    bool _connectionsSuspended;              ///< true: all new connections should not be allowed
+    QString _connectionsSuspendedReason;     ///< User visible reason for suspension
 };
 
-#endif // _LINKMANAGER_H_
+#endif

src/comm/OpalLink.cc

@@ -457,7 +457,7 @@ int8_t OpalLink::rescaleControllerOutput(double raw)
     return static_cast<int8_t>((raw>=0?raw*127:raw*128));
 }
 
-bool OpalLink::connect()
+bool OpalLink::_connect(void)
 {
     short modelState;
 
@@ -480,7 +480,7 @@ bool OpalLink::connect()
     return connectState;
 }
 
-bool OpalLink::disconnect()
+bool OpalLink::_disconnect(void)
 {
     // OpalDisconnect returns void so its success or failure cannot be tested
     OpalDisconnect();

src/comm/OpalLink.h

@@ -79,15 +79,16 @@ public:
     qint64 getCurrentInDataRate() const;
     qint64 getCurrentOutDataRate() const;
 
-    bool connect();
-
-    bool disconnect();
-
     void run();
 
     int getOpalInstID() {
         return static_cast<int>(opalInstID);
     }
+    
+    // These are left unimplemented in order to cause linker errors which indicate incorrect usage of
+    // connect/disconnect on link directly. All connect/disconnect calls should be made through LinkManager.
+    bool connect(void);
+    bool disconnect(void);
 
 public slots:
 
@@ -144,6 +145,11 @@ protected:
     bool sendRCValues;
     bool sendRawController;
     bool sendPosition;
+    
+private:
+    // From LinkInterface
+    virtual bool _connect(void);
+    virtual bool _disconnect(void);
 };
 
 #endif // OPALLINK_H

src/comm/SerialLink.cc

@@ -85,7 +85,7 @@ void SerialLink::requestReset()
 
 SerialLink::~SerialLink()
 {
-    disconnect();
+    _disconnect();
     if(m_port) delete m_port;
     m_port = NULL;
 
@@ -395,7 +395,7 @@ void SerialLink::readBytes()
  *
  * @return True if connection has been disconnected, false if connection couldn't be disconnected.
  **/
-bool SerialLink::disconnect()
+bool SerialLink::_disconnect(void)
 {
     if (isRunning())
     {
@@ -405,8 +405,6 @@ bool SerialLink::disconnect()
         }
         wait(); // This will terminate the thread and close the serial port
 
-        emit connected(false);
-        emit disconnected();
         return true;
     }
 
@@ -421,11 +419,11 @@ bool SerialLink::disconnect()
  *
  * @return True if connection has been established, false if connection couldn't be established.
  **/
-bool SerialLink::connect()
+bool SerialLink::_connect(void)
 {   
     qDebug() << "CONNECT CALLED";
     if (isRunning())
-        disconnect();
+        _disconnect();
     {
         QMutexLocker locker(&this->m_stoppMutex);
         m_stopp = false;
@@ -436,12 +434,12 @@ bool SerialLink::connect()
 }
 
 /**
- * @brief This function is called indirectly by the connect() call.
+ * @brief This function is called indirectly by the _connect() call.
  *
- * The connect() function starts the thread and indirectly calls this method.
+ * The _connect() function starts the thread and indirectly calls this method.
  *
  * @return True if the connection could be established, false otherwise
- * @see connect() For the right function to establish the connection.
+ * @see _connect() For the right function to establish the connection.
  **/
 bool SerialLink::hardwareConnect(QString &type)
 {

src/comm/SerialLink.h

@@ -111,6 +111,11 @@ public:
     void run2();
 
     int getId() const;
+    
+    // These are left unimplemented in order to cause linker errors which indicate incorrect usage of
+    // connect/disconnect on link directly. All connect/disconnect calls should be made through LinkManager.
+    bool connect(void);
+    bool disconnect(void);
 
 signals: //[TODO] Refactor to Linkinterface
     void updateLink(LinkInterface*);
@@ -139,8 +144,6 @@ public slots:
      * @param size The size of the bytes array
      **/
     void writeBytes(const char* data, qint64 length);
-    bool connect();
-    bool disconnect();
 
     void linkError(QSerialPort::SerialPortError error);
 
@@ -161,6 +164,10 @@ protected:
     bool m_is_cdc;
 
 private:
+    // From LinkInterface
+    virtual bool _connect(void);
+    virtual bool _disconnect(void);
+
     volatile bool m_stopp;
     volatile bool m_reqReset;
     QMutex m_stoppMutex; // Mutex for accessing m_stopp

src/comm/TCPLink.cc

@@ -55,7 +55,7 @@ TCPLink::TCPLink(QHostAddress hostAddress, quint16 socketPort) :
 
 TCPLink::~TCPLink()
 {
-    disconnect();
+    _disconnect();
 
     // Tell the thread to exit
     quit();
@@ -77,7 +77,7 @@ void TCPLink::setHostAddress(QHostAddress hostAddress)
     bool reconnect = false;
     
 	if (this->isConnected()) {
-		disconnect();
+		_disconnect();
 		reconnect = true;
 	}
     
@@ -85,7 +85,7 @@ void TCPLink::setHostAddress(QHostAddress hostAddress)
     _resetName();
     
 	if (reconnect) {
-		connect();
+		_connect();
 	}
 }
 
@@ -99,7 +99,7 @@ void TCPLink::setPort(int port)
     bool reconnect = false;
     
 	if (this->isConnected()) {
-		disconnect();
+		_disconnect();
 		reconnect = true;
 	}
     
@@ -107,7 +107,7 @@ void TCPLink::setPort(int port)
     _resetName();
     
 	if (reconnect) {
-		connect();
+		_connect();
 	}
 }
 
@@ -181,7 +181,7 @@ void TCPLink::readBytes()
  *
  * @return True if connection has been disconnected, false if connection couldn't be disconnected.
  **/
-bool TCPLink::disconnect()
+bool TCPLink::_disconnect(void)
 {
 	quit();
 	wait();
@@ -204,7 +204,7 @@ bool TCPLink::disconnect()
  *
  * @return True if connection has been established, false if connection couldn't be established.
  **/
-bool TCPLink::connect()
+bool TCPLink::_connect(void)
 {
 	if (isRunning())
 	{

src/comm/TCPLink.h

@@ -45,10 +45,14 @@
 
 //#define TCPLINK_READWRITE_DEBUG   // Use to debug data reads/writes
 
+class TCPLinkUnitTest;
+
 class TCPLink : public LinkInterface
 {
     Q_OBJECT
     
+    friend class TCPLinkUnitTest;
+    
 public:
     TCPLink(QHostAddress hostAddress = QHostAddress::LocalHost, quint16 socketPort = 5760);
     ~TCPLink();
@@ -65,8 +69,6 @@ public:
     virtual int     getId(void) const;
     virtual QString getName(void) const;
     virtual bool    isConnected(void) const;
-    virtual bool    connect(void);
-    virtual bool    disconnect(void);
     virtual void    requestReset(void) {};
 
     // Extensive statistics for scientific purposes
@@ -74,6 +76,11 @@ public:
     qint64 getCurrentInDataRate() const;
     qint64 getCurrentOutDataRate() const;
     
+    // These are left unimplemented in order to cause linker errors which indicate incorrect usage of
+    // connect/disconnect on link directly. All connect/disconnect calls should be made through LinkManager.
+    bool connect(void);
+    bool disconnect(void);
+    
 public slots:
     void setHostAddress(const QString& hostAddress);
     void setPort(int port);
@@ -95,6 +102,10 @@ protected:
     virtual void run(void);
     
 private:
+    // From LinkInterface
+    virtual bool _connect(void);
+    virtual bool _disconnect(void);
+
     void _resetName(void);
 	bool _hardwareConnect(void);
 #ifdef TCPLINK_READWRITE_DEBUG

src/comm/UDPLink.cc

@@ -59,7 +59,7 @@ UDPLink::UDPLink(QHostAddress host, quint16 port) :
 
 UDPLink::~UDPLink()
 {
-    disconnect();
+    _disconnect();
 
     // Tell the thread to exit
     quit();
@@ -85,13 +85,13 @@ void UDPLink::setAddress(QHostAddress host)
     bool reconnect(false);
 	if(this->isConnected())
 	{
-		disconnect();
+		_disconnect();
 		reconnect = true;
 	}
 	this->host = host;
 	if(reconnect)
 	{
-		connect();
+		_connect();
 	}
 }
 
@@ -100,7 +100,7 @@ void UDPLink::setPort(int port)
 	bool reconnect(false);
 	if(this->isConnected())
 	{
-		disconnect();
+		_disconnect();
 		reconnect = true;
 	}
     this->port = port;
@@ -108,7 +108,7 @@ void UDPLink::setPort(int port)
 	emit nameChanged(this->name);
 	if(reconnect)
 	{
-		connect();
+		_connect();
 	}
 }
 
@@ -272,7 +272,7 @@ void UDPLink::readBytes()
  *
  * @return True if connection has been disconnected, false if connection couldn't be disconnected.
  **/
-bool UDPLink::disconnect()
+bool UDPLink::_disconnect(void)
 {
 	this->quit();
 	this->wait();
@@ -296,7 +296,7 @@ bool UDPLink::disconnect()
  *
  * @return True if connection has been established, false if connection couldn't be established.
  **/
-bool UDPLink::connect()
+bool UDPLink::_connect(void)
 {
 	if(this->isRunning())
 	{

src/comm/UDPLink.h

@@ -78,6 +78,11 @@ public:
     void run();
 
     int getId() const;
+    
+    // These are left unimplemented in order to cause linker errors which indicate incorrect usage of
+    // connect/disconnect on link directly. All connect/disconnect calls should be made through LinkManager.
+    bool connect(void);
+    bool disconnect(void);
 
 public slots:
     void setAddress(QHostAddress host);
@@ -96,8 +101,6 @@ public slots:
      * @param size The size of the bytes array
      **/
     void writeBytes(const char* data, qint64 length);
-    bool connect();
-    bool disconnect();
 
 protected:
     QString name;
@@ -114,6 +117,10 @@ protected:
     void setName(QString name);
 
 private:
+    // From LinkInterface
+    virtual bool _connect(void);