From 8e8d30202c34443df461c26516df82ce7cc7f18a Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 7 Aug 2026 16:23:48 +0200 Subject: [PATCH] fix(client/android): a Sony pad's buttons no longer wait on its calibration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes the parse gate in 26b0819f. The off-thread read, the claim token, the teardown ordering and its bounded wait all stand — only what happens in the gap changes. 26b0819f held every report back until the calibration read came home, so a pad that stalled on EP0 could feel dead for up to the link's 250 ms timeout: no buttons, no sticks, nothing. Reports are now forwarded immediately and their motion scaled by the nominal calibration until the real one lands. That gap is exactly the behaviour that shipped before f6de620f — acceleration ~18% short, gyro unscaled — for about a millisecond. Nobody can feel that. A controller that ignores a button press for a quarter of a second is not in the same category, and it is the only one of the two a user would ever report. It is also the safer of the two conservatisms available here. The rejected third option, forwarding motion as zeroes until the real numbers arrive, would have the host read a still pad as being in free fall — a lie about the physical world rather than an imprecision about it. The nominal constants are merely a slightly wrong scale. The token is more load-bearing under this, not less. With a gate, an unpublished calibration meant "parse nothing"; now it means "scale nominally", so begin() clearing the previous pad's value is the whole reason a re-claim falls back to the nominal constants instead of silently inheriting factory numbers belonging to a different unit — which are, in general, further off than nominal. The fallback therefore lives in the hand-off itself (MotionCalHandoff.effective) rather than as an elvis at the call site: restoring the gate now means changing the type's API, not deleting three characters in onReport. The tests moved with the contract. They assert the nominal calibration is what is in effect during the gap, rather than merely that the slot is empty — an empty slot is now compatible with either behaviour, so asserting on it would have let a regression pass. Added the case the change exists for: the same raw report, parsed either side of publication, forwards identical buttons and sticks while its gyro and acceleration convert differently. Mutation-checked three ways — dropping the nominal fallback fails all five cases, dropping begin's clear fails the inheritance case, dropping the token check fails three. Gate: `:kit:compileDebugKotlin`, `:kit:testDebugUnitTest` and `:app:compileDebugKotlin` green on a forced clean rerun, 62 cases across the module, 0 failed, with the five hand-off cases read back out of the JUnit XML. The on-glass re-verification f6de620f owes is still owed and unchanged. --- .../kotlin/io/unom/punktfunk/kit/DsCapture.kt | 34 +++--- .../io/unom/punktfunk/kit/MotionCalHandoff.kt | 30 +++-- .../punktfunk/kit/MotionCalHandoffTest.kt | 108 ++++++++++++++---- 3 files changed, 120 insertions(+), 52 deletions(-) diff --git a/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/DsCapture.kt b/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/DsCapture.kt index b68c1f91..789cb214 100644 --- a/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/DsCapture.kt +++ b/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/DsCapture.kt @@ -24,9 +24,10 @@ import android.view.InputDevice * 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 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. + * thread, with the nominal scaling standing in for the millisecond that read is in flight 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 @@ -57,7 +58,7 @@ class DsCapture( @Volatile private var pad: GamepadRouter.ExternalPad? = null /** 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]. */ + * link thread, which scales nominally until it lands — see [MotionCalHandoff]. */ private val motionCal = MotionCalHandoff() /** The thread doing the claim-time calibration read, kept for the teardown wait. */ @@ -133,8 +134,9 @@ class DsCapture( val m = DsDevice.modelFor(dev.productId) ?: return false if (!usb.start(dev)) return false // 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. + // claim forgets the last pad's calibration, so reports arriving while this pad's own read + // (below, off this thread) is in flight fall back to the nominal scaling rather than to + // another unit's factory numbers. val claim = motionCal.begin() model = m for (id in InputDevice.getDeviceIds()) { @@ -157,17 +159,18 @@ class DsCapture( * 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. + * timeout, and the interface must wait for neither. The pad is live throughout, its motion + * nominally scaled until this lands ([onReport]), so even a pad that never answers costs + * precision rather than the UI or the controller. * * 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. + // A read that throws would otherwise leave the capture on the nominal scaling with + // nothing in the log to say why — the one outcome that looks identical to a pad whose + // calibration is genuinely nominal. Publish the fallback explicitly, and say so. val cal = runCatching { readMotionCal(m) }.getOrElse { Log.w(TAG, "motion calibration read failed — nominal scaling", it) DsDevice.MotionCal.NOMINAL @@ -262,11 +265,10 @@ class DsCapture( private fun onReport(report: ByteArray, len: Int) { val m = model ?: 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 + // Nominal scaling until this claim's calibration read lands (see MotionCalHandoff): for + // that millisecond the pad behaves as it did before the read existed, which nobody can + // feel — unlike a pad whose buttons wait on a control transfer. + if (!DsDevice.parseState(m, report, len, state, motionCal.effective)) 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 diff --git a/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/MotionCalHandoff.kt b/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/MotionCalHandoff.kt index f8a34351..1bad299e 100644 --- a/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/MotionCalHandoff.kt +++ b/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/MotionCalHandoff.kt @@ -6,16 +6,20 @@ package io.unom.punktfunk.kit * * [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: + * a moment after the capture goes live. Reports in that gap are scaled by + * [DsDevice.MotionCal.NOMINAL] and forwarded like any other ([effective]): for about a millisecond + * the pad behaves exactly as it did before the calibration read existed — acceleration a little + * short, gyro unscaled — which nobody can feel, whereas a pad that ignores its buttons until an + * EP0 read comes back is very obvious. * - * - **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 + * What the hand-off is actually for is the two things that gap must NOT do, neither of which a + * plain field gives: + * + * - **Fall back to the previous pad's numbers instead of the nominal ones.** Calibration is per + * unit, so the last controller's scale factors are simply wrong for this one — more wrong, in + * general, than the nominal constants. [begin] forgets them, which is what makes the gap + * nominal rather than inherited. + * - **Let a read that outlived its claim publish.** 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. * @@ -28,8 +32,12 @@ internal class MotionCalHandoff { @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 + /** + * The calibration to scale the next report with: the live claim's own, or the nominal fallback + * while its read is still in flight. Never null — a report is always forwarded, never held + * back waiting for a control transfer. + */ + val effective: DsDevice.MotionCal get() = cal ?: DsDevice.MotionCal.NOMINAL /** Open a claim: forget the previous pad's calibration, and take this claim's token. */ @Synchronized diff --git a/clients/android/kit/src/test/kotlin/io/unom/punktfunk/kit/MotionCalHandoffTest.kt b/clients/android/kit/src/test/kotlin/io/unom/punktfunk/kit/MotionCalHandoffTest.kt index e8776ee9..c46cb6d6 100644 --- a/clients/android/kit/src/test/kotlin/io/unom/punktfunk/kit/MotionCalHandoffTest.kt +++ b/clients/android/kit/src/test/kotlin/io/unom/punktfunk/kit/MotionCalHandoffTest.kt @@ -1,24 +1,28 @@ package io.unom.punktfunk.kit +import org.junit.Assert.assertEquals 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. + * thread. Two things are pinned here, and both are about the gap before the read comes back. + * + * What the gap DOES: the pad streams, scaled by the nominal calibration — the behaviour that + * shipped before the read existed. What it must NOT do: inherit the previous pad's factory numbers + * (calibration is per unit), or accept a read that outlived its claim, which an unplug, a stop, or + * a re-claim can all cause. */ 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. + * A calibration whose gyro reads [rawLsbPerDegS] raw LSB per °/s and whose accel sits at + * [accelZero] raw counts at 0 g, so two of them are told apart by what they DO — identity + * alone would let a regression that returns the wrong instance still look right. */ - private fun cal(rawLsbPerDegS: Int): DsDevice.MotionCal { + private fun cal(rawLsbPerDegS: Int, accelZero: Int = 0): 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) @@ -28,25 +32,64 @@ class MotionCalHandoffTest { } 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(7 + 4 * i, span / 2) // gyro plus + put(9 + 4 * i, -span / 2) // gyro minus + put(23 + 4 * i, accelZero + 8192) // accel plus / minus: 8192 raw LSB per g + put(25 + 4 * i, accelZero - 8192) } put(19, speed) put(21, speed) return DsDevice.MotionCal.parse(blob, 0x05) } + /** One DS5 input report: cross held, sticks centred, gyro pitch 1600 raw, accel z 8000 raw. */ + private fun report(): ByteArray = ByteArray(64).also { + it[0] = 0x01 + it[1] = 0x80.toByte(); it[2] = 0x80.toByte(); it[3] = 0x80.toByte(); it[4] = 0x80.toByte() + it[8] = (0x08 or 0x20).toByte() // hat neutral | cross + it[16] = 0x40; it[17] = 0x06 // gyro pitch = 1600 + it[26] = 0x40; it[27] = 0x1F // accel z = 8000 + it[33] = 0x80.toByte(); it[37] = 0x80.toByte() // no touch contacts + } + @Test - fun `a claim's calibration is invisible until its read lands`() { + fun `a claim scales nominally until its read lands`() { val h = MotionCalHandoff() - assertNull("nothing is claimed yet", h.current) + assertSame(DsDevice.MotionCal.NOMINAL, h.effective) val claim = h.begin() - assertNull("the read is still in flight — the parse must not run", h.current) + assertSame("the read is in flight — scale nominally, do not wait", DsDevice.MotionCal.NOMINAL, h.effective) val read = cal(16) assertTrue(h.publish(claim, read)) - assertSame(read, h.current) + assertSame(read, h.effective) + } + + /** + * The whole point of scaling nominally instead of holding reports back: a pad answers its + * buttons from the first report, and only its motion changes when the calibration arrives. + */ + @Test + fun `a report in the gap is forwarded, nominally scaled, and rescales once the read lands`() { + val h = MotionCalHandoff() + val claim = h.begin() + val r = report() + + val gap = DsDevice.State() + assertTrue( + "a report must still be parsed while the read is in flight", + DsDevice.parseState(DsDevice.Model.DUALSENSE, r, 64, gap, h.effective), + ) + assertEquals("buttons reach the wire immediately", Gamepad.BTN_A, gap.buttons) + assertEquals("and so do sticks", 128, gap.lsX) + assertEquals("nominal gyro is the raw count", 1600, gap.gyro[0]) + assertEquals("nominal accel is ×10000/8192", 8000L * 10000 / 8192, gap.accel[2].toLong()) + + assertTrue(h.publish(claim, cal(16, accelZero = 100))) + val live = DsDevice.State() + assertTrue(DsDevice.parseState(DsDevice.Model.DUALSENSE, r, 64, live, h.effective)) + assertEquals("buttons do not depend on the calibration", gap.buttons, live.buttons) + assertEquals("1600 raw at 16 LSB/°·s = 100 °/s = 2000 wire", 2000, live.gyro[0]) + assertNotEquals("the same raw report must convert differently now", gap.gyro[0], live.gyro[0]) + assertNotEquals(gap.accel[2], live.accel[2]) } @Test @@ -55,29 +98,41 @@ class MotionCalHandoffTest { 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) + assertSame(DsDevice.MotionCal.NOMINAL, h.effective) } @Test - fun `a new claim never inherits the previous pad's calibration`() { + fun `a new claim scales nominally rather than inheriting 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 + val hot = cal(4, accelZero = 400) // a pad reading 4 raw LSB per °/s, well off nominal assertTrue(h.publish(first, hot)) + assertSame(hot, h.effective) // 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) + assertSame( + "the next pad starts on the nominal scaling, NOT the last pad's factory numbers", + DsDevice.MotionCal.NOMINAL, + h.effective, + ) assertFalse("the first pad's read may not scale the second pad", h.publish(first, hot)) - assertNull(h.current) + assertSame(DsDevice.MotionCal.NOMINAL, h.effective) + + // And that fallback is a real difference, not two names for the same numbers: the inherited + // calibration would have turned this pad's motion into something else entirely. + val r = report() + val nominal = DsDevice.State() + val inherited = DsDevice.State() + DsDevice.parseState(DsDevice.Model.DUALSENSE, r, 64, nominal, h.effective) + DsDevice.parseState(DsDevice.Model.DUALSENSE, r, 64, inherited, hot) + assertNotEquals(inherited.gyro[0], nominal.gyro[0]) + assertNotEquals(inherited.accel[2], nominal.accel[2]) 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)) + assertSame(slow, h.effective) } @Test @@ -87,8 +142,11 @@ class MotionCalHandoffTest { h.end() // DsCapture.stop h.end() // …and the unplug that followed it assertFalse(h.publish(claim, cal(16))) + assertSame(DsDevice.MotionCal.NOMINAL, h.effective) val next = h.begin() assertNotEquals(claim, next) - assertTrue(h.publish(next, cal(16))) + val read = cal(16) + assertTrue(h.publish(next, read)) + assertSame(read, h.effective) } }