From 0dd18040b324ab5ba33359df57d7c5d8d4292e50 Mon Sep 17 00:00:00 2001 From: Valentin Platzgummer Date: Thu, 4 Feb 2021 12:20:11 +0100 Subject: [PATCH] nemo.cpp: deadlock when calling stop during synchronization removed --- .../MeasurementComplexItem.cc | 1 + .../MeasurementComplexItem.h | 2 +- src/MeasurementComplexItem/NemoInterface.cpp | 174 ++++++++++-------- src/MeasurementComplexItem/NemoInterface.h | 2 - .../geometry/MeasurementArea.cc | 23 +-- .../geometry/MeasurementArea.h | 5 +- .../geometry/ProgressArray.h | 1 - .../nemo_interface/TaskDispatcher.cpp | 10 +- .../nemo_interface/TaskDispatcher.h | 3 +- 9 files changed, 107 insertions(+), 114 deletions(-) diff --git a/src/MeasurementComplexItem/MeasurementComplexItem.cc b/src/MeasurementComplexItem/MeasurementComplexItem.cc index 76948c5ea..1ccbb4fcb 100644 --- a/src/MeasurementComplexItem/MeasurementComplexItem.cc +++ b/src/MeasurementComplexItem/MeasurementComplexItem.cc @@ -1,5 +1,6 @@ #include "MeasurementComplexItem.h" +#include "AreaData.h" #include "CircularGenerator.h" #include "LinearGenerator.h" #include "NemoInterface.h" diff --git a/src/MeasurementComplexItem/MeasurementComplexItem.h b/src/MeasurementComplexItem/MeasurementComplexItem.h index f3ec040bc..1e84cff87 100644 --- a/src/MeasurementComplexItem/MeasurementComplexItem.h +++ b/src/MeasurementComplexItem/MeasurementComplexItem.h @@ -9,9 +9,9 @@ #include "QGCQGeoCoordinate.h" #include "SettingsFact.h" -#include "AreaData.h" #include "geometry/ProgressArray.h" +class AreaData; class RoutingThread; class RoutingResult; diff --git a/src/MeasurementComplexItem/NemoInterface.cpp b/src/MeasurementComplexItem/NemoInterface.cpp index 4d2c4477d..8ae036bbb 100644 --- a/src/MeasurementComplexItem/NemoInterface.cpp +++ b/src/MeasurementComplexItem/NemoInterface.cpp @@ -181,7 +181,7 @@ private: // Worker functions. static bool _callAddTiles(const QVector> &tileArray, - Dispatcher &d, Rosbridge &rb); + Dispatcher &disp, Rosbridge &rb); static bool _callClearTiles(Dispatcher &d, Rosbridge &rb); static ProgressArray _callGetProgress(const IDArray &pIdArray, Dispatcher &d, Rosbridge &rb); @@ -880,9 +880,11 @@ void NemoInterface::Impl::_synchronize() { if (!synced) { auto success = this->_clearTilesImpl(); if (!success) { - Lock lk(this->_m); - _setStateNotSafe(STATE::SYNC_ERROR); - _doActionNotSafe(); + Lock lk1(this->_m); + if (this->_state == STATE::SYS_SYNC) { + _setStateNotSafe(STATE::SYNC_ERROR); + _doActionNotSafe(); + } return QVariant(false); } @@ -894,9 +896,11 @@ void NemoInterface::Impl::_synchronize() { success = this->_addTilesImpl(pTileArray, pIdArray); if (!success) { - Lock lk(this->_m); - _setStateNotSafe(STATE::SYNC_ERROR); - _doActionNotSafe(); + Lock lk2(this->_m); + if (this->_state == STATE::SYS_SYNC) { + _setStateNotSafe(STATE::SYNC_ERROR); + _doActionNotSafe(); + } return QVariant(false); } } else { @@ -914,12 +918,14 @@ void NemoInterface::Impl::_synchronize() { if (!ret) { Lock lk(this->_m); - this->_setStateNotSafe(STATE::SYNC_ERROR); - this->_doActionNotSafe(); - lk.unlock(); - this->_setWarningString("Getting progress failed."); - qCDebug(NemoInterfaceLog) - << "_addTilesImpl(): _updateProgress failed: unknown id."; + if (this->_state == STATE::SYS_SYNC) { + this->_setStateNotSafe(STATE::SYNC_ERROR); + this->_doActionNotSafe(); + lk.unlock(); + this->_setWarningString("Getting progress failed."); + qCDebug(NemoInterfaceLog) + << "_addTilesImpl(): _updateProgress failed: unknown id."; + } return QVariant(false); } } @@ -976,7 +982,6 @@ void NemoInterface::Impl::_doActionNotSafe() { static bool resetDone = false; switch (this->_state) { case STATE::STOPPED: { - std::promise p; INVM(this->_parent, ([this]() mutable { this->_timeoutTimer.stop(); this->_restartTimer.stop(); @@ -991,7 +996,6 @@ void NemoInterface::Impl::_doActionNotSafe() { _setInfoStringNotSafe(""); _setWarningStringNotSafe(""); - } break; case STATE::START_BRIDGE: { @@ -999,14 +1003,20 @@ void NemoInterface::Impl::_doActionNotSafe() { ([this]() mutable { this->_restartTimer.start(RESTART_INTERVAl); })) this->_pRosbridge->start(); + _setInfoStringNotSafe(""); + _setWarningStringNotSafe(""); } break; case STATE::WEBSOCKET_DETECTED: resetDone = false; this->_setStateNotSafe(STATE::TRY_SETUP); this->_doActionNotSafe(); + _setInfoStringNotSafe(""); + _setWarningStringNotSafe(""); break; case STATE::TRY_SETUP: this->_doSetup(); + _setInfoStringNotSafe(""); + _setWarningStringNotSafe(""); break; case STATE::READY: { INVM(this->_parent, @@ -1023,10 +1033,13 @@ void NemoInterface::Impl::_doActionNotSafe() { case STATE::USER_SYNC: case STATE::SYS_SYNC: case STATE::SYNC_ERROR: + _setInfoStringNotSafe(""); + _setWarningStringNotSafe(""); break; case STATE::HEARTBEAT_TIMEOUT: { INVM(this->_parent, ([this]() mutable { this->_syncTimer.stop(); })) - + _setInfoStringNotSafe(""); + _setWarningStringNotSafe(""); } break; case STATE::WEBSOCKET_TIMEOUT: { INVM(this->_parent, ([this]() mutable { @@ -1049,7 +1062,7 @@ void NemoInterface::Impl::_doActionNotSafe() { } bool NemoInterface::Impl::_callAddTiles( - const QVector> &tileArray, Dispatcher &d, + const QVector> &tileArray, Dispatcher &disp, Rosbridge &rb) { // qDebug() << "_callAddTiles called"; @@ -1091,14 +1104,14 @@ bool NemoInterface::Impl::_callAddTiles( // wait for response. auto tStart = hrc::now(); bool abort = true; - do { - auto status = future_response.wait_for(std::chrono::milliseconds(100)); + while (hrc::now() - tStart < maxResponseTime && + !disp.isInterruptionRequested()) { + auto status = future_response.wait_for(std::chrono::milliseconds(10)); if (status == std::future_status::ready) { abort = false; break; } - } while (hrc::now() - tStart < maxResponseTime || - d.isInterruptionRequested()); + } if (abort) { qCWarning(NemoInterfaceLog) @@ -1149,14 +1162,14 @@ bool NemoInterface::Impl::_callRemoveTiles(const IDArray &pIdArray, // wait for response. auto tStart = hrc::now(); bool abort = true; - do { - auto status = future_response.wait_for(std::chrono::milliseconds(100)); + while (hrc::now() - tStart < maxResponseTime && + !disp.isInterruptionRequested()) { + auto status = future_response.wait_for(std::chrono::milliseconds(10)); if (status == std::future_status::ready) { abort = false; break; } - } while (hrc::now() - tStart < maxResponseTime || - disp.isInterruptionRequested()); + } if (abort) { qCWarning(NemoInterfaceLog) @@ -1188,14 +1201,14 @@ bool NemoInterface::Impl::_callClearTiles(Dispatcher &disp, Rosbridge &rb) { // wait for response. auto tStart = hrc::now(); bool abort = true; - do { - auto status = future_response.wait_for(std::chrono::milliseconds(100)); + while (hrc::now() - tStart < maxResponseTime && + !disp.isInterruptionRequested()) { + auto status = future_response.wait_for(std::chrono::milliseconds(10)); if (status == std::future_status::ready) { abort = false; break; } - } while (hrc::now() - tStart < maxResponseTime || - disp.isInterruptionRequested()); + } if (abort) { qCWarning(NemoInterfaceLog) << "Websocket not responding to request."; @@ -1251,14 +1264,14 @@ ProgressArray NemoInterface::Impl::_callGetProgress(const IDArray &pIdArray, // wait for response. auto tStart = hrc::now(); bool abort = true; - do { - auto status = future_response.wait_for(std::chrono::milliseconds(100)); + while (hrc::now() - tStart < maxResponseTime && + !disp.isInterruptionRequested()) { + auto status = future_response.wait_for(std::chrono::milliseconds(10)); if (status == std::future_status::ready) { abort = false; break; } - } while (hrc::now() - tStart < maxResponseTime || - disp.isInterruptionRequested()); + } if (abort) { qCWarning(NemoInterfaceLog) @@ -1302,14 +1315,14 @@ ProgressArray NemoInterface::Impl::_callGetAllProgress(Dispatcher &disp, // wait for response. auto tStart = hrc::now(); bool abort = true; - do { - auto status = future_response.wait_for(std::chrono::milliseconds(100)); + while (hrc::now() - tStart < maxResponseTime && + !disp.isInterruptionRequested()) { + auto status = future_response.wait_for(std::chrono::milliseconds(10)); if (status == std::future_status::ready) { abort = false; break; } - } while (hrc::now() - tStart < maxResponseTime || - disp.isInterruptionRequested()); + } if (abort) { qCWarning(NemoInterfaceLog) @@ -1368,14 +1381,14 @@ NemoInterface::Impl::_callGetAllTiles(Dispatcher &disp, Rosbridge &rb) { // wait for response. auto tStart = hrc::now(); bool abort = true; - do { - auto status = future_response.wait_for(std::chrono::milliseconds(100)); + while (hrc::now() - tStart < maxResponseTime && + !disp.isInterruptionRequested()) { + auto status = future_response.wait_for(std::chrono::milliseconds(10)); if (status == std::future_status::ready) { abort = false; break; } - } while (hrc::now() - tStart < maxResponseTime || - disp.isInterruptionRequested()); + } if (abort) { qCWarning(NemoInterfaceLog) @@ -1386,7 +1399,7 @@ NemoInterface::Impl::_callGetAllTiles(Dispatcher &disp, Rosbridge &rb) { return *future_response.get(); } -QString NemoInterface::Impl::_callGetVersion(Dispatcher &d, Rosbridge &rb) { +QString NemoInterface::Impl::_callGetVersion(Dispatcher &disp, Rosbridge &rb) { // create response handler. typedef QString ResponseType; @@ -1412,14 +1425,14 @@ QString NemoInterface::Impl::_callGetVersion(Dispatcher &d, Rosbridge &rb) { // wait for response. auto tStart = hrc::now(); bool abort = true; - do { - auto status = future_response.wait_for(std::chrono::milliseconds(100)); + while (hrc::now() - tStart < maxResponseTime && + !disp.isInterruptionRequested()) { + auto status = future_response.wait_for(std::chrono::milliseconds(10)); if (status == std::future_status::ready) { abort = false; break; } - } while (hrc::now() - tStart < maxResponseTime || - d.isInterruptionRequested()); + } if (abort) { qCWarning(NemoInterfaceLog) @@ -1653,36 +1666,42 @@ bool NemoInterface::Impl::_addTilesImpl( auto ret = this->_addTilesRemote(*pTileArray); if (!ret) { Lock lk(this->_m); - this->_setStateNotSafe(STATE::SYNC_ERROR); - this->_doActionNotSafe(); - lk.unlock(); - this->_setWarningString("Adding tiles failed."); - qCDebug(NemoInterfaceLog) << "_addTilesImpl(): _addTilesRemote return " - "false: different tiles with same id."; + if (this->_state == STATE::SYS_SYNC || this->_state == STATE::USER_SYNC) { + this->_setStateNotSafe(STATE::SYNC_ERROR); + this->_doActionNotSafe(); + lk.unlock(); + this->_setWarningString("Adding tiles failed."); + qCDebug(NemoInterfaceLog) << "_addTilesImpl(): _addTilesRemote return " + "false: different tiles with same id."; + } return false; } // fetch progress auto array = this->_callGetProgress(*pIdArray, disp, *rb); if (array.size() != pIdArray->size()) { - Lock lk(this->_m); - this->_setStateNotSafe(STATE::SYNC_ERROR); - this->_doActionNotSafe(); - lk.unlock(); - this->_setWarningString("Getting progress failed. This might " - "indicate a poor connection."); + Lock lk1(this->_m); + if (this->_state == STATE::SYS_SYNC || this->_state == STATE::USER_SYNC) { + this->_setStateNotSafe(STATE::SYNC_ERROR); + this->_doActionNotSafe(); + lk1.unlock(); + this->_setWarningString("Getting progress failed. This might " + "indicate a poor connection."); + } return false; } ret = this->_updateProgress(array); if (!ret) { - Lock lk(this->_m); - this->_setStateNotSafe(STATE::SYNC_ERROR); - this->_doActionNotSafe(); - lk.unlock(); - this->_setWarningString("Getting progress failed."); - qCDebug(NemoInterfaceLog) - << "_addTilesImpl(): _updateProgress failed: unknown id."; + Lock lk2(this->_m); + if (this->_state == STATE::SYS_SYNC || this->_state == STATE::USER_SYNC) { + this->_setStateNotSafe(STATE::SYNC_ERROR); + this->_doActionNotSafe(); + lk2.unlock(); + this->_setWarningString("Getting progress failed."); + qCDebug(NemoInterfaceLog) + << "_addTilesImpl(): _updateProgress failed: unknown id."; + } return false; } @@ -1697,13 +1716,16 @@ bool NemoInterface::Impl::_removeTilesImpl( auto success = this->_callRemoveTiles(*pIdArray, disp, *rb); if (!success) { Lock lk(this->_m); - this->_setStateNotSafe(STATE::SYNC_ERROR); - this->_doActionNotSafe(); - lk.unlock(); - this->_setWarningString("Removing tiles failed. This might " - "indicate a poor connection."); + if (this->_state == STATE::SYS_SYNC || this->_state == STATE::USER_SYNC) { + this->_setStateNotSafe(STATE::SYNC_ERROR); + this->_doActionNotSafe(); + lk.unlock(); + this->_setWarningString("Removing tiles failed. This might " + "indicate a poor connection."); + } return false; } + this->_removeTilesRemote(*pIdArray); return true; @@ -1716,11 +1738,13 @@ bool NemoInterface::Impl::_clearTilesImpl() { auto success = this->_callClearTiles(disp, *rb); if (!success) { Lock lk(this->_m); - this->_setStateNotSafe(STATE::SYNC_ERROR); - this->_doActionNotSafe(); - lk.unlock(); - this->_setWarningString("Clear tiles failed. This might " - "indicate a poor connection."); + if (this->_state == STATE::SYS_SYNC || this->_state == STATE::USER_SYNC) { + this->_setStateNotSafe(STATE::SYNC_ERROR); + this->_doActionNotSafe(); + lk.unlock(); + this->_setWarningString("Clear tiles failed. This might " + "indicate a poor connection."); + } return false; } this->_clearTilesRemote(); diff --git a/src/MeasurementComplexItem/NemoInterface.h b/src/MeasurementComplexItem/NemoInterface.h index 93180efca..a83cf7576 100644 --- a/src/MeasurementComplexItem/NemoInterface.h +++ b/src/MeasurementComplexItem/NemoInterface.h @@ -13,8 +13,6 @@ #include "TilePtrArray.h" #include "geometry/ProgressArray.h" -// Singelton class used to interface measurement devices implementing the nemo -// interface. class NemoInterface : public QObject { Q_OBJECT class Impl; diff --git a/src/MeasurementComplexItem/geometry/MeasurementArea.cc b/src/MeasurementComplexItem/geometry/MeasurementArea.cc index 4f9a4fa95..ed0757323 100644 --- a/src/MeasurementComplexItem/geometry/MeasurementArea.cc +++ b/src/MeasurementComplexItem/geometry/MeasurementArea.cc @@ -15,6 +15,8 @@ #include "QGCLoggingCategory.h" #include "QmlObjectListHelper.h" +#include "MeasurementComplexItem/nemo_interface/MeasurementTile.h" + #ifndef MAX_TILES #define MAX_TILES 1000 #endif @@ -602,27 +604,6 @@ void MeasurementArea::setState(MeasurementArea::STATE s) { } } -void MeasurementArea::updateIds(const QList &array) { - for (const auto &diff : array) { - - auto it = _indexMap.find(diff.oldTile.id()); - - if (it != _indexMap.end()) { - int tileIndex = it->second; - auto *tile = _tiles->value(tileIndex); - if (diff.oldTile.coordinateList() == tile->coordinateList()) { - // Change id and update _tileMap. - const auto newId = diff.newTile.id(); - tile->setId(newId); - _indexMap.erase(it); - auto ret = _indexMap.insert(std::make_pair(newId, tileIndex)); - Q_ASSERT(ret.second == true /*insert success?*/); - Q_UNUSED(ret); - } - } - } -} - bool getTiles(const FPolygon &area, Length tileHeight, Length tileWidth, Area minTileArea, std::vector &tiles, BoundingBox &bbox) { diff --git a/src/MeasurementComplexItem/geometry/MeasurementArea.h b/src/MeasurementComplexItem/geometry/MeasurementArea.h index 82a751535..fda7c4b10 100644 --- a/src/MeasurementComplexItem/geometry/MeasurementArea.h +++ b/src/MeasurementComplexItem/geometry/MeasurementArea.h @@ -6,10 +6,8 @@ #include #include "GeoArea.h" -#include "MeasurementComplexItem/nemo_interface/MeasurementTile.h" -#include "ProgressArray.h" -#include "TileDiff.h" +#include "ProgressArray.h" #include "SettingsFact.h" class MeasurementArea : public GeoArea { @@ -61,7 +59,6 @@ signals: void progressChanged(); public slots: - void updateIds(const QList &array); void updateProgress(const ProgressArray &array); Q_INVOKABLE void randomProgress(); Q_INVOKABLE void resetProgress(); diff --git a/src/MeasurementComplexItem/geometry/ProgressArray.h b/src/MeasurementComplexItem/geometry/ProgressArray.h index 26dbe82e8..87430a0cf 100644 --- a/src/MeasurementComplexItem/geometry/ProgressArray.h +++ b/src/MeasurementComplexItem/geometry/ProgressArray.h @@ -2,7 +2,6 @@ #define PROGRESSARRAY_H #include -#include #include "comm/ros_bridge/include/messages/nemo_msgs/labeled_progress.h" diff --git a/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.cpp b/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.cpp index 3f1cfa2f5..655095ddc 100644 --- a/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.cpp +++ b/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.cpp @@ -52,11 +52,7 @@ void TaskDispatcher::start() { ULock lk1(this->_mutex); if (!_running) { this->_running = true; - auto p = std::make_shared>(); - _threadFuture = p->get_future(); - QtConcurrent::run([this, p = std::move(p)]() mutable { - return this->run(std::move(*p)); - }); + QtConcurrent::run([this]() mutable { return this->run(); }); lk1.unlock(); } } @@ -65,7 +61,6 @@ void TaskDispatcher::stop() { ULock lk1(this->_mutex); if (_running) { this->_running = false; - _threadFuture.wait(); lk1.unlock(); } } @@ -92,7 +87,7 @@ std::size_t TaskDispatcher::pendingTasks() { bool TaskDispatcher::idle() { return this->pendingTasks() == 0; } -void TaskDispatcher::run(std::promise p) { +void TaskDispatcher::run() { while (true) { ULock lk1(this->_mutex); @@ -116,7 +111,6 @@ void TaskDispatcher::run(std::promise p) { break; } } - p.set_value(); } } // namespace nemo_interface diff --git a/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.h b/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.h index 3c000b18c..3093cc987 100644 --- a/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.h +++ b/src/MeasurementComplexItem/nemo_interface/TaskDispatcher.h @@ -56,12 +56,11 @@ public: bool idle(); protected: - void run(std::promise p); + void run(); private: std::deque> _taskQueue; std::deque> _promiseQueue; - std::future _threadFuture; bool _running; std::mutex _mutex; }; -- 2.22.0