fix(gatt): honour the requested ATT MTU, and declare it as a connect option - #139
Open
Apollon77 wants to merge 2 commits into
Open
fix(gatt): honour the requested ATT MTU, and declare it as a connect option#139Apollon77 wants to merge 2 commits into
Apollon77 wants to merge 2 commits into
Conversation
…option The HCI bindings already read `mtu` from the connect parameters and pass it to `Gatt` as the desired ATT MTU, but two things stopped it from being usable. `exchangeMtu` stored the peer's response verbatim. Core Spec Vol 3 Part F 3.4.2.2 defines ATT_MTU as the lower of the two RX MTUs, and the response carries the peer's own value rather than one already reduced to what was asked for. Requesting less than the peer offers therefore had no effect: a client that asked for 23 and met a peripheral answering 247 ended up using 247. This only stayed invisible because the common case requests 256 and peripherals answer below it, so the peer's value happened to be the smaller one already. The requested value also reached `writeUInt16LE` unvalidated, where a negative or oversized number throws on the connection-complete path and a non-numeric one silently becomes an MTU of zero. It is now range checked once, and a value that cannot be requested falls back to the default with a debug line. With those fixed the option is worth declaring, so `ConnectOptions` gains `mtu`, and `Peripheral.connect` and `Peripheral.connectAsync` gain the options parameter they have always accepted at runtime. The callback-only overload is declared first so existing calls keep resolving to it. The declaration records that this is honoured by the HCI bindings alone. CoreBluetooth exposes `maximumWriteValueLengthForType:`, WinRT exposes `GattSession.MaxPduSize` and BlueZ exposes neither over D-Bus; all three negotiate the MTU themselves and only report the result, so there is nothing for the other backends to request. One existing test asserted that a peer answering 0x3312 raised the MTU to 13074, which is the behaviour this corrects; it now expects the requested 256. Refs stoprocent#138 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Apollon77
force-pushed
the
fix/connect-options-mtu-typings
branch
from
August 16, 2026 13:04
01bd151 to
7c99a1a
Compare
…rface bindings warnings BlueZ negotiates the ATT MTU itself and exposes no way to request one, so the dbus backend ignored the `mtu` connect option silently. It now warns, the way it already does for `setScanParameters`, `setAddress` and `broadcast`. That pattern was not reaching anyone, though: `warning` was the one bindings event `Noble._registerListeners` did not forward, so five existing warnings from this backend were emitted onto an object no consumer holds. Forwarded now, which also makes the constructor's `console.warn` handler apply to them. CoreBluetooth and WinRT cannot warn the same way — their bindings are native and have no JavaScript seam at the connect call. Refs stoprocent#138 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
|
please rebase |
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
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.
This started as the declaration-only change described in #138, but an adversarial review of it turned up a runtime bug that made the option worth declaring only after it was fixed. The PR now does both.
The option did not actually work
exchangeMtustored the peer's response verbatim:Core Spec Vol 3 Part F 3.4.2.2 defines ATT_MTU as the lower of the two RX MTUs, and the response carries the peer's own value rather than one already reduced to what was requested. So requesting less than the peer offers had no effect: ask for 23, meet a peripheral answering 247, and the link used 247. This stayed invisible because the common case requests 256 and peripherals answer below it, so the peer's value was already the smaller one — every capture I have looks correct for that reason alone.
Fixed with
Math.min(this._desired_mtu, ...). For anyone requesting the 256 default against a peripheral answering below it, behaviour is unchanged.The requested value was unvalidated
It reached
writeUInt16LEraw, so a typed-legalnumbercould break a connection:exchangeMturuns from the connection-complete path, so a throw there is not recoverable for that connect. It is now range checked once in one place, and anything unusable falls back to the 256 default with a debug line rather than being sent or thrown.Declarations
ConnectOptionsgainsmtu, andPeripheral.connect/Peripheral.connectAsyncgain the options parameter they have always accepted at runtime (lib/peripheral.js:36-58already handles both(callback)and(options, callback)). The callback-only overload is declared first so existing calls keep resolving to it, andmtuis optional, so nothing that compiles today stops compiling.The doc records that only the HCI bindings honour it, and why: CoreBluetooth exposes
maximumWriteValueLengthForType:, WinRT exposesGattSession.MaxPduSize, and BlueZ exposes nothing equivalent over D-Bus — all three negotiate the MTU themselves and only report the result, so there is nothing for the other backends to request. If you would rather the other backends warned when handed an option they cannot honour, the waysetScanParametersdoes on dbus, I am happy to add that; I left it out here to keep this off the same file as #137.One existing test changed
exchangeMtu ATT_OP_MTU_RESPasserted that a peer answering0x3312raised the MTU to 13074. That is the behaviour being corrected, so it now expects the requested 256. Flagging it explicitly rather than burying it in the diff.Verification
Six new tests: peer smaller than requested, peer larger than requested, the request buffer for an explicit 23, the default when nothing is asked, accepted boundary values, and rejection of unusable ones. I checked each production hunk by reverting it and confirming a named test fails —
Math.min, the validation call, the range bounds and the integer check each have a test that goes red without them.Local full suite: 19 suites, 828 passed, 1 expected skip. Lint clean.
tsc --noEmit --strictonindex.d.tsis clean.Not verified on hardware. The
Math.minchange only alters behaviour when a peripheral answers above the requested MTU, which I have no capture of.Refs #138
Second commit: not silently ignoring it elsewhere
Since this makes
mtua documented, typed option while only one backend can honour it, the dbus backend now warns instead of dropping it, using the same shape it already has forsetScanParameters,setAddressandbroadcast.While wiring that up I found the pattern was not reaching anyone:
warningis the one bindings eventNoble._registerListenersdoes not forward, so the five warnings this backend already emits went to an object no consumer holds. That is forwarded now, which also brings them under theconsole.warnhandler the Noble constructor installs. If you would rather not change what reaches consumers, say so and I will drop that hunk — the warning itself is then cosmetic, so I would drop both.CoreBluetooth and WinRT cannot do the same: their bindings are native, with no JavaScript seam at the connect call.
Three more tests, each mutation-proven: the warning fires for
{ mtu }, stays quiet without it, and Noble forwards a bindings warning.Merges cleanly with #137, which touches a different part of the same file — checked with
git merge-tree, not assumed.