Merge pull request #4 from rockerbacon/fix-runtime-error

Fix error with  asleep device + critical section protection + general refactoring

.gitignore

@@ -0,0 +1,4 @@
+/build
+*.swp
+*~
+/compile_commands.json

CMakeLists.txt

@@ -1,6 +1,7 @@
 cmake_minimum_required(VERSION 3.10)
 project(logiops)
 
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra")
 set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 -Wall -Wextra")
 

src/logid/Device.cpp

@@ -248,19 +248,7 @@ void ReceiverHandler::handleEvent(const HIDPP::Report &event)
     {
         case HIDPP10::IReceiver::DeviceUnpaired:
         {
-            // Find device, stop it, and delete it
+            finder->stopAndDeleteDevice(dev->path, event.deviceIndex());
-            auto it = finder->devices.begin();
-            while (it != finder->devices.end())
-            {
-                if(it->first->path == dev->path && it->first->index == event.deviceIndex())
-                {
-                    log_printf(INFO, "%s (Device %d on %s) unpaired.", it->first->name.c_str(), event.deviceIndex(), dev->path.c_str());
-                    it->first->stop();
-                    it->second.join();
-                    finder->devices.erase(it);
-                }
-                else it++;
-            }
             break;
         }
         case HIDPP10::IReceiver::DevicePaired:

src/logid/DeviceFinder.cpp

@@ -9,20 +9,93 @@
 #include <thread>
 #include <fstream>
 #include <sstream>
+#include <chrono>
 
 #include "DeviceFinder.h"
 #include "util.h"
 #include "Device.h"
 
+#define MAX_CONNECTION_TRIES 10
+#define TIME_BETWEEN_CONNECTION_TRIES 1s
+
+void stopAndDeleteConnectedDevice (ConnectedDevice &connected_device)
+{
+    log_printf(INFO, "%s (Device %d on %s) disconnected", connected_device.device->name.c_str(), connected_device.device->index, connected_device.device->path.c_str()); 
+    connected_device.device->stop();
+    connected_device.associatedThread.join();
+    delete(connected_device.device);
+}
+
+DeviceFinder::~DeviceFinder()
+{
+    this->devices_mutex.lock();
+        for (auto it = this->devices.begin(); it != this->devices.end(); it++) {
+            for (auto jt = it->second.begin(); jt != it->second.end(); jt++) {
+                stopAndDeleteConnectedDevice(jt->second);
+            }
+        }
+    this->devices_mutex.unlock();
+}
+
+void DeviceFinder::insertNewDevice(const std::string &path, HIDPP::DeviceIndex index)
+{
+    Device *device = new Device(path, index);
+
+    this->devices_mutex.lock();
+        log_printf(INFO, "%s detected: device %d on %s", device->name.c_str(), index, path.c_str());
+        auto path_bucket = this->devices.emplace(path, std::map<HIDPP::DeviceIndex, ConnectedDevice>()).first;
+        path_bucket->second.emplace(index, ConnectedDevice{
+            device,
+            std::thread([device]() {
+                device->start();
+            })
+        });
+    this->devices_mutex.unlock();
+}
+
+void DeviceFinder::stopAndDeleteAllDevicesIn (const std::string &path)
+{
+    this->devices_mutex.lock();
+        auto path_bucket = this->devices.find(path);
+        if (path_bucket != this->devices.end())
+        {
+            for (auto& index_bucket : path_bucket->second) {
+                stopAndDeleteConnectedDevice(index_bucket.second);
+            }
+            this->devices.erase(path_bucket);
+        }
+    this->devices_mutex.unlock();
+
+    log_printf(WARN, "Attempted to disconnect not previously connected devices on %s", path.c_str());
+}
+
+void DeviceFinder::stopAndDeleteDevice (const std::string &path, HIDPP::DeviceIndex index)
+{
+    this->devices_mutex.lock();
+        auto path_bucket = this->devices.find(path);
+        if (path_bucket != this->devices.end())
+        {
+            auto index_bucket = path_bucket->second.find(index);
+            if (index_bucket != path_bucket->second.end())
+            {
+                stopAndDeleteConnectedDevice(index_bucket->second);
+                path_bucket->second.erase(index_bucket);
+            }
+        }
+    this->devices_mutex.unlock();
+
+    log_printf(WARN, "Attempted to disconnect not previously connected device %d on %s", index, path.c_str());
+}
+    
+
 void DeviceFinder::addDevice(const char *path)
 {
+    using namespace std::chrono_literals;
+
     std::string string_path(path);
     // Asynchronously scan device
     std::thread{[=]()
     {
-        std::vector<Device*> _devs;
-        const int max_tries = 10;
-        const int try_delay = 250000;
 
         //Check if device is an HID++ device and handle it accordingly
         try
@@ -35,10 +108,14 @@ void DeviceFinder::addDevice(const char *path)
                 HIDPP::WirelessDevice3, HIDPP::WirelessDevice4,
                 HIDPP::WirelessDevice5, HIDPP::WirelessDevice6})
             {
+
                 if(!has_receiver_index && index == HIDPP::WirelessDevice1)
                     break;
-                for(int i = 0; i < max_tries; i++)
+
-                {
+                bool device_not_connected = true;
+                bool device_unknown = false;
+                int remaining_tries = MAX_CONNECTION_TRIES;
+                do {
                     try
                     {
                         HIDPP::Device d(&dispatcher, index);
@@ -49,72 +126,60 @@ void DeviceFinder::addDevice(const char *path)
                             has_receiver_index = true;
                         if(major > 1) // HID++ 2.0 devices only
                         {
-                            auto dev = new Device(string_path, index);
+                            this->insertNewDevice(string_path, index);
-                            _devs.push_back(dev);
-                            log_printf(INFO, "%s detected: device %d on %s", d.name().c_str(), index, string_path.c_str());
                         }
-                        break;
+                        device_not_connected = false;
                     }
                     catch(HIDPP10::Error &e)
                     {
-                        if(e.errorCode() != HIDPP10::Error::UnknownDevice)
+                        if (e.errorCode() == HIDPP10::Error::ResourceError)
                         {
-                            if(i == max_tries - 1)
+                            if(remaining_tries == 1)
-                                log_printf(ERROR, "Error while querying %s, wireless device %d: %s", string_path.c_str(), index, e.what());
+                            {
-                            else usleep(try_delay);
+                                log_printf(WARN, "While querying %s (possibly asleep), wireless device %d: %s", string_path.c_str(), index, e.what());
+                                remaining_tries += MAX_CONNECTION_TRIES;  // asleep devices may raise a resource error, so do not count this try
+                            } 
                         }
-                        else break;
+                        else if(e.errorCode() != HIDPP10::Error::UnknownDevice)
+                        {
+                            if(remaining_tries == 1)
+                                log_printf(ERROR, "While querying %s, wireless device %d: %s", string_path.c_str(), index, e.what());
+                        }
+                        else device_unknown = true;
                     }
                     catch(HIDPP20::Error &e)
                     {
                         if(e.errorCode() != HIDPP20::Error::UnknownDevice)
                         {
-                            if(i == max_tries - 1)
+                            if(remaining_tries == 1)
                                 log_printf(ERROR, "Error while querying %s, device %d: %s", string_path.c_str(), index, e.what());
-                            else usleep(try_delay);
                         }
-                        else break;
+                        else device_unknown = true;
                     }
                     catch(HIDPP::Dispatcher::TimeoutError &e)
                     {
-                        if(i == max_tries - 1)
+                        if(remaining_tries == 1)
                             log_printf(ERROR, "Device %s (index %d) timed out.", string_path.c_str(), index);
-                        else usleep(try_delay);
                     }
                     catch(std::runtime_error &e)
                     {
-                        if(i == max_tries - 1)
+                        if(remaining_tries == 1)
                             log_printf(ERROR, "Runtime error on device %d on %s: %s", index, string_path.c_str(), e.what());
-                        else usleep(try_delay);
                     }
-                }
+
+                    remaining_tries--;
+                    std::this_thread::sleep_for(TIME_BETWEEN_CONNECTION_TRIES);
+
+                } while (device_not_connected && !device_unknown && remaining_tries > 0);
             } 
         }
         catch(HIDPP::Dispatcher::NoHIDPPReportException &e) { }
         catch(std::system_error &e) { log_printf(WARN, "Failed to open %s: %s", string_path.c_str(), e.what()); }
 
-        for(auto dev : _devs)
-            devices.insert({dev, std::thread{[dev]() { dev->start(); }}});
-
     }}.detach();
 }
 
 void DeviceFinder::removeDevice(const char* path)
 {
-    // Iterate through Devices, stop all in path
+    this->stopAndDeleteAllDevicesIn(std::string(path));
-    auto it = devices.begin();
-    while (it != devices.end())
-    {
-        if(it->first->path == path)
-        {
-            log_printf(INFO, "%s on %s disconnected.", it->first->name.c_str(), path);
-            it->first->stop();
-            it->second.join();
-            delete(it->first);
-            devices.erase(it);
-            it++;
-        }
-        else
-            it++;
-    }
 }

src/logid/DeviceFinder.h

@@ -1,5 +1,4 @@
-#ifndef DEVICEFINDER_H
+#pragma once
-#define DEVICEFINDER_H
 
 #include <hid/DeviceMonitor.h>
 #include <hidpp/SimpleDispatcher.h>
@@ -8,19 +7,32 @@
 #include <hidpp20/IReprogControls.h>
 #include <map>
 #include <thread>
+#include <mutex>
+
 #include "Device.h"
 
 class Device;
 
+struct ConnectedDevice {
+	Device *device;
+	std::thread associatedThread;
+};
+
 class DeviceFinder : public HID::DeviceMonitor
 {
 public:
-    std::map<Device*, std::thread> devices;
+	~DeviceFinder();
+
+	void insertNewDevice (const std::string &path, HIDPP::DeviceIndex index);
+	void stopAndDeleteAllDevicesIn (const std::string &path);
+	void stopAndDeleteDevice (const std::string &path, HIDPP::DeviceIndex index);
 protected:
     void addDevice(const char* path);
     void removeDevice(const char* path);
+private:
+	std::mutex devices_mutex;
+    std::map<std::string, std::map<HIDPP::DeviceIndex, ConnectedDevice>> devices;
 };
 
 extern DeviceFinder* finder;
 
-#endif //DEVICEFINDER_H