From da56e434974e66ce7fdb2ed47c6327dcf966219b Mon Sep 17 00:00:00 2001 From: Joel Winarske Date: Sun, 12 Jul 2026 15:30:04 -0700 Subject: [PATCH 1/2] Fold InputReader into the loop and make UPower construction async Completes the BlueZ event-loop migration: the controllers now run entirely on the single EventLoop thread with no per-device threads and no in-callback blocking D-Bus calls. InputReader is now an EventSource: - The device is opened and its descriptors/feature reports are read in the constructor (non-blocking fd); dispatch() reads and decodes one report per readable event. valid() reports whether initialisation succeeded. - Its worker thread, stop eventfd and private epoll are gone. The controllers add the reader to the loop on device arrival and retire it on removal. EventLoop::retire() added: unregisters a source and destroys it at a safe point (after the current dispatch pass), so a udev "remove" handler can drop the reader for the device that just went away without a use-after-free on an fd still in the current poll set. UPower construction is now asynchronous: - UPowerClient fetches properties via GetAllAsync and enumerates devices via callMethodAsync; UPowerDisplayDevice fetches properties via GetAllAsync. The single loop thread is no longer blocked by these round-trips when a controller with a battery connects. Pending calls are cancelled by unregisterProxy() if the object is destroyed first. UPowerClient keeps its mutex because it is also used by the standalone upower example, which still runs on the connection's own async event-loop thread. Verified at runtime: the clients start, the loop drives the D-Bus callbacks, the async UPower GetAll reply is delivered (onPropertiesChanged fires), and SIGTERM shuts down cleanly. The HID input data path needs real controller hardware and is build-verified only. Signed-off-by: Joel Winarske --- src/bluez/horipad_steam/horipad_steam.cc | 20 +- src/bluez/horipad_steam/horipad_steam.h | 6 +- src/bluez/horipad_steam/input_reader.cc | 297 +++++++----------- src/bluez/horipad_steam/input_reader.h | 37 +-- src/bluez/horipad_steam/main.cc | 2 +- src/bluez/ps5_dual_sense/dual_sense.cc | 20 +- src/bluez/ps5_dual_sense/dual_sense.h | 6 +- src/bluez/ps5_dual_sense/input_reader.cc | 308 +++++++------------ src/bluez/ps5_dual_sense/input_reader.h | 39 +-- src/bluez/ps5_dual_sense/main.cc | 2 +- src/bluez/xbox_controller/input_reader.cc | 291 +++++++----------- src/bluez/xbox_controller/input_reader.h | 31 +- src/bluez/xbox_controller/main.cc | 2 +- src/bluez/xbox_controller/xbox_controller.cc | 20 +- src/bluez/xbox_controller/xbox_controller.h | 6 +- src/upower/upower_client.h | 43 ++- src/upower/upower_display_device.cc | 25 +- src/utils/event_loop.cc | 12 + src/utils/event_loop.h | 11 + 19 files changed, 515 insertions(+), 663 deletions(-) diff --git a/src/bluez/horipad_steam/horipad_steam.cc b/src/bluez/horipad_steam/horipad_steam.cc index 0187a6a..5c21b84 100644 --- a/src/bluez/horipad_steam/horipad_steam.cc +++ b/src/bluez/horipad_steam/horipad_steam.cc @@ -32,7 +32,7 @@ const std::vector> input_match_params_usb = {"ID_MODEL_ID", "01ab"}, {"TAGS", ":seat:"}}; -HoripadSteam::HoripadSteam(sdbus::IConnection& connection) +HoripadSteam::HoripadSteam(sdbus::IConnection& connection, EventLoop& loop) : ProxyInterfaces(connection, sdbus::ServiceName(INTERFACE_NAME), sdbus::ObjectPath("/")), @@ -46,15 +46,17 @@ HoripadSteam::HoripadSteam(sdbus::IConnection& connection) if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { if (input_reader_) { - input_reader_->stop(); - input_reader_.reset(); + // Retire (not reset) so the reader outlives this + // dispatch pass; the loop destroys it safely. + loop_.retire(std::move(input_reader_)); } } if (!get_hidraw_devices(input_match_params_bt)) { get_hidraw_devices(input_match_params_usb); } } - }) { + }), + loop_(loop) { if (!get_hidraw_devices(input_match_params_bt)) { get_hidraw_devices(input_match_params_usb); } @@ -153,8 +155,14 @@ void HoripadSteam::onInterfacesAdded( !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); if (!input_reader_) { - input_reader_ = std::make_unique(hidraw_device); - input_reader_->start(); + auto reader = std::make_unique(hidraw_device); + if (reader->valid()) { + loop_.add(reader.get()); + input_reader_ = std::move(reader); + } else { + LOG_ERROR("Failed to initialize hidraw reader: {}", + hidraw_device); + } } } } diff --git a/src/bluez/horipad_steam/horipad_steam.h b/src/bluez/horipad_steam/horipad_steam.h index 3894aea..fd4348e 100644 --- a/src/bluez/horipad_steam/horipad_steam.h +++ b/src/bluez/horipad_steam/horipad_steam.h @@ -31,7 +31,7 @@ class HoripadSteam final public Hidraw, public UdevMonitor { public: - explicit HoripadSteam(sdbus::IConnection& connection); + HoripadSteam(sdbus::IConnection& connection, EventLoop& loop); ~HoripadSteam() override; @@ -53,6 +53,10 @@ class HoripadSteam final std::map> input1_; std::unique_ptr input_reader_; + // The loop that polls the InputReader source; used to add it on device + // arrival and retire it on removal. + EventLoop& loop_; + void onInterfacesAdded( const sdbus::ObjectPath& objectPath, const std::map #include -#include #include -#include #include -#include -#include +#include #include #include "../../utils/logging.h" #include "../hidraw.hpp" #include "input_reader.h" -InputReader::InputReader(std::string device) - : device_(std::move(device)), - stop_flag_(false), - stop_event_fd_(::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) { - if (!stop_event_fd_.valid()) { - LOG_ERROR("Failed to create eventfd: {}", strerror(errno)); - } +InputReader::InputReader(std::string device) : device_(std::move(device)) { + open_and_init(); } -void InputReader::start() { - LOG_DEBUG("InputReader start: {}", device_); - if (thread_.joinable()) { - return; // already running - } - stop_flag_ = false; - thread_ = std::thread([this] { read_input(); }); -} +void InputReader::open_and_init() { + LOG_DEBUG("hidraw device: {}", device_); -void InputReader::stop() { - LOG_DEBUG("InputReader stop: {}", device_); - stop_flag_ = true; - // Wake the blocking epoll_wait so the loop observes stop_flag_ immediately. - if (stop_event_fd_.valid()) { - constexpr std::uint64_t one = 1; - if (::write(stop_event_fd_.get(), &one, sizeof(one)) < 0) { - LOG_ERROR("Failed to signal stop eventfd: {}", strerror(errno)); - } + // Non-blocking so dispatch()'s read() never stalls the event loop. + UniqueFd fd(open(device_.c_str(), O_RDWR | O_NONBLOCK | O_CLOEXEC)); + if (!fd.valid()) { + LOG_ERROR("unable to open device"); + return; } -} -InputReader::~InputReader() { - stop(); - if (thread_.joinable()) { - thread_.join(); + // Raw Info + hidraw_devinfo raw_dev_info{}; + if (const auto res = ioctl(fd.get(), HIDIOCGRAWINFO, &raw_dev_info); + res < 0) { + LOG_ERROR("HIDIOCGRAWINFO"); + return; } -} - -void InputReader::read_input() { - LOG_DEBUG("hidraw device: {}", device_); - - const UniqueFd fd(open(device_.c_str(), O_RDWR)); - - while (true) { - if (!fd.valid()) { - LOG_ERROR("unable to open device"); - break; - } - - // Raw Info - hidraw_devinfo raw_dev_info{}; - if (const auto res = ioctl(fd.get(), HIDIOCGRAWINFO, &raw_dev_info); - res < 0) { - LOG_ERROR("HIDIOCGRAWINFO"); - break; - } - LOG_INFO("bustype: {}", Hidraw::bus_str(raw_dev_info.bustype)); - LOG_INFO("Vendor ID: {:04X}", raw_dev_info.vendor); - LOG_INFO("Product ID: {:04X}", raw_dev_info.product); - - // Raw Name - std::array buf{}; - auto res = ioctl(fd.get(), HIDIOCGRAWNAME(buf.size()), buf.data()); - if (res < 0) { - LOG_ERROR("HIDIOCGRAWNAME"); - break; - } - buf.back() = '\0'; // guarantee null-termination - LOG_INFO("HID Name: {}", buf.data()); + product_ = raw_dev_info.product; + LOG_INFO("bustype: {}", Hidraw::bus_str(raw_dev_info.bustype)); + LOG_INFO("Vendor ID: {:04X}", raw_dev_info.vendor); + LOG_INFO("Product ID: {:04X}", raw_dev_info.product); + + // Raw Name + std::array buf{}; + auto res = ioctl(fd.get(), HIDIOCGRAWNAME(buf.size()), buf.data()); + if (res < 0) { + LOG_ERROR("HIDIOCGRAWNAME"); + return; + } + buf.back() = '\0'; // guarantee null-termination + LOG_INFO("HID Name: {}", buf.data()); + + // Raw Physical Location + res = ioctl(fd.get(), HIDIOCGRAWPHYS(buf.size()), buf.data()); + if (res < 0) { + LOG_ERROR("HIDIOCGRAWPHYS"); + return; + } + buf.back() = '\0'; // guarantee null-termination + LOG_INFO("HID Physical Location: {}", buf.data()); + + // Report Descriptor Size + int desc_size = 0; + res = ioctl(fd.get(), HIDIOCGRDESCSIZE, &desc_size); + if (res < 0) { + LOG_ERROR("HIDIOCGRDESCSIZE"); + return; + } + LOG_INFO("Report Descriptor Size: {}", desc_size); - // Raw Physical Location - res = ioctl(fd.get(), HIDIOCGRAWPHYS(buf.size()), buf.data()); - if (res < 0) { - LOG_ERROR("HIDIOCGRAWPHYS"); - break; - } - buf.back() = '\0'; // guarantee null-termination - LOG_INFO("HID Physical Location: {}", buf.data()); + if (desc_size < 0 || + static_cast(desc_size) > HID_MAX_DESCRIPTOR_SIZE) { + LOG_ERROR("Invalid report descriptor size: {}", desc_size); + return; + } - // Report Descriptor Size - int desc_size = 0; - res = ioctl(fd.get(), HIDIOCGRDESCSIZE, &desc_size); - if (res < 0) { - LOG_ERROR("HIDIOCGRDESCSIZE"); - break; - } - LOG_INFO("Report Descriptor Size: {}", desc_size); + // Report Descriptor + hidraw_report_descriptor rpt_desc{}; + rpt_desc.size = desc_size; + res = ioctl(fd.get(), HIDIOCGRDESC, &rpt_desc); + if (res < 0) { + LOG_ERROR("HIDIOCGRDESC"); + return; + } - if (desc_size < 0 || - static_cast(desc_size) > HID_MAX_DESCRIPTOR_SIZE) { - LOG_ERROR("Invalid report descriptor size: {}", desc_size); - break; - } + std::ostringstream os; + os << "Report Descriptor\n"; + os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); + LOG_INFO(os.str()); - // Report Descriptor - hidraw_report_descriptor rpt_desc{}; - rpt_desc.size = desc_size; - res = ioctl(fd.get(), HIDIOCGRDESC, &rpt_desc); - if (res < 0) { - LOG_ERROR("HIDIOCGRDESC"); - break; - } + // Initialisation succeeded: keep the fd so the loop can poll it. + fd_ = std::move(fd); +} - std::ostringstream os; - os << "Report Descriptor\n"; - os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); - LOG_INFO(os.str()); +void InputReader::dispatch(const short revents) { + if ((revents & (POLLHUP | POLLERR)) != 0) { + // Device went away; the udev "remove" handler will retire this source. + return; + } - // Wait on both the hidraw fd and the stop eventfd so a blocking read can - // be interrupted immediately when stop() is called from another thread. - const UniqueFd epoll_fd(epoll_create1(EPOLL_CLOEXEC)); - if (!epoll_fd.valid()) { - LOG_ERROR("epoll_create1 failed: {}", strerror(errno)); - break; - } - epoll_event ev{}; - ev.events = EPOLLIN; - ev.data.fd = fd.get(); - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, fd.get(), &ev) == -1) { - LOG_ERROR("epoll_ctl(hidraw) failed: {}", strerror(errno)); - break; - } - if (stop_event_fd_.valid()) { - ev.data.fd = stop_event_fd_.get(); - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, stop_event_fd_.get(), &ev) == - -1) { - LOG_ERROR("epoll_ctl(stop) failed: {}", strerror(errno)); - break; - } + std::array buffer{}; + const ssize_t result = read(fd_.get(), buffer.data(), buffer.size()); + if (result < 0) { + if (errno == EINTR || errno == EAGAIN) { + return; } + LOG_ERROR("read failed: {}", strerror(errno)); + return; + } + if (result == 0) { + return; + } - while (!stop_flag_) { - std::array poll_events{}; - const int nfds = epoll_wait(epoll_fd.get(), poll_events.data(), - poll_events.size(), -1); - if (nfds == -1) { - if (errno == EINTR) { - continue; - } - LOG_ERROR("epoll_wait failed: {}", strerror(errno)); - break; - } - - bool stop_requested = false; - bool data_ready = false; - for (int i = 0; i < nfds; ++i) { - if (stop_event_fd_.valid() && - poll_events.at(i).data.fd == stop_event_fd_.get()) { - stop_requested = true; - } else if (poll_events.at(i).data.fd == fd.get()) { - data_ready = true; - } - } - if (stop_requested) { - break; - } - if (!data_ready) { - continue; - } - - std::array buffer{}; - const ssize_t result = read(fd.get(), buffer.data(), buffer.size()); - if (result < 0) { - if (errno == EINTR || errno == EAGAIN) { - continue; - } - LOG_ERROR("read failed: {}", strerror(errno)); - break; - } - if (result == 0) { - continue; - } - - if (raw_dev_info.product == 0x01ab || raw_dev_info.product == 0x0196) { - if (const auto report_id = buffer.at(0); report_id == 7) { - inputReport07_t input_report07{}; - std::memcpy(&input_report07, buffer.data(), - std::min(sizeof(inputReport07_t), buffer.size())); - PrintInputReport7(input_report07); - } else if (report_id == 10) { - inputReport10_t input_report10{}; - std::memcpy(&input_report10, buffer.data(), - std::min(sizeof(inputReport10_t), buffer.size())); - PrintInputReport10(input_report10); - } else if (report_id == 12) { - inputReport12_t input_report12{}; - std::memcpy(&input_report12, buffer.data(), - std::min(sizeof(inputReport12_t), buffer.size())); - PrintInputReport12(input_report12); - } else if (report_id == 14) { - inputReport14_t input_report14{}; - std::memcpy(&input_report14, buffer.data(), - std::min(sizeof(inputReport14_t), buffer.size())); - PrintInputReport14(input_report14); - } else { - LOG_ERROR("Unknown report id: {}", report_id); - } - } + if (product_ == 0x01ab || product_ == 0x0196) { + if (const auto report_id = buffer.at(0); report_id == 7) { + inputReport07_t input_report07{}; + std::memcpy(&input_report07, buffer.data(), + std::min(sizeof(inputReport07_t), buffer.size())); + PrintInputReport7(input_report07); + } else if (report_id == 10) { + inputReport10_t input_report10{}; + std::memcpy(&input_report10, buffer.data(), + std::min(sizeof(inputReport10_t), buffer.size())); + PrintInputReport10(input_report10); + } else if (report_id == 12) { + inputReport12_t input_report12{}; + std::memcpy(&input_report12, buffer.data(), + std::min(sizeof(inputReport12_t), buffer.size())); + PrintInputReport12(input_report12); + } else if (report_id == 14) { + inputReport14_t input_report14{}; + std::memcpy(&input_report14, buffer.data(), + std::min(sizeof(inputReport14_t), buffer.size())); + PrintInputReport14(input_report14); + } else { + LOG_ERROR("Unknown report id: {}", report_id); } - break; } - - // fd is automatically closed by UniqueFd destructor. } std::string InputReader::dpad_to_string(const Direction dpad) { diff --git a/src/bluez/horipad_steam/input_reader.h b/src/bluez/horipad_steam/input_reader.h index 773dc15..8af5647 100644 --- a/src/bluez/horipad_steam/input_reader.h +++ b/src/bluez/horipad_steam/input_reader.h @@ -12,16 +12,21 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ -#define SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ +#ifndef SRC_BLUEZ_HORIPAD_STEAM_INPUT_READER_HPP_ +#define SRC_BLUEZ_HORIPAD_STEAM_INPUT_READER_HPP_ -#include -#include +#include +#include +#include "../../utils/event_loop.h" #include "../../utils/unique_fd.h" #include "horipad_stream_01ab_0196.h" -class InputReader { +/// Reads and decodes hidraw input reports for a HoriPad Steam controller as an +/// EventSource: the hidraw fd is polled by the EventLoop and dispatch() reads +/// and prints one report per readable event. Register it with the loop only +/// when valid(). +class InputReader final : public EventSource { public: enum class Direction : uint8_t { North = 0, @@ -36,24 +41,20 @@ class InputReader { }; explicit InputReader(std::string device); + ~InputReader() override = default; - void start(); + [[nodiscard]] bool valid() const { return fd_.valid(); } - void stop(); - - ~InputReader(); + [[nodiscard]] int fd() const override { return fd_.get(); } + void dispatch(short revents) override; private: std::string device_; - std::atomic stop_flag_; - // eventfd used to interrupt the blocking read loop immediately on stop(). - UniqueFd stop_event_fd_; - // Worker thread that owns the blocking read loop. Joined in the destructor - // before any other member is torn down, so the loop can never outlive this - // object (no use-after-free) and never blocks the D-Bus/main thread. - std::thread thread_; + UniqueFd fd_; + std::uint16_t product_ = 0; - void read_input(); + // Opens the device and reads its descriptor; leaves fd_ invalid on failure. + void open_and_init(); static std::string dpad_to_string(Direction dpad); @@ -66,4 +67,4 @@ class InputReader { static void PrintInputReport14(const inputReport14_t& input_report14); }; -#endif // SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ +#endif // SRC_BLUEZ_HORIPAD_STEAM_INPUT_READER_HPP_ diff --git a/src/bluez/horipad_steam/main.cc b/src/bluez/horipad_steam/main.cc index f473439..adab3bf 100644 --- a/src/bluez/horipad_steam/main.cc +++ b/src/bluez/horipad_steam/main.cc @@ -34,7 +34,7 @@ int main() { const auto connection = sdbus::createSystemBusConnection(); - HoripadSteam client(*connection); + HoripadSteam client(*connection, loop); loop.add(&client); // HoripadSteam is a UdevMonitor (an EventSource) LOG_INFO("HoriPad Steam client running - Press Ctrl+C to exit"); diff --git a/src/bluez/ps5_dual_sense/dual_sense.cc b/src/bluez/ps5_dual_sense/dual_sense.cc index 622b8fa..78d1847 100644 --- a/src/bluez/ps5_dual_sense/dual_sense.cc +++ b/src/bluez/ps5_dual_sense/dual_sense.cc @@ -31,7 +31,7 @@ const std::vector> input_match_usb = { {"ID_MODEL_ID", "0ce6"}, {"TAGS", ":seat:"}}; -DualSense::DualSense(sdbus::IConnection& connection) +DualSense::DualSense(sdbus::IConnection& connection, EventLoop& loop) : ProxyInterfaces(connection, sdbus::ServiceName(INTERFACE_NAME), sdbus::ObjectPath("/")), @@ -45,15 +45,17 @@ DualSense::DualSense(sdbus::IConnection& connection) if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { if (input_reader_) { - input_reader_->stop(); - input_reader_.reset(); + // Retire (not reset) so the reader outlives this + // dispatch pass; the loop destroys it safely. + loop_.retire(std::move(input_reader_)); } } if (!get_hidraw_devices(input_match_bt)) { get_hidraw_devices(input_match_usb); } } - }) { + }), + loop_(loop) { if (!get_hidraw_devices(input_match_bt)) { get_hidraw_devices(input_match_usb); } @@ -155,8 +157,14 @@ void DualSense::onInterfacesAdded( !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); if (!input_reader_) { - input_reader_ = std::make_unique(hidraw_device); - input_reader_->start(); + auto reader = std::make_unique(hidraw_device); + if (reader->valid()) { + loop_.add(reader.get()); + input_reader_ = std::move(reader); + } else { + LOG_ERROR("Failed to initialize hidraw reader: {}", + hidraw_device); + } } } } diff --git a/src/bluez/ps5_dual_sense/dual_sense.h b/src/bluez/ps5_dual_sense/dual_sense.h index 98166b0..41c4e19 100644 --- a/src/bluez/ps5_dual_sense/dual_sense.h +++ b/src/bluez/ps5_dual_sense/dual_sense.h @@ -32,7 +32,7 @@ class DualSense final public Hidraw, public UdevMonitor { public: - explicit DualSense(sdbus::IConnection& connection); + DualSense(sdbus::IConnection& connection, EventLoop& loop); ~DualSense() override; @@ -55,6 +55,10 @@ class DualSense final std::map> upower_clients_; std::unique_ptr input_reader_; + // The loop that polls the InputReader source; used to add it on device + // arrival and retire it on removal. + EventLoop& loop_; + void onInterfacesAdded( const sdbus::ObjectPath& objectPath, const std::map #include -#include #include #include -#include #include -#include -#include +#include #include #include "../../utils/logging.h" #include "../hidraw.hpp" #include "input_reader.h" -InputReader::InputReader(std::string device) - : device_(std::move(device)), - stop_flag_(false), - stop_event_fd_(::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) { - if (!stop_event_fd_.valid()) { - LOG_ERROR("Failed to create eventfd: {}", strerror(errno)); - } +InputReader::InputReader(std::string device) : device_(std::move(device)) { + open_and_init(); } -void InputReader::start() { - LOG_DEBUG("InputReader start: {}", device_); - if (thread_.joinable()) { - return; // already running - } - stop_flag_ = false; - thread_ = std::thread([this] { read_input(); }); -} +void InputReader::open_and_init() { + LOG_DEBUG("hidraw device: {}", device_); -void InputReader::stop() { - LOG_DEBUG("InputReader stop: {}", device_); - stop_flag_ = true; - // Wake the blocking epoll_wait so the loop observes stop_flag_ immediately. - if (stop_event_fd_.valid()) { - constexpr std::uint64_t one = 1; - if (::write(stop_event_fd_.get(), &one, sizeof(one)) < 0) { - LOG_ERROR("Failed to signal stop eventfd: {}", strerror(errno)); - } + // Non-blocking so dispatch()'s read() never stalls the event loop. + UniqueFd fd(open(device_.c_str(), O_RDWR | O_NONBLOCK | O_CLOEXEC)); + if (!fd.valid()) { + LOG_ERROR("unable to open device"); + return; } -} -InputReader::~InputReader() { - stop(); - if (thread_.joinable()) { - thread_.join(); + // Raw Info + hidraw_devinfo raw_dev_info{}; + if (const auto res = ioctl(fd.get(), HIDIOCGRAWINFO, &raw_dev_info); + res < 0) { + LOG_ERROR("HIDIOCGRAWINFO"); + return; } -} + product_ = raw_dev_info.product; + LOG_INFO("bustype: {}", Hidraw::bus_str(raw_dev_info.bustype)); + LOG_INFO("Vendor ID: {:04X}", raw_dev_info.vendor); + LOG_INFO("Product ID: {:04X}", raw_dev_info.product); + + // Raw Name + std::array buf{}; + auto res = ioctl(fd.get(), HIDIOCGRAWNAME(buf.size()), buf.data()); + if (res < 0) { + LOG_ERROR("HIDIOCGRAWNAME"); + return; + } + buf.back() = '\0'; // guarantee null-termination + LOG_INFO("HID Name: {}", buf.data()); + + // Raw Physical Location + res = ioctl(fd.get(), HIDIOCGRAWPHYS(buf.size()), buf.data()); + if (res < 0) { + LOG_ERROR("HIDIOCGRAWPHYS"); + return; + } + buf.back() = '\0'; // guarantee null-termination + LOG_INFO("HID Physical Location: {}", buf.data()); + + // Report Descriptor Size + int desc_size = 0; + res = ioctl(fd.get(), HIDIOCGRDESCSIZE, &desc_size); + if (res < 0) { + LOG_ERROR("HIDIOCGRDESCSIZE"); + return; + } + LOG_INFO("Report Descriptor Size: {}", desc_size); -void InputReader::read_input() { - LOG_DEBUG("hidraw device: {}", device_); + if (desc_size < 0 || + static_cast(desc_size) > HID_MAX_DESCRIPTOR_SIZE) { + LOG_ERROR("Invalid report descriptor size: {}", desc_size); + return; + } - const UniqueFd fd(open(device_.c_str(), O_RDWR)); + // Report Descriptor + hidraw_report_descriptor rpt_desc{}; + rpt_desc.size = desc_size; + res = ioctl(fd.get(), HIDIOCGRDESC, &rpt_desc); + if (res < 0) { + LOG_ERROR("HIDIOCGRDESC"); + return; + } - while (true) { - if (!fd.valid()) { - LOG_ERROR("unable to open device"); - break; - } + std::ostringstream os; + os << "Report Descriptor\n"; + os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); + LOG_INFO(os.str()); - // Raw Info - hidraw_devinfo raw_dev_info{}; - if (const auto res = ioctl(fd.get(), HIDIOCGRAWINFO, &raw_dev_info); - res < 0) { - LOG_ERROR("HIDIOCGRAWINFO"); - break; - } - LOG_INFO("bustype: {}", Hidraw::bus_str(raw_dev_info.bustype)); - LOG_INFO("Vendor ID: {:04X}", raw_dev_info.vendor); - LOG_INFO("Product ID: {:04X}", raw_dev_info.product); - - // Raw Name - std::array buf{}; - auto res = ioctl(fd.get(), HIDIOCGRAWNAME(buf.size()), buf.data()); - if (res < 0) { - LOG_ERROR("HIDIOCGRAWNAME"); - break; - } - buf.back() = '\0'; // guarantee null-termination - LOG_INFO("HID Name: {}", buf.data()); - - // Raw Physical Location - res = ioctl(fd.get(), HIDIOCGRAWPHYS(buf.size()), buf.data()); - if (res < 0) { - LOG_ERROR("HIDIOCGRAWPHYS"); - break; - } - buf.back() = '\0'; // guarantee null-termination - LOG_INFO("HID Physical Location: {}", buf.data()); - - // Report Descriptor Size - int desc_size = 0; - res = ioctl(fd.get(), HIDIOCGRDESCSIZE, &desc_size); - if (res < 0) { - LOG_ERROR("HIDIOCGRDESCSIZE"); - break; - } - LOG_INFO("Report Descriptor Size: {}", desc_size); + // Get Features + GetControllerCalibrationData(fd.get(), + hw_cal_data_); // enables extended report for BT + GetControllerMacAll(fd.get(), controller_and_host_mac_); + GetControllerVersion(fd.get(), version_); - if (desc_size < 0 || - static_cast(desc_size) > HID_MAX_DESCRIPTOR_SIZE) { - LOG_ERROR("Invalid report descriptor size: {}", desc_size); - break; - } + // Initialisation succeeded: keep the fd so the loop can poll it. + fd_ = std::move(fd); +} - // Report Descriptor - hidraw_report_descriptor rpt_desc{}; - rpt_desc.size = desc_size; - res = ioctl(fd.get(), HIDIOCGRDESC, &rpt_desc); - if (res < 0) { - LOG_ERROR("HIDIOCGRDESC"); - break; - } +void InputReader::dispatch(const short revents) { + if ((revents & (POLLHUP | POLLERR)) != 0) { + // Device went away; the udev "remove" handler will retire this source. + return; + } - std::ostringstream os; - os << "Report Descriptor\n"; - os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); - LOG_INFO(os.str()); - - // Get Features - GetControllerCalibrationData( - fd.get(), hw_cal_data_); // enables extended report for BT - GetControllerMacAll(fd.get(), controller_and_host_mac_); - GetControllerVersion(fd.get(), version_); - - // Wait on both the hidraw fd and the stop eventfd so a blocking read can - // be interrupted immediately when stop() is called from another thread. - const UniqueFd epoll_fd(epoll_create1(EPOLL_CLOEXEC)); - if (!epoll_fd.valid()) { - LOG_ERROR("epoll_create1 failed: {}", strerror(errno)); - break; + // Buffer sized to the largest report (ReportIn31) so no report is truncated. + std::array + buffer{}; + const ssize_t result = read(fd_.get(), buffer.data(), buffer.size()); + if (result < 0) { + if (errno == EINTR || errno == EAGAIN) { + return; } - epoll_event ev{}; - ev.events = EPOLLIN; - ev.data.fd = fd.get(); - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, fd.get(), &ev) == -1) { - LOG_ERROR("epoll_ctl(hidraw) failed: {}", strerror(errno)); - break; - } - if (stop_event_fd_.valid()) { - ev.data.fd = stop_event_fd_.get(); - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, stop_event_fd_.get(), &ev) == - -1) { - LOG_ERROR("epoll_ctl(stop) failed: {}", strerror(errno)); - break; - } - } - - while (!stop_flag_) { - std::array poll_events{}; - const int nfds = epoll_wait(epoll_fd.get(), poll_events.data(), - poll_events.size(), -1); - if (nfds == -1) { - if (errno == EINTR) { - continue; - } - LOG_ERROR("epoll_wait failed: {}", strerror(errno)); - break; - } - - bool stop_requested = false; - bool data_ready = false; - for (int i = 0; i < nfds; ++i) { - if (stop_event_fd_.valid() && - poll_events.at(i).data.fd == stop_event_fd_.get()) { - stop_requested = true; - } else if (poll_events.at(i).data.fd == fd.get()) { - data_ready = true; - } - } - if (stop_requested) { - break; - } - if (!data_ready) { - continue; - } - - // Buffer sized to the largest report (ReportIn31) so no report is - // truncated on read. - std::array - buffer{}; - const ssize_t result = read(fd.get(), buffer.data(), buffer.size()); - if (result < 0) { - if (errno == EINTR || errno == EAGAIN) { - continue; - } - LOG_ERROR("read failed: {}", strerror(errno)); - break; - } - if (result == 0) { - continue; - } - const auto bytes_read = static_cast(result); - - if (raw_dev_info.product == 0x0CE6) { - if (const auto report_id = buffer.at(0); report_id == 1) { - USBGetStateData input_report01{}; - std::memcpy(&input_report01, buffer.data(), - std::min(sizeof(USBGetStateData), bytes_read)); - PrintControllerStateUsb(input_report01, hw_cal_data_); - } else if (report_id == 49) { - ReportIn31 input_report31{}; - std::memcpy(&input_report31, buffer.data(), - std::min(sizeof(ReportIn31), bytes_read)); - if (input_report31.Data.HasHID) { - LOG_INFO("[ReportIn31] Has HID"); - PrintControllerStateUsb(input_report31.Data.State.StateData, - hw_cal_data_); - } else if (input_report31.Data.HasMic) { - LOG_INFO("[ReportIn31] Has Microphone"); - } - } else { - LOG_ERROR("Unknown report id: {}", report_id); - } + LOG_ERROR("read failed: {}", strerror(errno)); + return; + } + if (result == 0) { + return; + } + const auto bytes_read = static_cast(result); + + if (product_ == 0x0CE6) { + if (const auto report_id = buffer.at(0); report_id == 1) { + USBGetStateData input_report01{}; + std::memcpy(&input_report01, buffer.data(), + std::min(sizeof(USBGetStateData), bytes_read)); + PrintControllerStateUsb(input_report01, hw_cal_data_); + } else if (report_id == 49) { + ReportIn31 input_report31{}; + std::memcpy(&input_report31, buffer.data(), + std::min(sizeof(ReportIn31), bytes_read)); + if (input_report31.Data.HasHID) { + LOG_INFO("[ReportIn31] Has HID"); + PrintControllerStateUsb(input_report31.Data.State.StateData, + hw_cal_data_); + } else if (input_report31.Data.HasMic) { + LOG_INFO("[ReportIn31] Has Microphone"); } + } else { + LOG_ERROR("Unknown report id: {}", report_id); } - break; } - - // fd is automatically closed by UniqueFd destructor. } int InputReader::GetControllerMacAll(const int fd, diff --git a/src/bluez/ps5_dual_sense/input_reader.h b/src/bluez/ps5_dual_sense/input_reader.h index 5c7c60d..ef54cbb 100644 --- a/src/bluez/ps5_dual_sense/input_reader.h +++ b/src/bluez/ps5_dual_sense/input_reader.h @@ -12,25 +12,30 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ -#define SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ +#ifndef SRC_BLUEZ_PS5_DUAL_SENSE_INPUT_READER_HPP_ +#define SRC_BLUEZ_PS5_DUAL_SENSE_INPUT_READER_HPP_ #include -#include -#include +#include +#include +#include "../../utils/event_loop.h" #include "../../utils/unique_fd.h" #include "dual_sense_0ce6.h" -class InputReader { +/// Reads and decodes hidraw input reports for a PS5 DualSense controller as an +/// EventSource. The device is opened and its feature reports fetched in the +/// constructor; dispatch() then reads and prints one input report per readable +/// event on the loop thread. Register it with the loop only when valid(). +class InputReader final : public EventSource { public: explicit InputReader(std::string device); + ~InputReader() override = default; - void start(); + [[nodiscard]] bool valid() const { return fd_.valid(); } - void stop(); - - ~InputReader(); + [[nodiscard]] int fd() const override { return fd_.get(); } + void dispatch(short revents) override; private: struct CalibrationData { @@ -46,20 +51,16 @@ class InputReader { }; std::string device_; - std::atomic stop_flag_; - // eventfd used to interrupt the blocking read loop immediately on stop(). - UniqueFd stop_event_fd_; + UniqueFd fd_; + std::uint16_t product_ = 0; ReportFeatureInMacAll controller_and_host_mac_{}; ReportFeatureInVersion version_{}; HardwareCalibrationData hw_cal_data_{}; - // Worker thread that owns the blocking read loop. Joined in the destructor - // before any other member is torn down, so the loop can never outlive this - // object (no use-after-free) and never blocks the D-Bus/main thread. - std::thread thread_; - - void read_input(); + // Opens the device, reads its descriptors and feature reports; leaves fd_ + // invalid on failure. + void open_and_init(); static std::string dpad_to_string(Direction dpad); static std::string power_state_to_string(PowerState state); @@ -85,4 +86,4 @@ class InputReader { static void PrintControllerStateBt(BTSimpleGetStateData const& state); }; -#endif // SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ +#endif // SRC_BLUEZ_PS5_DUAL_SENSE_INPUT_READER_HPP_ diff --git a/src/bluez/ps5_dual_sense/main.cc b/src/bluez/ps5_dual_sense/main.cc index 1179f71..02c7e67 100644 --- a/src/bluez/ps5_dual_sense/main.cc +++ b/src/bluez/ps5_dual_sense/main.cc @@ -34,7 +34,7 @@ int main() { const auto connection = sdbus::createSystemBusConnection(); - DualSense client(*connection); + DualSense client(*connection, loop); loop.add(&client); // DualSense is a UdevMonitor (an EventSource) LOG_INFO("PS5 DualSense client running - Press Ctrl+C to exit"); diff --git a/src/bluez/xbox_controller/input_reader.cc b/src/bluez/xbox_controller/input_reader.cc index 4b6c19c..2baacf4 100644 --- a/src/bluez/xbox_controller/input_reader.cc +++ b/src/bluez/xbox_controller/input_reader.cc @@ -14,217 +14,134 @@ #include #include -#include #include -#include #include -#include -#include +#include #include #include "../../utils/logging.h" #include "../hidraw.hpp" #include "input_reader.h" -InputReader::InputReader(std::string device) - : device_(std::move(device)), - stop_flag_(false), - stop_event_fd_(::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) { - if (!stop_event_fd_.valid()) { - LOG_ERROR("Failed to create eventfd: {}", strerror(errno)); - } +InputReader::InputReader(std::string device) : device_(std::move(device)) { + open_and_init(); } -void InputReader::start() { - LOG_DEBUG("InputReader start: {}", device_); - if (thread_.joinable()) { - return; // already running - } - stop_flag_ = false; - thread_ = std::thread([this] { read_input(); }); -} +void InputReader::open_and_init() { + LOG_DEBUG("hidraw device: {}", device_); -void InputReader::stop() { - LOG_DEBUG("InputReader stop: {}", device_); - stop_flag_ = true; - // Wake the blocking epoll_wait so the loop observes stop_flag_ immediately. - if (stop_event_fd_.valid()) { - constexpr std::uint64_t one = 1; - if (::write(stop_event_fd_.get(), &one, sizeof(one)) < 0) { - LOG_ERROR("Failed to signal stop eventfd: {}", strerror(errno)); - } + // Non-blocking so dispatch()'s read() never stalls the event loop. + UniqueFd fd(open(device_.c_str(), O_RDWR | O_NONBLOCK | O_CLOEXEC)); + if (!fd.valid()) { + LOG_ERROR("unable to open device"); + return; } -} -InputReader::~InputReader() { - stop(); - if (thread_.joinable()) { - thread_.join(); + // Raw Info + hidraw_devinfo raw_dev_info{}; + if (const auto res = ioctl(fd.get(), HIDIOCGRAWINFO, &raw_dev_info); + res < 0) { + LOG_ERROR("HIDIOCGRAWINFO"); + return; } -} - -void InputReader::read_input() { - LOG_DEBUG("hidraw device: {}", device_); - - const UniqueFd fd(open(device_.c_str(), O_RDWR)); - - while (true) { - if (!fd.valid()) { - LOG_ERROR("unable to open device"); - break; - } - - // Raw Info - hidraw_devinfo raw_dev_info{}; - if (const auto res = ioctl(fd.get(), HIDIOCGRAWINFO, &raw_dev_info); - res < 0) { - LOG_ERROR("HIDIOCGRAWINFO"); - break; - } - LOG_INFO("bustype: {}", Hidraw::bus_str(raw_dev_info.bustype)); - LOG_INFO("Vendor ID: {:04X}", raw_dev_info.vendor); - LOG_INFO("Product ID: {:04X}", raw_dev_info.product); - - // Raw Name - std::array buf{}; - auto res = ioctl(fd.get(), HIDIOCGRAWNAME(buf.size()), buf.data()); - if (res < 0) { - LOG_ERROR("HIDIOCGRAWNAME"); - break; - } - buf.back() = '\0'; // guarantee null-termination - LOG_INFO("HID Name: {}", buf.data()); + product_ = raw_dev_info.product; + LOG_INFO("bustype: {}", Hidraw::bus_str(raw_dev_info.bustype)); + LOG_INFO("Vendor ID: {:04X}", raw_dev_info.vendor); + LOG_INFO("Product ID: {:04X}", raw_dev_info.product); + + // Raw Name + std::array buf{}; + auto res = ioctl(fd.get(), HIDIOCGRAWNAME(buf.size()), buf.data()); + if (res < 0) { + LOG_ERROR("HIDIOCGRAWNAME"); + return; + } + buf.back() = '\0'; // guarantee null-termination + LOG_INFO("HID Name: {}", buf.data()); + + // Raw Physical Location + res = ioctl(fd.get(), HIDIOCGRAWPHYS(buf.size()), buf.data()); + if (res < 0) { + LOG_ERROR("HIDIOCGRAWPHYS"); + return; + } + buf.back() = '\0'; // guarantee null-termination + LOG_INFO("HID Physical Location: {}", buf.data()); + + // Report Descriptor Size + int desc_size = 0; + res = ioctl(fd.get(), HIDIOCGRDESCSIZE, &desc_size); + if (res < 0) { + LOG_ERROR("HIDIOCGRDESCSIZE"); + return; + } + LOG_INFO("Report Descriptor Size: {}", desc_size); - // Raw Physical Location - res = ioctl(fd.get(), HIDIOCGRAWPHYS(buf.size()), buf.data()); - if (res < 0) { - LOG_ERROR("HIDIOCGRAWPHYS"); - break; - } - buf.back() = '\0'; // guarantee null-termination - LOG_INFO("HID Physical Location: {}", buf.data()); + if (desc_size < 0 || + static_cast(desc_size) > HID_MAX_DESCRIPTOR_SIZE) { + LOG_ERROR("Invalid report descriptor size: {}", desc_size); + return; + } - // Report Descriptor Size - int desc_size = 0; - res = ioctl(fd.get(), HIDIOCGRDESCSIZE, &desc_size); - if (res < 0) { - LOG_ERROR("HIDIOCGRDESCSIZE"); - break; - } - LOG_INFO("Report Descriptor Size: {}", desc_size); + // Report Descriptor + hidraw_report_descriptor rpt_desc{}; + rpt_desc.size = desc_size; + res = ioctl(fd.get(), HIDIOCGRDESC, &rpt_desc); + if (res < 0) { + LOG_ERROR("HIDIOCGRDESC"); + return; + } - if (desc_size < 0 || - static_cast(desc_size) > HID_MAX_DESCRIPTOR_SIZE) { - LOG_ERROR("Invalid report descriptor size: {}", desc_size); - break; - } + std::ostringstream os; + os << "Report Descriptor\n"; + os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); + LOG_INFO(os.str()); - // Report Descriptor - hidraw_report_descriptor rpt_desc{}; - rpt_desc.size = desc_size; - res = ioctl(fd.get(), HIDIOCGRDESC, &rpt_desc); - if (res < 0) { - LOG_ERROR("HIDIOCGRDESC"); - break; - } + // Initialisation succeeded: keep the fd so the loop can poll it. + fd_ = std::move(fd); +} - std::ostringstream os; - os << "Report Descriptor\n"; - os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); - LOG_INFO(os.str()); +void InputReader::dispatch(const short revents) { + if ((revents & (POLLHUP | POLLERR)) != 0) { + // Device went away; the udev "remove" handler will retire this source. + return; + } - // Wait on both the hidraw fd and the stop eventfd so a blocking read can - // be interrupted immediately when stop() is called from another thread. - const UniqueFd epoll_fd(epoll_create1(EPOLL_CLOEXEC)); - if (!epoll_fd.valid()) { - LOG_ERROR("epoll_create1 failed: {}", strerror(errno)); - break; + std::array buffer{}; + const ssize_t result = read(fd_.get(), buffer.data(), buffer.size()); + if (result < 0) { + if (errno == EINTR || errno == EAGAIN) { + return; } - epoll_event ev{}; - ev.events = EPOLLIN; - ev.data.fd = fd.get(); - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, fd.get(), &ev) == -1) { - LOG_ERROR("epoll_ctl(hidraw) failed: {}", strerror(errno)); - break; - } - if (stop_event_fd_.valid()) { - ev.data.fd = stop_event_fd_.get(); - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, stop_event_fd_.get(), &ev) == - -1) { - LOG_ERROR("epoll_ctl(stop) failed: {}", strerror(errno)); - break; - } - } - - while (!stop_flag_) { - std::array events{}; - const int nfds = - epoll_wait(epoll_fd.get(), events.data(), events.size(), -1); - if (nfds == -1) { - if (errno == EINTR) { - continue; - } - LOG_ERROR("epoll_wait failed: {}", strerror(errno)); - break; - } - - bool stop_requested = false; - bool data_ready = false; - for (int i = 0; i < nfds; ++i) { - if (stop_event_fd_.valid() && - events.at(i).data.fd == stop_event_fd_.get()) { - stop_requested = true; - } else if (events.at(i).data.fd == fd.get()) { - data_ready = true; - } - } - if (stop_requested) { - break; - } - if (!data_ready) { - continue; - } - - std::array buffer{}; - const ssize_t result = read(fd.get(), buffer.data(), buffer.size()); - if (result < 0) { - if (errno == EINTR || errno == EAGAIN) { - continue; - } - LOG_ERROR("read failed: {}", strerror(errno)); - break; - } - if (result == 0) { - continue; - } - const auto bytes_read = static_cast(result); - - if (raw_dev_info.product == 0x02FD) { - if (const auto report_id = buffer.at(0); report_id == 1) { - inputReport01_t input_report01{}; - std::memcpy(&input_report01, buffer.data(), - std::min(sizeof(inputReport01_t), bytes_read)); - PrintInputReport1(input_report01); - } else if (report_id == 2) { - inputReport02_t input_report02{}; - std::memcpy(&input_report02, buffer.data(), - std::min(sizeof(inputReport02_t), bytes_read)); - PrintInputReport2(input_report02); - } else if (report_id == 4) { - inputReport04_t input_report04{}; - std::memcpy(&input_report04, buffer.data(), - std::min(sizeof(inputReport04_t), bytes_read)); - PrintInputReport4(input_report04); - } else { - LOG_ERROR("Unknown report id: {}", report_id); - } - } + LOG_ERROR("read failed: {}", strerror(errno)); + return; + } + if (result == 0) { + return; + } + const auto bytes_read = static_cast(result); + + if (product_ == 0x02FD) { + if (const auto report_id = buffer.at(0); report_id == 1) { + inputReport01_t input_report01{}; + std::memcpy(&input_report01, buffer.data(), + std::min(sizeof(inputReport01_t), bytes_read)); + PrintInputReport1(input_report01); + } else if (report_id == 2) { + inputReport02_t input_report02{}; + std::memcpy(&input_report02, buffer.data(), + std::min(sizeof(inputReport02_t), bytes_read)); + PrintInputReport2(input_report02); + } else if (report_id == 4) { + inputReport04_t input_report04{}; + std::memcpy(&input_report04, buffer.data(), + std::min(sizeof(inputReport04_t), bytes_read)); + PrintInputReport4(input_report04); + } else { + LOG_ERROR("Unknown report id: {}", report_id); } - break; } - - // fd is automatically closed by UniqueFd destructor. } std::string InputReader::dpad_to_string(const Direction dpad) { diff --git a/src/bluez/xbox_controller/input_reader.h b/src/bluez/xbox_controller/input_reader.h index cab6a3c..c13468b 100644 --- a/src/bluez/xbox_controller/input_reader.h +++ b/src/bluez/xbox_controller/input_reader.h @@ -15,13 +15,18 @@ #ifndef SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ #define SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ -#include -#include +#include +#include +#include "../../utils/event_loop.h" #include "../../utils/unique_fd.h" #include "xbox_controller_02fd.h" -class InputReader { +/// Reads and decodes hidraw input reports for an Xbox controller as an +/// EventSource: the hidraw fd is polled by the EventLoop and dispatch() reads +/// and prints one report per readable event. valid() reports whether the +/// device opened and initialised; register it with the loop only when true. +class InputReader final : public EventSource { public: enum class Direction : uint8_t { None = 0, @@ -36,24 +41,20 @@ class InputReader { }; explicit InputReader(std::string device); + ~InputReader() override = default; - void start(); + [[nodiscard]] bool valid() const { return fd_.valid(); } - void stop(); - - ~InputReader(); + [[nodiscard]] int fd() const override { return fd_.get(); } + void dispatch(short revents) override; private: std::string device_; - std::atomic stop_flag_; - // eventfd used to interrupt the blocking read loop immediately on stop(). - UniqueFd stop_event_fd_; - // Worker thread that owns the blocking read loop. Joined in the destructor - // before any other member is torn down, so the loop can never outlive this - // object (no use-after-free) and never blocks the D-Bus/main thread. - std::thread thread_; + UniqueFd fd_; + std::uint16_t product_ = 0; - void read_input(); + // Opens the device and reads its descriptors; leaves fd_ invalid on failure. + void open_and_init(); static std::string dpad_to_string(Direction dpad); diff --git a/src/bluez/xbox_controller/main.cc b/src/bluez/xbox_controller/main.cc index eabd84d..e59c008 100644 --- a/src/bluez/xbox_controller/main.cc +++ b/src/bluez/xbox_controller/main.cc @@ -26,7 +26,7 @@ int main() { SignalSource signals(loop); loop.add(&signals); - XboxController client(*connection); + XboxController client(*connection, loop); loop.add(&client); // XboxController is a UdevMonitor (an EventSource) LOG_INFO("Xbox controller client running - Press Ctrl+C to exit"); diff --git a/src/bluez/xbox_controller/xbox_controller.cc b/src/bluez/xbox_controller/xbox_controller.cc index ee23853..4083471 100644 --- a/src/bluez/xbox_controller/xbox_controller.cc +++ b/src/bluez/xbox_controller/xbox_controller.cc @@ -33,7 +33,7 @@ const std::vector> input_match_params_usb = {"ID_USB_MODEL_ID", "02ea"}, {"TAGS", ":seat:"}}; -XboxController::XboxController(sdbus::IConnection& connection) +XboxController::XboxController(sdbus::IConnection& connection, EventLoop& loop) : ProxyInterfaces(connection, sdbus::ServiceName(INTERFACE_NAME), sdbus::ObjectPath("/")), @@ -47,15 +47,17 @@ XboxController::XboxController(sdbus::IConnection& connection) if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { if (input_reader_) { - input_reader_->stop(); - input_reader_.reset(); + // Retire (not reset) so the reader outlives this + // dispatch pass; the loop destroys it safely. + loop_.retire(std::move(input_reader_)); } } if (!get_hidraw_devices(input_match_params_bt)) { get_hidraw_devices(input_match_params_usb); } } - }) { + }), + loop_(loop) { if (!get_hidraw_devices(input_match_params_bt)) { get_hidraw_devices(input_match_params_usb); } @@ -155,8 +157,14 @@ void XboxController::onInterfacesAdded( !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); if (!input_reader_) { - input_reader_ = std::make_unique(hidraw_device); - input_reader_->start(); + auto reader = std::make_unique(hidraw_device); + if (reader->valid()) { + loop_.add(reader.get()); + input_reader_ = std::move(reader); + } else { + LOG_ERROR("Failed to initialize hidraw reader: {}", + hidraw_device); + } } } } diff --git a/src/bluez/xbox_controller/xbox_controller.h b/src/bluez/xbox_controller/xbox_controller.h index 78e6d02..04534a5 100644 --- a/src/bluez/xbox_controller/xbox_controller.h +++ b/src/bluez/xbox_controller/xbox_controller.h @@ -32,7 +32,7 @@ class XboxController final public Hidraw, public UdevMonitor { public: - explicit XboxController(sdbus::IConnection& connection); + XboxController(sdbus::IConnection& connection, EventLoop& loop); ~XboxController() override; @@ -56,6 +56,10 @@ class XboxController final std::map> upower_clients_; std::unique_ptr input_reader_; + // The loop that polls the InputReader source; used to add it on device + // arrival and retire it on removal. + EventLoop& loop_; + void onInterfacesAdded( const sdbus::ObjectPath& objectPath, const std::mapGetAll(UPower_proxy::INTERFACE_NAME); - UPowerClient::onPropertiesChanged( - sdbus::InterfaceName(UPower_proxy::INTERFACE_NAME), properties, {}); - for (const auto devices = EnumerateDevices(); - const auto& device : devices) { - UPowerClient::onDeviceAdded(device); - } + // Fetch properties and enumerate devices asynchronously so construction + // never blocks the caller's event loop on a D-Bus round-trip. The replies + // are delivered on whichever loop drives this connection; pending calls are + // cancelled by unregisterProxy() if this object is destroyed first. + GetAllAsync( + sdbus::InterfaceName(UPower_proxy::INTERFACE_NAME), + // sdbus requires the error argument by value (function_traits). + // NOLINTNEXTLINE(performance-unnecessary-value-param) + [this]( + std::optional error, + const std::map& properties) { + if (error) { + LOG_ERROR("UPower GetAll failed: {} - {}", error->getName(), + error->getMessage()); + return; + } + onPropertiesChanged( + sdbus::InterfaceName(UPower_proxy::INTERFACE_NAME), properties, + {}); + }); + getProxy() + .callMethodAsync("EnumerateDevices") + .onInterface(UPower_proxy::INTERFACE_NAME) + // sdbus requires the error argument by value (function_traits). + // NOLINTNEXTLINE(performance-unnecessary-value-param) + .uponReplyInvoke([this](std::optional error, + const std::vector& devices) { + if (error) { + LOG_ERROR("UPower EnumerateDevices failed: {} - {}", + error->getName(), error->getMessage()); + return; + } + for (const auto& device : devices) { + onDeviceAdded(device); + } + }); } virtual ~UPowerClient() { diff --git a/src/upower/upower_display_device.cc b/src/upower/upower_display_device.cc index 02bf902..9af4f3f 100644 --- a/src/upower/upower_display_device.cc +++ b/src/upower/upower_display_device.cc @@ -24,13 +24,24 @@ UPowerDisplayDevice::UPowerDisplayDevice(sdbus::IConnection& connection, objectPath), object_path_(objectPath) { registerProxy(); - try { - const auto properties = this->GetAll("org.freedesktop.UPower.Device"); - UPowerDisplayDevice::onPropertiesChanged( - sdbus::InterfaceName("org.freedesktop.UPower.Device"), properties, {}); - } catch (const sdbus::Error& e) { - LOG_ERROR("UPowerDisplayDevice::UPowerDisplayDevice: {}", e.what()); - } + // Fetch properties asynchronously so construction never blocks the loop on a + // D-Bus round-trip. A pending call is cancelled by unregisterProxy() if this + // object is destroyed before the reply arrives. + GetAllAsync( + sdbus::InterfaceName("org.freedesktop.UPower.Device"), + // sdbus requires the error argument by value (function_traits). + // NOLINTNEXTLINE(performance-unnecessary-value-param) + [this](std::optional error, + const std::map& properties) { + if (error) { + LOG_ERROR("UPowerDisplayDevice GetAll failed: {} - {}", + error->getName(), error->getMessage()); + return; + } + onPropertiesChanged( + sdbus::InterfaceName("org.freedesktop.UPower.Device"), properties, + {}); + }); } UPowerDisplayDevice::~UPowerDisplayDevice() { diff --git a/src/utils/event_loop.cc b/src/utils/event_loop.cc index 3cea78d..060ad8e 100644 --- a/src/utils/event_loop.cc +++ b/src/utils/event_loop.cc @@ -56,6 +56,15 @@ void EventLoop::remove(EventSource* source) { wake(); } +void EventLoop::retire(std::unique_ptr source) { + to_remove_.push_back(source.get()); + // Hold ownership until the next apply_pending() destroys it, so the object + // outlives the current dispatch pass (its fd may still be in this + // iteration's poll set). + to_retire_.push_back(std::move(source)); + wake(); +} + void EventLoop::stop(const int exit_code) noexcept { exit_code_.store(exit_code, std::memory_order_relaxed); running_.store(false, std::memory_order_relaxed); @@ -67,6 +76,9 @@ void EventLoop::apply_pending() { std::erase(sources_, source); } to_remove_.clear(); + // Now that any retired sources are out of sources_ and the previous poll set + // is gone, it is safe to destroy them. + to_retire_.clear(); for (auto* source : to_add_) { if (std::ranges::find(sources_, source) == sources_.end()) { diff --git a/src/utils/event_loop.h b/src/utils/event_loop.h index c11fe73..62aed38 100644 --- a/src/utils/event_loop.h +++ b/src/utils/event_loop.h @@ -16,6 +16,7 @@ #define SRC_UTILS_EVENT_LOOP_H #include +#include #include #include @@ -73,6 +74,13 @@ class EventLoop { /// dispatch() — reset it after the next iteration (or after run() returns). void remove(EventSource* source); + /// Unregister a source and take ownership of destroying it at a safe point + /// (after the current dispatch pass completes). Use this to drop a source + /// from within a callback — e.g. a udev "remove" handler retiring the reader + /// for the device that just went away — without risking a use-after-free on + /// a source whose fd is still in the poll set for the current iteration. + void retire(std::unique_ptr source); + /// Drive `bus` and every registered source until stop() is called or an /// unrecoverable error occurs. Returns the code passed to stop() (0 on a /// clean external stop, non-zero on internal error). @@ -94,6 +102,9 @@ class EventLoop { std::vector sources_; std::vector to_add_; std::vector to_remove_; + // Sources awaiting destruction; kept alive until the next apply_pending() so + // a source retired mid-dispatch outlives the current poll iteration. + std::vector> to_retire_; }; #endif // SRC_UTILS_EVENT_LOOP_H From 0c9a19e02e8f874a574f725191d1b06f573c048c Mon Sep 17 00:00:00 2001 From: Joel Winarske Date: Sun, 12 Jul 2026 15:49:35 -0700 Subject: [PATCH 2/2] Fix event-loop review findings (POLLHUP spin, add+retire UAF) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the event-loop work surfaced two high-severity bugs and several smaller issues: - POLLHUP busy-spin: poll() reports POLLHUP/POLLERR level-triggered and unmaskable, so a hung-up device fd stayed "ready" every iteration and InputReader::dispatch() returned early without reading or removing it — spinning the loop at ~100% CPU until the udev "remove" event arrived (and unbounded if it never did). EventLoop::run() now stops polling a source as soon as its dispatch reports POLLHUP/POLLERR. Covered by a new regression test (a closed pipe reproduces the hangup deterministically). - Use-after-free in apply_pending(): a source added and retired within the same iteration had its object destroyed via to_retire_ and then its dangling pointer re-inserted via to_add_. Adds now skip any source also scheduled for removal that round, and retired objects are destroyed last. - stop() now uses release/acquire ordering for running_/exit_code_ instead of relaxed. - UPowerClient / UPowerDisplayDevice: restore the try/catch around property parsing that the sync->async conversion dropped, so a Variant type mismatch cannot escape the async reply slot. - xbox main: construct SignalSource before createSystemBusConnection, matching ps5/horipad and the "block signals before any thread starts" invariant. - horipad InputReader: clamp the report memcpy to bytes_read (like ps5/xbox) rather than buffer.size(). - SignalSource: unblock the signal mask if signalfd() fails, so the process is not left unkillable via SIGINT/SIGTERM. Signed-off-by: Joel Winarske --- src/bluez/horipad_steam/input_reader.cc | 9 +-- src/bluez/xbox_controller/main.cc | 8 ++- src/upower/upower_client.h | 13 +++- src/upower/upower_display_device.cc | 13 +++- src/utils/event_loop.cc | 38 +++++++---- src/utils/event_loop_test.cc | 87 ++++++++++++++++++++++--- src/utils/signal_source.h | 8 ++- 7 files changed, 143 insertions(+), 33 deletions(-) diff --git a/src/bluez/horipad_steam/input_reader.cc b/src/bluez/horipad_steam/input_reader.cc index a58fd33..55840ea 100644 --- a/src/bluez/horipad_steam/input_reader.cc +++ b/src/bluez/horipad_steam/input_reader.cc @@ -120,27 +120,28 @@ void InputReader::dispatch(const short revents) { if (result == 0) { return; } + const auto bytes_read = static_cast(result); if (product_ == 0x01ab || product_ == 0x0196) { if (const auto report_id = buffer.at(0); report_id == 7) { inputReport07_t input_report07{}; std::memcpy(&input_report07, buffer.data(), - std::min(sizeof(inputReport07_t), buffer.size())); + std::min(sizeof(inputReport07_t), bytes_read)); PrintInputReport7(input_report07); } else if (report_id == 10) { inputReport10_t input_report10{}; std::memcpy(&input_report10, buffer.data(), - std::min(sizeof(inputReport10_t), buffer.size())); + std::min(sizeof(inputReport10_t), bytes_read)); PrintInputReport10(input_report10); } else if (report_id == 12) { inputReport12_t input_report12{}; std::memcpy(&input_report12, buffer.data(), - std::min(sizeof(inputReport12_t), buffer.size())); + std::min(sizeof(inputReport12_t), bytes_read)); PrintInputReport12(input_report12); } else if (report_id == 14) { inputReport14_t input_report14{}; std::memcpy(&input_report14, buffer.data(), - std::min(sizeof(inputReport14_t), buffer.size())); + std::min(sizeof(inputReport14_t), bytes_read)); PrintInputReport14(input_report14); } else { LOG_ERROR("Unknown report id: {}", report_id); diff --git a/src/bluez/xbox_controller/main.cc b/src/bluez/xbox_controller/main.cc index e59c008..48eb573 100644 --- a/src/bluez/xbox_controller/main.cc +++ b/src/bluez/xbox_controller/main.cc @@ -18,14 +18,16 @@ int main() { try { - const auto connection = sdbus::createSystemBusConnection(); - // Single-threaded loop: it drives the D-Bus connection, the udev monitor, - // and signal delivery, so every callback runs on this thread. + // and signal delivery, so every callback runs on this thread. Construct the + // SignalSource first so SIGINT/SIGTERM are blocked before any thread is + // started and can only be delivered via the loop's signalfd. EventLoop loop; SignalSource signals(loop); loop.add(&signals); + const auto connection = sdbus::createSystemBusConnection(); + XboxController client(*connection, loop); loop.add(&client); // XboxController is a UdevMonitor (an EventSource) diff --git a/src/upower/upower_client.h b/src/upower/upower_client.h index f945b45..ff3446a 100644 --- a/src/upower/upower_client.h +++ b/src/upower/upower_client.h @@ -51,9 +51,16 @@ class UPowerClient final error->getMessage()); return; } - onPropertiesChanged( - sdbus::InterfaceName(UPower_proxy::INTERFACE_NAME), properties, - {}); + // A property whose runtime type differs from the expected one makes + // Variant::get() throw; contain it so it can't escape the reply + // slot (the pre-async code wrapped GetAll in the same guard). + try { + onPropertiesChanged( + sdbus::InterfaceName(UPower_proxy::INTERFACE_NAME), properties, + {}); + } catch (const sdbus::Error& e) { + LOG_ERROR("UPower property parse failed: {}", e.what()); + } }); getProxy() .callMethodAsync("EnumerateDevices") diff --git a/src/upower/upower_display_device.cc b/src/upower/upower_display_device.cc index 9af4f3f..d31e2f3 100644 --- a/src/upower/upower_display_device.cc +++ b/src/upower/upower_display_device.cc @@ -38,9 +38,16 @@ UPowerDisplayDevice::UPowerDisplayDevice(sdbus::IConnection& connection, error->getName(), error->getMessage()); return; } - onPropertiesChanged( - sdbus::InterfaceName("org.freedesktop.UPower.Device"), properties, - {}); + // A property whose runtime type differs from the expected one makes + // Variant::get() throw; contain it so it can't escape the reply slot + // (the pre-async code wrapped GetAll in the same guard). + try { + onPropertiesChanged( + sdbus::InterfaceName("org.freedesktop.UPower.Device"), properties, + {}); + } catch (const sdbus::Error& e) { + LOG_ERROR("UPowerDisplayDevice property parse failed: {}", e.what()); + } }); } diff --git a/src/utils/event_loop.cc b/src/utils/event_loop.cc index 060ad8e..7fef2db 100644 --- a/src/utils/event_loop.cc +++ b/src/utils/event_loop.cc @@ -66,8 +66,10 @@ void EventLoop::retire(std::unique_ptr source) { } void EventLoop::stop(const int exit_code) noexcept { + // release/acquire so run() observes exit_code_ once it sees running_ == false + // (independently of the eventfd/poll barriers). exit_code_.store(exit_code, std::memory_order_relaxed); - running_.store(false, std::memory_order_relaxed); + running_.store(false, std::memory_order_release); wake(); // write() is async-signal-safe } @@ -75,25 +77,31 @@ void EventLoop::apply_pending() { for (auto* source : to_remove_) { std::erase(sources_, source); } - to_remove_.clear(); - // Now that any retired sources are out of sources_ and the previous poll set - // is gone, it is safe to destroy them. - to_retire_.clear(); for (auto* source : to_add_) { + // Skip a source that is also being removed/retired this round: added and + // retired within the same iteration, its object is about to be destroyed + // by the to_retire_ clear below, so re-inserting its pointer would dangle. + if (std::ranges::find(to_remove_, source) != to_remove_.end()) { + continue; + } if (std::ranges::find(sources_, source) == sources_.end()) { sources_.push_back(source); } } to_add_.clear(); + to_remove_.clear(); + // Now that any retired sources are out of sources_ and cannot be re-added, it + // is safe to destroy them. + to_retire_.clear(); } int EventLoop::run(sdbus::IConnection& bus) { running_.store(true, std::memory_order_relaxed); - while (running_.load(std::memory_order_relaxed)) { + while (running_.load(std::memory_order_acquire)) { apply_pending(); - if (!running_.load(std::memory_order_relaxed)) { + if (!running_.load(std::memory_order_acquire)) { break; } @@ -158,12 +166,20 @@ int EventLoop::run(sdbus::IConnection& bus) { // of the loop), so indexing the snapshot built above stays valid even if a // dispatch() calls add()/remove(). for (std::size_t i = 0; i < sources_.size(); ++i) { - if (const short revents = pfds[first_source_idx + i].revents; - revents != 0) { - sources_[i]->dispatch(revents); + const short revents = pfds[first_source_idx + i].revents; + if (revents == 0) { + continue; + } + sources_[i]->dispatch(revents); + // poll() reports POLLHUP/POLLERR level-triggered and unmaskable, so a + // hung-up or errored fd stays "ready" every iteration. Stop polling it + // (deferred to the next apply_pending()) so it can't spin the loop while + // its owner tears the source down. + if ((revents & (POLLHUP | POLLERR)) != 0) { + remove(sources_[i]); } } } - return exit_code_.load(std::memory_order_relaxed); + return exit_code_.load(std::memory_order_acquire); } diff --git a/src/utils/event_loop_test.cc b/src/utils/event_loop_test.cc index 7d4cd2b..b5007ca 100644 --- a/src/utils/event_loop_test.cc +++ b/src/utils/event_loop_test.cc @@ -20,6 +20,7 @@ // 3. stop() unblocks run() and returns the requested code. // A timerfd acts as a hard deadline so the test can never hang. +#include #include #include #include @@ -27,6 +28,7 @@ #include #include +#include #include #include #include @@ -103,6 +105,69 @@ class DeadlineSource final : public EventSource { bool fired_ = false; }; +// Counts how many times it is dispatched; used to prove a POLLHUP fd is dropped +// rather than spun on. +class HupSource final : public EventSource { + public: + explicit HupSource(const int fd) : fd_(fd) {} + [[nodiscard]] int fd() const override { return fd_; } + void dispatch(short /*revents*/) override { ++count_; } + [[nodiscard]] int count() const { return count_; } + + private: + int fd_; + int count_ = 0; +}; + +// Stops the loop cleanly when its timer fires. +class StopTimer final : public EventSource { + public: + StopTimer(const int fd, EventLoop& loop) : fd_(fd), loop_(loop) {} + [[nodiscard]] int fd() const override { return fd_; } + void dispatch(short /*revents*/) override { loop_.stop(0); } + + private: + int fd_; + EventLoop& loop_; +}; + +// Regression test for the POLLHUP busy-spin: a closed pipe's read end reports +// POLLHUP level-triggered on every poll(). The loop must dispatch it once and +// then stop polling it, so the dispatch count stays tiny instead of spinning to +// thousands before the 300ms deadline. +bool test_pollhup_removal(sdbus::IConnection& connection) { + std::array pipefd{-1, -1}; + if (::pipe2(pipefd.data(), O_CLOEXEC | O_NONBLOCK) != 0) { + LOG_ERROR("EventLoop HUP test: pipe2 failed: {}", strerror(errno)); + return false; + } + UniqueFd read_end(pipefd[0]); + ::close(pipefd[1]); // closing the write end makes read_end report POLLHUP + + EventLoop loop; + HupSource hup(read_end.get()); + loop.add(&hup); + + UniqueFd timer(::timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC)); + itimerspec spec{}; + spec.it_value.tv_nsec = 300'000'000; // 300ms + if (!timer.valid() || ::timerfd_settime(timer.get(), 0, &spec, nullptr) < 0) { + LOG_ERROR("EventLoop HUP test: timer setup failed: {}", strerror(errno)); + return false; + } + StopTimer stopper(timer.get(), loop); + loop.add(&stopper); + + loop.run(connection); + + // With the fix, the HUP source is dispatched once and then removed; without + // it the loop spins and dispatches thousands of times before the deadline. + const bool ok = hup.count() <= 3; + LOG_INFO("EventLoop HUP test: dispatched {} time(s) ({})", hup.count(), + ok ? "PASS" : "FAIL"); + return ok; +} + } // namespace int main() { @@ -172,15 +237,21 @@ int main() { const int rc = loop.run(*connection); - const bool passed = rc == 0 && !deadline.fired() && - tokens.count() == kTokens && done.reply_ok; - if (passed) { - LOG_INFO("EventLoop test: PASS (tokens={}, reply_ok={})", tokens.count(), - done.reply_ok); + const bool sources_ok = rc == 0 && !deadline.fired() && + tokens.count() == kTokens && done.reply_ok; + if (!sources_ok) { + LOG_ERROR( + "EventLoop test: FAIL (rc={}, deadline_fired={}, tokens={}, " + "reply_ok={})", + rc, deadline.fired(), tokens.count(), done.reply_ok); + } + + const bool hup_ok = test_pollhup_removal(*connection); + + if (sources_ok && hup_ok) { + LOG_INFO("EventLoop test: PASS (tokens={}, reply_ok={}, hup ok)", + tokens.count(), done.reply_ok); return 0; } - LOG_ERROR( - "EventLoop test: FAIL (rc={}, deadline_fired={}, tokens={}, reply_ok={})", - rc, deadline.fired(), tokens.count(), done.reply_ok); return 1; } diff --git a/src/utils/signal_source.h b/src/utils/signal_source.h index 29a686d..fb75258 100644 --- a/src/utils/signal_source.h +++ b/src/utils/signal_source.h @@ -47,7 +47,13 @@ class SignalSource final : public EventSource { } fd_ = UniqueFd(::signalfd(-1, &mask_, SFD_CLOEXEC | SFD_NONBLOCK)); if (!fd_.valid()) { - LOG_ERROR("SignalSource: signalfd failed: {}", strerror(errno)); + LOG_ERROR( + "SignalSource: signalfd failed: {}; restoring default signal " + "disposition", + strerror(errno)); + // Undo the block, otherwise the signals stay blocked with no consumer and + // the process can no longer be stopped via SIGINT/SIGTERM. + sigprocmask(SIG_UNBLOCK, &mask_, nullptr); } }