Fix LinkManager link removal

src/comm/LinkInterface.h

@@ -138,6 +138,11 @@ public:
     {
         return getCurrentDataRate(outDataIndex, outDataWriteTimes, outDataWriteAmounts);
     }
+    
+    // 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:
 

src/comm/LinkManager.cc

@@ -92,7 +92,7 @@ void LinkManager::add(LinkInterface* link)
     
     if (!_links.contains(link))
     {
-        connect(link, SIGNAL(destroyed(QObject*)), this, SLOT(removeObj(QObject*)));
+        connect(link, SIGNAL(destroyed(QObject*)), this, SLOT(_removeLink(QObject*)));
         _links.append(link);
         _dataMutex.unlock();
         emit newLink(link);
@@ -174,7 +174,7 @@ bool LinkManager::disconnectAll()
     foreach (LinkInterface* link, _links)
     {
         Q_ASSERT(link);
-        if (!link->disconnect()) {
+        if (!disconnectLink(link)) {
             allDisconnected = false;
         }
     }
@@ -200,39 +200,33 @@ bool LinkManager::disconnectLink(LinkInterface* link)
     return link->_disconnect();
 }
 
-void LinkManager::removeObj(QObject* link)
+void LinkManager::_removeLink(QObject* obj)
 {
     // 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)
+    
+    Q_ASSERT(obj);
+    
+    LinkInterface* link = static_cast<LinkInterface*>(obj);
+    
+    _dataMutex.lock();
+    for (int i=0; i < _links.size(); i++)
     {
-        _dataMutex.lock();
-        for (int i=0; i < _links.size(); i++)
-        {
-            if(link==_links.at(i))
-            {
-                _links.removeAt(i); //remove from link list
-            }
-        }
-        // Remove link from protocol map
-        QList<ProtocolInterface* > protocols = _protocolLinks.keys(link);
-        foreach (ProtocolInterface* proto, protocols)
+        if(link==_links.at(i))
         {
-            _protocolLinks.remove(proto, link);
+            _links.removeAt(i); //remove from link list
         }
-        _dataMutex.unlock();
-
-        // Emit removal of link
-        emit linkRemoved(link);
-
-        return true;
     }
-    return false;
+    // Remove link from protocol map
+    QList<ProtocolInterface* > protocols = _protocolLinks.keys(link);
+    foreach (ProtocolInterface* proto, protocols)
+    {
+        _protocolLinks.remove(proto, link);
+    }
+    _dataMutex.unlock();
+
+    // Emit removal of link
+    emit linkRemoved(link);
 }
 
 /**

src/comm/LinkManager.h

@@ -88,13 +88,11 @@ public:
     void setConnectionsAllowed(void) { _connectionsSuspended = false; }
 
 public slots:
-
+    /// @brief Adds the link to the LinkManager. No need to remove a link you added. Just delete it and LinkManager will pick up on
+    ///         that and remove the link from the list.
     void add(LinkInterface* link);
     void addProtocol(LinkInterface* link, ProtocolInterface* protocol);
 
-    void removeObj(QObject* obj);
-    bool removeLink(LinkInterface* link);
-
     bool connectAll();
     bool connectLink(LinkInterface* link);
 
@@ -111,6 +109,8 @@ private:
     
     static LinkManager* _instance;
     
+    void _removeLink(QObject* obj);
+    
     QList<LinkInterface*> _links;
     QMultiMap<ProtocolInterface*,LinkInterface*> _protocolLinks;
     QMutex _dataMutex;

src/qgcunittest/LinkManagerTest.cc

+#include "UASUnitTest.h"
+#include <stdio.h>
+#include <QObject>
+#include <QThread>
+
+UASUnitTest::UASUnitTest()
+{
+}
+//This function is called after every test
+void UASUnitTest::init()
+{
+    mav = new MAVLinkProtocol();
+    uas = new UAS(mav, QThread::currentThread(), UASID);
+    uas->deleteSettings();
+}
+//this function is called after every test
+void UASUnitTest::cleanup()
+{
+    delete uas;
+    uas = NULL;
+
+    delete mav;
+    mav = NULL;
+}
+
+void UASUnitTest::getUASID_test()
+{
+    // Test a default ID of zero is assigned
+    UAS* uas2 = new UAS(mav, QThread::currentThread());
+    QCOMPARE(uas2->getUASID(), 0);
+    delete uas2;
+
+    // Test that the chosen ID was assigned at construction
+    QCOMPARE(uas->getUASID(), UASID);
+
+    // Make sure that no other ID was set
+    QEXPECT_FAIL("", "When you set an ID it does not use the default ID of 0", Continue);
+    QCOMPARE(uas->getUASID(), 0);
+
+    // Make sure that ID >= 0
+    QCOMPARE(uas->getUASID(), 100);
+
+}
+
+void UASUnitTest::getUASName_test()
+{
+  // Test that the name is build as MAV + ID
+  QCOMPARE(uas->getUASName(), "MAV " + QString::number(UASID));
+
+}
+
+void UASUnitTest::getUpTime_test()
+{
+    UAS* uas2 = new UAS(mav, QThread::currentThread());
+    // Test that the uptime starts at zero to a
+    // precision of seconds
+    QCOMPARE(floor(uas2->getUptime()/1000.0), 0.0);
+
+    // Sleep for three seconds
+    QTest::qSleep(3000);
+
+    // Test that the up time is computed correctly to a
+    // precision of seconds
+    QCOMPARE(floor(uas2->getUptime()/1000.0), 3.0);
+
+    delete uas2;
+}
+
+void UASUnitTest::getCommunicationStatus_test()
+{
+    // Verify that upon construction the Comm status is disconnected
+    QCOMPARE(uas->getCommunicationStatus(), static_cast<int>(UASInterface::COMM_DISCONNECTED));
+}
+
+void UASUnitTest::filterVoltage_test()
+{
+    float verificar=uas->filterVoltage(0.4f);
+
+    // We allow the voltage returned to be within a small delta
+    const float allowedDelta = 0.05f;
+    const float desiredVoltage = 7.36f;
+    QVERIFY(verificar > (desiredVoltage - allowedDelta) && verificar < (desiredVoltage + allowedDelta));
+}
+
+void UASUnitTest:: getAutopilotType_test()
+{
+    int type = uas->getAutopilotType();
+    // Verify that upon construction the autopilot is set to -1
+    QCOMPARE(type, -1);
+}
+
+void UASUnitTest::setAutopilotType_test()
+{
+    uas->setAutopilotType(2);
+    // Verify that the autopilot is set
+    QCOMPARE(uas->getAutopilotType(), 2);
+}
+
+//verify that the correct status is returned if a certain statue is given to uas
+void UASUnitTest::getStatusForCode_test()
+{
+    QString state, desc;
+    state = "";
+    desc = "";
+
+    uas->getStatusForCode(MAV_STATE_UNINIT, state, desc);
+    QVERIFY(state == "UNINIT");
+
+    uas->getStatusForCode(MAV_STATE_UNINIT, state, desc);
+    QVERIFY(state == "UNINIT");
+
+    uas->getStatusForCode(MAV_STATE_BOOT, state, desc);
+    QVERIFY(state == "BOOT");
+
+    uas->getStatusForCode(MAV_STATE_CALIBRATING, state, desc);
+    QVERIFY(state == "CALIBRATING");
+
+    uas->getStatusForCode(MAV_STATE_ACTIVE, state, desc);
+    QVERIFY(state == "ACTIVE");
+
+    uas->getStatusForCode(MAV_STATE_STANDBY, state, desc);
+    QVERIFY(state == "STANDBY");
+
+    uas->getStatusForCode(MAV_STATE_CRITICAL, state, desc);
+    QVERIFY(state == "CRITICAL");
+
+    uas->getStatusForCode(MAV_STATE_EMERGENCY, state, desc);
+    QVERIFY(state == "EMERGENCY");
+
+    uas->getStatusForCode(MAV_STATE_POWEROFF, state, desc);
+    QVERIFY(state == "SHUTDOWN");
+
+    uas->getStatusForCode(5325, state, desc);
+    QVERIFY(state == "UNKNOWN");
+}
+
+void UASUnitTest::getLocalX_test()
+{
+   QCOMPARE(uas->getLocalX(), 0.0);
+}
+void UASUnitTest::getLocalY_test()
+{
+    QCOMPARE(uas->getLocalY(), 0.0);
+}
+void UASUnitTest::getLocalZ_test()
+{
+    QCOMPARE(uas->getLocalZ(), 0.0);
+}
+void UASUnitTest::getLatitude_test()
+{
+    QCOMPARE(uas->getLatitude(), 0.0);
+}
+void UASUnitTest::getLongitude_test()
+{
+    QCOMPARE(uas->getLongitude(), 0.0);
+}
+void UASUnitTest::getAltitudeAMSL_test()
+{
+    QCOMPARE(uas->getAltitudeAMSL(), 0.0);
+}
+void UASUnitTest::getAltitudeRelative_test()
+{
+    QCOMPARE(uas->getAltitudeRelative(), 0.0);
+}
+void UASUnitTest::getRoll_test()
+{
+    QCOMPARE(uas->getRoll(), 0.0);
+}
+void UASUnitTest::getPitch_test()
+{
+    QCOMPARE(uas->getPitch(), 0.0);
+}
+void UASUnitTest::getYaw_test()
+{
+    QCOMPARE(uas->getYaw(), 0.0);
+}
+
+void UASUnitTest::getSelected_test()
+{
+    QCOMPARE(uas->getSelected(), false);
+}
+
+void UASUnitTest::getSystemType_test()
+{    //check that system type is set to MAV_TYPE_GENERIC when initialized
+    QCOMPARE(uas->getSystemType(), 0);
+    uas->setSystemType(13);
+    QCOMPARE(uas->getSystemType(), 13);
+}
+
+void UASUnitTest::getAirframe_test()
+{
+    //when uas is constructed, airframe is set to QGC_AIRFRAME_GENERIC
+    QVERIFY(uas->getAirframe() == UASInterface::QGC_AIRFRAME_GENERIC);
+}
+
+void UASUnitTest::setAirframe_test()
+{
+    //check at construction, that airframe=0 (GENERIC)
+    QVERIFY(uas->getAirframe() == UASInterface::QGC_AIRFRAME_GENERIC);
+
+    //check that set airframe works
+    uas->setAirframe(UASInterface::QGC_AIRFRAME_HEXCOPTER);
+    QVERIFY(uas->getAirframe() == UASInterface::QGC_AIRFRAME_HEXCOPTER);
+
+    //check that setAirframe will not assign a number to airframe, that is 
+    //not defined in the enum 
+    uas->setAirframe(UASInterface::QGC_AIRFRAME_END_OF_ENUM);
+    QVERIFY(uas->getAirframe() == UASInterface::QGC_AIRFRAME_HEXCOPTER);
+}
+
+void UASUnitTest::getWaypointList_test()
+{
+    QList<Waypoint*> kk = uas->getWaypointManager()->getWaypointEditableList();
+    QCOMPARE(kk.count(), 0);
+
+    Waypoint* wp = new Waypoint(0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,false, false, MAV_FRAME_GLOBAL, MAV_CMD_MISSION_START, "blah");
+    uas->getWaypointManager()->addWaypointEditable(wp, true);
+
+    kk = uas->getWaypointManager()->getWaypointEditableList();
+    QCOMPARE(kk.count(), 1);
+
+    wp = new Waypoint();
+    uas->getWaypointManager()->addWaypointEditable(wp, false);
+
+    kk = uas->getWaypointManager()->getWaypointEditableList();
+    QCOMPARE(kk.count(), 2);
+
+    uas->getWaypointManager()->removeWaypoint(1);
+    kk = uas->getWaypointManager()->getWaypointEditableList();
+    QCOMPARE(kk.count(), 1);
+
+    uas->getWaypointManager()->removeWaypoint(0);
+    kk = uas->getWaypointManager()->getWaypointEditableList();
+    QCOMPARE(kk.count(), 0);
+
+    qDebug()<<"disconnect SIGNAL waypointListChanged";
+
+}
+
+void UASUnitTest::getWaypoint_test()
+{
+    Waypoint* wp = new Waypoint(0,5.6,2.0,3.0,0.0,0.0,0.0,0.0,false, false, MAV_FRAME_GLOBAL, MAV_CMD_MISSION_START, "blah");
+
+    uas->getWaypointManager()->addWaypointEditable(wp, true);
+
+    QList<Waypoint*> wpList = uas->getWaypointManager()->getWaypointEditableList();
+
+    QCOMPARE(wpList.count(), 1);
+    QCOMPARE(static_cast<quint16>(0), static_cast<Waypoint*>(wpList.at(0))->getId());
+
+    Waypoint*  wp3 = new Waypoint(1, 5.6, 2.0, 3.0);
+    uas->getWaypointManager()->addWaypointEditable(wp3, true);
+    wpList = uas->getWaypointManager()->getWaypointEditableList();
+    Waypoint* wp2 = static_cast<Waypoint*>(wpList.at(0));
+
+    QCOMPARE(wpList.count(), 2);
+    QCOMPARE(wp3->getX(), wp2->getX());
+    QCOMPARE(wp3->getY(), wp2->getY());
+    QCOMPARE(wp3->getZ(), wp2->getZ());
+    QCOMPARE(wpList.at(1)->getId(), static_cast<quint16>(1));
+    QCOMPARE(wp3->getFrame(), MAV_FRAME_GLOBAL);
+    QCOMPARE(wp3->getFrame(), wp2->getFrame());
+
+    delete wp3;
+    delete wp;
+}
+
+void UASUnitTest::signalWayPoint_test()
+{
+    QSignalSpy spy(uas->getWaypointManager(), SIGNAL(waypointEditableListChanged()));
+
+    Waypoint* wp = new Waypoint(0,1.0,0.0,0.0,0.0,0.0,0.0,0.0,false, false, MAV_FRAME_GLOBAL, MAV_CMD_MISSION_START, "blah");
+    uas->getWaypointManager()->addWaypointEditable(wp, true);
+
+    QCOMPARE(spy.count(), 1); // 1 listChanged for add wayPoint
+    uas->getWaypointManager()->removeWaypoint(0);
+    QCOMPARE(spy.count(), 2); // 2 listChanged for remove wayPoint
+
+    QSignalSpy spyDestroyed(uas->getWaypointManager(), SIGNAL(destroyed()));
+    QVERIFY(spyDestroyed.isValid());
+    QCOMPARE( spyDestroyed.count(), 0 );
+
+    delete uas;// delete(destroyed) uas for validating
+    uas = NULL;
+    QCOMPARE(spyDestroyed.count(), 1);// count destroyed uas should are 1
+    uas = new UAS(mav, QThread::currentThread(), UASID);
+    QSignalSpy spy2(uas->getWaypointManager(), SIGNAL(waypointEditableListChanged()));
+    QCOMPARE(spy2.count(), 0);
+    Waypoint* wp2 = new Waypoint(0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,false, false, MAV_FRAME_GLOBAL, MAV_CMD_MISSION_START, "blah");
+
+    uas->getWaypointManager()->addWaypointEditable(wp2, true);
+    QCOMPARE(spy2.count(), 1);
+
+    uas->getWaypointManager()->clearWaypointList();
+    QList<Waypoint*> wpList = uas->getWaypointManager()->getWaypointEditableList();
+    QCOMPARE(wpList.count(), 1);
+    delete uas;
+    uas = NULL;
+    delete wp2;
+}
+
+void UASUnitTest::signalUASLink_test()
+{
+    QSignalSpy spy(uas, SIGNAL(modeChanged(int,QString,QString)));
+    uas->setMode(2, 0);
+    QCOMPARE(spy.count(), 0);// not solve for UAS not receiving message from UAS
+
+    QSignalSpy spyS(LinkManager::instance(), SIGNAL(newLink(LinkInterface*)));
+    SerialLink* link = new SerialLink();
+
+    LinkManager::instance()->add(link);
+    LinkManager::instance()->addProtocol(link, mav);
+    QCOMPARE(spyS.count(), 1);
+
+    LinkManager::instance()->add(link);
+    LinkManager::instance()->addProtocol(link, mav);
+    QCOMPARE(spyS.count(), 1);// not add SerialLink, exist in list
+
+    SerialLink* link2 = new SerialLink();
+    LinkManager::instance()->add(link2);
+    LinkManager::instance()->addProtocol(link2, mav);
+    QCOMPARE(spyS.count(), 2);// add SerialLink, not exist in list
+
+    QList<LinkInterface*> links = LinkManager::instance()->getLinks();
+    foreach(LinkInterface* link, links)
+    {
+        qDebug()<< link->getName();
+        qDebug()<< QString::number(link->getId());
+        QVERIFY(link != NULL);
+        uas->addLink(link);
+    }
+
+    SerialLink* ff = static_cast<SerialLink*>(uas->getLinks()->at(0));
+
+    QCOMPARE(ff->isConnected(), false);
+
+    QCOMPARE(ff->isRunning(), false);
+
+    QCOMPARE(ff->isFinished(), false);
+
+    QCOMPARE(links.count(), uas->getLinks()->count());
+    QCOMPARE(uas->getLinks()->count(), 2);
+
+    delete link2;
+    QCOMPARE(LinkManager::instance()->getLinks().count(), 1);
+    QCOMPARE(uas->getLinks()->count(), 1);
+
+    QCOMPARE(static_cast<LinkInterface*>(LinkManager::instance()->getLinks().at(0))->getId(),
+             static_cast<LinkInterface*>(uas->getLinks()->at(0))->getId());
+
+    SerialLink* link3 = new SerialLink();
+    LinkManager::instance()->add(link3);
+    LinkManager::instance()->addProtocol(link3, mav);
+    QCOMPARE(spyS.count(), 3);
+    QCOMPARE(LinkManager::instance()->getLinks().count(), 2);
+
+    //all the links in LinkManager must be deleted because LinkManager::instance
+    //is static. 
+    delete link3;
+    QCOMPARE(LinkManager::instance()->getLinks().count(), 1);
+    delete link;
+
+    QCOMPARE(LinkManager::instance()->getLinks().count(), 0);
+}
+
+void UASUnitTest::signalIdUASLink_test()
+{
+
+    QCOMPARE(LinkManager::instance()->getLinks().count(), 0);
+    SerialLink* myLink = new SerialLink();
+    myLink->setPortName("COM 17");
+    LinkManager::instance()->add(myLink);
+    LinkManager::instance()->addProtocol(myLink, mav);
+
+    SerialLink* myLink2 = new SerialLink();
+    myLink2->setPortName("COM 18");
+    LinkManager::instance()->add(myLink2);
+    LinkManager::instance()->addProtocol(myLink2, mav);
+
+    SerialLink* myLink3 = new SerialLink();
+    myLink3->setPortName("COM 19");
+    LinkManager::instance()->add(myLink3);
+    LinkManager::instance()->addProtocol(myLink3, mav);
+
+    SerialLink* myLink4 = new SerialLink();
+    myLink4->setPortName("COM 20");
+    LinkManager::instance()->add(myLink4);
+    LinkManager::instance()->addProtocol(myLink4, mav);
+
+    QCOMPARE(LinkManager::instance()->getLinks().count(), 4);
+
+    QList<LinkInterface*> links = LinkManager::instance()->getLinks();
+
+    LinkInterface* a = static_cast<LinkInterface*>(links.at(0));
+    LinkInterface* b = static_cast<LinkInterface*>(links.at(1));
+    LinkInterface* c = static_cast<LinkInterface*>(links.at(2));
+    LinkInterface* d = static_cast<LinkInterface*>(links.at(3));
+    QCOMPARE(a->getName(), QString("COM 17"));
+    QCOMPARE(b->getName(), QString("COM 18"));
+    QCOMPARE(c->getName(), QString("COM 19"));
+    QCOMPARE(d->getName(), QString("COM 20"));
+
+    delete myLink4;
+    delete myLink3;
+    delete myLink2;
+    delete myLink;
+
+    QCOMPARE(LinkManager::instance()->getLinks().count(), 0);
+}

src/qgcunittest/LinkManagerTest.h

+/*=====================================================================
+ 
+ QGroundControl Open Source Ground Control Station
+ 
+ (c) 2009 - 2014 QGROUNDCONTROL PROJECT <http://www.qgroundcontrol.org>
+ 
+ This file is part of the QGROUNDCONTROL project
+ 
+ QGROUNDCONTROL is free software: you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+ 
+ QGROUNDCONTROL is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU General Public License for more details.
+ 
+ You should have received a copy of the GNU General Public License
+ along with QGROUNDCONTROL. If not, see <http://www.gnu.org/licenses/>.
+ 
+ ======================================================================*/
+
+#ifndef UASUNITTEST_H
+#define UASUNITTEST_H
+
+#include <QObject>
+
+#include "LinkManager.h"
+
+/// @file
+///     @brief LinkManager Unit Test
+///
+///     @author Don Gagne <don@thegagnes.com>
+
+
+class LinkManagerTest : public QObject
+{
+    Q_OBJECT
+    
+public:
+    LinkManagerTest(void);
+    
+private slots:
+    void initTestCase(void);
+    void init(void);
+    void cleanup(void);
+    
+    void _setUAS_test(void);
+    void _minRCChannels_test(void);
+    void _fullCalibration_test(void);
+};
+
+DECLARE_TEST(LinkManagerTest)
+
+#endif

src/qgcunittest/UASUnitTest.cc

@@ -341,10 +341,7 @@ void UASUnitTest::signalUASLink_test()
     QCOMPARE(links.count(), uas->getLinks()->count());
     QCOMPARE(uas->getLinks()->count(), 2);
 
-    LinkInterface* ff99 = static_cast<LinkInterface*>(links.at(1));
-    LinkManager::instance()->removeLink(ff99);
     delete link2;
-
     QCOMPARE(LinkManager::instance()->getLinks().count(), 1);
     QCOMPARE(uas->getLinks()->count(), 1);
 
@@ -359,10 +356,8 @@ void UASUnitTest::signalUASLink_test()
 
     //all the links in LinkManager must be deleted because LinkManager::instance
     //is static. 
-    LinkManager::instance()->removeLink(link3);
     delete link3;
     QCOMPARE(LinkManager::instance()->getLinks().count(), 1);
-    LinkManager::instance()->removeLink(link);
     delete link;
 
     QCOMPARE(LinkManager::instance()->getLinks().count(), 0);
@@ -405,13 +400,9 @@ void UASUnitTest::signalIdUASLink_test()
     QCOMPARE(c->getName(), QString("COM 19"));
     QCOMPARE(d->getName(), QString("COM 20"));
 
-    LinkManager::instance()->removeLink(myLink4);
     delete myLink4;
-    LinkManager::instance()->removeLink(myLink3);
     delete myLink3;
-    LinkManager::instance()->removeLink(myLink2);
     delete myLink2;
-    LinkManager::instance()->removeLink(myLink);
     delete myLink;
 
     QCOMPARE(LinkManager::instance()->getLinks().count(), 0);

src/ui/CommConfigurationWindow.cc

@@ -376,10 +376,7 @@ void CommConfigurationWindow::remove()
     action=NULL;
 
     if(link) {
-        LinkManager::instance()->removeLink(link); //remove link from LinkManager list
-        link->disconnect(); //disconnect port, and also calls terminate() to stop the thread
-        if (link->isRunning()) link->terminate(); // terminate() the serial thread just in case it is still running
-        link->wait(); // wait() until thread is stoped before deleting
+        LinkManager::instance()->disconnectLink(link);  // disconnect connection
         link->deleteLater();
     }
     link=NULL;

src/ui/QGCMAVLinkLogPlayer.cc

@@ -330,7 +330,6 @@ bool QGCMAVLinkLogPlayer::loadLogFile(const QString& file)
     if (logLink)
     {
         LinkManager::instance()->disconnectLink(logLink);
-        LinkManager::instance()->removeLink(logLink);
         logLink->deleteLater();
     }
     logLink = new MAVLinkSimulationLink("");