fix: BLE page-turner defects + simulated-hardware bench for both remote encodings - #1
Open
ShahriarAhnaf wants to merge 6 commits into
Open
fix: BLE page-turner defects + simulated-hardware bench for both remote encodings#1ShahriarAhnaf wants to merge 6 commits into
ShahriarAhnaf wants to merge 6 commits into
Conversation
…ing") Five defects found while driving the branch end-to-end against a simulated BLE HID page-turner. 1. BleButtonMapActivity::onEnter() wiped the entire bleKeyMap and saved it before the user did anything, so opening "Map Remote Buttons" — including opening it only to read the binding list this activity renders — destroyed every existing mapping, with no confirmation and no undo. It also made that list permanently empty, since onEnter had just cleared what it displays. Nothing needed the wipe: assignCapturedKey() already drops any other key bound to the action being assigned and reuses the slot of a re-captured key, so neither a stale action nor a duplicate binding can survive a re-map. 2. Keys arriving during Step::SelectFunction stayed latched in the capture buffer. Since the host synthesizes auto-repeats for a held key, a repeat that landed while the user was choosing a function was then consumed as the *next* button they "pressed". Stale captures are now discarded while selecting. 3. In capture mode pollBle() overwrote the captured key on every iteration of its drain loop, so when two reports arrived in one frame the key the user actually pressed was replaced by whatever followed it. Keep the first unconsumed key instead. 4. The reader's status bar replaced the book title with "BT Connecting..." whenever Bluetooth was enabled and not connected, unbounded. A bonded remote that has gone to sleep stops advertising until a button is pressed on it, so the title could be held hostage indefinitely for a connection that was not in progress. It now requires the host to actually be running with a bond, and gives the title back after a bounded window. EpubReaderMenuActivity had the mirror bug, reporting "connecting" when the lifecycle had deliberately stopped the stack; that state is PAUSED. 5. bleKeyMap has no defaults and pollBle() drops any key it cannot resolve, so a remote that has never been through the mapping screen is inert — its keys are decoded correctly and then discarded, which presents as broken rather than unconfigured. Standard navigation keys (arrows, page up/down, enter, escape) now have out-of-the-box bindings, consulted only for keys the user has not bound, so explicit mappings still win. Vendor and consumer-page codes carry no portable meaning and remain capture-and-assign.
Runs the real image on a simulated Xteink X4 and pairs it with a firmware-less
BLE HID peer over a simulated air interface -- real scan, SMP, session key, GATT
discovery and input reports, nothing stubbed between the firmware and the link.
Both remote encodings are parameterised into every test, because they take
different routes through BleKeyboardHost::onReportIngest and only the first was
ever exercised:
* keyboard -- 8-byte boot-protocol report carrying a HID usage. Takes the
standard keyboard slot path, surfaces as a SpecialKey, and so must page
without any mapping now that the default bindings exist.
* bitmap -- what a Hanlinyue "Free 2" actually emits, per the teardown posted
on crosspoint-reader#2418: 3-byte reports, top 20 00 00, bottom 04 00 00,
explicit release 00 00 00. Not keyboard usages and not consumer usage IDs;
they reach the generic fallback where the identity is the first non-zero
byte. Vendor codes carry no portable meaning, so these stay
capture-and-assign -- the test pins the decode, not a default binding.
The Report Map assertion is load-bearing beyond discovery: the map is 49 octets
for the keyboard peer and longer once the bitmap peer adds its consumer
collection, so it cannot be read in one ATT response at the default MTU. A
regression in Read Blob continuation fails there rather than silently.
Payload and board model are simulator artifacts that are not in this repo, so
the tests skip in a bare checkout instead of failing.
Author
Verification statusLocal build: passes. The two red checks are pre-existing on
Run locally it passes with a single defect, in a file this branch does not touch: Since CI runs
What is verified, and what is notVerified: it compiles and links; the bitmap peer's encoding round-trips through the Not verified: no hardware. The default-binding path and the mapping-screen fixes are |
Author
Correction: CI built this fineThe The job then moved on to the That is PlatformIO failing to build its own virtualenv, not this code — the same So all three red checks are environmental or pre-existing:
|
…ote's bytes
The bitmap peer was fitted. BITMAP_TOP = 0x20 and BITMAP_BOTTOM = 0x04 were
constants lifted from one teardown of one remote and held until the bench
matched it, which proves the bench can replay a Free 2 and nothing about
whether the firmware supports HID page-turners in general.
A host does not work that way: it derives a report's layout from the device's
report descriptor, where Report Size and Report Count give the bit geometry and
the Input item's Variable-vs-Array flag decides whether a control is one bit of
a bitfield or a usage code placed in an array slot.
hid_descriptor.py builds the Report Map and the conforming report bytes from a
single declaration, so the bytes follow from the descriptor. Under that
construction the Free 2's 20 00 00 stops being a magic number and becomes bit 5
of a variable input -- button 6 of a 24-bit button bitmap -- and 04 00 00 is
button 3. Both reproduce exactly, as does a boot keyboard's arrow-key report,
without a copied byte anywhere. Changing which button is pressed makes the peer
a different remote.
test_hid_descriptor.py needs no simulator, payload or hardware, so unlike the
rest of test/sim it runs in a bare checkout. Besides pinning the geometry and
the variable-vs-array distinction, it documents two defects in the host's
report-map handling, whose own comment concedes it is "a hint, not a full
descriptor parse":
* the scan walks bytes rather than HID short items, so the data bytes of a
legal 2-byte Usage item (0x0A 0x05 0x07, Usage 0x0705) are misread as
Usage Page (Keyboard);
* it only matches the 1-byte Usage Page item (0x05 nn), so a vendor-defined
page declared as 0x06 nn nn is invisible to it.
Both are covered against a correct short-item walk, with a positive control
showing the shortcut agrees on an ordinary keyboard descriptor -- which is why
the defect survived. The downstream heuristics (extractPrimaryCode's
"first non-zero byte" fallback, the gamepad axis quantisation) are patches for
the absent parser, and each new remote shape needs another one.
…perly Two spec gaps in the peer, both invisible while only one permissive host talked to it. HOGP requires the HID Device to expose a single Device Information Service instance, and requires that instance to include the PnP ID characteristic -- it is how a host identifies vendor and product before reading a single report. The peer exposed HID and Battery but no DIS at all, so it was not a conformant HID device even though CrossPoint was happy to pair with it. Added, with the pid.codes open-source VID so it identifies as a test device rather than impersonating a vendor. The sibling import now uses __file__ rather than the working directory, which needed the ScriptedBlePeer fix in simantic-core: peer scripts previously had no __file__ and no directory on sys.path, so they could not be split across files at all.
The peer clicked on a timer and nothing could intervene. With the scripted-peer
command channel in simantic-core, a test drives it directly:
peer.Command = "click=6" # press-and-release button 6
peer.Command = "press=3" # hold
peer.Command = "autoclick=0" # stop the free-running repeat
Which button a name maps to, and what a click even is, lives in this script
rather than in the simulator, so the same peripheral running a different script
is a different BLE slave.
Both surfaced only once commands could actually be sent, which is the point of having a runtime surface at all. Commands replaced the pending queue instead of appending to it, so writes arriving faster than the timer drains them lost all but the last. Driving click=6, press=3 and release=3 in quick succession delivered one report -- 000000 -- because each command overwrote the one before it. A test pressing twice in a row would have silently lost the first press. on_timer logged "sending HID report" whether or not anything was subscribed, so reports that never reached the air appeared in the log as if they had. That is the same failure that made a dead link read as a working one earlier in this work; unsubscribed reports are now logged as dropped.
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.
Found while driving
feat-bluetoothend to end against a simulated Xteink X4 with afirmware-less BLE HID page-turner on the other end of a simulated air interface.
The link, SMP, GATT discovery and HID reports all worked; the page still never
turned, and chasing why turned up five defects.
Fixes
BleButtonMapActivity::onEnter()wiped every mapping. It cleared the wholebleKeyMapand saved it before the user did anything, so opening Map RemoteButtons — including opening it only to read the binding list this very activity
renders — destroyed all existing mappings, with no confirmation and no undo. It
also made that list permanently empty. Nothing needed the wipe:
assignCapturedKey()already drops any other key bound to the action it isassigning and reuses the slot of a re-captured key.
Keys arriving during function selection were latched and reused. The host
synthesizes auto-repeats for a held key, so a repeat landing while the user was
choosing a function was still in the buffer on return to
WaitForKeyand gotconsumed as the next button they "pressed".
pollBle()kept the last key of a frame, not the first. When two reportsarrived in one frame the key the user actually pressed was overwritten.
"BT Connecting…" could hold the book title forever. The reader replaced the
status-bar title whenever Bluetooth was enabled and not connected, unbounded. A
bonded remote that has gone to sleep stops advertising until a button is pressed
on it, so the title was held hostage for a connection that was not in progress.
Now requires the host to actually be running with a bond, and gives the title
back after a bounded window.
EpubReaderMenuActivityhad the mirror bug,reporting "connecting" when the lifecycle had deliberately stopped the stack —
that state is PAUSED.
A remote that has never been mapped is inert.
bleKeyMaphas no defaults andpollBle()drops anything it cannot resolve, so keys were decoded correctly andthen discarded — which presents as broken hardware rather than an unconfigured
one. This is what actually blocked the page turn. Standard navigation keys
(arrows, page up/down, enter, escape) now have out-of-the-box bindings, consulted
only for keys the user has not bound, so explicit mappings still win. Vendor and
consumer-page codes carry no portable meaning and remain capture-and-assign.
This also addresses the request in crosspoint-reader#2418 to "auto remap some known keys — mainly
arrows and enter".
Tests
test/sim/adds a bench that runs the real image against a simulated peer, withboth remote encodings parameterised into every test — they take different routes
through
BleKeyboardHost::onReportIngestand only the first was ever exercised:path, surfaces as a
SpecialKey, so it must now page with no mapping.feat: Adds Bluetooth page turner support (not to be merged) crosspoint-reader/crosspoint-reader#2418: 3-byte reports, top
20 00 00, bottom04 00 00, release00 00 00.Neither keyboard usages nor consumer usage IDs; reaches the generic fallback where
the identity is the first non-zero byte.
The Report Map assertion is load-bearing beyond discovery: the map exceeds one ATT
response at the default MTU, so a regression in Read Blob continuation fails there
rather than silently.
Payload and board model are simulator artifacts not in this repo, so the tests skip
in a bare checkout instead of failing.
Not fixed here
The other things this run surfaced, left alone deliberately: leaving the Bluetooth
screen tears the whole NimBLE stack down (
BluetoothSettingsActivityis the onlyactivity with
keepsBluetoothAlive()), so the remote drops on every trip to thehome screen; and the 2-3 s press-to-turn latency and the OOM
abort()s othersreported on crosspoint-reader#2418 are untouched.