fix(client/android): plugging in a Sony pad could hitch the interface

Supersedes the synchronous calibration read f6de620f shipped an hour ago. The ordering it protected is kept; the blocking it cost is not. f6de620f read the pad's calibration inline in DsCapture.startUsb, which runs on the main thread — the stream's setup path, and the USB-permission broadcast. The read is a blocking EP0 control transfer: a pad that is there answers in about a millisecond, but a pad that is stalling takes the link's whole 250 ms write timeout, and either way the interface was waiting on a controller. That is the wrong thread for it. It now runs on its own daemon thread, one per claim, named pf-ds-cal — the same shape HidUsbLink already uses for its reader rather than a second style. A pathological stall now delays the pad's motion by a moment instead of freezing the UI. What kept the ordering honest before was "assign the calibration before `model`", since `model` is what lets the link thread into the parse. That reasoning stands, so the gate simply moved: MotionCalHandoff holds the claim's calibration, starts null, and onReport parses nothing until it lands. No report is ever scaled by the last pad's numbers — those are per unit — nor by the nominal fallback the real read is about to replace. Dropping the first millisecond of a capture costs nothing: the reports carry absolute state, so the next one says everything the dropped one would have. The calibration is what got deferred, not `model`, and that is deliberate. Keeping `model` synchronous keeps isActive, the teardown writes, the feedback sinks and the active-changed true/false pairing meaning exactly what they meant yesterday — and, more to the point, it makes a late completion structurally unable to resurrect a dead capture. A straggler can only ever publish a calibration, and nothing is parsed while `model` is null. Teardown, which is where this sort of change actually bites. Both stop() and the unplug path end the claim before they close anything: ending burns the token, so a read that lands afterwards publishes nothing and says so in the log. They then wait, bounded at 500 ms and normally already over, for the read to let go of the connection they are about to close — closing a descriptor with a transfer in flight pulls it out from under the kernel, the same rule the pad-audio borrow follows. It cannot deadlock: the reading thread blocks on the EP0 transfer and on the hand-off's own monitor, never on anything a teardown holds. If a pad has stopped answering entirely the wait elapses and teardown proceeds regardless, which is the same exposure the feedback writes already carry and better than an interface that never comes back. Tested where it is testable. MotionCalHandoff is the piece that carries the hazard and it is pure, so it has its own test: nothing is visible until the read lands, a read that outlived its claim publishes nothing, a re-claim never inherits the previous pad's calibration, and a doubled end still refuses every outstanding token. Mutation-checked both ways — deleting the token check fails 3 of them, deleting begin's clear fails the fourth. Not covered: DsCapture's own claim/teardown ordering is not unit-testable in this module — there is no Robolectric, and the class builds a main-Looper Handler and needs a UsbManager — so it is argued in comments rather than pinned. The on-glass re-verification f6de620f owes is unchanged and still owed. Gate: `:kit:compileDebugKotlin`, `:kit:testDebugUnitTest` (61 cases across the module, 0 failed) and `:app:compileDebugKotlin` green, with the four new cases confirmed present in the JUnit XML rather than assumed from a green build.
2026-08-07 16:10:22 +02:00
parent f6de620f34
commit 26b0819f5c
3 changed files with 237 additions and 17 deletions
@@ -23,9 +23,10 @@ import android.view.InputDevice
 * Input: parse ([DsDevice.parseState]) → typed mirror on an [GamepadRouter.ExternalPad] (buttons
 * diffed, axes on-change — the exit chord participates like any pad) + the rich plane (touch
 * normalized to the wire's 0..65535 screen space on-change; motion forwarded per report, rescaled
- * into the wire's units by this pad's own calibration, read once at claim). The wire slot is
- * claimed when the capture engages, with the first parsed report as the fallback for a claim that
- * found no free index, and freed on unplug/[stop], so indices never leak.
+ * into the wire's units by this pad's own calibration — read once per claim, off the claiming
+ * thread, so parsing starts a millisecond in rather than the UI waiting on a control transfer).
+ * The wire slot is claimed when the capture engages, with the first parsed report as the fallback
+ * for a claim that found no free index, and freed on unplug/[stop], so indices never leak.
 *
 * Feedback: implements [GamepadFeedback.PadFeedbackSink] — rumble / trigger / lightbar / player
 * LED events addressed to this pad's wire index become USB output reports on the physical pad
@@ -55,9 +56,12 @@ class DsCapture(
    @Volatile private var model: DsDevice.Model? = null
    @Volatile private var pad: GamepadRouter.ExternalPad? = null

-    /** This pad's factory motion scale, read once per capture (see [readMotionCal]). Written on
-     *  the claiming thread before [model], which is what the link thread's parse reads it under. */
-    @Volatile private var motionCal = DsDevice.MotionCal.NOMINAL
+    /** This pad's factory motion scale, read once per capture on [calReader] and handed to the
+     *  link thread. Null until that read lands — see [MotionCalHandoff] and [onReport]. */
+    private val motionCal = MotionCalHandoff()
+
+    /** The thread doing the claim-time calibration read, kept for the teardown wait. */
+    @Volatile private var calReader: Thread? = null

    // Typed-mirror diff state (wire units) + rich-plane on-change mirrors. Link thread only.
    private val state = DsDevice.State()
@@ -128,9 +132,10 @@ class DsCapture(
        if (model != null) return false
        val m = DsDevice.modelFor(dev.productId) ?: return false
        if (!usb.start(dev)) return false
-        // Before `model`, which is what gates the link thread's parse: a report must never be
-        // scaled by the previous pad's calibration, or by the fallback once the real one is known.
-        motionCal = readMotionCal(m)
+        // Before `model`, which is what lets the link thread into the parse at all: opening the
+        // claim forgets the last pad's calibration, so no report can be scaled by it while this
+        // pad's own read (below, off this thread) is in flight.
+        val claim = motionCal.begin()
        model = m
        for (id in InputDevice.getDeviceIds()) {
            val d = InputDevice.getDevice(id) ?: continue
@@ -142,18 +147,68 @@ class DsCapture(
        Log.i(TAG, "Sony pad captured over USB: PID=0x%04x model=%s".format(dev.productId, m))
        ensureSlot(m)
        onActiveChanged?.invoke(true)
+        readMotionCalAsync(m, claim)
        return true
    }

    /**
-     * Read this pad's IMU calibration, ONCE, while claiming it — the feature report that says how
-     * many raw counts this individual unit puts on a °/s and on a g ([DsDevice.MotionCal]).
+     * Start this claim's calibration read, on its own thread.
     *
-     * At claim time and nowhere else: the read is a blocking EP0 control transfer (bounded by the
-     * link's write timeout, answered in about a millisecond by a pad that is there), and the
-     * calibration is fixed for the life of the connection, so doing it per input report would buy
-     * nothing and cost the capture its latency. A pad that refuses keeps the nominal scaling
-     * rather than losing motion altogether.
+     * Off the caller's thread because [startUsb] runs on the main one — stream setup, and the
+     * USB-permission broadcast — and the read is a blocking EP0 control transfer: a pad that is
+     * there answers in about a millisecond, but one that is stalling takes the link's whole write
+     * timeout, and the interface must wait for neither. The pad goes live a millisecond later
+     * instead, because the link thread parses nothing until the calibration lands ([onReport]);
+     * a pathological stall then delays motion rather than freezing the UI.
+     *
+     * One thread per claim, daemon and named, matching how [HidUsbLink] runs its reader; it is
+     * awaited by [awaitCalRead] before the connection it reads from can be closed.
+     */
+    private fun readMotionCalAsync(m: DsDevice.Model, claim: Int) {
+        val t = Thread({
+            // Never leave the gate shut: a read that fails or throws still has to publish
+            // something, or this capture would forward no motion at all for its whole life.
+            val cal = runCatching { readMotionCal(m) }.getOrElse {
+                Log.w(TAG, "motion calibration read failed — nominal scaling", it)
+                DsDevice.MotionCal.NOMINAL
+            }
+            // Discarded when the claim is already over (unplug, stop, or a re-claim beat us here):
+            // scaling the NEXT pad by this one's factory numbers would be worse than not reading.
+            if (!motionCal.publish(claim, cal)) {
+                Log.i(TAG, "motion calibration arrived after the claim ended — discarded")
+            }
+        }, "pf-ds-cal")
+        calReader = t
+        t.isDaemon = true
+        t.start()
+    }
+
+    /**
+     * Wait for an in-flight calibration read to let go of the USB connection, before a teardown
+     * closes it.
+     *
+     * Not politeness: the read is a control transfer on the very connection [HidUsbLink.stop] is
+     * about to close, and closing a descriptor with a transfer in flight pulls it out from under
+     * the kernel — the same rule the pad-audio borrow follows. Bounded, and in every case but a
+     * pad that has stopped answering the thread is long gone, so this returns immediately. It can
+     * never deadlock: the reading thread waits on nothing this one holds ([MotionCalHandoff] has
+     * its own monitor, and the read itself takes no lock).
+     */
+    private fun awaitCalRead() {
+        val t = calReader ?: return
+        calReader = null
+        if (!t.isAlive) return
+        runCatching { t.join(CAL_JOIN_MS) }
+        if (t.isAlive) Log.w(TAG, "calibration read still in flight at teardown")
+    }
+
+    /**
+     * Read this pad's IMU calibration — the feature report that says how many raw counts this
+     * individual unit puts on a °/s and on a g ([DsDevice.MotionCal]).
+     *
+     * Once, at claim time, and nowhere else: the calibration is fixed for the life of the
+     * connection, so doing it per input report would buy nothing and cost the capture its latency.
+     * A pad that refuses keeps the nominal scaling rather than losing motion altogether.
     */
    private fun readMotionCal(m: DsDevice.Model): DsDevice.MotionCal {
        val blob = usb.getReport(HidUsbLink.REPORT_TYPE_FEATURE, m.calReportId, m.calReportLen)
@@ -192,6 +247,10 @@ class DsCapture(
            resetRichFeedback(m)
        }
        disarmBackstop()
+        // End the claim before waiting on it: a calibration that lands after this publishes
+        // nothing, and then the wait makes sure nothing is still reading the connection below.
+        motionCal.end()
+        awaitCalRead()
        usb.stop()
        val wasActive = model != null
        model = null
@@ -203,7 +262,11 @@ class DsCapture(

    private fun onReport(report: ByteArray, len: Int) {
        val m = model ?: return
-        if (!DsDevice.parseState(m, report, len, state, motionCal)) return
+        // This claim's calibration read is still in flight. Dropping the report beats parsing it
+        // with a fallback that is about to be replaced: the reports carry absolute state, so the
+        // next one (1–4 ms away) says everything this one would have.
+        val cal = motionCal.current ?: return
+        if (!DsDevice.parseState(m, report, len, state, cal)) return
        // Normally claimed already, at capture time; this is the retry for a capture that engaged
        // while every wire index was taken.
        val p = pad ?: ensureSlot(m) ?: return // all 16 taken — drop until one frees
@@ -314,6 +377,10 @@ class DsCapture(
        val wasActive = model != null
        model = null
        releaseSlot()
+        // As in stop(): end the claim so a late calibration publishes nothing, then wait for the
+        // read to let go of the connection the line below closes.
+        motionCal.end()
+        awaitCalRead()
        // Release the transport too: the link only *signals* the drop, so without this an unplug
        // left its connection open, its interfaces claimed and its detach receiver registered.
        usb.stop()
@@ -518,5 +585,9 @@ class DsCapture(
        /** How soon to retry a rumble stop whose write was rejected. Short: the motors are running
         *  and the host has already moved on, so nothing else is coming to silence them. */
        const val STOP_RETRY_MS = 100L
+
+        /** Teardown's budget for an in-flight calibration read. Comfortably past the link's own
+         *  EP0 timeout, so it only ever elapses for a pad that has stopped answering entirely. */
+        const val CAL_JOIN_MS = 500L
    }
 }
@@ -0,0 +1,55 @@
+package io.unom.punktfunk.kit
+
+/**
+ * The hand-off of one claim's motion calibration, from the thread that reads it off the pad to the
+ * link thread that scales every input report with it.
+ *
+ * [DsCapture] reads a captured Sony pad's calibration feature report **off** the claiming thread —
+ * it is a blocking EP0 control transfer and the claim runs on the UI's thread — so the value lands
+ * a moment after the capture goes live. Two things have to hold across that gap, and a plain field
+ * gives neither:
+ *
+ *  - **No report is ever scaled by the wrong pad's numbers.** [begin] forgets whatever the last
+ *    capture published, so the link thread reads null — "no calibration yet", parse nothing — rather
+ *    than inheriting the previous controller's scale factors, which are per unit and simply wrong
+ *    for this one. It is also why the gap is a *drop* rather than a fallback: the fallback is what
+ *    the read is about to replace, and a millisecond of unparsed reports costs nothing (they carry
+ *    absolute state, and the next one is 1–4 ms behind).
+ *  - **A read that outlived its claim publishes nothing.** An unplug, a [DsCapture.stop] and a fast
+ *    re-claim can all land while a read is in flight; [publish] only accepts a value whose token is
+ *    still the live claim's, so a straggler can never scale a pad it never read.
+ *
+ * Thread-safe: claimed and ended by the claiming thread, published by the reading thread, read by
+ * the link thread.
+ */
+internal class MotionCalHandoff {
+    /** Handed out by [begin] and burned by [end] — never reused, so a straggler can't match. */
+    private var token = 0
+
+    @Volatile private var cal: DsDevice.MotionCal? = null
+
+    /** The live claim's calibration, or null while its read is still in flight. */
+    val current: DsDevice.MotionCal? get() = cal
+
+    /** Open a claim: forget the previous pad's calibration, and take this claim's token. */
+    @Synchronized
+    fun begin(): Int {
+        cal = null
+        return ++token
+    }
+
+    /** End the live claim. Nothing read under an older token can land after this. */
+    @Synchronized
+    fun end() {
+        cal = null
+        token++
+    }
+
+    /** Publish [value] if [claim] is still the live claim; returns whether it landed. */
+    @Synchronized
+    fun publish(claim: Int, value: DsDevice.MotionCal): Boolean {
+        if (claim != token) return false
+        cal = value
+        return true
+    }
+}
@@ -0,0 +1,94 @@
+package io.unom.punktfunk.kit
+
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotEquals
+import org.junit.Assert.assertNull
+import org.junit.Assert.assertSame
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * The claim/read hand-off that lets [DsCapture] read a pad's motion calibration off the claiming
+ * thread. What is pinned here is what happens when the read does NOT come back inside its claim:
+ * an unplug, a stop, or a re-claim can all land first, and a straggler that published anyway would
+ * scale a pad it never read — the one hazard the threading introduces.
+ */
+class MotionCalHandoffTest {
+    /**
+     * A calibration whose gyro scale is [rawLsbPerDegS] raw LSB per °/s, so two of them are told
+     * apart by what they do and not only by identity.
+     */
+    private fun cal(rawLsbPerDegS: Int): DsDevice.MotionCal {
+        val speed = 500 // speed_plus = speed_minus, so speed_2x = 1000
+        val span = rawLsbPerDegS * 1000 // |plus − bias| + |minus − bias| = span
+        val blob = ByteArray(41)
+        fun put(o: Int, v: Int) {
+            blob[o] = (v and 0xFF).toByte()
+            blob[o + 1] = ((v shr 8) and 0xFF).toByte()
+        }
+        blob[0] = 0x05
+        for (i in 0 until 3) {
+            put(7 + 4 * i, span / 2) // plus
+            put(9 + 4 * i, -span / 2) // minus
+            put(23 + 4 * i, 8192) // accel plus / minus: nominal, not what this test is about
+            put(25 + 4 * i, -8192)
+        }
+        put(19, speed)
+        put(21, speed)
+        return DsDevice.MotionCal.parse(blob, 0x05)
+    }
+
+    @Test
+    fun `a claim's calibration is invisible until its read lands`() {
+        val h = MotionCalHandoff()
+        assertNull("nothing is claimed yet", h.current)
+        val claim = h.begin()
+        assertNull("the read is still in flight — the parse must not run", h.current)
+        val read = cal(16)
+        assertTrue(h.publish(claim, read))
+        assertSame(read, h.current)
+    }
+
+    @Test
+    fun `a read that outlived its claim publishes nothing`() {
+        val h = MotionCalHandoff()
+        val claim = h.begin()
+        h.end() // unplug, or DsCapture.stop, while the read was in flight
+        assertFalse("a straggler may not publish into a dead claim", h.publish(claim, cal(16)))
+        assertNull(h.current)
+    }
+
+    @Test
+    fun `a new claim never inherits the previous pad's calibration`() {
+        val h = MotionCalHandoff()
+        val first = h.begin()
+        val hot = cal(4) // a pad whose gyro reads 4 raw LSB per °/s
+        assertTrue(h.publish(first, hot))
+
+        // Re-claimed without an end() in between — the pad was swapped while a read was in flight.
+        val second = h.begin()
+        assertNotEquals(first, second)
+        assertNull("the next pad starts with no calibration, not the last one's", h.current)
+        assertFalse("the first pad's read may not scale the second pad", h.publish(first, hot))
+        assertNull(h.current)
+
+        val slow = cal(32)
+        assertTrue(h.publish(second, slow))
+        assertSame(slow, h.current)
+        // And the two really are different scales, so the assertions above are about a real
+        // difference rather than two names for the same numbers.
+        assertNotEquals(hot.gyroToWire(0, 100), slow.gyroToWire(0, 100))
+    }
+
+    @Test
+    fun `ending a claim twice still refuses every outstanding token`() {
+        val h = MotionCalHandoff()
+        val claim = h.begin()
+        h.end() // DsCapture.stop
+        h.end() // …and the unplug that followed it
+        assertFalse(h.publish(claim, cal(16)))
+        val next = h.begin()
+        assertNotEquals(claim, next)
+        assertTrue(h.publish(next, cal(16)))
+    }
+}