diff --git a/src/logid/Configuration.h b/src/logid/Configuration.h index 5c90974..621f0bd 100644 --- a/src/logid/Configuration.h +++ b/src/logid/Configuration.h @@ -30,7 +30,7 @@ namespace logid { namespace defaults { - static constexpr double io_timeout = 500; + static constexpr double io_timeout = 1000; static constexpr int gesture_threshold = 50; } diff --git a/src/logid/backend/hidpp/Device.cpp b/src/logid/backend/hidpp/Device.cpp index eb61dd2..96472e2 100644 --- a/src/logid/backend/hidpp/Device.cpp +++ b/src/logid/backend/hidpp/Device.cpp @@ -51,10 +51,10 @@ Device::InvalidDevice::Reason Device::InvalidDevice::code() const noexcept { } Device::Device(const std::string& path, DeviceIndex index, - std::shared_ptr monitor, double timeout) : + const std::shared_ptr& monitor, double timeout) : io_timeout(duration_cast( duration(timeout))), - _raw_device(std::make_shared(path, std::move(monitor))), + _raw_device(std::make_shared(path, monitor)), _receiver(nullptr), _path(path), _index(index) { _init(); } @@ -133,7 +133,7 @@ void Device::_init() { auto rsp = sendReport( {ReportType::Short, _index, hidpp20::FeatureID::ROOT, hidpp20::Root::Ping, - LOGID_HIDPP_SOFTWARE_ID}); + hidpp::softwareID}); if (rsp.deviceIndex() != _index) { throw InvalidDevice(InvalidDevice::VirtualNode); } @@ -152,12 +152,9 @@ void Device::_init() { [index = this->_index]( const std::vector& report) -> bool { - return (report[Offset::Type] == - Report::Type::Short || - report[Offset::Type] == - Report::Type::Long) && - (report[Offset::DeviceIndex] == - index); + return (report[Offset::Type] == Report::Type::Short || + report[Offset::Type] == Report::Type::Long) && + (report[Offset::DeviceIndex] == index); }, [this](const std::vector& report) -> void { Report _report(report); @@ -212,13 +209,13 @@ Device::~Device() { } Device::EvHandlerId Device::addEventHandler(EventHandler handler) { - std::lock_guard lock(_event_handler_lock); + std::unique_lock lock(_event_handler_mutex); _event_handlers.emplace_front(std::move(handler)); return _event_handlers.cbegin(); } void Device::removeEventHandler(EvHandlerId id) { - std::lock_guard lock(_event_handler_lock); + std::unique_lock lock(_event_handler_mutex); _event_handlers.erase(id); } @@ -226,66 +223,82 @@ void Device::handleEvent(Report& report) { if (responseReport(report)) return; - std::lock_guard lock(_event_handler_lock); + std::shared_lock lock(_event_handler_mutex); for (auto& handler: _event_handlers) if (handler.condition(report)) handler.callback(report); } Report Device::sendReport(const Report& report) { - std::lock_guard lock(_send_lock); - { - std::lock_guard sl(_slot_lock); - _report_slot = {}; - } - sendReportNoResponse(report); - std::unique_lock wait(_resp_wait_lock); - bool valid = _resp_cv.wait_for( - wait, io_timeout, [this]() { - std::lock_guard sl(_slot_lock); - return _report_slot.has_value(); + /* Must complete transaction before next send */ + std::lock_guard send_lock(_send_mutex); + _sent_sub_id = report.subId(); + std::unique_lock lock(_response_mutex); + _sendReport(report); + + bool valid = _response_cv.wait_for( + lock, io_timeout, [this]() { + return _response.has_value(); }); - if (!valid) + if (!valid) { + _sent_sub_id.reset(); throw TimeoutError(); - - std::lock_guard sl(_slot_lock); - - { - Report::Hidpp10Error error{}; - if (_report_slot.value().isError10(&error)) - throw hidpp10::Error(error.error_code); } - { - Report::Hidpp20Error error{}; - if (_report_slot.value().isError20(&error)) - throw hidpp20::Error(error.error_code); + + Response response = _response.value(); + _response.reset(); + _sent_sub_id.reset(); + + if (std::holds_alternative(response)) { + return std::get(response); + } else if(std::holds_alternative(response)) { + throw hidpp20::Error(std::get(response).error_code); + } else if(std::holds_alternative(response)) { + throw hidpp20::Error(std::get(response).error_code); } - return _report_slot.value(); + + // Should not be reached + throw std::runtime_error("unhandled variant type"); } bool Device::responseReport(const Report& report) { - if (_send_lock.try_lock()) { - _send_lock.unlock(); + std::lock_guard lock(_response_mutex); + Response response = report; + uint8_t sub_id; + + Report::Hidpp10Error hidpp10_error{}; + Report::Hidpp20Error hidpp20_error{}; + if (report.isError10(&hidpp10_error)) { + sub_id = hidpp10_error.sub_id; + response = hidpp10_error; + } else if (report.isError20(&hidpp20_error)) { + sub_id = hidpp20_error.feature_index; + response = hidpp20_error; + } else { + sub_id = report.subId(); + } + + if (sub_id == _sent_sub_id) { + _response = response; + _response_cv.notify_all(); + return true; + } else { return false; } - // Basic check to see if the report is a response - if ((report.swId() != LOGID_HIDPP_SOFTWARE_ID) - && report.subId() < 0x80) - return false; - - std::lock_guard lock(_slot_lock); - _report_slot = report; - _resp_cv.notify_all(); - return true; } -void Device::sendReportNoResponse(Report report) { +void Device::_sendReport(Report report) { reportFixup(report); _raw_device->sendReport(report.rawReport()); } +void Device::sendReportNoACK(const Report& report) { + std::lock_guard lock(_send_mutex); + _sendReport(report); +} + void Device::reportFixup(Report& report) const { switch (report.type()) { case Report::Type::Short: diff --git a/src/logid/backend/hidpp/Device.h b/src/logid/backend/hidpp/Device.h index 8976203..9536a73 100644 --- a/src/logid/backend/hidpp/Device.h +++ b/src/logid/backend/hidpp/Device.h @@ -20,6 +20,7 @@ #define LOGID_BACKEND_HIDPP_DEVICE_H #include +#include #include #include #include @@ -64,7 +65,7 @@ namespace logid::backend::hidpp { }; Device(const std::string& path, DeviceIndex index, - std::shared_ptr monitor, double timeout); + const std::shared_ptr& monitor, double timeout); Device(std::shared_ptr raw_device, DeviceIndex index, double timeout); @@ -93,7 +94,7 @@ namespace logid::backend::hidpp { virtual Report sendReport(const Report& report); - void sendReportNoResponse(Report report); + virtual void sendReportNoACK(const Report& report); void handleEvent(Report& report); @@ -101,10 +102,15 @@ namespace logid::backend::hidpp { // Returns whether the report is a response virtual bool responseReport(const Report& report); + void _sendReport(Report report); + void reportFixup(Report& report) const; const std::chrono::milliseconds io_timeout; - uint8_t supported_reports; + uint8_t supported_reports{}; + + std::mutex _response_mutex; + std::condition_variable _response_cv; private: void _init(); @@ -115,16 +121,17 @@ namespace logid::backend::hidpp { DeviceIndex _index; std::tuple _version; - uint16_t _pid; + uint16_t _pid{}; std::string _name; - std::mutex _send_lock; - std::mutex _resp_wait_lock; - std::condition_variable _resp_cv; - std::mutex _slot_lock; - std::optional _report_slot; + std::mutex _send_mutex; - std::mutex _event_handler_lock; + typedef std::variant Response; + + std::optional _response; + std::optional _sent_sub_id{}; + + std::shared_mutex _event_handler_mutex; std::list _event_handlers; }; } diff --git a/src/logid/backend/hidpp/defs.h b/src/logid/backend/hidpp/defs.h index 2bde9ae..253f3c0 100644 --- a/src/logid/backend/hidpp/defs.h +++ b/src/logid/backend/hidpp/defs.h @@ -19,8 +19,6 @@ #ifndef LOGID_BACKEND_HIDPP_DEFS_H #define LOGID_BACKEND_HIDPP_DEFS_H -#define LOGID_HIDPP_SOFTWARE_ID 2 - #include namespace logid::backend::hidpp { @@ -42,6 +40,10 @@ namespace logid::backend::hidpp { WirelessDevice6 = 6, }; + static constexpr uint8_t softwareID = 2; + /* For sending reports with no response, use a different SW ID */ + static constexpr uint8_t noAckSoftwareID = 1; + static constexpr std::size_t ShortParamLength = 3; static constexpr std::size_t LongParamLength = 16; } diff --git a/src/logid/backend/hidpp10/Device.cpp b/src/logid/backend/hidpp10/Device.cpp index bfec9b9..4ec4af9 100644 --- a/src/logid/backend/hidpp10/Device.cpp +++ b/src/logid/backend/hidpp10/Device.cpp @@ -25,15 +25,13 @@ using namespace logid::backend; using namespace logid::backend::hidpp10; -Device::Device(const std::string& path, - hidpp::DeviceIndex index, - std::shared_ptr monitor, double timeout) : - hidpp::Device(path, index, std::move(monitor), timeout) { +Device::Device(const std::string& path, hidpp::DeviceIndex index, + const std::shared_ptr& monitor, double timeout) : + hidpp::Device(path, index, monitor, timeout) { assert(version() == std::make_tuple(1, 0)); } -Device::Device(std::shared_ptr raw_dev, - hidpp::DeviceIndex index, +Device::Device(std::shared_ptr raw_dev, hidpp::DeviceIndex index, double timeout) : hidpp::Device(std::move(raw_dev), index, timeout) { assert(version() == std::make_tuple(1, 0)); } @@ -45,44 +43,34 @@ Device::Device(const std::shared_ptr& receiver, assert(version() == std::make_tuple(1, 0)); } -hidpp::Report Device::sendReport(const hidpp::Report& report) { - decltype(_responses)::iterator response_slot; - while (true) { - { - std::lock_guard lock(_response_lock); - response_slot = _responses.find(report.subId()); - if (response_slot == _responses.end()) { - response_slot = _responses.emplace( - report.subId(), std::optional()).first; - break; - } - } - std::unique_lock lock(_response_wait_lock); - _response_cv.wait(lock, [this, sub_id = report.subId()]() { - std::lock_guard lock(_response_lock); - return _responses.find(sub_id) != _responses.end(); - }); - } +void Device::ResponseSlot::reset() { + response.reset(); + sub_id.reset(); +} - sendReportNoResponse(report); - std::unique_lock wait(_response_wait_lock); +hidpp::Report Device::sendReport(const hidpp::Report& report) { + auto& response_slot = _responses[report.subId() % SubIDCount]; + + std::unique_lock lock(_response_mutex); + _response_cv.wait(lock, [&response_slot]() { + return !response_slot.sub_id.has_value(); + }); + response_slot.sub_id = report.subId(); + + _sendReport(report); bool valid = _response_cv.wait_for( - wait, io_timeout, - [this, &response_slot]() { - std::lock_guard lock(_response_lock); - return response_slot->second.has_value(); + lock, io_timeout, + [&response_slot]() { + return response_slot.response.has_value(); }); if (!valid) { - std::lock_guard lock(_response_lock); - _responses.erase(response_slot); + response_slot.reset(); throw TimeoutError(); } - std::lock_guard lock(_response_lock); - assert(response_slot->second.has_value()); - auto response = response_slot->second.value(); - _responses.erase(response_slot); + auto response = response_slot.response.value(); + response_slot.reset(); if (std::holds_alternative(response)) return std::get(response); else // if(std::holds_alternative(response)) @@ -90,7 +78,7 @@ hidpp::Report Device::sendReport(const hidpp::Report& report) { } bool Device::responseReport(const hidpp::Report& report) { - std::lock_guard lock(_response_lock); + std::lock_guard lock(_response_mutex); uint8_t sub_id; bool is_error = false; @@ -102,15 +90,15 @@ bool Device::responseReport(const hidpp::Report& report) { sub_id = report.subId(); } - auto response_slot = _responses.find(sub_id); - if (response_slot == _responses.end()) + auto& response_slot = _responses[sub_id % SubIDCount]; + + if (!response_slot.sub_id.has_value() || response_slot.sub_id.value() != sub_id) return false; if (is_error) { - response_slot->second = static_cast( - hidpp10_error.error_code); + response_slot.response = static_cast(hidpp10_error.error_code); } else { - response_slot->second = report; + response_slot.response = report; } _response_cv.notify_all(); diff --git a/src/logid/backend/hidpp10/Device.h b/src/logid/backend/hidpp10/Device.h index 37b7ed0..9dc8034 100644 --- a/src/logid/backend/hidpp10/Device.h +++ b/src/logid/backend/hidpp10/Device.h @@ -24,12 +24,13 @@ #include "../hidpp/Device.h" #include "Error.h" +#include "defs.h" namespace logid::backend::hidpp10 { class Device : public hidpp::Device { public: Device(const std::string& path, hidpp::DeviceIndex index, - std::shared_ptr monitor, double timeout); + const std::shared_ptr& monitor, double timeout); Device(std::shared_ptr raw_dev, hidpp::DeviceIndex index, double timeout); @@ -51,15 +52,16 @@ namespace logid::backend::hidpp10 { bool responseReport(const hidpp::Report& report) final; private: - std::mutex _response_lock; - std::mutex _response_wait_lock; - std::condition_variable _response_cv; - typedef std::variant Response; - std::map> _responses; + struct ResponseSlot { + std::optional response; + std::optional sub_id; + void reset(); + }; + std::array _responses; - std::vector accessRegister(uint8_t sub_id, - uint8_t address, const std::vector& params); + std::vector accessRegister( + uint8_t sub_id, uint8_t address, const std::vector& params); }; } diff --git a/src/logid/backend/hidpp10/defs.h b/src/logid/backend/hidpp10/defs.h index df20622..ff18ed4 100644 --- a/src/logid/backend/hidpp10/defs.h +++ b/src/logid/backend/hidpp10/defs.h @@ -24,8 +24,10 @@ namespace logid::backend::hidpp10 { SetRegisterShort = 0x80, GetRegisterShort = 0x81, SetRegisterLong = 0x82, - GetRegisterLong = 0x83 + GetRegisterLong = 0x83, }; + + static constexpr size_t SubIDCount = 4; } #endif //LOGID_BACKEND_HIDPP10_DEFS_H \ No newline at end of file diff --git a/src/logid/backend/hidpp20/Device.cpp b/src/logid/backend/hidpp20/Device.cpp index 8e650bd..defaabb 100644 --- a/src/logid/backend/hidpp20/Device.cpp +++ b/src/logid/backend/hidpp20/Device.cpp @@ -26,9 +26,8 @@ using namespace logid::backend; using namespace logid::backend::hidpp20; Device::Device(const std::string& path, hidpp::DeviceIndex index, - std::shared_ptr monitor, double timeout) : - hidpp::Device(path, index, - std::move(monitor), timeout) { + const std::shared_ptr& monitor, double timeout) : + hidpp::Device(path, index, monitor, timeout) { // TODO: Fix version check if (std::get<0>(version()) < 2) throw std::runtime_error("Invalid HID++ version"); @@ -68,7 +67,7 @@ std::vector Device::callFunction(uint8_t feature_index, throw hidpp::Report::InvalidReportID(); hidpp::Report request(type, deviceIndex(), feature_index, function, - LOGID_HIDPP_SOFTWARE_ID); + hidpp::softwareID); std::copy(params.begin(), params.end(), request.paramBegin()); auto response = this->sendReport(request); @@ -87,101 +86,85 @@ void Device::callFunctionNoResponse(uint8_t feature_index, uint8_t function, else throw hidpp::Report::InvalidReportID(); - hidpp::Report request(type, deviceIndex(), feature_index, function, - LOGID_HIDPP_SOFTWARE_ID); + hidpp::Report request(type, deviceIndex(), feature_index, function, hidpp::softwareID); std::copy(params.begin(), params.end(), request.paramBegin()); - this->sendReportNoResponse(request); + this->sendReportNoACK(request); } hidpp::Report Device::sendReport(const hidpp::Report& report) { - decltype(_responses)::iterator response_slot; + auto& response_slot = _responses[report.feature() % _responses.size()]; - while (true) { - { - std::lock_guard lock(_response_lock); - if (_responses.empty()) { - response_slot = _responses.emplace( - 2, std::optional()).first; - break; - } else if (_responses.size() < response_slots) { - uint8_t i = 0; - for (auto& x: _responses) { - if (x.first != i + 1) { - ++i; - break; - } - i = x.first; - } - assert(_responses.count(i) == 0); + std::unique_lock response_lock(_response_mutex); + _response_cv.wait(response_lock, [&response_slot]() { + return !response_slot.feature.has_value(); + }); - response_slot = _responses.emplace( - i, std::optional()).first; - break; - } - } + response_slot.feature = report.feature(); - std::unique_lock lock(_response_wait_lock); - _response_cv.wait(lock, [this, sub_id = report.subId()]() { - std::lock_guard lock(_response_lock); - return _responses.size() < response_slots; - }); - } + _sendReport(report); - { - std::lock_guard lock(_response_lock); - hidpp::Report mod_report{report}; - mod_report.setSwId(response_slot->first); - sendReportNoResponse(std::move(mod_report)); - } - - std::unique_lock wait(_response_wait_lock); - bool valid = _response_cv.wait_for(wait, io_timeout, - [this, &response_slot]() { - std::lock_guard lock(_response_lock); - return response_slot->second.has_value(); - }); + bool valid = _response_cv.wait_for( + response_lock, io_timeout, + [&response_slot]() { + return response_slot.response.has_value(); + }); if (!valid) { - std::lock_guard lock(_response_lock); - _responses.erase(response_slot); + response_slot.reset(); throw TimeoutError(); } - std::lock_guard lock(_response_lock); - assert(response_slot->second.has_value()); - auto response = response_slot->second.value(); - _responses.erase(response_slot); + assert(response_slot.response.has_value()); + auto response = response_slot.response.value(); + response_slot.reset(); + if (std::holds_alternative(response)) return std::get(response); else // if(std::holds_alternative(response)) throw Error(std::get(response)); } +void Device::sendReportNoACK(const hidpp::Report& report) { + hidpp::Report no_ack_report(report); + no_ack_report.setSwId(hidpp::noAckSoftwareID); + _sendReport(std::move(no_ack_report)); +} + bool Device::responseReport(const hidpp::Report& report) { - std::lock_guard lock(_response_lock); - uint8_t sw_id; + auto& response_slot = _responses[report.feature() % _responses.size()]; + std::lock_guard lock(_response_mutex); + uint8_t sw_id, feature; bool is_error = false; hidpp::Report::Hidpp20Error hidpp20_error{}; if (report.isError20(&hidpp20_error)) { is_error = true; sw_id = hidpp20_error.software_id; + feature = hidpp20_error.feature_index; } else { sw_id = report.swId(); + feature = report.feature(); } - auto response_slot = _responses.find(sw_id); - if (response_slot == _responses.end()) + if (sw_id != hidpp::softwareID) return false; + if (!response_slot.feature || response_slot.feature.value() != feature) { + return false; + } + if (is_error) { - response_slot->second = static_cast( - hidpp20_error.error_code); + response_slot.response = static_cast(hidpp20_error.error_code); } else { - response_slot->second = report; + response_slot.response = report; } _response_cv.notify_all(); return true; } + +void Device::ResponseSlot::reset() { + response.reset(); + feature.reset(); +} diff --git a/src/logid/backend/hidpp20/Device.h b/src/logid/backend/hidpp20/Device.h index 7fe32bf..4a0e3ae 100644 --- a/src/logid/backend/hidpp20/Device.h +++ b/src/logid/backend/hidpp20/Device.h @@ -30,7 +30,7 @@ namespace logid::backend::hidpp20 { class Device : public hidpp::Device { public: Device(const std::string& path, hidpp::DeviceIndex index, - std::shared_ptr monitor, double timeout); + const std::shared_ptr& monitor, double timeout); Device(std::shared_ptr raw_device, hidpp::DeviceIndex index, double timeout); @@ -51,17 +51,21 @@ namespace logid::backend::hidpp20 { hidpp::Report sendReport(const hidpp::Report& report) final; + void sendReportNoACK(const hidpp::Report& report) final; + protected: bool responseReport(const hidpp::Report& report) final; private: - std::mutex _response_lock; - std::mutex _response_wait_lock; - std::condition_variable _response_cv; - - static constexpr int response_slots = 14; typedef std::variant Response; - std::map> _responses; + struct ResponseSlot { + std::optional response; + std::optional feature; + void reset(); + }; + + /* Multiplex responses on lower nibble of SubID, ignore upper nibble for space */ + std::array _responses; }; } diff --git a/src/logid/backend/hidpp20/EssentialFeature.cpp b/src/logid/backend/hidpp20/EssentialFeature.cpp index a52975d..3eadfcb 100644 --- a/src/logid/backend/hidpp20/EssentialFeature.cpp +++ b/src/logid/backend/hidpp20/EssentialFeature.cpp @@ -36,8 +36,7 @@ std::vector EssentialFeature::callFunction(uint8_t function_id, else throw hidpp::Report::InvalidReportID(); - hidpp::Report request(type, _device->deviceIndex(), _index, function_id, - LOGID_HIDPP_SOFTWARE_ID); + hidpp::Report request(type, _device->deviceIndex(), _index, function_id, hidpp::softwareID); std::copy(params.begin(), params.end(), request.paramBegin()); auto response = _device->sendReport(request); diff --git a/src/logid/backend/raw/IOMonitor.cpp b/src/logid/backend/raw/IOMonitor.cpp index e89fa00..11bce03 100644 --- a/src/logid/backend/raw/IOMonitor.cpp +++ b/src/logid/backend/raw/IOMonitor.cpp @@ -86,8 +86,8 @@ void IOMonitor::_listen() { _is_running = true; while (_is_running) { - { - std::unique_lock lock(_interrupt_lock); + if (_interrupting) { + std::unique_lock lock(_interrupt_mutex); _interrupt_cv.wait(lock, [this]() { return !(bool) _interrupting; }); diff --git a/src/logid/backend/raw/IOMonitor.h b/src/logid/backend/raw/IOMonitor.h index 9453dc9..d26f955 100644 --- a/src/logid/backend/raw/IOMonitor.h +++ b/src/logid/backend/raw/IOMonitor.h @@ -64,7 +64,7 @@ namespace logid::backend::raw { std::atomic_bool _is_running; std::atomic_bool _interrupting; - std::mutex _interrupt_lock; + std::mutex _interrupt_mutex; std::condition_variable _interrupt_cv; const int _epoll_fd; diff --git a/src/logid/backend/raw/RawDevice.cpp b/src/logid/backend/raw/RawDevice.cpp index 9535535..9f5497c 100644 --- a/src/logid/backend/raw/RawDevice.cpp +++ b/src/logid/backend/raw/RawDevice.cpp @@ -155,13 +155,13 @@ void RawDevice::sendReport(const std::vector& report) { } RawDevice::EvHandlerId RawDevice::addEventHandler(RawEventHandler handler) { - std::lock_guard lock(_event_handler_lock); + 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::lock_guard lock(_event_handler_lock); + std::unique_lock lock(_event_handler_mutex); _event_handlers.erase(id); } @@ -185,7 +185,7 @@ void RawDevice::_readReports() { } void RawDevice::_handleEvent(const std::vector& report) { - std::unique_lock lock(_event_handler_lock); + std::shared_lock lock(_event_handler_mutex); for (auto& handler: _event_handlers) if (handler.condition(report)) handler.callback(report); diff --git a/src/logid/backend/raw/RawDevice.h b/src/logid/backend/raw/RawDevice.h index 030c304..ee4e2c9 100644 --- a/src/logid/backend/raw/RawDevice.h +++ b/src/logid/backend/raw/RawDevice.h @@ -21,8 +21,7 @@ #include #include -#include -#include +#include #include #include #include @@ -85,7 +84,7 @@ namespace logid::backend::raw { std::shared_ptr _io_monitor; std::list _event_handlers; - std::mutex _event_handler_lock; + std::shared_mutex _event_handler_mutex; void _handleEvent(const std::vector& report); };