diff --git a/src/app/AppRoot.cpp b/src/app/AppRoot.cpp index d2b286e..e0e7541 100644 --- a/src/app/AppRoot.cpp +++ b/src/app/AppRoot.cpp @@ -7,7 +7,9 @@ #include "desktop/GenericDesktop.h" #include "input/UinputInjector.h" #include "logging/LogManager.h" +#include #include +#include namespace logitune { @@ -165,6 +167,19 @@ void AppRoot::wireSignals() this, &AppRoot::onPhysicalDeviceAdded); connect(&m_deviceManager, &DeviceManager::physicalDeviceRemoved, this, &AppRoot::onPhysicalDeviceRemoved); + connect(&m_deviceManager, &DeviceManager::physicalDeviceTransportReady, + this, &AppRoot::onPhysicalDeviceTransportReady); + + // systemd-logind PrepareForSleep(before): before=true before suspend, + // before=false on resume. Re-apply profiles on resume to restore device + // state that the firmware reset while powered down. + QDBusConnection::systemBus().connect( + QStringLiteral("org.freedesktop.login1"), + QStringLiteral("/org/freedesktop/login1"), + QStringLiteral("org.freedesktop.login1.Manager"), + QStringLiteral("PrepareForSleep"), + this, + SLOT(onPrepareForSleep(bool))); // Gesture keystroke edits in the UI. saveCurrentProfile re-serializes // the displayed profile; the orchestrator's own userChangedSomething @@ -304,6 +319,32 @@ void AppRoot::onPhysicalDeviceRemoved(PhysicalDevice *device) m_deviceModel.setSelectedIndex(0); } +// A new transport attached to an existing PhysicalDevice (e.g. Bolt receiver +// reconnects while a BT session is still alive, or rapid udev remove+add +// where the PhysicalDevice object was not destroyed). physicalDeviceAdded is +// NOT re-emitted in this case, so profile re-application needs this path. +void AppRoot::onPhysicalDeviceTransportReady(PhysicalDevice *device) +{ + m_profileOrchestrator.onTransportSetupComplete(device); +} + +// systemd-logind signals PrepareForSleep(true) before suspend and +// PrepareForSleep(false) on resume. On resume we wait 2 s for HID++ to +// stabilise, then re-apply the stored profile to every known device. +// This covers the common case where the Bolt receiver does not send a +// 0x41 reconnect notification after the laptop wakes from sleep. +void AppRoot::onPrepareForSleep(bool beforeSleep) +{ + if (beforeSleep) + return; + + QTimer::singleShot(2000, this, [this]() { + qCInfo(lcApp) << "system resumed from sleep — re-applying device profiles"; + for (PhysicalDevice *pd : m_deviceManager.physicalDevices()) + m_profileOrchestrator.onTransportSetupComplete(pd); + }); +} + // Carousel selection changed. Refresh the UI from the newly-selected // device's cached profile. No file I/O, no seeding, no hardware apply — // one-time device provisioning happens in onPhysicalDeviceAdded. diff --git a/src/app/AppRoot.h b/src/app/AppRoot.h index 8f82000..78dff58 100644 --- a/src/app/AppRoot.h +++ b/src/app/AppRoot.h @@ -72,6 +72,8 @@ class AppRoot : public QObject { private slots: void onPhysicalDeviceAdded(PhysicalDevice *device); void onPhysicalDeviceRemoved(PhysicalDevice *device); + void onPhysicalDeviceTransportReady(PhysicalDevice *device); + void onPrepareForSleep(bool beforeSleep); private: void wireSignals(); diff --git a/src/core/DeviceManager.cpp b/src/core/DeviceManager.cpp index 30fc1fe..c2ba94a 100644 --- a/src/core/DeviceManager.cpp +++ b/src/core/DeviceManager.cpp @@ -520,6 +520,12 @@ void DeviceManager::probeDevice(const QString &devNode) emit physicalDeviceAdded(pdPtr); } else { pdIt->second->attachTransport(sessionPtr); + // Signal that a transport just (re-)attached to an existing device so + // the profile orchestrator can re-apply settings. The standard + // transportSetupComplete path only fires when the signal was already + // wired — but enumerateAndSetup() emits setupComplete() before + // attachTransport() connects the signal, so we notify explicitly here. + emit physicalDeviceTransportReady(pdIt->second.get()); } } diff --git a/src/core/DeviceManager.h b/src/core/DeviceManager.h index 678facf..643fd9b 100644 --- a/src/core/DeviceManager.h +++ b/src/core/DeviceManager.h @@ -57,6 +57,11 @@ class DeviceManager : public QObject { void physicalDeviceAdded(PhysicalDevice *device); void physicalDeviceRemoved(PhysicalDevice *device); + // Emitted when a new transport attaches to an EXISTING PhysicalDevice. + // physicalDeviceAdded is NOT re-emitted in this case, so profile + // re-application must also connect to this signal. + void physicalDeviceTransportReady(PhysicalDevice *device); + void unknownDeviceDetected(uint16_t pid); private slots: diff --git a/src/core/DeviceSession.cpp b/src/core/DeviceSession.cpp index bca9bab..17a676f 100644 --- a/src/core/DeviceSession.cpp +++ b/src/core/DeviceSession.cpp @@ -138,6 +138,28 @@ QString DeviceSession::deviceId() const .arg(m_devicePid, 4, 16, QLatin1Char('0')); } +// --------------------------------------------------------------------------- +// rebuildCommandProcessor() +// --------------------------------------------------------------------------- +// Point the command processor at the FeatureDispatcher and Transport that are +// current *now*. enumerateAndSetup() builds a fresh FeatureDispatcher on every +// run, so a processor carried over from a previous run would keep a raw +// pointer to the destroyed dispatcher and use it on the next queued command. +// Tearing the old one down first also drops any commands queued against it. +void DeviceSession::rebuildCommandProcessor() +{ + if (m_commandProcessor) { + m_commandProcessor->clear(); + m_commandProcessor->stop(); + m_commandProcessor.reset(); + } + if (!m_features || !m_transport) + return; + m_commandProcessor = std::make_unique( + m_features.get(), m_transport.get(), m_deviceIndex); + m_commandProcessor->start(); +} + // --------------------------------------------------------------------------- // enumerateAndSetup() // --------------------------------------------------------------------------- @@ -451,11 +473,7 @@ void DeviceSession::enumerateAndSetup() m_lastResponseTime = QDateTime::currentMSecsSinceEpoch(); m_enumerating = false; - if (!m_commandProcessor && m_features && m_transport) { - m_commandProcessor = std::make_unique( - m_features.get(), m_transport.get(), m_deviceIndex); - m_commandProcessor->start(); - } + rebuildCommandProcessor(); // Start periodic battery polling (60s) if (!m_batteryPollTimer) { diff --git a/src/core/DeviceSession.h b/src/core/DeviceSession.h index 1f5de69..11554fd 100644 --- a/src/core/DeviceSession.h +++ b/src/core/DeviceSession.h @@ -14,7 +14,7 @@ #include #include -namespace logitune::test { class AppRootFixture; } +namespace logitune::test { class AppRootFixture; class DeviceSessionFixture; } namespace logitune { @@ -24,6 +24,7 @@ class IDevice; class DeviceSession : public QObject { Q_OBJECT friend class test::AppRootFixture; + friend class test::DeviceSessionFixture; public: DeviceSession(std::unique_ptr device, @@ -128,6 +129,7 @@ class DeviceSession : public QObject { private: void checkSleepWake(); bool isDirectDevice(uint16_t pid) const; + void rebuildCommandProcessor(); DeviceRegistry *m_registry = nullptr; const IDevice *m_activeDevice = nullptr; diff --git a/src/core/hidpp/CommandProcessor.h b/src/core/hidpp/CommandProcessor.h index 3027493..a424a87 100644 --- a/src/core/hidpp/CommandProcessor.h +++ b/src/core/hidpp/CommandProcessor.h @@ -43,6 +43,10 @@ class CommandProcessor : public QObject { /// Number of pending commands. int pending() const; + /// The dispatcher this processor sends through. Held by raw pointer, so + /// whoever replaces the dispatcher must replace the processor with it. + const FeatureDispatcher *features() const { return m_features; } + signals: void queueDrained(); diff --git a/tests/helpers/AppRootFixture.h b/tests/helpers/AppRootFixture.h index 46bfabf..ef7e048 100644 --- a/tests/helpers/AppRootFixture.h +++ b/tests/helpers/AppRootFixture.h @@ -179,6 +179,14 @@ class AppRootFixture : public ::testing::Test { m_ctrl->m_profileModel.selectTab(hwIndex); } + /// Fire the DeviceManager signal that marks a transport re-attaching to a + /// PhysicalDevice that already exists. DeviceManager::probeDevice emits it + /// from a udev callback and needs a real /dev node, so tests raise it here + /// and exercise everything downstream of it. + void emitTransportReady(PhysicalDevice *device) { + emit m_ctrl->m_deviceManager.physicalDeviceTransportReady(device); + } + void pressButton(uint16_t controlId) { m_ctrl->m_buttonDispatcher.onDivertedButtonPressed(controlId, true); } diff --git a/tests/helpers/DeviceSessionFixture.h b/tests/helpers/DeviceSessionFixture.h new file mode 100644 index 0000000..7d5ac8f --- /dev/null +++ b/tests/helpers/DeviceSessionFixture.h @@ -0,0 +1,48 @@ +#pragma once +#include +#include + +#include "DeviceRegistry.h" +#include "DeviceSession.h" +#include "helpers/TestFixtures.h" +#include "hidpp/FeatureDispatcher.h" +#include "hidpp/HidrawDevice.h" + +namespace logitune::test { + +/// Reaches into the transport-owned internals of a DeviceSession. +/// +/// enumerateAndSetup() cannot run without hardware — FeatureDispatcher:: +/// enumerate() fails against /dev/null and the function bails before it +/// reaches anything worth asserting on. Tests therefore drive the one step +/// that a re-enumeration performs on already-built state: swap the +/// dispatcher, then rebuild what points at it. +class DeviceSessionFixture : public ::testing::Test { +protected: + void SetUp() override { + ensureApp(); + auto hidraw = std::make_unique("/dev/null"); + m_session = std::make_unique( + std::move(hidraw), 0xFF, QStringLiteral("Bluetooth"), &m_registry); + } + + /// The half of enumerateAndSetup() that survives without a real device: + /// a fresh FeatureDispatcher replaces the previous one, and everything + /// holding a pointer into it has to be rebuilt. + void reenumerate() { + m_session->m_features = std::make_unique(); + m_session->rebuildCommandProcessor(); + } + + hidpp::FeatureDispatcher *dispatcher() const { + return m_session->m_features.get(); + } + hidpp::CommandProcessor *processor() const { + return m_session->m_commandProcessor.get(); + } + + DeviceRegistry m_registry; + std::unique_ptr m_session; +}; + +} // namespace logitune::test diff --git a/tests/test_device_reconnect.cpp b/tests/test_device_reconnect.cpp index 02954cb..341ca13 100644 --- a/tests/test_device_reconnect.cpp +++ b/tests/test_device_reconnect.cpp @@ -61,3 +61,48 @@ TEST_F(AppRootFixture, ButtonDiversionsMatchHwProfile) { pressButton(0x0056); EXPECT_EQ(m_injector->lastArg("injectKeystroke"), "Alt+Right"); } + +// --------------------------------------------------------------------------- +// Transport re-attach +// --------------------------------------------------------------------------- +// When a transport re-attaches to a PhysicalDevice that already exists — +// a Bolt receiver coming back while the Bluetooth session is still alive, or +// a udev remove+add fast enough that the PhysicalDevice was never destroyed — +// DeviceManager does not emit physicalDeviceAdded a second time. Nothing then +// re-applies the profile, and the device keeps whatever its firmware reset to +// (issue #132). +// +// DeviceManager::probeDevice needs a real /dev node, so these tests start one +// step downstream: at the signal it emits and the AppRoot slot behind it. + +TEST_F(AppRootFixture, TransportReadyReappliesStoredProfile) { + const QString kSerial = QStringLiteral("mock-serial"); + profileEngine().cachedProfile(kSerial, QStringLiteral("default")).dpi = 2400; + + // The link dropped and the device came back at its firmware default. + m_session->setDPI(400); + ASSERT_EQ(m_session->currentDPI(), 400); + + emitTransportReady(m_physicalDevice); + + EXPECT_EQ(m_session->currentDPI(), 2400); +} + +// The re-applied profile is the one the device is actually on, not "default". +// A per-app profile that was active before the drop has to survive it. +TEST_F(AppRootFixture, TransportReadyReappliesActiveAppProfile) { + const QString kSerial = QStringLiteral("mock-serial"); + createAppProfile("google-chrome", "Google Chrome"); + // createAppProfile writes the profile to disk; the cache is what + // onTransportSetupComplete reads, so set the DPI there too. + profileEngine().cachedProfile(kSerial, QStringLiteral("Google Chrome")).dpi = 3200; + focusApp("google-chrome"); + ASSERT_EQ(profileEngine().hardwareProfile(kSerial), + QStringLiteral("Google Chrome")); + + m_session->setDPI(400); + + emitTransportReady(m_physicalDevice); + + EXPECT_EQ(m_session->currentDPI(), 3200); +} diff --git a/tests/test_device_session.cpp b/tests/test_device_session.cpp index 1ca2902..71866f9 100644 --- a/tests/test_device_session.cpp +++ b/tests/test_device_session.cpp @@ -4,6 +4,7 @@ #include "DeviceRegistry.h" #include "hidpp/HidrawDevice.h" #include "hidpp/HidppTypes.h" +#include "helpers/DeviceSessionFixture.h" using namespace logitune; using namespace logitune::hidpp; @@ -208,3 +209,47 @@ TEST_F(DeviceSessionTest, AccessorsReturnNullables) { EXPECT_NE(session->transport(), nullptr); EXPECT_NE(session->device(), nullptr); } + +// --------------------------------------------------------------------------- +// Command processor lifetime across re-enumeration +// --------------------------------------------------------------------------- +// enumerateAndSetup() builds a new FeatureDispatcher every time it runs. The +// CommandProcessor holds that dispatcher by raw pointer, so a processor kept +// from an earlier run points at freed memory the moment the dispatcher is +// replaced. Issue #132 hit this on every reconnect. + +using logitune::test::DeviceSessionFixture; + +TEST_F(DeviceSessionFixture, FirstEnumerationBuildsProcessorForCurrentDispatcher) { + ASSERT_EQ(processor(), nullptr); + + reenumerate(); + + ASSERT_NE(processor(), nullptr); + EXPECT_EQ(processor()->features(), dispatcher()); +} + +TEST_F(DeviceSessionFixture, ReEnumerationRepointsProcessorAtNewDispatcher) { + reenumerate(); + auto *firstDispatcher = dispatcher(); + ASSERT_NE(firstDispatcher, nullptr); + ASSERT_EQ(processor()->features(), firstDispatcher); + + reenumerate(); + + // The dispatcher the first processor was built against is gone. Reading + // through it is the use-after-free, so the processor must have followed + // the swap rather than survived it. + ASSERT_NE(dispatcher(), firstDispatcher); + EXPECT_EQ(processor()->features(), dispatcher()); +} + +TEST_F(DeviceSessionFixture, RebuildDropsCommandsQueuedAgainstTheOldDispatcher) { + reenumerate(); + processor()->enqueue(logitune::hidpp::FeatureId::AdjustableDPI, 0x03, {}); + ASSERT_GT(processor()->pending(), 0); + + reenumerate(); + + EXPECT_EQ(processor()->pending(), 0); +}