CMakeLists.txt

@@ -6,7 +6,6 @@ project(logiops)
 
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra")
-set(CMAKE_CXX_FLAGS_NONE "${CMAKE_CXX_FLAGS_NONE} -DNDEBUG")
 set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 -Wall -Wextra")
 
 set(CMAKE_CXX_STANDARD 20)

TESTED.md

@@ -5,7 +5,6 @@ This is not by any means an exhaustive list. Many more devices are supported but
 
 | Device              | Compatible? |              Config Name               |
 | :-----------------: | :---------: | :------------------------------------: |
-| MX Master 3S        |     Yes     | `MX Master 3S`                         |
 | MX Master 3         |     Yes     | `Wireless Mouse MX Master 3`           |
 | MX Master 3 for Mac |     Yes     | `MX Master 3 for Mac`                  |
 | MX Master 2S        |     Yes     | `Wireless Mouse MX Master 2S`          |

src/ipcgull

@@ -1 +1 @@
-Subproject commit cd0f9a8cefb5b2545e163fceb249fdbcbaf666aa
+Subproject commit 4f22a43e3380dc1e9c0a490201f4d745390b623f

src/logid/DeviceManager.cpp

@@ -66,11 +66,11 @@ void DeviceManager::addDevice(std::string path) {
 
     // Check if device is ignored before continuing
     {
-        auto raw_dev = raw::RawDevice::make(path, self<DeviceManager>().lock());
+        raw::RawDevice raw_dev(path, self<DeviceManager>().lock());
         if (config()->ignore.has_value() &&
-            config()->ignore.value().contains(raw_dev->productId())) {
+            config()->ignore.value().contains(raw_dev.productId())) {
             logPrintf(DEBUG, "%s: Device 0x%04x ignored.",
-                      path.c_str(), raw_dev->productId());
+                      path.c_str(), raw_dev.productId());
             return;
         }
     }

src/logid/backend/hidpp/Device.cpp

@@ -53,7 +53,7 @@ Device::Device(const std::string& path, DeviceIndex index,
                const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout) :
         io_timeout(duration_cast<milliseconds>(
                 duration<double, std::milli>(timeout))),
-        _raw_device(raw::RawDevice::make(path, monitor)),
+        _raw_device(std::make_shared<raw::RawDevice>(path, monitor)),
         _receiver(nullptr), _path(path), _index(index) {
 }
 

src/logid/backend/hidpp20/features/ReprogControls.cpp

@@ -129,7 +129,7 @@ ReprogControls::ControlInfo ReprogControls::getControlIdInfo(uint16_t cid) {
     return report;
 }
 
-void ReprogControls::setControlReporting(uint16_t cid, ControlInfo info) {
+void ReprogControls::setControlReporting(uint8_t cid, ControlInfo info) {
     // This function does not exist pre-v4 and cannot be emulated, ignore.
     (void) cid;
     (void) info; // Suppress unused warnings
@@ -173,7 +173,7 @@ ReprogControls::ControlInfo ReprogControlsV4::getControlReporting(uint16_t cid)
     return info;
 }
 
-void ReprogControlsV4::setControlReporting(uint16_t cid, ControlInfo info) {
+void ReprogControlsV4::setControlReporting(uint8_t cid, ControlInfo info) {
     std::vector<uint8_t> params(5);
     params[0] = (cid >> 8) & 0xff;
     params[1] = cid & 0xff;

src/logid/backend/hidpp20/features/ReprogControls.h

@@ -100,7 +100,7 @@ namespace logid::backend::hidpp20 {
         [[nodiscard]] virtual ControlInfo getControlReporting(uint16_t cid);
 
         // Only controlId (for remap) and flags will be read
-        virtual void setControlReporting(uint16_t cid, ControlInfo info);
+        virtual void setControlReporting(uint8_t cid, ControlInfo info);
 
         [[nodiscard]] static std::set<uint16_t> divertedButtonEvent(const hidpp::Report& report);
 
@@ -162,7 +162,7 @@ namespace logid::backend::hidpp20 {
 
         [[nodiscard]] ControlInfo getControlReporting(uint16_t cid) override;
 
-        void setControlReporting(uint16_t cid, ControlInfo info) override;
+        void setControlReporting(uint8_t cid, ControlInfo info) override;
 
         explicit ReprogControlsV4(Device* dev);
 

src/logid/backend/raw/DeviceMonitor.cpp

@@ -89,25 +89,23 @@ void DeviceMonitor::ready() {
     _ready = true;
 
     _io_monitor->add(_fd, {
-            [self_weak = _self]() {
-                if (auto self = self_weak.lock()) {
-                    struct udev_device* device = udev_monitor_receive_device(self->_udev_monitor);
-                    std::string action = udev_device_get_action(device);
-                    std::string dev_node = udev_device_get_devnode(device);
+            [this]() {
+                struct udev_device* device = udev_monitor_receive_device(_udev_monitor);
+                std::string action = udev_device_get_action(device);
+                std::string dev_node = udev_device_get_devnode(device);
 
-                    if (action == "add")
-                        run_task([self_weak, dev_node]() {
-                            if (auto self = self_weak.lock())
-                                self->_addHandler(dev_node);
-                        });
-                    else if (action == "remove")
-                        run_task([self_weak, dev_node]() {
-                            if (auto self = self_weak.lock())
-                                self->_removeHandler(dev_node);
-                        });
+                if (action == "add")
+                    run_task([self_weak = _self, dev_node]() {
+                        if (auto self = self_weak.lock())
+                            self->_addHandler(dev_node);
+                    });
+                else if (action == "remove")
+                    run_task([self_weak = _self, dev_node]() {
+                        if (auto self = self_weak.lock())
+                            self->_removeHandler(dev_node);
+                    });
 
-                    udev_device_unref(device);
-                }
+                udev_device_unref(device);
             },
             []() {
                 throw std::runtime_error("udev hangup");

src/logid/backend/raw/IOMonitor.cpp

@@ -16,7 +16,7 @@
  *
  */
 #include <backend/raw/IOMonitor.h>
-#include <util/log.h>
+#include <cassert>
 #include <optional>
 
 extern "C"
@@ -36,6 +36,55 @@ IOHandler::IOHandler(std::function<void()> r,
         error(std::move(err)) {
 }
 
+class IOMonitor::io_lock {
+    std::optional<std::unique_lock<std::mutex>> _lock;
+    IOMonitor* _io_monitor;
+    const uint64_t counter = 1;
+
+public:
+    explicit io_lock(IOMonitor* io_monitor) : _io_monitor(io_monitor) {
+        _io_monitor->_interrupting = true;
+        [[maybe_unused]] ssize_t ret = ::write(_io_monitor->_event_fd, &counter, sizeof(counter));
+        assert(ret == sizeof(counter));
+        _lock.emplace(_io_monitor->_run_mutex);
+    }
+
+    io_lock(const io_lock&) = delete;
+
+    io_lock& operator=(const io_lock&) = delete;
+
+    io_lock(io_lock&& o) noexcept: _lock(std::move(o._lock)), _io_monitor(o._io_monitor) {
+        o._lock.reset();
+        o._io_monitor = nullptr;
+    }
+
+    io_lock& operator=(io_lock&& o) noexcept {
+        if (this != &o) {
+            _lock = std::move(o._lock);
+            _io_monitor = o._io_monitor;
+            o._lock.reset();
+            o._io_monitor = nullptr;
+        }
+
+        return *this;
+    }
+
+    ~io_lock() noexcept {
+        if (_lock && _io_monitor) {
+            uint64_t buf{};
+            [[maybe_unused]] const ssize_t ret = ::read(
+                    _io_monitor->_event_fd, &buf, sizeof(counter));
+
+            assert(ret != -1);
+
+            if (buf == counter) {
+                _io_monitor->_interrupting = false;
+                _io_monitor->_interrupt_cv.notify_one();
+            }
+        }
+    }
+};
+
 IOMonitor::IOMonitor() : _epoll_fd(epoll_create1(0)),
                          _event_fd(eventfd(0, EFD_NONBLOCK)) {
     if (_epoll_fd < 0) {
@@ -57,7 +106,12 @@ IOMonitor::IOMonitor() : _epoll_fd(epoll_create1(0)),
         throw std::system_error(errno, std::generic_category());
     }
 
-    _fds.emplace(_event_fd, nullptr);
+    _fds.emplace(std::piecewise_construct, std::forward_as_tuple(_event_fd),
+                 std::forward_as_tuple([]() {}, []() {
+                     throw std::runtime_error("event_fd hangup");
+                 }, []() {
+                     throw std::runtime_error("event_fd error");
+                 }));
 
     _io_thread = std::make_unique<std::thread>([this]() {
         _listen();
@@ -68,100 +122,70 @@ IOMonitor::~IOMonitor() noexcept {
     _stop();
 
     if (_event_fd >= 0)
-        ::close(_event_fd);
+        close(_event_fd);
 
     if (_epoll_fd >= 0)
-        ::close(_epoll_fd);
+        close(_epoll_fd);
 }
 
 void IOMonitor::_listen() {
     std::unique_lock lock(_run_mutex);
     std::vector<struct epoll_event> events;
 
-    if (_is_running)
-        throw std::runtime_error("IOMonitor double run");
-
     _is_running = true;
 
     while (_is_running) {
+        if (_interrupting) {
+            _interrupt_cv.wait(lock, [this]() {
+                return !_interrupting;
+            });
+
+            if (!_is_running)
+                break;
+        }
+
         if (events.size() != _fds.size())
             events.resize(_fds.size());
-
         int ev_count = ::epoll_wait(_epoll_fd, events.data(), (int) events.size(), -1);
         for (int i = 0; i < ev_count; ++i) {
-            std::shared_ptr<IOHandler> handler;
-
-            if (events[i].data.fd == _event_fd) {
-                if (events[i].events & EPOLLIN) {
-                    lock.unlock();
-                    /* Wait until done yielding */
-                    const std::lock_guard yield_lock(_yield_mutex);
-                    uint64_t event;
-                    while (-1 != ::eventfd_read(_event_fd, &event)) { }
-                    lock.lock();
-                }
-            } else {
-                try {
-                    handler = _fds.at(events[i].data.fd);
-                } catch (std::out_of_range& e) {
-                    continue;
-                }
-
-                lock.unlock();
-                try {
-                    if (events[i].events & EPOLLIN)
-                        handler->read();
-                    if (events[i].events & EPOLLHUP)
-                        handler->hangup();
-                    if (events[i].events & EPOLLERR)
-                        handler->error();
-                } catch (std::exception& e) {
-                    logPrintf(ERROR, "Unhandled I/O handler error: %s", e.what());
-                }
-                lock.lock();
-
-            }
+            const auto& handler = _fds.at(events[i].data.fd);
+            if (events[i].events & EPOLLIN)
+                handler.read();
+            if (events[i].events & EPOLLHUP)
+                handler.hangup();
+            if (events[i].events & EPOLLERR)
+                handler.error();
         }
     }
 }
 
 void IOMonitor::_stop() noexcept {
-    _is_running = false;
-    _yield();
+    {
+        [[maybe_unused]] const io_lock lock(this);
+        _is_running = false;
+    }
     _io_thread->join();
 }
 
-std::unique_lock<std::mutex> IOMonitor::_yield() noexcept {
-    /* Prevent listener thread from grabbing lock during yielding */
-    std::unique_lock yield_lock(_yield_mutex);
-
-    std::unique_lock run_lock(_run_mutex, std::try_to_lock);
-    if (!run_lock.owns_lock()) {
-        ::eventfd_write(_event_fd, 1);
-        run_lock = std::unique_lock<std::mutex>(_run_mutex);
-    }
-
-    return run_lock;
-}
-
 void IOMonitor::add(int fd, IOHandler handler) {
-    const auto lock = _yield();
+    [[maybe_unused]] const io_lock lock(this);
 
     struct epoll_event event{};
     event.events = EPOLLIN | EPOLLHUP | EPOLLERR;
     event.data.fd = fd;
 
-    if (!_fds.contains(fd)) {
-        if (::epoll_ctl(_epoll_fd, EPOLL_CTL_ADD, fd, &event))
-            throw std::system_error(errno, std::generic_category());
-        _fds.emplace(fd, std::make_shared<IOHandler>(std::move(handler)));
-    } else {
+    // TODO: EPOLL_CTL_MOD
+    if (_fds.contains(fd))
         throw std::runtime_error("duplicate io fd");
-    }
+
+    if (::epoll_ctl(_epoll_fd, EPOLL_CTL_ADD, fd, &event))
+        throw std::system_error(errno, std::generic_category());
+    _fds.emplace(fd, std::move(handler));
 }
 
 void IOMonitor::remove(int fd) noexcept {
-    const auto lock = _yield();
+    [[maybe_unused]] const io_lock lock(this);
+
     ::epoll_ctl(_epoll_fd, EPOLL_CTL_DEL, fd, nullptr);
     _fds.erase(fd);
 }

src/logid/backend/raw/IOMonitor.h

@@ -21,10 +21,9 @@
 #include <atomic>
 #include <functional>
 #include <map>
-#include <memory>
 #include <mutex>
-#include <condition_variable>
 #include <thread>
+#include <condition_variable>
 
 namespace logid::backend::raw {
     struct IOHandler {
@@ -54,21 +53,24 @@ namespace logid::backend::raw {
         void add(int fd, IOHandler handler);
 
         void remove(int fd) noexcept;
+
     private:
         void _listen(); // This is a blocking call
         void _stop() noexcept;
-        std::unique_lock<std::mutex> _yield() noexcept;
 
         std::unique_ptr<std::thread> _io_thread;
 
-        std::mutex _run_mutex;
-        std::mutex _yield_mutex;
-
-        std::map<int, std::shared_ptr<IOHandler>> _fds;
+        std::map<int, IOHandler> _fds;
+        mutable std::mutex _run_mutex;
         std::atomic_bool _is_running;
 
+        std::atomic_bool _interrupting;
+        std::condition_variable _interrupt_cv;
+
         const int _epoll_fd;
         const int _event_fd;
+
+        class io_lock;
     };
 }
 

src/logid/backend/raw/RawDevice.cpp

@@ -117,22 +117,11 @@ RawDevice::RawDevice(std::string path, const std::shared_ptr<DeviceMonitor>& mon
         auto phys = get_phys(_fd);
         _sub_device = std::regex_match(phys, virtual_path_regex);
     }
-}
 
-void RawDevice::_ready() {
     _io_monitor->add(_fd, {
-            [self_weak = _self]() {
-                if (auto self = self_weak.lock())
-                    self->_readReports();
-            },
-            [self_weak = _self]() {
-                if (auto self = self_weak.lock())
-                    self->_valid = false;
-            },
-            [self_weak = _self]() {
-                if (auto self = self_weak.lock())
-                    self->_valid = false;
-            }
+            [this]() { _readReports(); },
+            [this]() { _valid = false; },
+            [this]() { _valid = false; }
     });
 }
 

src/logid/backend/raw/RawDevice.h

@@ -34,16 +34,7 @@ namespace logid::backend::raw {
 
     class IOMonitor;
 
-    template <typename T>
-    class RawDeviceWrapper : public T {
-    public:
-        template <typename... Args>
-        RawDeviceWrapper(Args... args) : T(std::forward<Args>(args)...) { }
-    };
-
     class RawDevice {
-        template <typename>
-        friend class RawDeviceWrapper;
     public:
         static constexpr int max_data_length = 32;
         typedef RawEventHandler EventHandler;
@@ -60,14 +51,7 @@ namespace logid::backend::raw {
             BusType bus_type;
         };
 
-        template <typename... Args>
-        static std::shared_ptr<RawDevice> make(Args... args) {
-            auto raw_dev = std::make_shared<RawDeviceWrapper<RawDevice>>(
-                    std::forward<Args>(args)...);
-            raw_dev->_self = raw_dev;
-            raw_dev->_ready();
-            return raw_dev;
-        }
+        RawDevice(std::string path, const std::shared_ptr<DeviceMonitor>& monitor);
 
         ~RawDevice() noexcept;
 
@@ -95,10 +79,6 @@ namespace logid::backend::raw {
         [[nodiscard]] EventHandlerLock<RawDevice> addEventHandler(RawEventHandler handler);
 
     private:
-        RawDevice(std::string path, const std::shared_ptr<DeviceMonitor>& monitor);
-
-        void _ready();
-
         void _readReports();
 
         std::atomic_bool _valid;
@@ -111,8 +91,6 @@ namespace logid::backend::raw {
 
         std::shared_ptr<IOMonitor> _io_monitor;
 
-        std::weak_ptr<RawDevice> _self;
-
         bool _sub_device = false;
 
         std::shared_ptr<EventHandlerList<RawDevice>> _event_handlers;

src/logid/config/group.h

@@ -22,7 +22,6 @@
 #include <type_traits>
 #include <functional>
 #include <utility>
-#include <algorithm>
 
 namespace logid::config {
     template<typename T>

src/logid/logiops-dbus.conf.in

@@ -3,12 +3,11 @@
  "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
 
 <busconfig>
-  <policy context="default">
-    <deny receive_sender="pizza.pixl.LogiOps"/>
-  </policy>
-
   <policy user="root">
     <allow own="pizza.pixl.LogiOps"/>
+  </policy>
+
+  <policy context="default">
     <allow send_destination="pizza.pixl.LogiOps"/>
     <allow receive_sender="pizza.pixl.LogiOps"/>
   </policy>