From ffd5e054a1c097bb9f5c96619a02734ff7f92d42 Mon Sep 17 00:00:00 2001 From: pixl Date: Mon, 1 May 2023 21:44:01 -0400 Subject: [PATCH] Bind EventHandler lifetimes to owner --- src/logid/backend/EventHandlerList.h | 87 +++++++++++++++++++ src/logid/backend/hidpp/Device.cpp | 85 ++++++++---------- src/logid/backend/hidpp/Device.h | 23 +++-- src/logid/backend/hidpp10/ReceiverMonitor.cpp | 27 ++---- src/logid/backend/hidpp10/ReceiverMonitor.h | 10 +-- src/logid/backend/raw/RawDevice.cpp | 20 ++--- src/logid/backend/raw/RawDevice.h | 15 ++-- src/logid/features/DeviceStatus.cpp | 7 +- src/logid/features/DeviceStatus.h | 7 +- src/logid/features/HiresScroll.cpp | 7 +- src/logid/features/HiresScroll.h | 4 +- src/logid/features/RemapButton.cpp | 7 +- src/logid/features/RemapButton.h | 4 +- src/logid/features/ThumbWheel.cpp | 7 +- src/logid/features/ThumbWheel.h | 4 +- 15 files changed, 164 insertions(+), 150 deletions(-) create mode 100644 src/logid/backend/EventHandlerList.h diff --git a/src/logid/backend/EventHandlerList.h b/src/logid/backend/EventHandlerList.h new file mode 100644 index 0000000..1f30523 --- /dev/null +++ b/src/logid/backend/EventHandlerList.h @@ -0,0 +1,87 @@ +/* + * Copyright 2019-2023 PixlOne + * + * This program 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. + * + * This program 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 this program. If not, see . + * + */ + + +#ifndef LOGID_EVENTHANDLERLIST_H +#define LOGID_EVENTHANDLERLIST_H + +#include +#include +#include +#include + +template +class EventHandlerLock; + +template +struct EventHandlerList { + typedef std::list list_t; + typedef list_t::const_iterator iterator_t; + + std::list list; + mutable std::shared_mutex mutex; + + void remove(iterator_t iterator) { + std::unique_lock lock(mutex); + list.erase(iterator); + } +}; + +template +class EventHandlerLock { + typedef EventHandlerList list_t; + typedef typename list_t::iterator_t iterator_t; + + friend T; + + std::weak_ptr _list; + iterator_t _iterator; + + EventHandlerLock(const std::shared_ptr& list, iterator_t iterator) : + _list (list), _iterator (iterator) { + } +public: + EventHandlerLock() = default; + + EventHandlerLock(const EventHandlerLock&) = delete; + + EventHandlerLock(EventHandlerLock&& o) noexcept : _list (o._list), _iterator (o._iterator) { + o._list.reset(); + } + + EventHandlerLock& operator=(const EventHandlerLock&) = delete; + + EventHandlerLock& operator=(EventHandlerLock&& o) noexcept { + this->_list = o._list; + this->_iterator = o._iterator; + o._list.reset(); + + return *this; + } + + ~EventHandlerLock() { + if(auto list = _list.lock()) + list->remove(_iterator); + } + + [[nodiscard]] bool empty() const noexcept { + return _list.expired(); + } +}; + +#endif //LOGID_EVENTHANDLERLIST_H diff --git a/src/logid/backend/hidpp/Device.cpp b/src/logid/backend/hidpp/Device.cpp index 38cd0e1..b501350 100644 --- a/src/logid/backend/hidpp/Device.cpp +++ b/src/logid/backend/hidpp/Device.cpp @@ -108,6 +108,8 @@ const std::tuple& Device::version() const { } void Device::_init() { + _event_handlers = std::make_shared>(); + supported_reports = getSupportedReports(_raw_device->reportDescriptor()); if (!supported_reports) throw InvalidDevice(InvalidDevice::NoHIDPPReport); @@ -135,12 +137,7 @@ void Device::_init() { } } catch (hidpp10::Error& e) { // Ignore - } catch (std::exception& e) { - _raw_device->removeEventHandler(_raw_handler); - throw; } - - _raw_device->removeEventHandler(_raw_handler); } _raw_handler = _raw_device->addEventHandler( @@ -156,69 +153,55 @@ void Device::_init() { }}); try { - try { - hidpp20::Root root(this); - _version = root.getVersion(); - } catch (hidpp10::Error& e) { - // Valid HID++ 1.0 devices should send an InvalidSubID error - if (e.code() != hidpp10::Error::InvalidSubID) - throw; + hidpp20::Root root(this); + _version = root.getVersion(); + } catch (hidpp10::Error& e) { + // Valid HID++ 1.0 devices should send an InvalidSubID error + if (e.code() != hidpp10::Error::InvalidSubID) + throw; - // HID++ 2.0 is not supported, assume HID++ 1.0 - _version = std::make_tuple(1, 0); - } + // HID++ 2.0 is not supported, assume HID++ 1.0 + _version = std::make_tuple(1, 0); + } - if (!_receiver) { - _pid = _raw_device->productId(); - if (std::get<0>(_version) >= 2) { - try { - hidpp20::DeviceName deviceName(this); - _name = deviceName.getName(); - } catch (hidpp20::UnsupportedFeature& e) { - _name = _raw_device->name(); - } - } else { + if (!_receiver) { + _pid = _raw_device->productId(); + if (std::get<0>(_version) >= 2) { + try { + hidpp20::DeviceName deviceName(this); + _name = deviceName.getName(); + } catch (hidpp20::UnsupportedFeature& e) { _name = _raw_device->name(); } } else { - if (std::get<0>(_version) >= 2) { - try { - hidpp20::DeviceName deviceName(this); - _name = deviceName.getName(); - } catch (hidpp20::UnsupportedFeature& e) { - _name = _receiver->getDeviceName(_index); - } - } else { + _name = _raw_device->name(); + } + } else { + if (std::get<0>(_version) >= 2) { + try { + hidpp20::DeviceName deviceName(this); + _name = deviceName.getName(); + } catch (hidpp20::UnsupportedFeature& e) { _name = _receiver->getDeviceName(_index); } + } else { + _name = _receiver->getDeviceName(_index); } - } catch (std::exception& e) { - _raw_device->removeEventHandler(_raw_handler); - throw; } } -Device::~Device() { - _raw_device->removeEventHandler(_raw_handler); -} - -Device::EvHandlerId Device::addEventHandler(EventHandler handler) { - std::unique_lock lock(_event_handler_mutex); - _event_handlers.emplace_front(std::move(handler)); - return _event_handlers.cbegin(); -} - -void Device::removeEventHandler(EvHandlerId id) { - std::unique_lock lock(_event_handler_mutex); - _event_handlers.erase(id); +EventHandlerLock Device::addEventHandler(EventHandler handler) { + std::unique_lock lock(_event_handlers->mutex); + _event_handlers->list.emplace_front(std::move(handler)); + return {_event_handlers, _event_handlers->list.cbegin()}; } void Device::handleEvent(Report& report) { if (responseReport(report)) return; - std::shared_lock lock(_event_handler_mutex); - for (auto& handler: _event_handlers) + std::shared_lock lock(_event_handlers->mutex); + for (auto& handler: _event_handlers->list) if (handler.condition(report)) handler.callback(report); } diff --git a/src/logid/backend/hidpp/Device.h b/src/logid/backend/hidpp/Device.h index 766b1b2..48f1d33 100644 --- a/src/logid/backend/hidpp/Device.h +++ b/src/logid/backend/hidpp/Device.h @@ -28,6 +28,7 @@ #include #include #include +#include namespace logid::backend::hidpp10 { // Need to define here for a constructor @@ -36,14 +37,13 @@ namespace logid::backend::hidpp10 { namespace logid::backend::hidpp { struct DeviceConnectionEvent; - struct EventHandler { - std::function condition; - std::function callback; - }; class Device { public: - typedef std::list::const_iterator EvHandlerId; + struct EventHandler { + std::function condition; + std::function callback; + }; class InvalidDevice : std::exception { public: @@ -76,8 +76,6 @@ namespace logid::backend::hidpp { Device(const std::shared_ptr& receiver, DeviceIndex index, double timeout); - virtual ~Device(); - [[nodiscard]] const std::string& devicePath() const; [[nodiscard]] DeviceIndex deviceIndex() const; @@ -88,9 +86,7 @@ namespace logid::backend::hidpp { [[nodiscard]] uint16_t pid() const; - EvHandlerId addEventHandler(EventHandler handler); - - void removeEventHandler(EvHandlerId id); + EventHandlerLock addEventHandler(EventHandler handler); virtual Report sendReport(const Report& report); @@ -117,7 +113,7 @@ namespace logid::backend::hidpp { void _init(); std::shared_ptr _raw_device; - raw::RawDevice::EvHandlerId _raw_handler; + EventHandlerLock _raw_handler; std::shared_ptr _receiver; std::string _path; DeviceIndex _index; @@ -133,9 +129,10 @@ namespace logid::backend::hidpp { std::optional _response; std::optional _sent_sub_id{}; - std::shared_mutex _event_handler_mutex; - std::list _event_handlers; + std::shared_ptr> _event_handlers; }; + + typedef Device::EventHandler EventHandler; } #endif //LOGID_BACKEND_HIDPP_DEVICE_H \ No newline at end of file diff --git a/src/logid/backend/hidpp10/ReceiverMonitor.cpp b/src/logid/backend/hidpp10/ReceiverMonitor.cpp index be3900b..db65f89 100644 --- a/src/logid/backend/hidpp10/ReceiverMonitor.cpp +++ b/src/logid/backend/hidpp10/ReceiverMonitor.cpp @@ -32,22 +32,8 @@ ReceiverMonitor::ReceiverMonitor(const std::string& path, _receiver->setNotifications(notification_flags); } -ReceiverMonitor::~ReceiverMonitor() { - if (_pair_status_handler.has_value()) - _receiver->removeEventHandler(_pair_status_handler.value()); - - if (_passkey_ev_handler.has_value()) - _receiver->removeEventHandler(_passkey_ev_handler.value()); - - if (_discover_ev_handler.has_value()) - _receiver->removeEventHandler(_discover_ev_handler.value()); - - if (_connect_ev_handler.has_value()) - _receiver->rawDevice()->removeEventHandler(_connect_ev_handler.value()); -} - void ReceiverMonitor::ready() { - if (!_connect_ev_handler.has_value()) { + if (_connect_ev_handler.empty()) { _connect_ev_handler = _receiver->rawDevice()->addEventHandler( {[](const std::vector& report) -> bool { if (report[Offset::Type] == Report::Type::Short || @@ -86,7 +72,7 @@ void ReceiverMonitor::ready() { }); } - if (!_discover_ev_handler.has_value()) { + if (_discover_ev_handler.empty()) { _discover_ev_handler = _receiver->addEventHandler( {[](const hidpp::Report& report) -> bool { return (report.subId() == Receiver::DeviceDiscovered) && @@ -108,7 +94,7 @@ void ReceiverMonitor::ready() { }); } - if (!_passkey_ev_handler.has_value()) { + if (_passkey_ev_handler.empty()) { _passkey_ev_handler = _receiver->addEventHandler( {[](const hidpp::Report& report) -> bool { return report.subId() == Receiver::PasskeyRequest && @@ -126,7 +112,7 @@ void ReceiverMonitor::ready() { }); } - if (!_pair_status_handler.has_value()) { + if (_pair_status_handler.empty()) { _pair_status_handler = _receiver->addEventHandler( {[](const hidpp::Report& report) -> bool { return report.subId() == Receiver::DiscoveryStatus || @@ -166,7 +152,7 @@ void ReceiverMonitor::enumerate() { } void ReceiverMonitor::waitForDevice(hidpp::DeviceIndex index) { - auto handler_id = std::make_shared(); + auto handler_id = std::make_shared>(); *handler_id = _receiver->rawDevice()->addEventHandler( {[index](const std::vector& report) -> bool { @@ -180,9 +166,8 @@ void ReceiverMonitor::waitForDevice(hidpp::DeviceIndex index) { event.fromTimeoutCheck = true; spawn_task([this, event, handler_id]() { - assert(handler_id); + *handler_id = {}; try { - _receiver->rawDevice()->removeEventHandler(*handler_id); addDevice(event); } catch (std::exception& e) { logPrintf(ERROR, "Failed to add device %d to receiver on %s: %s", diff --git a/src/logid/backend/hidpp10/ReceiverMonitor.h b/src/logid/backend/hidpp10/ReceiverMonitor.h index 27f17fb..8a3358c 100644 --- a/src/logid/backend/hidpp10/ReceiverMonitor.h +++ b/src/logid/backend/hidpp10/ReceiverMonitor.h @@ -32,8 +32,6 @@ namespace logid::backend::hidpp10 { const std::shared_ptr& monitor, double timeout); - virtual ~ReceiverMonitor(); - void enumerate(); protected: @@ -68,11 +66,11 @@ namespace logid::backend::hidpp10 { DeviceDiscoveryEvent _discovery_event; PairState _pair_state = NotPairing; - std::optional _connect_ev_handler; + EventHandlerLock _connect_ev_handler; - std::optional _discover_ev_handler; - std::optional _passkey_ev_handler; - std::optional _pair_status_handler; + EventHandlerLock _discover_ev_handler; + EventHandlerLock _passkey_ev_handler; + EventHandlerLock _pair_status_handler; }; } diff --git a/src/logid/backend/raw/RawDevice.cpp b/src/logid/backend/raw/RawDevice.cpp index efb67f9..213b546 100644 --- a/src/logid/backend/raw/RawDevice.cpp +++ b/src/logid/backend/raw/RawDevice.cpp @@ -74,7 +74,8 @@ std::string get_name(int fd) { RawDevice::RawDevice(std::string path, const std::shared_ptr& monitor) : _valid(true), _path(std::move(path)), _fd(get_fd(_path)), _dev_info(get_dev_info(_fd)), _name(get_name(_fd)), - _report_desc(getReportDescriptor(_fd)), _io_monitor(monitor->ioMonitor()) { + _report_desc(getReportDescriptor(_fd)), _io_monitor(monitor->ioMonitor()), + _event_handlers(std::make_shared>()) { _io_monitor->add(_fd, { [this]() { _readReports(); }, [this]() { _valid = false; }, @@ -154,15 +155,10 @@ void RawDevice::sendReport(const std::vector& report) { "sendReport write failed"); } -RawDevice::EvHandlerId RawDevice::addEventHandler(RawEventHandler handler) { - std::unique_lock lock(_event_handler_mutex); - _event_handlers.emplace_front(std::move(handler)); - return _event_handlers.cbegin(); -} - -void RawDevice::removeEventHandler(RawDevice::EvHandlerId id) { - std::unique_lock lock(_event_handler_mutex); - _event_handlers.erase(id); +EventHandlerLock RawDevice::addEventHandler(RawEventHandler handler) { + std::unique_lock lock(_event_handlers->mutex); + _event_handlers->list.emplace_front(std::move(handler)); + return {_event_handlers, _event_handlers->list.cbegin()}; } void RawDevice::_readReports() { @@ -185,8 +181,8 @@ void RawDevice::_readReports() { } void RawDevice::_handleEvent(const std::vector& report) { - std::shared_lock lock(_event_handler_mutex); - for (auto& handler: _event_handlers) + std::shared_lock lock(_event_handlers->mutex); + for (auto& handler : _event_handlers->list) if (handler.condition(report)) handler.callback(report); } diff --git a/src/logid/backend/raw/RawDevice.h b/src/logid/backend/raw/RawDevice.h index d41d0c9..222cd12 100644 --- a/src/logid/backend/raw/RawDevice.h +++ b/src/logid/backend/raw/RawDevice.h @@ -19,6 +19,8 @@ #ifndef LOGID_BACKEND_RAWDEVICE_H #define LOGID_BACKEND_RAWDEVICE_H +#include +#include #include #include #include @@ -26,7 +28,6 @@ #include #include #include -#include namespace logid::backend::raw { class DeviceMonitor; @@ -36,15 +37,14 @@ namespace logid::backend::raw { class RawDevice { public: static constexpr int max_data_length = 32; - typedef std::list::const_iterator EvHandlerId; + typedef RawEventHandler EventHandler; struct dev_info { int16_t vid; int16_t pid; }; - RawDevice(std::string path, - const std::shared_ptr& monitor); + RawDevice(std::string path, const std::shared_ptr& monitor); ~RawDevice() noexcept; @@ -65,9 +65,7 @@ namespace logid::backend::raw { void sendReport(const std::vector& report); - EvHandlerId addEventHandler(RawEventHandler handler); - - void removeEventHandler(EvHandlerId id); + [[nodiscard]] EventHandlerLock addEventHandler(RawEventHandler handler); private: void _readReports(); @@ -82,8 +80,7 @@ namespace logid::backend::raw { std::shared_ptr _io_monitor; - std::list _event_handlers; - std::shared_mutex _event_handler_mutex; + std::shared_ptr> _event_handlers; void _handleEvent(const std::vector& report); }; diff --git a/src/logid/features/DeviceStatus.cpp b/src/logid/features/DeviceStatus.cpp index b26bb61..a88bdbf 100644 --- a/src/logid/features/DeviceStatus.cpp +++ b/src/logid/features/DeviceStatus.cpp @@ -38,17 +38,12 @@ DeviceStatus::DeviceStatus(logid::Device* dev) : DeviceFeature(dev) { } } -DeviceStatus::~DeviceStatus() noexcept { - if (_ev_handler.has_value()) - _device->hidpp20().removeEventHandler(_ev_handler.value()); -} - void DeviceStatus::configure() { // Do nothing } void DeviceStatus::listen() { - if (!_ev_handler.has_value()) { + if (_ev_handler.empty()) { _ev_handler = _device->hidpp20().addEventHandler( { [index = _wireless_device_status->featureIndex()]( diff --git a/src/logid/features/DeviceStatus.h b/src/logid/features/DeviceStatus.h index d98122f..0da10b9 100644 --- a/src/logid/features/DeviceStatus.h +++ b/src/logid/features/DeviceStatus.h @@ -28,16 +28,13 @@ namespace logid::features { public: explicit DeviceStatus(Device* dev); - ~DeviceStatus() noexcept override; - void configure() final; void listen() final; private: - std::optional _ev_handler; - std::shared_ptr - _wireless_device_status; + EventHandlerLock _ev_handler; + std::shared_ptr _wireless_device_status; }; } diff --git a/src/logid/features/HiresScroll.cpp b/src/logid/features/HiresScroll.cpp index 4afd281..3c7cef4 100644 --- a/src/logid/features/HiresScroll.cpp +++ b/src/logid/features/HiresScroll.cpp @@ -74,11 +74,6 @@ HiresScroll::HiresScroll(Device* dev) : _ipc_interface = dev->ipcNode()->make_interface(this); } -HiresScroll::~HiresScroll() noexcept { - if (_ev_handler.has_value()) - _device->hidpp20().removeEventHandler(_ev_handler.value()); -} - void HiresScroll::configure() { std::shared_lock lock(_config_mutex); _configure(); @@ -93,7 +88,7 @@ void HiresScroll::_configure() { void HiresScroll::listen() { std::shared_lock lock(_config_mutex); - if (!_ev_handler.has_value()) { + if (_ev_handler.empty()) { _ev_handler = _device->hidpp20().addEventHandler( { [index = _hires_scroll->featureIndex()]( diff --git a/src/logid/features/HiresScroll.h b/src/logid/features/HiresScroll.h index b987d95..b4a7980 100644 --- a/src/logid/features/HiresScroll.h +++ b/src/logid/features/HiresScroll.h @@ -32,8 +32,6 @@ namespace logid::features { public: explicit HiresScroll(Device* dev); - ~HiresScroll() noexcept override; - void configure() final; void listen() final; @@ -43,7 +41,7 @@ namespace logid::features { void setMode(uint8_t mode); private: - std::optional _ev_handler; + EventHandlerLock _ev_handler; void _makeGesture(std::shared_ptr& gesture, std::optional& config, diff --git a/src/logid/features/RemapButton.cpp b/src/logid/features/RemapButton.cpp index 423b5e2..46ffa25 100644 --- a/src/logid/features/RemapButton.cpp +++ b/src/logid/features/RemapButton.cpp @@ -91,18 +91,13 @@ RemapButton::RemapButton(Device* dev) : DeviceFeature(dev), } } -RemapButton::~RemapButton() noexcept { - if (_ev_handler.has_value()) - _device->hidpp20().removeEventHandler(_ev_handler.value()); -} - void RemapButton::configure() { for (const auto& button: _buttons) button.second->configure(); } void RemapButton::listen() { - if (!_ev_handler.has_value()) { + if (_ev_handler.empty()) { _ev_handler = _device->hidpp20().addEventHandler( { [index = _reprog_controls->featureIndex()]( diff --git a/src/logid/features/RemapButton.h b/src/logid/features/RemapButton.h index 4870972..5de3583 100644 --- a/src/logid/features/RemapButton.h +++ b/src/logid/features/RemapButton.h @@ -85,8 +85,6 @@ namespace logid::features { public: explicit RemapButton(Device* dev); - ~RemapButton() noexcept override; - void configure() final; void listen() final; @@ -114,7 +112,7 @@ namespace logid::features { }; std::shared_ptr _ipc_interface; - std::optional _ev_handler; + EventHandlerLock _ev_handler; }; } diff --git a/src/logid/features/ThumbWheel.cpp b/src/logid/features/ThumbWheel.cpp index 6b83fad..f0e8bdd 100644 --- a/src/logid/features/ThumbWheel.cpp +++ b/src/logid/features/ThumbWheel.cpp @@ -109,11 +109,6 @@ ThumbWheel::ThumbWheel(Device* dev) : DeviceFeature(dev), _wheel_info(), _ipc_interface = dev->ipcNode()->make_interface(this); } -ThumbWheel::~ThumbWheel() noexcept { - if (_ev_handler.has_value()) - _device->hidpp20().removeEventHandler(_ev_handler.value()); -} - void ThumbWheel::configure() { std::shared_lock lock(_config_mutex); if (_config.has_value()) { @@ -124,7 +119,7 @@ void ThumbWheel::configure() { } void ThumbWheel::listen() { - if (!_ev_handler.has_value()) { + if (_ev_handler.empty()) { _ev_handler = _device->hidpp20().addEventHandler( { [index = _thumb_wheel->featureIndex()] diff --git a/src/logid/features/ThumbWheel.h b/src/logid/features/ThumbWheel.h index 4a448a3..fc528ac 100644 --- a/src/logid/features/ThumbWheel.h +++ b/src/logid/features/ThumbWheel.h @@ -28,8 +28,6 @@ namespace logid::features { public: explicit ThumbWheel(Device* dev); - ~ThumbWheel() noexcept override; - void configure() final; void listen() final; @@ -88,7 +86,7 @@ namespace logid::features { mutable std::shared_mutex _config_mutex; std::optional& _config; - std::optional _ev_handler; + EventHandlerLock _ev_handler; }; }