Compare commits

..
Author SHA1 Message Date
enricobuehler e5453aebb7 fix(client/windows): "Open log folder" stops opening Documents
windows / build (aarch64-pc-windows-msvc) (pull_request) Successful in 3m5s
windows / build (x86_64-pc-windows-msvc) (pull_request) Successful in 3m55s
ci / rust (pull_request) Successful in 7m46s
ci / web (pull_request) Successful in 59s
ci / docs-site (pull_request) Successful in 1m18s
ci / rust-arm64 (pull_request) Successful in 1m36s
The button shipped in d839f4c2 opens the user's Documents folder instead of the log
directory on every packaged install. Nothing is wrong with the button — the path is.

The client ships as a full-trust MSIX package, and Windows redirects a packaged app's
%LOCALAPPDATA% writes into its private ...\Packages\<family>\LocalCache\Local\. The log
module creates and appends through that redirection without ever seeing it, so the
literal %LOCALAPPDATA%\punktfunk\logs it hands out is right to WRITE to and names a
directory that never exists on disk. Explorer runs outside the container: it resolves the
literal path, finds nothing, and — instead of failing — silently falls back to Documents.
An unpackaged dev run creates that directory for real, which is why this only ever showed
up in the field.

Two more places handed the same phantom path straight to the user, both added by the same
commit and both wrong in the same way: the "client log file" startup line, and the
failed-spawn banner's "Check <path>" — the one people are told to follow after a session
dies. Anyone who did landed in an empty or absent directory.

So the fix is one resolver, not three call-site patches. `real_dir` canonicalizes the
directory it just created, which resolves through the redirection on a packaged run and
changes nothing on an unpackaged one — no package identity to detect, no LocalCache path
to hand-assemble. `log_dir` stays as the write path and goes private so a future caller
can't reach for the wrong one; `path` now resolves too, which fixes both messages.

`canonicalize` always returns a `\\?\` verbatim path and Explorer refuses those (taking
the same silent Documents fallback), so `strip_verbatim` undoes the prefix — including
the `\\?\UNC\` form a roaming profile on a share resolves to. The button additionally
guards on `is_dir()`: if the resolve ever comes back wrong, the click does nothing rather
than landing the user somewhere misleading again.
2026-08-04 07:46:43 +02:00
6 changed files with 154 additions and 177 deletions
@@ -21,12 +21,8 @@ import os
private let log = Logger(subsystem: "io.unom.punktfunk", category: "gamepad")
/// Opens one connected Sony DualSense and forwards motor rumble to it over raw HID.
///
/// A caller that owns a particular pad passes the location id it wants (see
/// `open(preferringLocationID:)`); the renderer takes that from the `GCController` it is bound to,
/// so with two DualSenses attached each renderer drives its own device. Without a preference the
/// lowest location id wins an arbitrary but *stable* choice, where `Set.first` was neither.
/// Opens the first connected Sony DualSense and forwards motor rumble to it over raw HID.
/// Single-pad model (we forward exactly one controller), so the first match is the right one.
final class DualSenseHID {
private let manager: IOHIDManager
private var device: IOHIDDevice?
@@ -47,57 +43,9 @@ final class DualSenseHID {
deinit { close() }
/// The IOKit location id of the device this instance opened the handle a caller correlates
/// with its `GCController`. `nil` until a successful `open`.
private(set) var locationID: UInt32?
/// A device's location id, or `nil` if IOKit does not report one.
static func locationID(of dev: IOHIDDevice) -> UInt32? {
IOHIDDeviceGetProperty(dev, kIOHIDLocationIDKey as CFString) as? UInt32
}
/// Every connected DualSense/Edge, by location id what a caller pairs against its controllers.
static func attachedLocationIDs() -> [UInt32] {
let mgr = IOHIDManagerCreate(kCFAllocatorDefault, IOOptionBits(kIOHIDOptionsTypeNone))
let matches = productIDs.map { pid in
[kIOHIDVendorIDKey: vendorSony, kIOHIDProductIDKey: pid] as CFDictionary
}
IOHIDManagerSetDeviceMatchingMultiple(mgr, matches as CFArray)
guard IOHIDManagerOpen(mgr, IOOptionBits(kIOHIDOptionsTypeNone)) == kIOReturnSuccess else {
return []
}
defer { IOHIDManagerClose(mgr, IOOptionBits(kIOHIDOptionsTypeNone)) }
let devices = IOHIDManagerCopyDevices(mgr) as? Set<IOHIDDevice> ?? []
return devices.compactMap(locationID(of:)).sorted()
}
/// Which attached device to drive, as an index into `ids` the whole selection rule, pure so
/// it can be tested without an `IOHIDDevice` (which cannot be constructed).
///
/// `IOHIDManagerCopyDevices` returns an unordered `Set`, so the previous `Set.first` was not
/// merely arbitrary it can differ between two calls in one process. With two DualSenses that
/// made each renderer's paddevice binding a coin flip: both could land on the same device
/// (one pad's rumble coming out of the other, and the two per-instance write dedupes fighting
/// over it) or split by luck. An explicit location id makes the binding deterministic; the
/// lowest-id fallback at least makes it stable. `nil` ids sort last so a device IOKit cannot
/// place never displaces one it can.
static func preferredIndex(among ids: [UInt32?], preferring wanted: UInt32?) -> Int? {
if let wanted, let hit = ids.firstIndex(where: { $0 == wanted }) { return hit }
return ids.indices.min { (ids[$0] ?? .max) < (ids[$1] ?? .max) }
}
/// Pick the device to drive from everything attached (see [`preferredIndex`]).
static func pick(_ devices: Set<IOHIDDevice>, preferring wanted: UInt32?) -> IOHIDDevice? {
let ordered = Array(devices)
guard let i = preferredIndex(among: ordered.map(locationID(of:)), preferring: wanted) else {
return nil
}
return ordered[i]
}
/// Find and open a connected DualSense, preferring the one at `preferredLocationID`. Returns
/// false if none is present or it can't be opened (caller then falls back to CoreHaptics).
func open(preferringLocationID preferred: UInt32? = nil) -> Bool {
/// Find and open the first connected DualSense. Returns false if none is present or it can't
/// be opened (caller then falls back to CoreHaptics).
func open() -> Bool {
let matches = Self.productIDs.map { pid in
[kIOHIDVendorIDKey: Self.vendorSony, kIOHIDProductIDKey: pid] as CFDictionary
}
@@ -107,21 +55,13 @@ final class DualSenseHID {
return false
}
guard let devices = IOHIDManagerCopyDevices(manager) as? Set<IOHIDDevice>,
let dev = Self.pick(devices, preferring: preferred)
let dev = devices.first
else {
log.info("rumble: no DualSense HID device found — falling back to CoreHaptics")
IOHIDManagerClose(manager, IOOptionBits(kIOHIDOptionsTypeNone))
return false
}
device = dev
locationID = Self.locationID(of: dev)
if let preferred, locationID != preferred {
// Not fatal one pad still gets rumble but with two pads attached it means this
// renderer is driving the wrong one, and it is invisible without the log line.
log.error(
"rumble: wanted DualSense at location \(preferred, privacy: .public) but opened \(self.locationID.map(String.init) ?? "unknown", privacy: .public)"
)
}
let transport = IOHIDDeviceGetProperty(dev, kIOHIDTransportKey as CFString) as? String
bluetooth = transport?.lowercased().contains("bluetooth") ?? false
log.info("rumble: DualSense raw-HID rumble active (transport=\(self.transport, privacy: .public))")
@@ -130,16 +70,8 @@ final class DualSenseHID {
/// Drive the motors. `low` = left/heavy (low-frequency), `high` = right/light (high-frequency),
/// each 0...255. (0, 0) stops.
///
/// Returns whether the write reached the device. The caller needs this: it used to be logged
/// and swallowed, so a failed write still counted as a successful render. That matters most
/// for a **stop**, which has nothing behind it the renderer stamps its write clock even on
/// failure, the keepalive re-write only fires for non-zero levels, and the ticker is cancelled
/// once the target is `(0, 0)`. On USB there is no firmware timeout either, so a swallowed
/// stop left the motors running with nothing scheduled to try again.
@discardableResult
func rumble(low: UInt8, high: UInt8) -> Bool {
guard let dev = device else { return false }
func rumble(low: UInt8, high: UInt8) {
guard let dev = device else { return }
let report = bluetooth
? Self.bluetoothReport(low: low, high: high)
: Self.usbReport(low: low, high: high)
@@ -149,9 +81,7 @@ final class DualSenseHID {
}
if rc != kIOReturnSuccess {
log.error("rumble: IOHIDDeviceSetReport failed (0x\(String(format: "%08x", rc), privacy: .public))")
return false
}
return true
}
func close() {
@@ -117,15 +117,7 @@ public final class GamepadFeedback {
reset(slot.controller)
slots[pad] = nil
let renderer = withRouting { rumbleByPad.removeValue(forKey: pad) }
// OFF the main actor. `RumbleRenderer.stop()` is a `queue.sync`, and its body is a
// per-motor `CHHapticEngine.stop()` an XPC round trip to gamecontrollerd, which the
// renderer's own notes record as able to hang plus `DualSenseHID.close()`, whose
// blocking `IOHIDDeviceSetReport` goes to a device that has just departed. It also
// queues behind any in-flight `setup()`. This runs on every unplug and every pin
// change, and the main thread is what drives the presenter's CADisplayLink, so
// blocking here hitches the picture mid-stream. The renderer is already detached from
// routing above, so nothing observes it after this point.
if let renderer { Task.detached { renderer.stop() } }
renderer?.stop()
}
for (pad, controller) in want {
if let slot = slots[pad] {
@@ -290,12 +282,6 @@ public final class GamepadFeedback {
private func reset(_ controller: GCController?) {
guard let c = controller else { return }
c.playerIndex = .indexUnset
// Put the lightbar out too. This class is what turned it on (see the `Led` and
// `PlayerLeds` arms), and every DS write is valid-flag-selective, so a colour the game
// set stays lit in firmware after the stream ends back at the launcher, or for a pad
// that merely left the forwarded set. A DS4 is cleared incidentally because its player
// indicator IS the lightbar; a DualSense is not.
c.light?.color = GCColor(red: 0, green: 0, blue: 0)
if let ds = c.extendedGamepad as? GCDualSenseGamepad {
ds.leftTrigger.setModeOff()
ds.rightTrigger.setModeOff()
@@ -459,18 +459,6 @@ final class RumbleRenderer: @unchecked Sendable {
if split {
low = makeMotor(haptics, .leftHandle, sharpness: RumbleTuning.sharpnessLow)
high = makeMotor(haptics, .rightHandle, sharpness: RumbleTuning.sharpnessHigh)
// HALF a split is worse than none, and it used to pass silently: only the all-nil case
// below counts as failure, so one surviving handle left `ok` true and `reportHealth(nil)`
// announced HEALTHY. What actually rendered was wrong in a direction that depends on
// which handle died lose `high` and `render` falls to the combined branch (selected
// purely by `high != nil`), playing max(low, high) on the LEFT handle at the combined
// sharpness; lose `low` and the split branch's reconcile no-ops on the nil slot, so the
// heavy motor is discarded outright. Tear the survivor down and take the combined path,
// which at least renders both motors somewhere.
if low == nil || high == nil {
log.warning("rumble: only one split-handle engine came up — falling back to combined")
teardown() // disarms handlers, stops the survivor's players + engine, nils both
}
} else {
low = makeMotor(haptics, .default, sharpness: RumbleTuning.sharpnessCombined)
}
@@ -599,9 +587,7 @@ final class RumbleRenderer: @unchecked Sendable {
#if os(macOS)
guard let c, c.extendedGamepad is GCDualSenseGamepad else { return false }
let hid = DualSenseHID()
// Ask for the device this renderer's controller actually is, so two attached DualSenses
// do not both get driven through whichever one an unordered Set happened to yield first.
guard hid.open(preferringLocationID: Self.hidLocationID(for: c)) else { return false }
guard hid.open() else { return false }
dualSenseHID = hid
return true
#else
@@ -609,24 +595,6 @@ final class RumbleRenderer: @unchecked Sendable {
#endif
}
#if os(macOS)
/// Correlate a `GCController` with an IOKit location id.
///
/// GameController exposes no location id, so there is no direct mapping. What it does expose is
/// a stable per-controller ordering, and IOKit's location ids are stable per port: pairing the
/// two by rank makes each renderer pick a *distinct* device, which is the property that was
/// missing. With one pad attached this is the same device it always was.
static func hidLocationID(for c: GCController) -> UInt32? {
let ids = DualSenseHID.attachedLocationIDs()
guard ids.count > 1 else { return ids.first }
let peers = GCController.controllers().filter { $0.extendedGamepad is GCDualSenseGamepad }
guard let rank = peers.firstIndex(where: { $0 === c }), rank < ids.count else {
return ids.first
}
return ids[rank]
}
#endif
/// Write the target to the DualSense over HID if that's the active backend; false not a
/// HID pad, so the caller renders via CoreHaptics. Deduped on the pad's 0...255 resolution,
/// with a periodic keepalive re-write while nonzero (the ticker calls back in here).
@@ -637,20 +605,8 @@ final class RumbleRenderer: @unchecked Sendable {
let keepalive = levels != (0, 0)
&& seconds(since: lastHidWrite.at) > RumbleTuning.hidKeepaliveSeconds
if levels != lastHidWrite.levels || keepalive {
if hid.rumble(low: levels.0, high: levels.1) {
lastHidWrite = (levels, .now())
} else {
// The write did not reach the device. Do NOT stamp the clock that would claim a
// render that never happened, and for a stop there is nothing behind it: the
// keepalive only re-writes non-zero levels and the ticker is cancelled once the
// target is (0, 0), so the motors would keep running with nothing scheduled.
// Drop the handle instead: the pad reverts to CoreHaptics, and a reconnect
// rebuilds it. Health is reported so the state is visible rather than silent.
log.error("rumble: HID write failed — dropping the handle, falling back")
closeHID()
reportHealth("Lost the direct connection to this DualSense; using the system path.")
return false
}
hid.rumble(low: levels.0, high: levels.1)
lastHidWrite = (levels, .now())
}
return true
#else
@@ -43,33 +43,5 @@ final class DualSenseHIDTests: XCTestCase {
let crc = DualSenseHID.crc32(seed: UInt8(ascii: "1"), Array("23456789".utf8))
XCTAssertEqual(crc, 0xCBF4_3926)
}
// MARK: - Device selection (B14)
/// With two DualSenses attached, each renderer must drive its OWN device. The old code took
/// `Set.first` from an unordered set, so the paddevice binding was a coin flip that could
/// point both renderers at the same pad.
func testPreferredIndexHonoursAnExplicitLocation() {
let ids: [UInt32?] = [0x1D18_0000, 0x1420_0000, 0x1411_0000]
XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: 0x1420_0000), 1)
XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: 0x1D18_0000), 0)
}
/// No preference (or one the pad no longer has): fall back to the LOWEST id arbitrary, but
/// stable across calls, which `Set.first` was not.
func testPreferredIndexFallsBackToTheLowestIdDeterministically() {
let ids: [UInt32?] = [0x1D18_0000, 0x1420_0000, 0x1411_0000]
XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: nil), 2)
// A wanted id that is gone (pad unplugged between enumeration and open) must not fail the
// open it degrades to the same stable fallback.
XCTAssertEqual(DualSenseHID.preferredIndex(among: ids, preferring: 0xDEAD_BEEF), 2)
}
/// A device IOKit reports no location for must never displace one it can place.
func testPreferredIndexSortsUnplaceableDevicesLast() {
XCTAssertEqual(DualSenseHID.preferredIndex(among: [nil, 0x1420_0000], preferring: nil), 1)
XCTAssertEqual(DualSenseHID.preferredIndex(among: [nil, nil], preferring: nil), 0)
XCTAssertNil(DualSenseHID.preferredIndex(among: [], preferring: nil))
}
}
#endif
+11 -6
View File
@@ -1045,13 +1045,18 @@ pub(crate) fn settings_page(
let ss = set_screen.clone();
button("Third-party licenses").on_click(move || ss.call(Screen::Licenses))
};
// The client log's home (%LOCALAPPDATA%\punktfunk\logs) — the file every "check the
// client log" message means, which until this row had no way in from the UI at all.
// The folder rather than the file so the rotated `.old` generation is in reach too.
// Best-effort, like the log itself: a missing dir or a failed spawn stays silent.
// The client log's home — the file every "check the client log" message means, which until
// this row had no way in from the UI at all. The folder rather than the file so the rotated
// `.old` generation is in reach too.
//
// `real_dir` (not the literal %LOCALAPPDATA% path) because Explorer lives outside our MSIX
// container: handed a path the package redirection keeps from ever existing, it silently
// opens the user's Documents folder instead of failing, which is precisely what this button
// shipped doing. The `is_dir` guard keeps that fallback unreachable — if the resolve ever
// comes back wrong, the click does nothing rather than landing somewhere misleading.
// Best-effort otherwise, like the log itself: a failed spawn stays silent.
let logs_button = button("Open log folder").on_click(|| {
if let Some(dir) = crate::logfile::log_dir() {
let _ = std::fs::create_dir_all(&dir);
if let Some(dir) = crate::logfile::real_dir().filter(|d| d.is_dir()) {
let _ = std::process::Command::new("explorer.exe").arg(&dir).spawn();
}
});
+131 -3
View File
@@ -10,6 +10,10 @@
//! Mirrors the host's convention (`%ProgramData%\punktfunk\logs`, size-capped): a file over
//! 10 MB is rotated to `.old` at the next client start, one generation kept. Everything is
//! best-effort — a missing/locked directory degrades to plain stderr, never a startup failure.
//!
//! Two paths, deliberately: [`log_dir`] is what we open files through, [`real_dir`] is where
//! they actually land. Under MSIX those differ, and only the second one is fit to show a user
//! or hand to Explorer.
use std::fs::{File, OpenOptions};
use std::io::{self, BufRead, Write};
@@ -21,14 +25,74 @@ const ROTATE_BYTES: u64 = 10 * 1024 * 1024;
static SINK: OnceLock<Option<Arc<Mutex<File>>>> = OnceLock::new();
/// The log directory — Settings ▸ About's "Open log folder" opens it in Explorer.
pub(crate) fn log_dir() -> Option<PathBuf> {
/// The log directory we WRITE through: `%LOCALAPPDATA%\punktfunk\logs`.
///
/// Correct to open files under, but NOT necessarily where the bytes land — see [`real_dir`].
/// Anything shown to a user or handed to another process wants that one instead.
fn log_dir() -> Option<PathBuf> {
Some(PathBuf::from(std::env::var_os("LOCALAPPDATA")?).join(r"punktfunk\logs"))
}
/// The log directory as it exists ON DISK — Settings ▸ About's "Open log folder" opens this in
/// Explorer, and [`path`] names it in the startup line and the failed-spawn banner.
///
/// The shipping client is a full-trust MSIX package, and Windows redirects a packaged app's
/// `%LOCALAPPDATA%` writes into its private `…\Packages\<family>\LocalCache\Local\…`. We create
/// and append through that redirection without ever seeing it, so [`log_dir`] is the right path
/// to WRITE to yet names a directory that never exists on disk. Explorer runs OUTSIDE the
/// container: it resolves the literal path, finds nothing, and silently falls back to the user's
/// Documents folder — which is exactly what "Open log folder" did in every packaged install, and
/// what the two "check <path>" messages pointed at. An unpackaged dev run creates the literal
/// directory for real, which is why this only ever showed up in the field.
///
/// Canonicalizing the directory we just created resolves through the redirection on a packaged
/// run and changes nothing on an unpackaged one, so there is no package identity to detect.
pub(crate) fn real_dir() -> Option<PathBuf> {
let dir = log_dir()?;
std::fs::create_dir_all(&dir).ok()?;
Some(std::fs::canonicalize(&dir).map_or(dir, strip_verbatim))
}
/// Undo the `\\?\` that [`std::fs::canonicalize`] always prefixes. Explorer refuses a verbatim
/// path — it would take the very same silent Documents fallback [`real_dir`] exists to avoid —
/// and it is noise in a line a user is meant to read and act on.
fn strip_verbatim(p: PathBuf) -> PathBuf {
use std::path::{Component, Prefix};
// Scoped so the borrow ends before the `return p` below can move it.
let head = match p.components().next() {
Some(Component::Prefix(pre)) => match pre.kind() {
// `\\?\C:\…` → `C:\…`
Prefix::VerbatimDisk(drive) => Some(PathBuf::from(format!(r"{}:\", drive as char))),
// `\\?\UNC\server\share\…` → `\\server\share\…` (a roaming profile on a share).
// Built through `OsString`, which appends verbatim — `PathBuf::push` would apply
// separator logic to the bare `\\` and mangle it.
Prefix::VerbatimUNC(server, share) => {
let mut unc = std::ffi::OsString::from(r"\\");
unc.push(server);
unc.push(r"\");
unc.push(share);
Some(PathBuf::from(unc))
}
// Already a plain path — nothing to undo.
_ => None,
},
_ => None,
};
let Some(mut out) = head else { return p };
// `skip(1)` drops the prefix; the `RootDir` that follows it is already in `head`.
out.extend(
p.components()
.skip(1)
.filter(|c| !matches!(c, Component::RootDir)),
);
out
}
/// The log file's path, for the "logs land here" startup line and the failed-spawn banner.
/// Resolved like [`real_dir`] — a path a user is told to check has to be the one on disk.
pub(crate) fn path() -> Option<PathBuf> {
Some(log_dir()?.join("client.log"))
Some(real_dir()?.join("client.log"))
}
/// Open (rotating first) and cache the sink. Called once at startup, before the tracing
@@ -97,3 +161,67 @@ pub(crate) fn forward_child_stderr(stderr: impl io::Read + Send + 'static) {
}
});
}
#[cfg(test)]
mod tests {
use super::*;
/// The shape `canonicalize` actually returns for a local profile. Explorer treats a `\\?\`
/// path as unresolvable and opens Documents instead, so the prefix has to come off.
#[test]
fn verbatim_disk_prefix_comes_off() {
let p = PathBuf::from(r"\\?\C:\Users\ada\AppData\Local\punktfunk\logs");
assert_eq!(
strip_verbatim(p),
PathBuf::from(r"C:\Users\ada\AppData\Local\punktfunk\logs")
);
}
/// The MSIX-redirected form is what the fix is for: same treatment, longer path.
#[test]
fn verbatim_disk_prefix_comes_off_for_the_package_local_cache() {
let p = PathBuf::from(
r"\\?\C:\Users\ada\AppData\Local\Packages\unom.Punktfunk_8wekyb3d8bbwe\LocalCache\Local\punktfunk\logs",
);
assert_eq!(
strip_verbatim(p),
PathBuf::from(
r"C:\Users\ada\AppData\Local\Packages\unom.Punktfunk_8wekyb3d8bbwe\LocalCache\Local\punktfunk\logs"
)
);
}
/// A roaming profile on a share canonicalizes to `\\?\UNC\…`; the plain UNC form is what
/// Explorer takes. `\\server\share` must survive intact — dropping either half, or letting
/// `PathBuf::push`'s separator logic at the bare `\\`, yields a path that opens nothing.
#[test]
fn verbatim_unc_prefix_becomes_a_plain_unc_path() {
let p = PathBuf::from(r"\\?\UNC\fileserv\profiles\ada\AppData\Local\punktfunk\logs");
assert_eq!(
strip_verbatim(p),
PathBuf::from(r"\\fileserv\profiles\ada\AppData\Local\punktfunk\logs")
);
}
/// An unpackaged dev run resolves to a path that was never verbatim — leave it alone.
#[test]
fn plain_path_is_untouched() {
let p = PathBuf::from(r"C:\Users\ada\AppData\Local\punktfunk\logs");
assert_eq!(strip_verbatim(p.clone()), p);
}
/// Whatever the run, the resolved directory is one Explorer can open: it exists, and it
/// carries no verbatim prefix. This is the button's actual precondition.
#[test]
fn real_dir_is_an_openable_directory() {
let Some(dir) = real_dir() else {
return; // no LOCALAPPDATA (not a normal user session) — nothing to assert
};
assert!(dir.is_dir(), "{} is not a directory", dir.display());
assert!(
!dir.to_string_lossy().starts_with(r"\\?\"),
"{} kept its verbatim prefix",
dir.display()
);
}
}