fix(device): re-apply profile when a transport re-attaches - #159
Open
m-wilkowski wants to merge 1 commit into
Open
fix(device): re-apply profile when a transport re-attaches#159m-wilkowski wants to merge 1 commit into
m-wilkowski wants to merge 1 commit into
Conversation
Two separate problems left a device sitting on its firmware defaults after sleep/wake or a receiver reconnect. DeviceManager::probeDevice attached the new transport to the existing PhysicalDevice without telling anyone. physicalDeviceAdded only fires for a serial the manager has not seen before, and the transportSetupComplete fan-in does not cover it either: DeviceSession::enumerateAndSetup emits setupComplete() before attachTransport() has connected to it, so the signal goes nowhere. Nothing re-applied the profile. A new physicalDeviceTransportReady signal covers that path. AppRoot also listens for logind's PrepareForSleep(false) and re-applies every stored profile on resume, for the case where the receiver sends no reconnect notification at all. enumerateAndSetup builds a fresh FeatureDispatcher on every run but kept the CommandProcessor from the previous one, which holds the dispatcher by raw pointer. From the second enumeration onward that pointer was dangling and the next queued command read through it. The rebuild now lives in rebuildCommandProcessor(), which tears the old processor down before building one against the current dispatcher. Both changes are covered: reverting either one fails the new tests. Fixes mmaher88#132
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #133 as requested in review: this is the fix on its own, no UI or branding changes. The QML,
Theme.qml, the icon/logo replacement and thetst_SideNav.qmlmargin expectation will follow in a second PR.Fixes #132.
The two bugs
1. Nothing re-applies the profile when a transport re-attaches.
DeviceManager::probeDeviceattaches the new transport to the existingPhysicalDeviceand stops there.physicalDeviceAddedonly fires for a serial the manager has not seen before, so it does not fire on re-attach. ThetransportSetupCompletefan-in does not cover it either, becauseDeviceSession::enumerateAndSetup()emitssetupComplete()beforeattachTransport()has connected to it — the signal is emitted into a connection that does not exist yet.New
DeviceManager::physicalDeviceTransportReadysignal covers that path, withAppRoot::onPhysicalDeviceTransportReadyforwarding to the orchestrator.AppRootadditionally subscribes to logind'sPrepareForSleep. On resume it waits 2 s for HID++ to settle and re-applies every stored profile. That covers the case where the Bolt receiver sends no0x41reconnect notification after wake, which is the one I originally hit.2. Use-after-free in
DeviceSession::enumerateAndSetup.The function builds a fresh
FeatureDispatcheron every run. The old guard wasif (!m_commandProcessor && ...), so on re-enumeration the existingCommandProcessorwas kept while the dispatcher it points at got rebuilt underneath it.CommandProcessorholds the dispatcher by raw pointer, so from the second enumeration onward that pointer was dangling and the next queued command read through it.The rebuild moved into
DeviceSession::rebuildCommandProcessor(), which tears the old processor down (clear, stop, reset) before constructing one against the current dispatcher. Extracting it also makes it reachable from a test —enumerateAndSetup()itself bails early without hardware.Tests
Five new tests, all hardware-free. Reverting either fix fails four of them; the fifth is the first-enumeration baseline that passes either way.
tests/test_device_reconnect.cpp— newAppRootFixture::emitTransportReady()helper raises the DeviceManager signal (probeDeviceruns off a udev callback and needs a real/devnode, so the tests start one step downstream):TransportReadyReappliesStoredProfileTransportReadyReappliesActiveAppProfile— the per-app profile that was active before the drop is the one that comes back, notdefaulttests/test_device_session.cppwith a newtests/helpers/DeviceSessionFixture.h:FirstEnumerationBuildsProcessorForCurrentDispatcherReEnumerationRepointsProcessorAtNewDispatcher— the regression guard for the dangling pointerRebuildDropsCommandsQueuedAgainstTheOldDispatcherCommandProcessor::features()is a new const accessor. Without it the test can only compare processor addresses, which proves less and risks the allocator handing back the same address.Verification
The one failure is
HyprlandDesktopFocus.ActiveWindowEventEmitsFocusSignal, which also fails on a cleanmasterat 6c1d66f. Unrelated to this branch.Rebased on current
master, so theAppRoot::wireSignalsoverlap with #148 is already resolved — it merged cleanly, no conflict.Not smoke-tested on an MX Master 3S. I have an MX Master 4 here and can run it against that if you want hardware confirmation before merge.