From c955d100233793bd78eb77b43293e347129d68f4 Mon Sep 17 00:00:00 2001 From: Claude <> Date: Fri, 11 Sep 2026 10:59:49 -0300 Subject: [PATCH 1/5] fix: reuse cleared profile slots and prefer custom profiles by name Firmware sets the ModeConfig 'configured' bit on first write and never clears it, so a deleted slot kept it and find_free_slot skipped the slot forever: "No free profile slot available" with two blank slots on screen. Python and Rust required !configured; C++ already tested the name sentinel only. Align all three on the name test. Profile lookup by name returned the first match, which is the preset when a custom profile shares its name. That made such a profile impossible to edit or delete. Prefer an editable slot, fall back to the preset so the existing "cannot delete preset" error still fires. Mark QC45 verified on hardware and record two firmware behaviours found while testing: wind block and CNC level are mutually exclusive in a ModeConfig write, and a 40-byte payload is rejected. Co-authored-by: Gabriel Pita --- README.md | 6 +-- cpp/src/connection.h | 22 ++++++---- python/pybmap/connection.py | 44 +++++++++++-------- python/pybmap/devices/qc45.py | 16 +++++-- python/tests/test_connection.py | 78 ++++++++++++++++++++++++++++++++- rust/src/connection.rs | 13 ++++-- 6 files changed, 141 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 5bf116b..ecf71b0 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ connection to the headphones. | **QuietComfort Headphones** | CNC 0-10 + Wind Block via ModeConfig | — | field observed | 2 user slots observed | — | Verified (`prince`) | | **QuietComfort 35 / 35 II** | ANR off/high/wind/low | — | — | — | Action remap (VPA/ANC) | Verified | | **QuietComfort Earbuds** | CNC 0-10 via direct SETGET | 3-band | — | 4 fixed modes | Remap | Verified (`lando`) | -| **QuietComfort 45** | CNC 0-10 via ModeConfig | 3-band | — | 2 user slots | Remap | Inferred (`duran`), untested on hardware | +| **QuietComfort 45** | CNC 0-10 via ModeConfig | 3-band | — | 2 user slots | Remap | Verified (`duran`) | | **Ultra Open Earbuds** | — (open-ear) | 3-band | — | switch only | — | Partial (`serena`), from device report | ### Device Roadmap @@ -114,8 +114,8 @@ pybmap.modalias(0x4082) # "bluetooth:v05A7p4082d0000" # Check support status pybmap.is_supported(0x4082) # True — has tested config pybmap.is_supported(0x4075) # True — QuietComfort Headphones (prince) -pybmap.is_supported(0x4039) # False — QC45, recognized but untested -pybmap.supported_devices() # [wolfcastle, baywolf, edith, prince, wolverine] +pybmap.is_supported(0x4039) # True — QuietComfort 45 (duran) +pybmap.supported_devices() # [wolfcastle, baywolf, duran, prince, wolverine, lando, edith, serena] pybmap.known_devices() # full catalog ``` diff --git a/cpp/src/connection.h b/cpp/src/connection.h index dadd391..88cf0cb 100644 --- a/cpp/src/connection.h +++ b/cpp/src/connection.h @@ -262,17 +262,21 @@ class BmapConnection { void delete_profile(const std::string& name) { auto all = modes(); + // Prefer an editable slot: a custom profile may share a preset's name, + // and matching the preset first makes that profile undeletable. + const ModeConfig* match = nullptr; for (auto& m : all) { - if (m.name == name) { - if (!m.editable) throw std::runtime_error("Cannot delete preset: " + name); - ModeConfig mc{}; - mc.mode_idx = m.mode_idx; - mc.name = "None"; - write_mode(m.mode_idx, mc); - return; - } + if (m.name != name) continue; + if (m.editable) { match = &m; break; } + if (!match) match = &m; } - throw std::runtime_error("Profile not found: " + name); + if (!match) throw std::runtime_error("Profile not found: " + name); + if (!match->editable) throw std::runtime_error("Cannot delete preset: " + name); + + ModeConfig mc{}; + mc.mode_idx = match->mode_idx; + mc.name = "None"; + write_mode(match->mode_idx, mc); } std::vector send_raw(const std::vector& data) { diff --git a/python/pybmap/connection.py b/python/pybmap/connection.py index 7ee51d4..191ddec 100644 --- a/python/pybmap/connection.py +++ b/python/pybmap/connection.py @@ -506,12 +506,7 @@ def create_profile(self, name, cnc_level=0, spatial=0, def update_profile(self, name, **settings): """Update an existing custom profile by name.""" - modes = self.modes() - found = None - for idx, config in modes.items(): - if config.name.lower() == name.lower(): - found = (idx, config) - break + found = self._find_profile(name) if found is None: raise BmapError("Profile '%s' not found" % name) idx, config = found @@ -519,14 +514,23 @@ def update_profile(self, name, **settings): raise BmapError("Cannot modify preset mode '%s'" % name) self._write_mode_from_config(idx, config, **settings) + def _find_profile(self, name): + """Look up a profile by name, preferring editable slots. + + A custom profile may carry the same name as a preset. Returning the + preset first makes that custom profile impossible to edit or delete. + """ + modes = self.modes() + matches = [(idx, cfg) for idx, cfg in sorted(modes.items()) + if cfg.name.lower() == name.lower()] + for idx, cfg in matches: + if cfg.editable: + return (idx, cfg) + return matches[0] if matches else None + def delete_profile(self, name): """Delete a custom profile by resetting its slot.""" - modes = self.modes() - found = None - for idx, config in modes.items(): - if config.name.lower() == name.lower(): - found = (idx, config) - break + found = self._find_profile(name) if found is None: raise BmapError("Profile '%s' not found" % name) idx, config = found @@ -607,13 +611,17 @@ def _current_mode_config(self): return modes[idx] def _find_free_slot(self, modes): - """Find the first unconfigured editable slot.""" + """Find the first free editable slot. + + A slot is free when its name is the "None" sentinel or blank. The + 'configured' bit is not part of the test: firmware sets it on first + write and never clears it, so a deleted slot keeps it and would + otherwise stay unusable. Matches find_free_slot() in the C++ library. + """ for idx in self._device.EDITABLE_SLOTS: - if idx in modes: - config = modes[idx] - if not config.configured and config.name.lower() in ("none", ""): - return idx - else: + if idx not in modes: + return idx + if modes[idx].name.strip().lower() in ("none", ""): return idx return None diff --git a/python/pybmap/devices/qc45.py b/python/pybmap/devices/qc45.py index afc0346..99745bb 100644 --- a/python/pybmap/devices/qc45.py +++ b/python/pybmap/devices/qc45.py @@ -3,11 +3,19 @@ Codename "duran", product ID 0x4039, CSR8670 platform. RFCOMM channel 8, requires INIT_PACKET (0,1) before responding. -Layout inferred from the Bose app's BMAP tables (#21); unverified on -hardware. The 47-byte STATUS / 39-byte SETGET ModeConfig format is shared -with QuietComfort Headphones (prince), whose parsers this module reuses. +Layout was inferred from the Bose app's BMAP tables (#21) and is now +confirmed on hardware, firmware 4.0.4-4360+de6a887. The 47-byte STATUS / +39-byte SETGET ModeConfig format is shared with QuietComfort Headphones +(prince), whose parsers this module reuses. -Capabilities expected from the APK tables: +Wind block and CNC level are mutually exclusive in a ModeConfig write: when +byte 38 (wind) is 1, the firmware stores 0 at byte 42 (cnc) regardless of +what was sent. Set wind block off to set a level. This matches QC35, where +wind is one of the ANR [1.6] enum values rather than a separate flag. + +A 40-byte ModeConfig payload is rejected with ERROR Length (01). 39 is correct. + +Capabilities confirmed on hardware: - Battery, firmware, serial, product name: GET works - Device name, sidetone, voice prompts: GET + SETGET works - Buttons: GET + SETGET works (Shortcut button with SwitchDevice action) diff --git a/python/tests/test_connection.py b/python/tests/test_connection.py index c22b3f1..cf485b2 100644 --- a/python/tests/test_connection.py +++ b/python/tests/test_connection.py @@ -5,7 +5,8 @@ from pybmap.protocol import bmap_packet from pybmap.constants import OP_GET, OP_SETGET, OP_STATUS, OP_RESULT, OP_ERROR from pybmap.errors import BmapError, BmapAuthError, BmapDeviceError -from pybmap.devices import qc_ultra2, qc_prince +from pybmap.devices import qc_ultra2, qc_prince, qc45 +from pybmap.types import ModeConfig class MockTransport: @@ -313,3 +314,78 @@ def test_regex(self): assert _MAC_RE.match("AA:bb:CC:dd:EE:ff") assert not _MAC_RE.match("AA:bb:CC:dd:EE:ff;rm") assert not _MAC_RE.match("AA-bb-CC-dd-EE-ff") + + +def _mode(idx, name, editable=True, configured=True): + """Build a ModeConfig row for slot-selection tests.""" + return ModeConfig( + mode_idx=idx, prompt="NONE", prompt_bytes=(0, 0), name=name, + cnc_level=0, auto_cnc=False, spatial=0, wind_block=False, + anc_toggle=False, editable=editable, configured=configured, + flags="", raw=b"", + ) + + +@pytest.fixture +def qc45_conn(): + return BmapConnection(MockTransport(), qc45) + + +class TestFreeSlot: + """Firmware leaves 'configured' set after a slot is cleared.""" + + def test_cleared_slot_is_reusable(self, qc45_conn): + modes = { + 0: _mode(0, "Quiet", editable=False), + 1: _mode(1, "Aware", editable=False), + 2: _mode(2, "None", configured=True), + 3: _mode(3, "Gym"), + } + assert qc45_conn._find_free_slot(modes) == 2 + + def test_blank_name_is_reusable(self, qc45_conn): + modes = {2: _mode(2, "", configured=True), 3: _mode(3, "Gym")} + assert qc45_conn._find_free_slot(modes) == 2 + + def test_named_slots_are_not_free(self, qc45_conn): + modes = {2: _mode(2, "Gym"), 3: _mode(3, "Commute")} + assert qc45_conn._find_free_slot(modes) is None + + def test_missing_slot_is_free(self, qc45_conn): + assert qc45_conn._find_free_slot({2: _mode(2, "Gym")}) == 3 + + +class TestProfileLookup: + """A custom profile may share a preset's name (issue #29).""" + + def test_prefers_editable_over_preset(self, qc45_conn, monkeypatch): + modes = { + 1: _mode(1, "Aware", editable=False), + 3: _mode(3, "Aware", editable=True), + } + monkeypatch.setattr(qc45_conn, "modes", lambda: modes) + idx, cfg = qc45_conn._find_profile("Aware") + assert idx == 3 and cfg.editable + + def test_delete_targets_custom_not_preset(self, qc45_conn, monkeypatch): + modes = { + 1: _mode(1, "Aware", editable=False), + 3: _mode(3, "Aware", editable=True), + } + monkeypatch.setattr(qc45_conn, "modes", lambda: modes) + written = [] + monkeypatch.setattr(qc45_conn, "_write_mode", + lambda slot, name, **kw: written.append(slot)) + qc45_conn.delete_profile("Aware") + assert written == [3] + + def test_preset_only_match_still_refused(self, qc45_conn, monkeypatch): + monkeypatch.setattr(qc45_conn, "modes", + lambda: {1: _mode(1, "Aware", editable=False)}) + with pytest.raises(BmapError, match="preset"): + qc45_conn.delete_profile("Aware") + + def test_unknown_name_raises(self, qc45_conn, monkeypatch): + monkeypatch.setattr(qc45_conn, "modes", lambda: {3: _mode(3, "Gym")}) + with pytest.raises(BmapError, match="not found"): + qc45_conn.delete_profile("Nope") diff --git a/rust/src/connection.rs b/rust/src/connection.rs index 08ce3e3..c7a77b4 100644 --- a/rust/src/connection.rs +++ b/rust/src/connection.rs @@ -525,8 +525,11 @@ impl BmapConnection { /// Delete a custom profile by name. pub fn delete_profile(&self, name: &str) -> BmapResult<()> { let modes = self.modes()?; + // Prefer an editable slot: a custom profile may share a preset's name, + // and matching the preset first makes that profile undeletable. let mc = modes.iter() - .find(|m| m.name.eq_ignore_ascii_case(name)) + .find(|m| m.editable && m.name.eq_ignore_ascii_case(name)) + .or_else(|| modes.iter().find(|m| m.name.eq_ignore_ascii_case(name))) .ok_or_else(|| BmapError::InvalidArg(format!("Profile '{}' not found", name)))?; if !mc.editable { return Err(BmapError::InvalidArg(format!("Cannot delete preset '{}'", name))); @@ -544,10 +547,14 @@ impl BmapConnection { // ── Internal Helpers ──────────────────────────────────────────────────── fn find_free_slot(&self, modes: &[ModeConfig]) -> BmapResult { + // A slot is free when its name is the "None" sentinel or blank. The + // `configured` bit is not part of the test: firmware sets it on first + // write and never clears it, so a deleted slot keeps it and would + // otherwise stay unusable. Matches find_free_slot() in the C++ library. for &slot in self.config.editable_slots { match modes.iter().find(|m| m.mode_idx == slot) { - Some(m) if !m.configured && m.name.eq_ignore_ascii_case("none") => return Ok(slot), - Some(m) if !m.configured && m.name.is_empty() => return Ok(slot), + Some(m) if m.name.trim().is_empty() => return Ok(slot), + Some(m) if m.name.trim().eq_ignore_ascii_case("none") => return Ok(slot), None => return Ok(slot), _ => continue, } From fd67037faef047cd74d047aba40c84f37252c169 Mon Sep 17 00:00:00 2001 From: Claude <> Date: Fri, 11 Sep 2026 11:17:10 -0300 Subject: [PATCH 2/5] fix: reject a response that came from a different address After the headset drops and reconnects, responses queued before the drop are still in the socket, so every read returns the previous request's answer. The payload is then handed to the wrong parser and surfaces as plausible looking data rather than an error: a device name of control bytes, a battery of 225, no current mode. Writes still appear to succeed. Measured on a QC45 by sending GETs to 2.2, 0.5 and 1.2 in a loop across a reconnect. 19 of 19 mismatches were exactly one response behind: 0.5 answered by 2.2, 1.2 answered by 0.5, 2.2 answered by 1.2. parse_response already returns the address, so compare it with the request and raise instead of parsing. A matching response is unaffected. set_eq talked to the transport directly and discarded the response, so it skipped this check and swallowed device errors. Route it through setget like every other setter. Co-authored-by: Gabriel Pita --- cpp/src/connection.h | 22 +++++++++++++++++++-- python/pybmap/__init__.py | 2 +- python/pybmap/connection.py | 33 +++++++++++++++++++++++--------- python/pybmap/errors.py | 9 +++++++++ python/tests/test_connection.py | 34 ++++++++++++++++++++++++++++++++- rust/src/connection.rs | 19 ++++++++++++++++++ rust/src/error.rs | 3 +++ 7 files changed, 109 insertions(+), 13 deletions(-) diff --git a/cpp/src/connection.h b/cpp/src/connection.h index 88cf0cb..b4efec1 100644 --- a/cpp/src/connection.h +++ b/cpp/src/connection.h @@ -229,7 +229,7 @@ class BmapConnection { auto pkt = bmap_packet(addr.fblock, addr.func, Operator::SetGet, payload); auto data = transport_->send_recv(pkt); auto resp = parse_response(data); - if (resp) check_error(*resp); + if (resp) { check_address(*resp, addr); check_error(*resp); } auto result = parse_buttons(resp ? resp->payload : std::vector{}); if (!result) throw std::runtime_error("Could not parse button remap response"); return *result; @@ -293,11 +293,28 @@ class BmapConnection { return *opt; } + // Reject a response that came from a different address. + // + // The firmware answers in order, so after a reconnect the socket can still + // hold responses queued before the drop and every read returns the previous + // request's answer. Without this check the payload is handed to the wrong + // parser and surfaces as plausible-looking data. + void check_address(const BmapResponse& resp, Addr addr) { + if (resp.fblock != addr.fblock || resp.func != addr.func) { + throw std::runtime_error( + "Response came from [" + std::to_string(resp.fblock) + "." + + std::to_string(resp.func) + "], expected [" + + std::to_string(addr.fblock) + "." + std::to_string(addr.func) + + "]. Reopen the connection."); + } + } + std::vector get(Addr addr) { auto pkt = bmap_packet(addr.fblock, addr.func, Operator::Get); auto data = transport_->send_recv(pkt); auto resp = parse_response(data); if (!resp) throw std::runtime_error("Empty response"); + check_address(*resp, addr); check_error(*resp); return resp->payload; } @@ -306,7 +323,7 @@ class BmapConnection { auto pkt = bmap_packet(addr.fblock, addr.func, Operator::SetGet, payload); auto data = transport_->send_recv(pkt); auto resp = parse_response(data); - if (resp) check_error(*resp); + if (resp) { check_address(*resp, addr); check_error(*resp); } } BmapResponse start(Addr addr, const std::vector& payload) { @@ -314,6 +331,7 @@ class BmapConnection { auto data = transport_->send_recv(pkt); auto resp = parse_response(data); if (!resp) throw std::runtime_error("Empty response"); + check_address(*resp, addr); check_error(*resp); return *resp; } diff --git a/python/pybmap/__init__.py b/python/pybmap/__init__.py index 07ca412..636b03c 100644 --- a/python/pybmap/__init__.py +++ b/python/pybmap/__init__.py @@ -25,7 +25,7 @@ ) from .errors import ( BmapError, BmapConnectionError, BmapAuthError, - BmapDeviceError, BmapTimeoutError, BmapNotFoundError, + BmapDeviceError, BmapTimeoutError, BmapNotFoundError, BmapDesyncError, ) from .types import DeviceStatus, ModeConfig, EqBand, ButtonMapping, BmapResponse from .protocol import bmap_packet, parse_response, parse_all_responses diff --git a/python/pybmap/connection.py b/python/pybmap/connection.py index 191ddec..d3d69a1 100644 --- a/python/pybmap/connection.py +++ b/python/pybmap/connection.py @@ -17,7 +17,7 @@ VOICE_LANGUAGES, ) from .protocol import bmap_packet, parse_response, parse_all_responses, fmt_response -from .errors import BmapError, BmapAuthError, BmapDeviceError +from .errors import BmapError, BmapAuthError, BmapDeviceError, BmapDesyncError from .types import DeviceStatus, AudioSettings @@ -53,12 +53,30 @@ def _feature(self, name): ) return features[name] + def _check_address(self, parsed, fblock, func): + """Reject a response that came from a different address. + + The firmware answers in order, so after a reconnect the socket can + still hold responses queued before the drop and every read returns + the previous request's answer. Without this check the payload is + handed to the wrong parser and surfaces as plausible-looking data. + """ + if parsed is None: + return None + if (parsed.fblock, parsed.func) != (fblock, func): + raise BmapDesyncError( + "Response came from [%d.%d], expected [%d.%d]. " + "Reopen the connection." + % (parsed.fblock, parsed.func, fblock, func) + ) + return parsed + def _get(self, feature_name): """Send a GET request and return the parsed payload.""" feat = self._feature(feature_name) fblock, func = feat["addr"] resp = self._transport.send_recv(bmap_packet(fblock, func, OP_GET)) - parsed = parse_response(resp) + parsed = self._check_address(parse_response(resp), fblock, func) if parsed is None: return None if parsed.op == OP_ERROR: @@ -75,7 +93,7 @@ def _setget(self, feature_name, payload): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_SETGET, payload) ) - parsed = parse_response(resp) + parsed = self._check_address(parse_response(resp), fblock, func) if parsed and parsed.op == OP_ERROR: self._raise_error(parsed) return parsed @@ -87,7 +105,7 @@ def _start(self, feature_name, payload=b""): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_START, payload) ) - parsed = parse_response(resp) + parsed = self._check_address(parse_response(resp), fblock, func) if parsed and parsed.op == OP_ERROR: self._raise_error(parsed) return parsed @@ -151,7 +169,7 @@ def mode_idx(self): feat = self._feature("current_mode") fblock, func = feat["addr"] resp = self._transport.send_recv(bmap_packet(fblock, func, OP_GET)) - parsed = parse_response(resp) + parsed = self._check_address(parse_response(resp), fblock, func) if parsed and parsed.payload: return parsed.payload[0] return None @@ -347,14 +365,11 @@ def set_eq(self, bass=0, mid=0, treble=0): """Set 3-band equalizer (-10 to +10 each).""" feat = self._feature("eq") builder = feat.get("builder") - fblock, func = feat["addr"] for band_id, val in enumerate([bass, mid, treble]): if not -10 <= val <= 10: raise ValueError("EQ value must be -10 to +10") payload = builder(val, band_id) if builder else bytes([val & 0xFF, band_id]) - self._transport.send_recv( - bmap_packet(fblock, func, OP_SETGET, payload) - ) + self._setget("eq", payload) def set_spatial(self, mode): """Set spatial audio mode ("off", "room", or "head").""" diff --git a/python/pybmap/errors.py b/python/pybmap/errors.py index 505fc0b..c64f67d 100644 --- a/python/pybmap/errors.py +++ b/python/pybmap/errors.py @@ -27,3 +27,12 @@ class BmapTimeoutError(BmapError): class BmapNotFoundError(BmapError): """No BMAP device found.""" + + +class BmapDesyncError(BmapConnectionError): + """A response carried a different address than the request. + + Seen after the headset drops and reconnects: responses queued before the + drop are still in the socket, so each read returns the previous request's + answer. Reopen the channel to clear it. + """ diff --git a/python/tests/test_connection.py b/python/tests/test_connection.py index cf485b2..6d7d5c2 100644 --- a/python/tests/test_connection.py +++ b/python/tests/test_connection.py @@ -4,7 +4,10 @@ from pybmap.connection import BmapConnection from pybmap.protocol import bmap_packet from pybmap.constants import OP_GET, OP_SETGET, OP_STATUS, OP_RESULT, OP_ERROR -from pybmap.errors import BmapError, BmapAuthError, BmapDeviceError +from pybmap.errors import ( + BmapError, BmapAuthError, BmapDeviceError, + BmapDesyncError, BmapConnectionError, +) from pybmap.devices import qc_ultra2, qc_prince, qc45 from pybmap.types import ModeConfig @@ -389,3 +392,32 @@ def test_unknown_name_raises(self, qc45_conn, monkeypatch): monkeypatch.setattr(qc45_conn, "modes", lambda: {3: _mode(3, "Gym")}) with pytest.raises(BmapError, match="not found"): qc45_conn.delete_profile("Nope") + + +class TestResponseAddressCheck: + """A response from the wrong address must not be parsed as the right one. + + Observed on a QC45 after the headset dropped and reconnected: the socket + still held responses queued before the drop, so every read returned the + previous request's answer. + """ + + def test_mismatched_address_raises(self, mock_dev): + # Ask for battery [2.2], answer with firmware [0.5]. + mock_dev._transport.responses[(2, 2)] = ( + bytes([0, 5, OP_STATUS, 3]) + b"4.0") + with pytest.raises(BmapDesyncError, match=r"\[0\.5\].*expected \[2\.2\]"): + mock_dev.battery() + + def test_matching_address_passes(self, mock_dev): + assert mock_dev.battery() == 80 + + def test_setget_checks_address_too(self, mock_dev): + mock_dev._transport.responses[(1, 7)] = ( + bytes([2, 2, OP_STATUS, 1]) + bytes([42])) + with pytest.raises(BmapDesyncError): + mock_dev.set_eq(1, 2, 3) + + def test_desync_is_a_connection_error(self): + # Callers that already retry on connection loss should retry on this. + assert issubclass(BmapDesyncError, BmapConnectionError) diff --git a/rust/src/connection.rs b/rust/src/connection.rs index c7a77b4..86d60ab 100644 --- a/rust/src/connection.rs +++ b/rust/src/connection.rs @@ -34,11 +34,28 @@ impl BmapConnection { )) } + /// Reject a response that came from a different address. + /// + /// The firmware answers in order, so after a reconnect the socket can + /// still hold responses queued before the drop and every read returns + /// the previous request's answer. Without this check the payload is + /// handed to the wrong parser and surfaces as plausible-looking data. + fn check_address(&self, resp: &BmapResponse, addr: Addr) -> BmapResult<()> { + if resp.fblock != addr.0 || resp.func != addr.1 { + return Err(BmapError::Desync(format!( + "Response came from [{}.{}], expected [{}.{}]. Reopen the connection.", + resp.fblock, resp.func, addr.0, addr.1 + ))); + } + Ok(()) + } + fn get(&self, addr: Addr) -> BmapResult> { let pkt = bmap_packet(addr.0, addr.1, Operator::Get, &[]); let data = self.transport.send_recv(&pkt)?; let resp = parse_response(&data) .ok_or_else(|| BmapError::Timeout("Empty response".into()))?; + self.check_address(&resp, addr)?; self.check_error(&resp)?; Ok(resp.payload) } @@ -48,6 +65,7 @@ impl BmapConnection { let data = self.transport.send_recv(&pkt)?; let resp = parse_response(&data) .ok_or_else(|| BmapError::Timeout("Empty response".into()))?; + self.check_address(&resp, addr)?; self.check_error(&resp)?; Ok(resp) } @@ -57,6 +75,7 @@ impl BmapConnection { let data = self.transport.send_recv(&pkt)?; let resp = parse_response(&data) .ok_or_else(|| BmapError::Timeout("Empty response".into()))?; + self.check_address(&resp, addr)?; self.check_error(&resp)?; Ok(resp) } diff --git a/rust/src/error.rs b/rust/src/error.rs index 7573270..96a5155 100644 --- a/rust/src/error.rs +++ b/rust/src/error.rs @@ -22,12 +22,15 @@ pub enum BmapError { Unsupported(String), /// Invalid argument. InvalidArg(String), + /// A response carried a different address than the request. + Desync(String), } impl fmt::Display for BmapError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::Connection(msg) => write!(f, "Connection error: {}", msg), + Self::Desync(msg) => write!(f, "Out of sync: {}", msg), Self::Auth(msg) => write!(f, "Authentication required: {}", msg), Self::Device { message, code } => write!(f, "Device error {}: {}", code, message), Self::Timeout(msg) => write!(f, "Timeout: {}", msg), From f69ba8cb0f8e4bf01badc8274b5af89505e58118 Mon Sep 17 00:00:00 2001 From: Aaron Bockelie Date: Mon, 28 Sep 2026 12:15:33 -0500 Subject: [PATCH 3/5] Bring Rust and C++ in line with the PR's Python fixes Mirror the free-slot, profile-lookup and address-check tests in Rust and C++. Route set_eq through setget and Rust set_mode through start() so every reply is address-checked. Match C++ find_free_slot and profile lookup to Python/Rust (name-only, case-insensitive, trimmed). GET, SETGET and START now reject an invalid or empty reply with the same device error in all three. C++ gains bmap::device_error and bmap::desync_error. create_profile refuses preset names. --- cpp/src/bmap.h | 1 + cpp/src/connection.h | 119 +++++++++++------ cpp/src/errors.h | 36 +++++ cpp/tests/test_connection.cpp | 145 ++++++++++++++++++++ docs/architecture.md | 15 ++- python/pybmap/connection.py | 79 +++++++---- python/tests/test_connection.py | 65 ++++++++- rust/src/connection.rs | 227 ++++++++++++++++++++++++++++---- 8 files changed, 595 insertions(+), 92 deletions(-) create mode 100644 cpp/src/errors.h diff --git a/cpp/src/bmap.h b/cpp/src/bmap.h index 7c3f361..b4ec2c0 100644 --- a/cpp/src/bmap.h +++ b/cpp/src/bmap.h @@ -2,6 +2,7 @@ // See docs/protocol.md for the protocol specification. #pragma once +#include "errors.h" #include "protocol.h" #include "transport.h" #include "device.h" diff --git a/cpp/src/connection.h b/cpp/src/connection.h index 5ee610a..69d1759 100644 --- a/cpp/src/connection.h +++ b/cpp/src/connection.h @@ -3,12 +3,14 @@ #include #include +#include #include #include #include #include #include "device.h" +#include "errors.h" #include "protocol.h" #include "transport.h" @@ -17,6 +19,28 @@ namespace bmap { /// The device name field is 32 bytes on every BMAP device seen so far. inline constexpr size_t MAX_NAME_BYTES = 31; +namespace detail { + +inline std::string trim_lower(const std::string& s) { + auto b = s.find_first_not_of(" \t\r\n"); + if (b == std::string::npos) return ""; + auto e = s.find_last_not_of(" \t\r\n"); + std::string out = s.substr(b, e - b + 1); + for (auto& c : out) c = static_cast(std::tolower(static_cast(c))); + return out; +} + +inline bool iequals(const std::string& a, const std::string& b) { + if (a.size() != b.size()) return false; + for (size_t i = 0; i < a.size(); i++) { + if (std::tolower(static_cast(a[i])) != + std::tolower(static_cast(b[i]))) return false; + } + return true; +} + +} // namespace detail + class BmapConnection { public: BmapConnection(std::unique_ptr transport, DeviceConfig config) @@ -148,15 +172,15 @@ class BmapConnection { auto addr = require(config_.current_mode, "current_mode"); uint8_t idx = 255; for (auto& [n, m] : config_.preset_modes) { - if (n == name) { idx = m.idx; break; } + if (detail::iequals(n, name)) { idx = m.idx; break; } } if (idx == 255) { auto all = modes(); for (auto& m : all) { - if (m.name == name) { idx = m.mode_idx; break; } + if (detail::iequals(m.name, name)) { idx = m.mode_idx; break; } } } - if (idx == 255) throw std::runtime_error("Unknown mode: " + name); + if (idx == 255) throw std::invalid_argument("Unknown mode: " + name); start(addr, {idx, static_cast(announce ? 1 : 0)}); } @@ -203,8 +227,7 @@ class BmapConnection { void set_eq(int8_t bass, int8_t mid, int8_t treble) { auto addr = require(config_.eq, "eq"); for (auto [band_id, val] : std::vector>{{0,bass},{1,mid},{2,treble}}) { - transport_->send_recv(bmap_packet(addr.fblock, addr.func, Operator::SetGet, - {static_cast(val), band_id})); + setget(addr, {static_cast(val), band_id}); } } @@ -259,13 +282,9 @@ class BmapConnection { ButtonMapping set_buttons(uint8_t button_id, uint8_t event, uint8_t action) { auto addr = require(config_.buttons, "buttons"); - auto payload = build_buttons(button_id, event, action); - auto pkt = bmap_packet(addr.fblock, addr.func, Operator::SetGet, payload); - auto data = transport_->send_recv(pkt); - auto resp = parse_response(data); - if (resp) { check_address(*resp, addr); check_error(*resp); } - auto result = parse_buttons(resp ? resp->payload : std::vector{}); - if (!result) throw std::runtime_error("Could not parse button remap response"); + auto resp = setget(addr, build_buttons(button_id, event, action)); + auto result = parse_buttons(resp.payload); + if (!result) throw device_error("Could not parse button remap response"); return *result; } @@ -282,6 +301,7 @@ class BmapConnection { uint8_t create_profile(const std::string& name, uint8_t cnc = 0, uint8_t spatial = 0, bool wind = true, bool anc = true) { auto all = modes(); + refuse_preset_name(name, all); auto slot = find_free_slot(all); ModeConfig mc{}; mc.mode_idx = slot; @@ -300,12 +320,12 @@ class BmapConnection { // and matching the preset first makes that profile undeletable. const ModeConfig* match = nullptr; for (auto& m : all) { - if (m.name != name) continue; + if (!detail::iequals(m.name, name)) continue; if (m.editable) { match = &m; break; } if (!match) match = &m; } - if (!match) throw std::runtime_error("Profile not found: " + name); - if (!match->editable) throw std::runtime_error("Cannot delete preset: " + name); + if (!match) throw std::invalid_argument("Profile '" + name + "' not found"); + if (!match->editable) throw std::invalid_argument("Cannot delete preset '" + name + "'"); ModeConfig mc{}; mc.mode_idx = match->mode_idx; @@ -335,7 +355,7 @@ class BmapConnection { // parser and surfaces as plausible-looking data. void check_address(const BmapResponse& resp, Addr addr) { if (resp.fblock != addr.fblock || resp.func != addr.func) { - throw std::runtime_error( + throw desync_error( "Response came from [" + std::to_string(resp.fblock) + "." + std::to_string(resp.func) + "], expected [" + std::to_string(addr.fblock) + "." + std::to_string(addr.func) + @@ -343,31 +363,31 @@ class BmapConnection { } } - std::vector get(Addr addr) { - auto pkt = bmap_packet(addr.fblock, addr.func, Operator::Get); - auto data = transport_->send_recv(pkt); + // Validate a single reply: present, from the right address, not ERROR. + // Every single-reply path (GET, SETGET, START) goes through here so a + // truncated frame, a desynced socket or a device error surface the same + // way in all of them. + BmapResponse check_reply(const std::vector& data, Addr addr) { auto resp = parse_response(data); - if (!resp) throw std::runtime_error("Invalid or empty response"); + if (!resp) throw device_error("Invalid or empty response"); check_address(*resp, addr); check_error(*resp); - return resp->payload; + return *resp; + } + + std::vector get(Addr addr) { + auto pkt = bmap_packet(addr.fblock, addr.func, Operator::Get); + return check_reply(transport_->send_recv(pkt), addr).payload; } - void setget(Addr addr, const std::vector& payload) { + BmapResponse setget(Addr addr, const std::vector& payload) { auto pkt = bmap_packet(addr.fblock, addr.func, Operator::SetGet, payload); - auto data = transport_->send_recv(pkt); - auto resp = parse_response(data); - if (resp) { check_address(*resp, addr); check_error(*resp); } + return check_reply(transport_->send_recv(pkt), addr); } BmapResponse start(Addr addr, const std::vector& payload) { auto pkt = bmap_packet(addr.fblock, addr.func, Operator::Start, payload); - auto data = transport_->send_recv(pkt); - auto resp = parse_response(data); - if (!resp) throw std::runtime_error("Empty response"); - check_address(*resp, addr); - check_error(*resp); - return *resp; + return check_reply(transport_->send_recv(pkt), addr); } std::vector start_drain(Addr addr, const std::vector& payload) { @@ -378,7 +398,7 @@ class BmapConnection { void check_error(const BmapResponse& resp) { if (resp.op == Operator::Error && !resp.payload.empty()) { - throw std::runtime_error(resp.fmt()); + throw device_error(resp.fmt(), resp.payload[0]); } } @@ -441,18 +461,39 @@ class BmapConnection { return {slot, mc}; } + // A slot is free when its name is the "None" sentinel or blank. The + // `configured` bit is not part of the test: firmware sets it on first + // write and never clears it, so a deleted slot keeps it and would + // otherwise stay unusable. Same rule as the Python and Rust libraries. uint8_t find_free_slot(const std::vector& all) { for (auto slot : config_.editable_slots) { - bool found = false; + const ModeConfig* row = nullptr; for (auto& m : all) { - if (m.mode_idx == slot && m.configured && m.name != "None") { - found = true; - break; - } + if (m.mode_idx == slot) { row = &m; break; } } - if (!found) return slot; + if (!row) return slot; + auto name = detail::trim_lower(row->name); + if (name.empty() || name == "none") return slot; + } + throw device_error("No free profile slot available"); + } + + // Refuse a custom profile name that matches a preset (any case). Mode + // switching resolves preset names first, so a custom profile named like + // a preset can never be selected by name. + void refuse_preset_name(const std::string& name, const std::vector& all) { + auto wanted = detail::trim_lower(name); + bool is_preset = false; + for (auto& [n, _] : config_.preset_modes) { + if (detail::trim_lower(n) == wanted) is_preset = true; + } + for (auto& m : all) { + if (!m.editable && detail::trim_lower(m.name) == wanted) is_preset = true; + } + if (is_preset) { + throw std::invalid_argument( + "'" + name + "' is a preset mode name; choose a different profile name"); } - throw std::runtime_error("No free profile slot available"); } ModeConfig current_mode_config() { diff --git a/cpp/src/errors.h b/cpp/src/errors.h new file mode 100644 index 0000000..8b6210e --- /dev/null +++ b/cpp/src/errors.h @@ -0,0 +1,36 @@ +// Exception types for BMAP protocol errors. +// +// Both derive from std::runtime_error, so callers that catch +// std::runtime_error or std::exception keep working. +#pragma once + +#include +#include +#include + +namespace bmap { + +/// The device answered with an ERROR, or the reply was invalid or empty. +/// Mirrors BmapDeviceError (Python) and BmapError::Device (Rust). +class device_error : public std::runtime_error { +public: + explicit device_error(const std::string& message, uint8_t code = 0) + : std::runtime_error(message), code_(code) {} + uint8_t code() const noexcept { return code_; } + +private: + uint8_t code_; +}; + +/// A response carried a different address than the request. +/// +/// Seen after the headset drops and reconnects: responses queued before the +/// drop are still in the socket, so each read returns the previous request's +/// answer. Reopen the connection to clear it. Mirrors BmapDesyncError +/// (Python) and BmapError::Desync (Rust). +class desync_error : public std::runtime_error { +public: + using std::runtime_error::runtime_error; +}; + +} // namespace bmap diff --git a/cpp/tests/test_connection.cpp b/cpp/tests/test_connection.cpp index 7f55523..3a61183 100644 --- a/cpp/tests/test_connection.cpp +++ b/cpp/tests/test_connection.cpp @@ -392,3 +392,148 @@ TEST(prince_set_anc_rejects_missing_toggle) { } ASSERT_TRUE(threw); } + +// ── Free slot / profile lookup / address check (mirror Python) ────────────── + +// A [31.6] STATUS frame in the 47-byte prince/QC45 ModeConfig layout. +static std::vector mode_frame(uint8_t idx, const std::string& name, bool editable) { + std::vector p(47, 0); + p[0] = idx; + p[3] = editable ? 1 : 0; + p[4] = 1; // configured: firmware never clears it + std::copy(name.begin(), name.end(), p.begin() + 6); + std::vector f = {31, 6, 0x03, static_cast(p.size())}; + f.insert(f.end(), p.begin(), p.end()); + return f; +} + +struct Qc45Modes { + MockTransport* raw; + std::unique_ptr dev; + + explicit Qc45Modes(const std::vector>& frames) { + raw = new MockTransport(); + std::vector all; + for (auto& f : frames) all.insert(all.end(), f.begin(), f.end()); + raw->responses[{31, 1}] = all; + raw->add(31, 6, 0x03, {0}); // ModeConfig write ack + dev = std::make_unique(std::unique_ptr(raw), qc45()); + } + + std::vector written_slots() const { + std::vector out; + for (auto& p : raw->sent) { + if (p.size() > 4 && p[0] == 31 && p[1] == 6 && p[2] == 0x02) out.push_back(p[4]); + } + return out; + } +}; + +template +static bool throws_with(F fn, const std::string& needle = "") { + try { fn(); } + catch (const E& e) { return std::string(e.what()).find(needle) != std::string::npos; } + catch (...) { return false; } + return false; +} + +TEST(free_slot_cleared_slot_is_reusable) { + Qc45Modes m({mode_frame(0, "Quiet", false), mode_frame(1, "Aware", false), + mode_frame(2, "None", true), mode_frame(3, "Gym", true)}); + ASSERT_EQ(m.dev->create_profile("Commute"), 2); + ASSERT_EQ(m.written_slots(), std::vector{2}); +} + +TEST(free_slot_blank_name_is_reusable) { + Qc45Modes m({mode_frame(2, " ", true), mode_frame(3, "Gym", true)}); + ASSERT_EQ(m.dev->create_profile("Commute"), 2); +} + +TEST(free_slot_named_slots_are_not_free) { + Qc45Modes m({mode_frame(2, "Gym", true), mode_frame(3, "Commute", true)}); + ASSERT_TRUE(throws_with([&]{ m.dev->create_profile("Run"); }, + "No free profile slot")); + ASSERT_TRUE(m.written_slots().empty()); +} + +TEST(free_slot_missing_slot_is_free) { + Qc45Modes m({mode_frame(2, "Gym", true)}); + ASSERT_EQ(m.dev->create_profile("Commute"), 3); +} + +TEST(create_profile_refuses_preset_name) { + Qc45Modes m({mode_frame(3, "Gym", true)}); + ASSERT_TRUE(throws_with( + [&]{ m.dev->create_profile(" AWARE"); }, "preset")); + ASSERT_TRUE(m.written_slots().empty()); +} + +TEST(profile_delete_targets_custom_not_preset) { + Qc45Modes m({mode_frame(1, "Aware", false), mode_frame(3, "Aware", true)}); + m.dev->delete_profile("aware"); + ASSERT_EQ(m.written_slots(), std::vector{3}); +} + +TEST(profile_preset_only_match_still_refused) { + Qc45Modes m({mode_frame(1, "Aware", false)}); + ASSERT_TRUE(throws_with( + [&]{ m.dev->delete_profile("Aware"); }, "preset")); + ASSERT_TRUE(m.written_slots().empty()); +} + +TEST(profile_unknown_name_raises) { + Qc45Modes m({mode_frame(3, "Gym", true)}); + ASSERT_TRUE(throws_with( + [&]{ m.dev->delete_profile("Nope"); }, "not found")); +} + +TEST(address_mismatch_raises_desync) { + // Ask for battery [2.2], answer with firmware [0.5]. + auto raw = new MockTransport(); + raw->responses[{2, 2}] = {0, 5, 0x03, 3, '4', '.', '0'}; + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + ASSERT_TRUE(throws_with([&]{ dev.battery(); }, "[0.5], expected [2.2]")); +} + +TEST(address_match_passes) { ASSERT_EQ(mock_qc_ultra2()->battery(), 80); } + +TEST(desync_is_a_runtime_error) { + // Callers that catch std::runtime_error keep catching it. + ASSERT_TRUE(throws_with([]{ throw desync_error("x"); })); +} + +TEST(set_eq_checks_address) { + auto raw = new MockTransport(); + raw->responses[{1, 7}] = {2, 2, 0x03, 1, 42}; + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + ASSERT_TRUE(throws_with([&]{ dev.set_eq(1, 2, 3); })); +} + +TEST(set_eq_surfaces_device_error) { + auto raw = new MockTransport(); + raw->add(1, 7, 0x04, {1}); // ERROR: length + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + bool ok = false; + try { dev.set_eq(1, 2, 3); } catch (const device_error& e) { ok = e.code() == 1; } + ASSERT_TRUE(ok); + ASSERT_EQ(raw->sent.size(), size_t{1}); +} + +TEST(set_mode_checks_address) { + auto raw = new MockTransport(); + raw->responses[{31, 3}] = {2, 2, 0x06, 0}; + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + ASSERT_TRUE(throws_with([&]{ dev.set_mode("aware"); })); +} + +TEST(empty_reply_is_device_error_on_every_path) { + auto raw = new MockTransport(); + raw->responses[{1, 10}] = {1, 10, 0x08, 0}; // unknown op (SETGET) + raw->responses[{31, 3}] = {31, 3, 0x06, 4, 1}; // truncated (START) + raw->responses[{1, 7}] = {}; // nothing (SETGET) + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + const std::string msg = "Invalid or empty response"; + ASSERT_TRUE(throws_with([&]{ dev.set_multipoint(true); }, msg)); + ASSERT_TRUE(throws_with([&]{ dev.set_mode("aware"); }, msg)); + ASSERT_TRUE(throws_with([&]{ dev.set_eq(0, 0, 0); }, msg)); +} diff --git a/docs/architecture.md b/docs/architecture.md index e083a43..98edf8e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -457,8 +457,11 @@ This makes feature dispatch a simple dict lookup at runtime. ``` BmapError ├── BmapConnectionError — socket/transport failures +│ └── BmapDesyncError — reply came from a different [fblock.func] ├── BmapAuthError — device returned error code 5 -├── BmapDeviceError — device returned other error codes +├── BmapDeviceError — device returned other error codes, or an +│ invalid/empty reply to GET, SETGET or START +├── BmapInvalidArgError — caller supplied invalid arguments ├── BmapTimeoutError — no response within timeout └── BmapNotFoundError — no device found during discovery ``` @@ -523,6 +526,7 @@ pub enum BmapError { NotFound(String), Unsupported(String), InvalidArg(String), + Desync(String), // reply came from a different [fblock.func] } ``` @@ -562,9 +566,12 @@ public: Tests use a `MockTransport` subclass. -**Error handling** uses `std::runtime_error` exceptions. No typed -hierarchy — auth errors (code 5) are not programmatically distinguishable -from other device errors without parsing the message string. The `require()` helper +**Error handling** uses standard exceptions plus two typed ones from +`errors.h`, both derived from `std::runtime_error`: `bmap::device_error` +(device ERROR reply, with `code()`, or an invalid/empty reply to GET, SETGET +or START) and `bmap::desync_error` (reply came from a different +`[fblock.func]`). Bad caller input throws `std::invalid_argument`. Auth +errors (code 5) are a `device_error` with `code() == 5`. The `require()` helper converts `std::nullopt` to an exception for unsupported features: ```cpp static Addr require(const std::optional& opt, const char* name) { diff --git a/python/pybmap/connection.py b/python/pybmap/connection.py index 7be8219..7baceb3 100644 --- a/python/pybmap/connection.py +++ b/python/pybmap/connection.py @@ -17,7 +17,10 @@ VOICE_LANGUAGES, ) from .protocol import bmap_packet, parse_response, parse_all_responses, fmt_response -from .errors import BmapError, BmapAuthError, BmapDeviceError, BmapDesyncError +from .errors import ( + BmapError, BmapAuthError, BmapDeviceError, BmapDesyncError, + BmapInvalidArgError, +) from .types import AudioSettings, BatteryStatus, DeviceStatus from .devices import parsers @@ -72,17 +75,26 @@ def _check_address(self, parsed, fblock, func): ) return parsed + def _check_reply(self, parsed, fblock, func): + """Validate a single reply: present, from the right address, not ERROR. + + Every single-reply path (GET, SETGET, START) goes through here so a + truncated frame, a desynced socket or a device error surface the same + way in all of them. + """ + parsed = self._check_address(parsed, fblock, func) + if parsed is None: + raise BmapDeviceError("Invalid or empty response") + if parsed.op == OP_ERROR: + self._raise_error(parsed) + return parsed + def _get_payload(self, feature_name): """Send a GET request and return its raw payload.""" feat = self._feature(feature_name) fblock, func = feat["addr"] resp = self._transport.send_recv(bmap_packet(fblock, func, OP_GET)) - parsed = self._check_address(parse_response(resp), fblock, func) - if parsed is None: - raise BmapDeviceError("Invalid or empty response") - if parsed.op == OP_ERROR: - self._raise_error(parsed) - return parsed.payload + return self._check_reply(parse_response(resp), fblock, func).payload def _get(self, feature_name): """Send a GET request and return its parsed payload.""" @@ -100,10 +112,7 @@ def _setget(self, feature_name, payload): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_SETGET, payload) ) - parsed = self._check_address(parse_response(resp), fblock, func) - if parsed and parsed.op == OP_ERROR: - self._raise_error(parsed) - return parsed + return self._check_reply(parse_response(resp), fblock, func) def _start(self, feature_name, payload=b""): """Send a START request and return the parsed response.""" @@ -112,10 +121,7 @@ def _start(self, feature_name, payload=b""): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_START, payload) ) - parsed = self._check_address(parse_response(resp), fblock, func) - if parsed and parsed.op == OP_ERROR: - self._raise_error(parsed) - return parsed + return self._check_reply(parse_response(resp), fblock, func) def _start_drain(self, feature_name, payload=b""): """Send a START request and drain all responses.""" @@ -370,7 +376,7 @@ def set_mode(self, name, announce=False): resp = self._start("current_mode", bytes([idx, 1 if announce else 0])) # Some firmware (QC Headphones "prince") acks START [31.3] with # PROCESSING and applies the switch asynchronously. - if resp and resp.op not in (OP_RESULT, OP_PROCESSING): + if resp.op not in (OP_RESULT, OP_PROCESSING): raise BmapDeviceError("Mode switch failed: %s" % fmt_response(resp)) def set_cnc(self, level): @@ -527,7 +533,7 @@ def set_buttons(self, button_id, event, action): payload = builder(button_id, event, action) resp = self._setget("buttons", payload) parser = feat.get("parser") - if resp and parser and resp.payload: + if parser and resp.payload: return parser(resp.payload) return resp @@ -543,9 +549,7 @@ def route(self, mac): feat = self._feature("routing") builder = feat.get("builder") payload = builder(mac) - resp = self._start("routing", payload) - if resp and resp.op == OP_ERROR: - self._raise_error(resp) + self._start("routing", payload) def pair(self): """Enter Bluetooth pairing mode.""" @@ -560,6 +564,7 @@ def create_profile(self, name, cnc_level=0, spatial=0, Returns the slot index used. """ modes = self.modes() + self._refuse_preset_name(name, modes) slot = self._find_free_slot(modes) if slot is None: raise BmapError("No free profile slot available") @@ -567,16 +572,43 @@ def create_profile(self, name, cnc_level=0, spatial=0, wind_block=wind_block, anc_toggle=anc_toggle) return slot - def update_profile(self, name, **settings): - """Update an existing custom profile by name.""" + def update_profile(self, name, rename=None, **settings): + """Update an existing custom profile by name. + + Args: + name: Current profile name (case-insensitive). + rename: Optional new name for the profile. + **settings: cnc_level, spatial, wind_block, anc_toggle overrides. + """ + if rename is not None: + settings["name"] = rename found = self._find_profile(name) if found is None: raise BmapError("Profile '%s' not found" % name) idx, config = found if not config.editable: raise BmapError("Cannot modify preset mode '%s'" % name) + new_name = settings.get("name") + if new_name is not None and new_name.strip().lower() != config.name.strip().lower(): + self._refuse_preset_name(new_name, self.modes()) self._write_mode_from_config(idx, config, **settings) + def _refuse_preset_name(self, name, modes): + """Refuse a custom profile name that matches a preset (any case). + + Mode switching resolves preset names first, so a custom profile named + like a preset can never be selected by name. + """ + wanted = name.strip().lower() + presets = {n.lower() for n in getattr(self._device, "PRESET_MODES", {})} + presets |= {cfg.name.strip().lower() for cfg in modes.values() + if not cfg.editable} + if wanted in presets: + raise BmapInvalidArgError( + "'%s' is a preset mode name; choose a different profile name" + % name + ) + def _find_profile(self, name): """Look up a profile by name, preferring editable slots. @@ -694,7 +726,8 @@ def _find_free_slot(self, modes): A slot is free when its name is the "None" sentinel or blank. The 'configured' bit is not part of the test: firmware sets it on first write and never clears it, so a deleted slot keeps it and would - otherwise stay unusable. Matches find_free_slot() in the C++ library. + otherwise stay unusable. Same rule as find_free_slot() in the Rust + and C++ libraries. """ for idx in self._device.EDITABLE_SLOTS: if idx not in modes: diff --git a/python/tests/test_connection.py b/python/tests/test_connection.py index 1bb9854..4e8d68f 100644 --- a/python/tests/test_connection.py +++ b/python/tests/test_connection.py @@ -9,7 +9,7 @@ from pybmap.constants import OP_GET, OP_SETGET, OP_STATUS, OP_RESULT, OP_ERROR from pybmap.errors import ( BmapError, BmapAuthError, BmapDeviceError, - BmapDesyncError, BmapConnectionError, + BmapDesyncError, BmapConnectionError, BmapInvalidArgError, ) from pybmap.devices import qc_ultra2, qc_ultra2_earbuds, qc_prince, qc45 from pybmap.types import ModeConfig @@ -542,3 +542,66 @@ def test_setget_checks_address_too(self, mock_dev): def test_desync_is_a_connection_error(self): # Callers that already retry on connection loss should retry on this. assert issubclass(BmapDesyncError, BmapConnectionError) + + +class TestSetEqResponse: + """set_eq checks each SETGET reply instead of discarding it.""" + + def test_device_error_surfaces_and_stops(self, mock_dev): + mock_dev._transport.add_response(1, 7, OP_ERROR, bytes([1])) + with pytest.raises(BmapDeviceError) as info: + mock_dev.set_eq(1, 2, 3) + assert info.value.error_code == 1 + assert len(mock_dev._transport.sent) == 1 + + def test_success_sends_three_bands(self, mock_dev): + mock_dev.set_eq(1, 2, 3) + assert [p[:3] for p in mock_dev._transport.sent] == [bytes([1, 7, OP_SETGET])] * 3 + + +class TestEmptyReply: + """GET, SETGET and START all reject an invalid or empty reply the same way.""" + + @pytest.mark.parametrize("call, key, response", [ + (lambda d: d.set_multipoint(True), (1, 10), bytes([1, 10, 0x08, 0])), + (lambda d: d.set_eq(0, 0, 0), (1, 7), b""), + (lambda d: d.set_mode("aware"), (31, 3), bytes([31, 3, OP_RESULT, 4, 1])), + (lambda d: d.power_off(), (7, 4), b""), + ]) + def test_raises_device_error(self, mock_dev, call, key, response): + mock_dev._transport.responses[key] = response + with pytest.raises(BmapDeviceError, match="Invalid or empty response"): + call(mock_dev) + + +class TestPresetNameRefused: + """A new custom profile may not take a preset's name (see #29).""" + + def test_create_refuses_preset_name(self, qc45_conn, monkeypatch): + monkeypatch.setattr(qc45_conn, "modes", lambda: {3: _mode(3, "Gym")}) + written = [] + monkeypatch.setattr(qc45_conn, "_write_mode", + lambda slot, name, **kw: written.append(slot)) + with pytest.raises(BmapInvalidArgError, match="preset"): + qc45_conn.create_profile(" AWARE") + assert written == [] + + def test_create_refuses_on_device_preset_name(self, qc45_conn, monkeypatch): + monkeypatch.setattr(qc45_conn, "modes", lambda: { + 1: _mode(1, "Focus", editable=False), 3: _mode(3, "Gym")}) + with pytest.raises(BmapInvalidArgError, match="preset"): + qc45_conn.create_profile("focus") + + def test_rename_to_preset_refused(self, qc45_conn, monkeypatch): + monkeypatch.setattr(qc45_conn, "modes", lambda: {3: _mode(3, "Gym")}) + with pytest.raises(BmapInvalidArgError, match="preset"): + qc45_conn.update_profile("Gym", rename="Quiet") + + def test_create_reuses_cleared_slot(self, qc45_conn, monkeypatch): + monkeypatch.setattr(qc45_conn, "modes", lambda: { + 2: _mode(2, "None"), 3: _mode(3, "Gym")}) + written = [] + monkeypatch.setattr(qc45_conn, "_write_mode", + lambda slot, name, **kw: written.append((slot, name))) + assert qc45_conn.create_profile("Commute") == 2 + assert written == [(2, "Commute")] diff --git a/rust/src/connection.rs b/rust/src/connection.rs index 7b27f0b..63e40b2 100644 --- a/rust/src/connection.rs +++ b/rust/src/connection.rs @@ -50,36 +50,37 @@ impl BmapConnection { Ok(()) } - fn get(&self, addr: Addr) -> BmapResult> { - let pkt = bmap_packet(addr.0, addr.1, Operator::Get, &[]); - let data = self.transport.send_recv(&pkt)?; - let resp = parse_response(&data).ok_or_else(|| BmapError::Device { + /// Validate a single reply: present, from the right address, not ERROR. + /// + /// Every single-reply path (GET, SETGET, START) goes through here so a + /// truncated frame, a desynced socket or a device error surface the same + /// way in all of them. + fn check_reply(&self, data: &[u8], addr: Addr) -> BmapResult { + let resp = parse_response(data).ok_or_else(|| BmapError::Device { message: "Invalid or empty response".into(), code: 0, })?; self.check_address(&resp, addr)?; self.check_error(&resp)?; - Ok(resp.payload) + Ok(resp) + } + + fn get(&self, addr: Addr) -> BmapResult> { + let pkt = bmap_packet(addr.0, addr.1, Operator::Get, &[]); + let data = self.transport.send_recv(&pkt)?; + Ok(self.check_reply(&data, addr)?.payload) } fn setget(&self, addr: Addr, payload: &[u8]) -> BmapResult { let pkt = bmap_packet(addr.0, addr.1, Operator::SetGet, payload); let data = self.transport.send_recv(&pkt)?; - let resp = parse_response(&data) - .ok_or_else(|| BmapError::Timeout("Empty response".into()))?; - self.check_address(&resp, addr)?; - self.check_error(&resp)?; - Ok(resp) + self.check_reply(&data, addr) } fn start(&self, addr: Addr, payload: &[u8]) -> BmapResult { let pkt = bmap_packet(addr.0, addr.1, Operator::Start, payload); let data = self.transport.send_recv(&pkt)?; - let resp = parse_response(&data) - .ok_or_else(|| BmapError::Timeout("Empty response".into()))?; - self.check_address(&resp, addr)?; - self.check_error(&resp)?; - Ok(resp) + self.check_reply(&data, addr) } /// Send START and drain all async responses. @@ -370,12 +371,7 @@ impl BmapConnection { .ok_or_else(|| BmapError::InvalidArg(format!("Unknown mode: {}", name)))? }; - let pkt = bmap_packet(addr.0, addr.1, Operator::Start, - &[idx, if announce { 1 } else { 0 }]); - let data = self.transport.send_recv(&pkt)?; - let resp = parse_response(&data) - .ok_or_else(|| BmapError::Timeout("No response".into()))?; - self.check_error(&resp)?; + let resp = self.start(addr, &[idx, if announce { 1 } else { 0 }])?; // Some firmware (QC Headphones "prince") acks START [31.3] with // PROCESSING and applies the switch asynchronously. if !matches!(resp.op, Operator::Result | Operator::Processing) { @@ -502,9 +498,7 @@ impl BmapConnection { } let addr = self.addr(self.config.eq)?; for (band_id, val) in [(0u8, bass), (1, mid), (2, treble)] { - let payload = [val as u8, band_id]; - let pkt = bmap_packet(addr.0, addr.1, Operator::SetGet, &payload); - self.transport.send_recv(&pkt)?; + self.setget(addr, &[val as u8, band_id])?; } Ok(()) } @@ -587,6 +581,7 @@ impl BmapConnection { pub fn create_profile(&self, name: &str, cnc_level: u8, spatial: u8, wind_block: bool, anc_toggle: bool) -> BmapResult { let modes = self.modes()?; + self.refuse_preset_name(name, &modes)?; let slot = self.find_free_slot(&modes)?; self.write_mode(slot, name, cnc_level, spatial, wind_block, anc_toggle, 0, 0)?; Ok(slot) @@ -616,11 +611,28 @@ impl BmapConnection { // ── Internal Helpers ──────────────────────────────────────────────────── + /// Refuse a custom profile name that matches a preset (any case). + /// + /// Mode switching resolves preset names first, so a custom profile named + /// like a preset can never be selected by name. + fn refuse_preset_name(&self, name: &str, modes: &[ModeConfig]) -> BmapResult<()> { + let wanted = name.trim(); + let is_preset = self.config.preset_modes.iter() + .any(|(n, _)| n.eq_ignore_ascii_case(wanted)) + || modes.iter() + .any(|m| !m.editable && m.name.trim().eq_ignore_ascii_case(wanted)); + if is_preset { + return Err(BmapError::InvalidArg(format!( + "'{}' is a preset mode name; choose a different profile name", name))); + } + Ok(()) + } + fn find_free_slot(&self, modes: &[ModeConfig]) -> BmapResult { // A slot is free when its name is the "None" sentinel or blank. The // `configured` bit is not part of the test: firmware sets it on first // write and never clears it, so a deleted slot keeps it and would - // otherwise stay unusable. Matches find_free_slot() in the C++ library. + // otherwise stay unusable. Same rule as the Python and C++ libraries. for &slot in self.config.editable_slots { match modes.iter().find(|m| m.mode_idx == slot) { Some(m) if m.name.trim().is_empty() => return Ok(slot), @@ -1146,4 +1158,169 @@ mod tests { let dev = BmapConnection::new(t, devices::qc_prince()); assert!(dev.set_anc(false).is_err()); } + + // ── Free slot / profile lookup / address check (mirror Python) ───── + + fn mode(idx: u8, name: &str, editable: bool, configured: bool) -> ModeConfig { + ModeConfig { + mode_idx: idx, name: name.into(), cnc_level: 0, spatial: 0, + wind_block: false, anc_toggle: false, editable, configured, + prompt_b1: 0, prompt_b2: 0, + } + } + + /// A [31.6] STATUS frame in the 47-byte prince/QC45 ModeConfig layout. + fn mode_frame(idx: u8, name: &str, editable: bool) -> Vec { + let mut p = vec![0u8; 47]; + p[0] = idx; + p[3] = editable as u8; + p[4] = 1; // configured + p[6..6 + name.len()].copy_from_slice(name.as_bytes()); + let mut f = vec![31, 6, 0x03, p.len() as u8]; + f.extend_from_slice(&p); + f + } + + fn qc45_with_modes(frames: &[Vec]) -> BmapConnection { + let mut t = MockTransport::new(); + t.responses.insert((31, 1), frames.concat()); + t.add(31, 6, 0x03, &[0]); // ModeConfig write ack + BmapConnection::new(t, devices::qc45()) + } + + fn written_slots(dev: &BmapConnection) -> Vec { + dev.transport.sent.borrow().iter() + .filter(|p| p[..3] == [31, 6, 0x02]) + .map(|p| p[4]) + .collect() + } + + fn qc45() -> BmapConnection { + BmapConnection::new(MockTransport::new(), devices::qc45()) + } + + #[test] + fn test_free_slot_cleared_slot_is_reusable() { + // Firmware leaves 'configured' set after a slot is cleared. + let modes = vec![ + mode(0, "Quiet", false, true), mode(1, "Aware", false, true), + mode(2, "None", true, true), mode(3, "Gym", true, true), + ]; + assert_eq!(qc45().find_free_slot(&modes).unwrap(), 2); + } + + #[test] + fn test_free_slot_blank_name_is_reusable() { + let modes = vec![mode(2, " ", true, true), mode(3, "Gym", true, true)]; + assert_eq!(qc45().find_free_slot(&modes).unwrap(), 2); + } + + #[test] + fn test_free_slot_named_slots_are_not_free() { + let modes = vec![mode(2, "Gym", true, true), mode(3, "Commute", true, true)]; + assert!(qc45().find_free_slot(&modes).is_err()); + } + + #[test] + fn test_free_slot_missing_slot_is_free() { + let modes = vec![mode(2, "Gym", true, true)]; + assert_eq!(qc45().find_free_slot(&modes).unwrap(), 3); + } + + #[test] + fn test_profile_delete_targets_custom_not_preset() { + let dev = qc45_with_modes(&[ + mode_frame(1, "Aware", false), mode_frame(3, "Aware", true), + ]); + dev.delete_profile("aware").unwrap(); + assert_eq!(written_slots(&dev), vec![3]); + } + + #[test] + fn test_profile_preset_only_match_still_refused() { + let dev = qc45_with_modes(&[mode_frame(1, "Aware", false)]); + assert!(matches!(dev.delete_profile("Aware"), + Err(BmapError::InvalidArg(m)) if m.contains("preset"))); + assert!(written_slots(&dev).is_empty()); + } + + #[test] + fn test_profile_unknown_name_raises() { + let dev = qc45_with_modes(&[mode_frame(3, "Gym", true)]); + assert!(matches!(dev.delete_profile("Nope"), + Err(BmapError::InvalidArg(m)) if m.contains("not found"))); + } + + #[test] + fn test_create_profile_refuses_preset_name() { + let dev = qc45_with_modes(&[mode_frame(3, "Gym", true)]); + assert!(matches!(dev.create_profile(" AWARE", 0, 0, false, false), + Err(BmapError::InvalidArg(m)) if m.contains("preset"))); + assert!(written_slots(&dev).is_empty()); + } + + #[test] + fn test_create_profile_reuses_cleared_slot() { + let dev = qc45_with_modes(&[ + mode_frame(2, "None", true), mode_frame(3, "Gym", true), + ]); + assert_eq!(dev.create_profile("Commute", 0, 0, false, false).unwrap(), 2); + assert_eq!(written_slots(&dev), vec![2]); + } + + #[test] + fn test_address_mismatch_raises_desync() { + // Ask for battery [2.2], answer with firmware [0.5]. + let mut t = MockTransport::new(); + let mut resp = vec![0, 5, 0x03, 3]; + resp.extend_from_slice(b"4.0"); + t.responses.insert((2, 2), resp); + let dev = BmapConnection::new(t, devices::qc_ultra2()); + assert!(matches!(dev.battery(), + Err(BmapError::Desync(m)) if m.contains("[0.5], expected [2.2]"))); + } + + #[test] + fn test_address_match_passes() { + assert_eq!(mock_qc_ultra2().battery().unwrap(), 80); + } + + #[test] + fn test_set_eq_checks_address() { + let mut t = MockTransport::new(); + t.responses.insert((1, 7), vec![2, 2, 0x03, 1, 42]); + let dev = BmapConnection::new(t, devices::qc_ultra2()); + assert!(matches!(dev.set_eq(1, 2, 3), Err(BmapError::Desync(_)))); + } + + #[test] + fn test_set_eq_surfaces_device_error() { + let mut t = MockTransport::new(); + t.add(1, 7, 0x04, &[1]); // ERROR: length + let dev = BmapConnection::new(t, devices::qc_ultra2()); + assert!(matches!(dev.set_eq(1, 2, 3), Err(BmapError::Device { code: 1, .. }))); + assert_eq!(dev.transport.sent.borrow().len(), 1); + } + + #[test] + fn test_set_mode_checks_address() { + let mut t = MockTransport::new(); + t.responses.insert((31, 3), vec![2, 2, 0x06, 0]); + let dev = BmapConnection::new(t, devices::qc_ultra2()); + assert!(matches!(dev.set_mode("aware", false), Err(BmapError::Desync(_)))); + } + + #[test] + fn test_empty_reply_is_device_error_on_every_path() { + let mut t = MockTransport::new(); + t.responses.insert((1, 10), vec![1, 10, 0x08, 0]); // unknown op + t.responses.insert((31, 3), vec![31, 3, 0x06, 4, 1]); // truncated + t.responses.insert((1, 7), vec![]); // nothing + let dev = BmapConnection::new(t, devices::qc_ultra2()); + let empty = |r: BmapResult<()>| matches!(r, Err(BmapError::Device { message, .. }) + if message == "Invalid or empty response"); + assert!(empty(dev.set_multipoint(true))); + assert!(empty(dev.set_mode("aware", false))); + assert!(empty(dev.set_eq(0, 0, 0))); + } } From 474fbd553e70d91af069a97bea6fc6af5be8d2ac Mon Sep 17 00:00:00 2001 From: Aaron Bockelie Date: Mon, 28 Sep 2026 12:15:33 -0500 Subject: [PATCH 4/5] Stop profile set and mode fallback from masking real errors profile set caught every BmapError from update_profile and created a profile instead, so a preset name made a duplicate and a desync made a stray profile. Decide update vs create from the profile list and let errors through. Pass renames via update_profile(rename=...), since a name= keyword collided with the positional name. The unknown-command fallback in all three CLIs now reports link and device errors, including desync, instead of "Unknown command". --- cpp/src/main.cpp | 4 ++ python/pybmap/cli.py | 34 +++++++++---- python/tests/test_cli.py | 107 +++++++++++++++++++++++++++++++++++++++ rust/src/main.rs | 19 ++++++- 4 files changed, 154 insertions(+), 10 deletions(-) diff --git a/cpp/src/main.cpp b/cpp/src/main.cpp index 070bd70..adab260 100644 --- a/cpp/src/main.cpp +++ b/cpp/src/main.cpp @@ -247,6 +247,10 @@ int main(int argc, char** argv) { try { dev.set_mode(cmd); std::cout << "OK: " << cmd << "\n"; + } catch (const bmap::desync_error&) { + throw; // A desynced link is not an unknown command. + } catch (const bmap::device_error&) { + throw; } catch (...) { std::cerr << "Unknown command: " << cmd << "\n"; return 1; diff --git a/python/pybmap/cli.py b/python/pybmap/cli.py index 212bdaf..d548d26 100644 --- a/python/pybmap/cli.py +++ b/python/pybmap/cli.py @@ -6,9 +6,18 @@ import pybmap from pybmap.constants import SPATIAL_NAMES, SIDETONE_NAMES, VOICE_LANGUAGES -from pybmap.errors import BmapError, BmapConnectionError, BmapInvalidArgError +from pybmap.errors import ( + BmapError, BmapConnectionError, BmapInvalidArgError, + BmapAuthError, BmapDeviceError, BmapTimeoutError, +) from pybmap.protocol import fmt_response +# Failures that mean the link or the device misbehaved, as opposed to a name +# that did not resolve. BmapDesyncError is a BmapConnectionError. +_LINK_OR_DEVICE_ERRORS = ( + BmapConnectionError, BmapAuthError, BmapDeviceError, BmapTimeoutError, +) + # ── ANSI Colors ────────────────────────────────────────────────────────────── if sys.stdout.isatty(): @@ -160,15 +169,20 @@ def cmd_profile_set(dev, args): settings["wind_block"] = bool_map.get(opts["wind"].lower(), 0) if "anc" in opts: settings["anc_toggle"] = bool_map.get(opts["anc"].lower(), 1) - if "name" in opts: - settings["name"] = opts["name"] - - try: - dev.update_profile(lookup_name, **settings) + new_name = opts.get("name") + + # Decide update vs create up front. Catching update_profile's errors and + # falling through to create turned "profile set Aware" into a duplicate + # of the preset, and a dropped link into a stray new profile. + wanted = lookup_name.lower() + if any(p.name.lower() == wanted for p in dev.profiles()): + dev.update_profile(lookup_name, rename=new_name, **settings) print("Updated: %s" % lookup_name) - except BmapError: - slot = dev.create_profile(lookup_name, **settings) - print("Created (slot %d): %s" % (slot, lookup_name)) + else: + # create_profile refuses preset names. + name = new_name or lookup_name + slot = dev.create_profile(name, **settings) + print("Created (slot %d): %s" % (slot, name)) def cmd_buttons(dev, args): @@ -469,6 +483,8 @@ def main(): try: dev.set_mode(sys.argv[1]) print("OK: %s" % sys.argv[1]) + except _LINK_OR_DEVICE_ERRORS: + raise # A dropped or desynced link is not an unknown command. except BmapError: print("Unknown command: %s" % cmd, file=sys.stderr) sys.exit(1) diff --git a/python/tests/test_cli.py b/python/tests/test_cli.py index 0d38f63..3e2a671 100644 --- a/python/tests/test_cli.py +++ b/python/tests/test_cli.py @@ -74,3 +74,110 @@ def test_mac_without_device_type_skips_bluetooth_hint(monkeypatch, capsys, devic err = capsys.readouterr().err assert "device_type is required" in err assert "Is Bluetooth on?" not in err + + +# ── profile set / mode fallback ───────────────────────────────────────────── + +from pybmap.errors import BmapDesyncError, BmapInvalidArgError # noqa: E402 +from pybmap.types import ModeConfig # noqa: E402 + + +def _profile(idx, name, editable=True): + return ModeConfig( + mode_idx=idx, prompt="NONE", prompt_bytes=(0, 0), name=name, + cnc_level=0, auto_cnc=False, spatial=0, wind_block=False, + anc_toggle=False, editable=editable, configured=True, + flags="", raw=b"", + ) + + +class ProfileDevice: + """Records which profile call cmd_profile_set made.""" + + def __init__(self, profiles, update_error=None, create_error=None): + self._profiles = profiles + self._update_error = update_error + self._create_error = create_error + self.calls = [] + + def profiles(self): + return self._profiles + + def update_profile(self, name, rename=None, **settings): + self.calls.append(("update", name) if rename is None else ("update", name, rename)) + if self._update_error: + raise self._update_error + + def create_profile(self, name, **settings): + self.calls.append(("create", name)) + if self._create_error: + raise self._create_error + return 3 + + +def test_profile_set_updates_existing(capsys): + dev = ProfileDevice([_profile(3, "Gym")]) + cli.cmd_profile_set(dev, ["gym", "cnc=4"]) + assert dev.calls == [("update", "gym")] + assert "Updated" in capsys.readouterr().out + + +def test_profile_set_creates_when_not_found(capsys): + dev = ProfileDevice([_profile(3, "Gym")]) + cli.cmd_profile_set(dev, ["Commute"]) + assert dev.calls == [("create", "Commute")] + assert "Created (slot 3)" in capsys.readouterr().out + + +def test_profile_set_preset_name_does_not_create(): + # "Aware" is listed as a preset slot: the update is refused, and that + # refusal must not turn into a duplicate custom "Aware". + dev = ProfileDevice([_profile(1, "Aware", editable=False)], + update_error=cli.BmapError("Cannot modify preset mode 'Aware'")) + with pytest.raises(cli.BmapError, match="preset"): + cli.cmd_profile_set(dev, ["Aware"]) + assert dev.calls == [("update", "Aware")] + + +def test_profile_set_preset_name_refused_by_create(): + dev = ProfileDevice([], create_error=BmapInvalidArgError("'Aware' is a preset mode name")) + with pytest.raises(BmapInvalidArgError): + cli.cmd_profile_set(dev, ["Aware"]) + + +def test_profile_set_desync_does_not_create(): + dev = ProfileDevice([_profile(3, "Gym")], update_error=BmapDesyncError("out of sync")) + with pytest.raises(BmapDesyncError): + cli.cmd_profile_set(dev, ["Gym"]) + assert dev.calls == [("update", "Gym")] + + +class ModeFallbackDevice: + preset_modes = {"quiet": {"idx": 0}} + + def __init__(self, error): + self._error = error + + def set_mode(self, name): + raise self._error + + def close(self): + pass + + +@pytest.mark.parametrize("error, expected", [ + (cli.BmapError("Unknown mode: bogus"), "Unknown command: bogus"), + (BmapDesyncError("Response came from [0.5], expected [31.3]"), "expected [31.3]"), +]) +def test_mode_fallback_only_hides_unknown_names(monkeypatch, capsys, error, expected): + monkeypatch.setattr(cli.sys, "argv", ["bosectl", "bogus"]) + monkeypatch.setattr(cli.pybmap, "connect", lambda **kw: ModeFallbackDevice(error)) + with pytest.raises(SystemExit): + cli.main() + assert expected in capsys.readouterr().err + + +def test_profile_set_rename_reaches_update(): + dev = ProfileDevice([_profile(3, "Gym")]) + cli.cmd_profile_set(dev, ["Gym", "name=Run"]) + assert dev.calls == [("update", "Gym", "Run")] diff --git a/rust/src/main.rs b/rust/src/main.rs index 52c7c28..6ba7725 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -242,10 +242,12 @@ fn main() { // Try as custom profile name match dev.set_mode(&cmd, false) { Ok(_) => Ok(println!("OK: {}", cmd)), - Err(_) => { + Err(e) if is_unknown_mode(&e) => { eprintln!("Unknown command: {}", cmd); process::exit(1); } + // A dropped or desynced link is not an unknown command. + Err(e) => Err(e), } } }; @@ -256,6 +258,12 @@ fn main() { } } +/// Whether a failed mode switch means "no such mode" rather than a link or +/// device failure that the user needs to see. +fn is_unknown_mode(e: &BmapError) -> bool { + matches!(e, BmapError::InvalidArg(_) | BmapError::Unsupported(_)) +} + /// Follow-up hint for a failed connect; setup mistakes are not Bluetooth problems. fn connection_hint(e: &BmapError) -> Option<&'static str> { match e { @@ -352,6 +360,15 @@ fn usage() { mod tests { use super::*; + #[test] + fn unknown_mode_falls_back_only_for_lookup_errors() { + assert!(is_unknown_mode(&BmapError::InvalidArg("Unknown mode: x".into()))); + assert!(is_unknown_mode(&BmapError::Unsupported("no modes".into()))); + assert!(!is_unknown_mode(&BmapError::Desync("[0.5] vs [31.3]".into()))); + assert!(!is_unknown_mode(&BmapError::Device { message: "x".into(), code: 0 })); + assert!(!is_unknown_mode(&BmapError::Connection("gone".into()))); + } + #[test] fn missing_device_type_skips_bluetooth_hint() { let err = connect(Some("00:11:22:33:44:55"), None).err().unwrap(); From dcbb36e544f3bbe09e598cef479c28e8db16be7c Mon Sep 17 00:00:00 2001 From: Aaron Bockelie Date: Mon, 28 Sep 2026 12:25:04 -0500 Subject: [PATCH 5/5] Skip stray frames instead of reporting a desync A late STATUS (prince applies START [31.3] after PROCESSING) or an unsolicited notification ahead of a reply made the next command fail with a desync and dropped the real reply. Pick the frame whose address matches the request out of the buffer; report a desync only when none does. The Linux transports also discard bytes already waiting on the socket before each send, as the macOS transport does. status() no longer turns a desync into default values. In C++, require() throws bmap::unsupported_error, set_mode checks for RESULT or PROCESSING like Python and Rust, and the CLI maps only unknown or unsupported modes to "Unknown command". --- cpp/src/connection.h | 52 +++++++++----- cpp/src/errors.h | 9 ++- cpp/src/main.cpp | 11 +-- cpp/src/transport.cpp | 15 ++++ cpp/tests/test_connection.cpp | 58 +++++++++++++++ docs/architecture.md | 18 ++--- python/pybmap/connection.py | 52 +++++++++----- python/pybmap/transport.py | 24 +++++++ python/tests/test_connection.py | 28 ++++++++ python/tests/test_transport_linux.py | 44 ++++++++++++ rust/src/connection.rs | 103 +++++++++++++++++++++------ rust/src/transport.rs | 24 +++++++ 12 files changed, 366 insertions(+), 72 deletions(-) diff --git a/cpp/src/connection.h b/cpp/src/connection.h index 69d1759..b34c1b9 100644 --- a/cpp/src/connection.h +++ b/cpp/src/connection.h @@ -181,7 +181,12 @@ class BmapConnection { } } if (idx == 255) throw std::invalid_argument("Unknown mode: " + name); - start(addr, {idx, static_cast(announce ? 1 : 0)}); + auto resp = start(addr, {idx, static_cast(announce ? 1 : 0)}); + // Some firmware (QC Headphones "prince") acks START [31.3] with + // PROCESSING and applies the switch asynchronously. + if (resp.op != Operator::Result && resp.op != Operator::Processing) { + throw device_error("Mode switch failed: " + resp.fmt()); + } } void set_cnc(uint8_t level) { @@ -343,24 +348,33 @@ class BmapConnection { DeviceConfig config_; static Addr require(const std::optional& opt, const char* name) { - if (!opt) throw std::runtime_error(std::string(name) + " not supported on this device"); + if (!opt) throw unsupported_error(std::string(name) + " not supported on this device"); return *opt; } - // Reject a response that came from a different address. + // Pick the frame answering `addr` out of a received buffer. + // + // The buffer can hold more than the reply: an unsolicited STATUS, or a + // late frame such as the STATUS prince sends after acking START [31.3] + // with PROCESSING. Those are skipped. If frames arrived but none came + // from the requested address, the socket is out of step with the + // requests: after a reconnect it can still hold answers queued before + // the drop, and every read returns the previous request's answer. + // Parsing that as the right reply would surface plausible-looking wrong + // data, so throw desync_error instead. // - // The firmware answers in order, so after a reconnect the socket can still - // hold responses queued before the drop and every read returns the previous - // request's answer. Without this check the payload is handed to the wrong - // parser and surfaces as plausible-looking data. - void check_address(const BmapResponse& resp, Addr addr) { - if (resp.fblock != addr.fblock || resp.func != addr.func) { - throw desync_error( - "Response came from [" + std::to_string(resp.fblock) + "." + - std::to_string(resp.func) + "], expected [" + - std::to_string(addr.fblock) + "." + std::to_string(addr.func) + - "]. Reopen the connection."); + // Returns nullopt when the buffer holds no valid frame at all. + std::optional select_reply(const std::vector& data, Addr addr) { + auto frames = parse_all_responses(data); + for (auto& f : frames) { + if (f.fblock == addr.fblock && f.func == addr.func) return f; } + if (frames.empty()) return std::nullopt; + throw desync_error( + "Response came from [" + std::to_string(frames[0].fblock) + "." + + std::to_string(frames[0].func) + "], expected [" + + std::to_string(addr.fblock) + "." + std::to_string(addr.func) + + "]. Reopen the connection."); } // Validate a single reply: present, from the right address, not ERROR. @@ -368,9 +382,8 @@ class BmapConnection { // truncated frame, a desynced socket or a device error surface the same // way in all of them. BmapResponse check_reply(const std::vector& data, Addr addr) { - auto resp = parse_response(data); + auto resp = select_reply(data, addr); if (!resp) throw device_error("Invalid or empty response"); - check_address(*resp, addr); check_error(*resp); return *resp; } @@ -415,9 +428,14 @@ class BmapConnection { return "custom(" + std::to_string(idx) + ")"; } + // Fall back to default_val for an optional status field, except on a + // desync: every later read on the same socket would be wrong too, so a + // snapshot built from defaults would hide it. template T safe_call(F fn, T default_val) { - try { return fn(); } catch (...) { return default_val; } + try { return fn(); } + catch (const desync_error&) { throw; } + catch (...) { return default_val; } } // Write audio settings via [31.10] preserving non-overridden fields. diff --git a/cpp/src/errors.h b/cpp/src/errors.h index 8b6210e..bf28d2c 100644 --- a/cpp/src/errors.h +++ b/cpp/src/errors.h @@ -1,6 +1,6 @@ // Exception types for BMAP protocol errors. // -// Both derive from std::runtime_error, so callers that catch +// All derive from std::runtime_error, so callers that catch // std::runtime_error or std::exception keep working. #pragma once @@ -22,6 +22,13 @@ class device_error : public std::runtime_error { uint8_t code_; }; +/// The connected device does not have the requested feature. +/// Mirrors BmapError::Unsupported (Rust). +class unsupported_error : public std::runtime_error { +public: + using std::runtime_error::runtime_error; +}; + /// A response carried a different address than the request. /// /// Seen after the headset drops and reconnects: responses queued before the diff --git a/cpp/src/main.cpp b/cpp/src/main.cpp index adab260..033dee0 100644 --- a/cpp/src/main.cpp +++ b/cpp/src/main.cpp @@ -247,11 +247,12 @@ int main(int argc, char** argv) { try { dev.set_mode(cmd); std::cout << "OK: " << cmd << "\n"; - } catch (const bmap::desync_error&) { - throw; // A desynced link is not an unknown command. - } catch (const bmap::device_error&) { - throw; - } catch (...) { + } catch (const std::invalid_argument&) { + // No such mode. Link, device and desync errors fall through + // to the handler below and are reported as errors. + std::cerr << "Unknown command: " << cmd << "\n"; + return 1; + } catch (const bmap::unsupported_error&) { std::cerr << "Unknown command: " << cmd << "\n"; return 1; } diff --git a/cpp/src/transport.cpp b/cpp/src/transport.cpp index fa8951f..5a73d38 100644 --- a/cpp/src/transport.cpp +++ b/cpp/src/transport.cpp @@ -61,7 +61,22 @@ std::vector RfcommTransport::send_recv_drain(const std::vector return send_recv_inner(packet, true); } +// Drop bytes already waiting on the socket before a new request. Late replies +// and unsolicited STATUS notifications would otherwise be read as the answer +// to the next request. Non-blocking and bounded, so a device that streams +// notifications cannot stall the request; a dead socket is left for the send +// to report. +static void discard_pending(int fd) { + constexpr int kMaxStaleChunks = 64; + uint8_t buf[4096]; + for (int i = 0; i < kMaxStaleChunks; i++) { + if (::recv(fd, buf, sizeof(buf), MSG_DONTWAIT) <= 0) break; + } +} + std::vector RfcommTransport::send_recv_inner(const std::vector& packet, bool drain) { + discard_pending(fd_); + // Send ssize_t sent = ::send(fd_, packet.data(), packet.size(), 0); if (sent < 0) { diff --git a/cpp/tests/test_connection.cpp b/cpp/tests/test_connection.cpp index 3a61183..6ee2a15 100644 --- a/cpp/tests/test_connection.cpp +++ b/cpp/tests/test_connection.cpp @@ -537,3 +537,61 @@ TEST(empty_reply_is_device_error_on_every_path) { ASSERT_TRUE(throws_with([&]{ dev.set_mode("aware"); }, msg)); ASSERT_TRUE(throws_with([&]{ dev.set_eq(0, 0, 0); }, msg)); } + +TEST(late_status_ahead_of_reply_is_skipped) { + // prince sends STATUS [31.3] after acking START with PROCESSING. + auto raw = new MockTransport(); + raw->responses[{2, 2}] = {31, 3, 0x03, 1, 0x01, 2, 2, 0x03, 4, 80, 0xff, 0xff, 0x00}; + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + ASSERT_EQ(dev.battery(), 80); +} + +TEST(only_foreign_frames_is_desync) { + auto raw = new MockTransport(); + raw->responses[{2, 2}] = {31, 3, 0x03, 1, 0x01, 0, 5, 0x03, 1, 0x34}; + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + ASSERT_TRUE(throws_with([&]{ dev.battery(); }, "[31.3], expected [2.2]")); +} + +TEST(setget_skips_stray_frame) { + auto raw = new MockTransport(); + raw->responses[{1, 10}] = {31, 3, 0x03, 1, 0x01, 1, 10, 0x03, 1, 0x07}; + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + dev.set_multipoint(true); +} + +TEST(status_does_not_swallow_desync) { + auto raw = new MockTransport(); + raw->add(2, 2, 0x03, {80, 0xff, 0xff, 0x00}); + raw->add(31, 3, 0x03, {0x00}); + raw->responses[{1, 7}] = {0, 5, 0x03, 1, 0x34}; + BmapConnection d(std::unique_ptr(raw), qc_ultra2()); + ASSERT_TRUE(throws_with([&]{ d.status(); })); +} + +TEST(set_mode_accepts_processing_ack) { + auto raw = new MockTransport(); + raw->add(31, 3, 0x07, {}); // PROCESSING: async ack (prince) + BmapConnection dev(std::unique_ptr(raw), qc_prince()); + dev.set_mode("quiet"); +} + +TEST(set_mode_accepts_result) { + auto raw = new MockTransport(); + raw->add(31, 3, 0x06, {0x01}); + BmapConnection dev(std::unique_ptr(raw), qc_ultra2()); + dev.set_mode("aware"); +} + +TEST(set_mode_rejects_unexpected_op) { + auto raw = new MockTransport(); + raw->add(31, 3, 0x03, {0}); // STATUS where RESULT/PROCESSING expected + BmapConnection dev(std::unique_ptr(raw), qc_prince()); + ASSERT_TRUE(throws_with([&]{ dev.set_mode("quiet"); }, "Mode switch failed")); +} + +TEST(unsupported_feature_is_unsupported_error) { + auto raw = new MockTransport(); + BmapConnection dev(std::unique_ptr(raw), qc35()); + ASSERT_TRUE(throws_with([&]{ dev.eq(); }, "not supported")); +} diff --git a/docs/architecture.md b/docs/architecture.md index 98edf8e..1fc6248 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -457,7 +457,8 @@ This makes feature dispatch a simple dict lookup at runtime. ``` BmapError ├── BmapConnectionError — socket/transport failures -│ └── BmapDesyncError — reply came from a different [fblock.func] +│ └── BmapDesyncError — no frame in the reply came from the +│ requested [fblock.func] ├── BmapAuthError — device returned error code 5 ├── BmapDeviceError — device returned other error codes, or an │ invalid/empty reply to GET, SETGET or START @@ -526,7 +527,7 @@ pub enum BmapError { NotFound(String), Unsupported(String), InvalidArg(String), - Desync(String), // reply came from a different [fblock.func] + Desync(String), // no reply frame came from the requested [fblock.func] } ``` @@ -566,16 +567,17 @@ public: Tests use a `MockTransport` subclass. -**Error handling** uses standard exceptions plus two typed ones from -`errors.h`, both derived from `std::runtime_error`: `bmap::device_error` +**Error handling** uses standard exceptions plus typed ones from +`errors.h`, all derived from `std::runtime_error`: `bmap::device_error` (device ERROR reply, with `code()`, or an invalid/empty reply to GET, SETGET -or START) and `bmap::desync_error` (reply came from a different -`[fblock.func]`). Bad caller input throws `std::invalid_argument`. Auth -errors (code 5) are a `device_error` with `code() == 5`. The `require()` helper +or START), `bmap::desync_error` (no frame in the reply came from the +requested `[fblock.func]`) and `bmap::unsupported_error` (feature missing on +this device). Bad caller input throws `std::invalid_argument`. Auth errors +(code 5) are a `device_error` with `code() == 5`. The `require()` helper converts `std::nullopt` to an exception for unsupported features: ```cpp static Addr require(const std::optional& opt, const char* name) { - if (!opt) throw std::runtime_error(std::string(name) + " not supported"); + if (!opt) throw unsupported_error(std::string(name) + " not supported on this device"); return *opt; } ``` diff --git a/python/pybmap/connection.py b/python/pybmap/connection.py index 7baceb3..198be73 100644 --- a/python/pybmap/connection.py +++ b/python/pybmap/connection.py @@ -16,7 +16,7 @@ OP_PROCESSING, SIDETONE_NAMES, SIDETONE_VALUES, SPATIAL_VALUES, VOICE_LANGUAGES, ) -from .protocol import bmap_packet, parse_response, parse_all_responses, fmt_response +from .protocol import bmap_packet, parse_all_responses, fmt_response from .errors import ( BmapError, BmapAuthError, BmapDeviceError, BmapDesyncError, BmapInvalidArgError, @@ -57,32 +57,40 @@ def _feature(self, name): ) return features[name] - def _check_address(self, parsed, fblock, func): - """Reject a response that came from a different address. + def _select_reply(self, data, fblock, func): + """Pick the frame answering [fblock.func] out of a received buffer. - The firmware answers in order, so after a reconnect the socket can - still hold responses queued before the drop and every read returns - the previous request's answer. Without this check the payload is - handed to the wrong parser and surfaces as plausible-looking data. + The buffer can hold more than the reply: an unsolicited STATUS, or a + late frame such as the STATUS prince sends after acking START [31.3] + with PROCESSING. Those are skipped. If frames arrived but none came + from the requested address, the socket is out of step with the + requests: after a reconnect it can still hold answers queued before + the drop, and every read returns the previous request's answer. + Parsing that as the right reply would surface plausible-looking wrong + data, so raise instead. + + Returns None when the buffer holds no valid frame at all. """ - if parsed is None: - return None - if (parsed.fblock, parsed.func) != (fblock, func): + frames = parse_all_responses(data) + for frame in frames: + if (frame.fblock, frame.func) == (fblock, func): + return frame + if frames: raise BmapDesyncError( "Response came from [%d.%d], expected [%d.%d]. " "Reopen the connection." - % (parsed.fblock, parsed.func, fblock, func) + % (frames[0].fblock, frames[0].func, fblock, func) ) - return parsed + return None - def _check_reply(self, parsed, fblock, func): + def _check_reply(self, data, fblock, func): """Validate a single reply: present, from the right address, not ERROR. Every single-reply path (GET, SETGET, START) goes through here so a truncated frame, a desynced socket or a device error surface the same way in all of them. """ - parsed = self._check_address(parsed, fblock, func) + parsed = self._select_reply(data, fblock, func) if parsed is None: raise BmapDeviceError("Invalid or empty response") if parsed.op == OP_ERROR: @@ -94,7 +102,7 @@ def _get_payload(self, feature_name): feat = self._feature(feature_name) fblock, func = feat["addr"] resp = self._transport.send_recv(bmap_packet(fblock, func, OP_GET)) - return self._check_reply(parse_response(resp), fblock, func).payload + return self._check_reply(resp, fblock, func).payload def _get(self, feature_name): """Send a GET request and return its parsed payload.""" @@ -112,7 +120,7 @@ def _setget(self, feature_name, payload): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_SETGET, payload) ) - return self._check_reply(parse_response(resp), fblock, func) + return self._check_reply(resp, fblock, func) def _start(self, feature_name, payload=b""): """Send a START request and return the parsed response.""" @@ -121,7 +129,7 @@ def _start(self, feature_name, payload=b""): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_START, payload) ) - return self._check_reply(parse_response(resp), fblock, func) + return self._check_reply(resp, fblock, func) def _start_drain(self, feature_name, payload=b""): """Send a START request and drain all responses.""" @@ -133,9 +141,15 @@ def _start_drain(self, feature_name, payload=b""): return parse_all_responses(data) def _safe_read(self, method, default): - """Call a read method, returning default on BmapError.""" + """Call a read method, returning default on BmapError. + + A desync is re-raised: every later read on the same socket would be + wrong too, so a status snapshot built from defaults would hide it. + """ try: return method() + except BmapDesyncError: + raise except BmapError: return default @@ -221,7 +235,7 @@ def mode_idx(self): feat = self._feature("current_mode") fblock, func = feat["addr"] resp = self._transport.send_recv(bmap_packet(fblock, func, OP_GET)) - parsed = self._check_address(parse_response(resp), fblock, func) + parsed = self._select_reply(resp, fblock, func) if parsed and parsed.payload: return parsed.payload[0] return None diff --git a/python/pybmap/transport.py b/python/pybmap/transport.py index a612239..15526ac 100644 --- a/python/pybmap/transport.py +++ b/python/pybmap/transport.py @@ -254,9 +254,33 @@ def close(self): pass self._sock = None + # Upper bound on chunks discarded before a send, so a device that + # streams notifications cannot stall the request. + _MAX_STALE_CHUNKS = 64 + + def _discard_pending(self): + """Drop bytes already waiting on the socket before a new request. + + Late replies and unsolicited STATUS notifications would otherwise + be read as the answer to the next request. Mirrors the queue + clear in the macOS transport. + """ + try: + self._sock.settimeout(0) + for _ in range(self._MAX_STALE_CHUNKS): + if not self._sock.recv(4096): + break # peer closed; the send will report it + except (BlockingIOError, InterruptedError, socket.timeout): + pass + except OSError: + pass # a dead socket fails the send with a clear error + finally: + self._sock.settimeout(self.timeout) + def send_recv(self, packet, drain=False): if not self._sock: raise BmapConnectionError("Not connected") + self._discard_pending() try: self._sock.send(packet) time.sleep(0.2) diff --git a/python/tests/test_connection.py b/python/tests/test_connection.py index 4e8d68f..ce3f466 100644 --- a/python/tests/test_connection.py +++ b/python/tests/test_connection.py @@ -605,3 +605,31 @@ def test_create_reuses_cleared_slot(self, qc45_conn, monkeypatch): lambda slot, name, **kw: written.append((slot, name))) assert qc45_conn.create_profile("Commute") == 2 assert written == [(2, "Commute")] + + +class TestStrayFrames: + """Late or unsolicited frames ahead of the reply are skipped, not fatal.""" + + def test_late_status_ahead_of_reply_is_skipped(self, mock_dev): + # prince sends STATUS [31.3] after acking START with PROCESSING. + mock_dev._transport.responses[(2, 2)] = ( + bytes([31, 3, OP_STATUS, 1, 0x01]) + + bytes([2, 2, OP_STATUS, 4, 80, 0xff, 0xff, 0x00])) + assert mock_dev.battery() == 80 + + def test_only_foreign_frames_is_desync(self, mock_dev): + mock_dev._transport.responses[(2, 2)] = ( + bytes([31, 3, OP_STATUS, 1, 0x01]) + bytes([0, 5, OP_STATUS, 1, 0x34])) + with pytest.raises(BmapDesyncError, match=r"\[31\.3\], expected \[2\.2\]"): + mock_dev.battery() + + def test_setget_skips_stray_frame(self, mock_dev): + mock_dev._transport.responses[(1, 10)] = ( + bytes([31, 3, OP_STATUS, 1, 0x01]) + bytes([1, 10, OP_STATUS, 1, 0x07])) + mock_dev.set_multipoint(True) + + def test_status_does_not_swallow_desync(self, mock_dev): + # A desync on any optional field must fail the snapshot, not default it. + mock_dev._transport.responses[(1, 7)] = bytes([0, 5, OP_STATUS, 1, 0x34]) + with pytest.raises(BmapDesyncError): + mock_dev.status() diff --git a/python/tests/test_transport_linux.py b/python/tests/test_transport_linux.py index 21b34e7..76fd59b 100644 --- a/python/tests/test_transport_linux.py +++ b/python/tests/test_transport_linux.py @@ -20,6 +20,17 @@ from pybmap.transport import RfcommTransport +@pytest.fixture(autouse=True) +def _no_stale_discard(request, monkeypatch): + """Keep the pre-send discard out of tests that script recv() calls. + + TestLinuxTransportDiscardPending exercises it directly. + """ + if request.cls is not None and request.cls.__name__ == "TestLinuxTransportDiscardPending": + return + monkeypatch.setattr(RfcommTransport, "_discard_pending", lambda self: None) + + class TestLinuxTransportInitialization: """Test transport initialization and basic setup.""" @@ -501,3 +512,36 @@ def test_rapid_reconnection(self, mock_sleep, mock_socket_class): # Should have created 3 sockets assert mock_socket_class.call_count == 3 + + +class TestLinuxTransportDiscardPending: + """Bytes left on the socket must not be read as the next reply.""" + + @patch("socket.socket") + @patch("time.sleep") + def test_stale_bytes_dropped_before_send(self, mock_sleep, mock_socket_class): + mock_sock = MagicMock() + mock_socket_class.return_value = mock_sock + stale = b"\x1f\x03\x03\x01\x00" # late [31.3] STATUS + reply = b"\x02\x02\x03\x01\x50" + mock_sock.recv.side_effect = [stale, BlockingIOError(), reply] + + transport = RfcommTransport("68:F2:1F:00:00:00") + transport.connect() + assert transport.send_recv(b"\x02\x02\x01\x00") == reply + + # Non-blocking during the discard, timeout restored before the send. + timeouts = [c[0][0] for c in mock_sock.settimeout.call_args_list] + assert timeouts == [3.0, 0, 3.0] + + @patch("socket.socket") + @patch("time.sleep") + def test_discard_is_bounded(self, mock_sleep, mock_socket_class): + mock_sock = MagicMock() + mock_socket_class.return_value = mock_sock + mock_sock.recv.return_value = b"\x1f\x03\x03\x01\x00" + + transport = RfcommTransport("68:F2:1F:00:00:00") + transport.connect() + transport._discard_pending() + assert mock_sock.recv.call_count == RfcommTransport._MAX_STALE_CHUNKS diff --git a/rust/src/connection.rs b/rust/src/connection.rs index 63e40b2..50f5cb0 100644 --- a/rust/src/connection.rs +++ b/rust/src/connection.rs @@ -2,7 +2,7 @@ use crate::device::*; use crate::error::{BmapError, BmapResult}; -use crate::protocol::{Operator, BmapResponse, bmap_packet, parse_response, parse_all_responses}; +use crate::protocol::{Operator, BmapResponse, bmap_packet, parse_all_responses}; use crate::transport::Transport; @@ -10,6 +10,17 @@ use crate::transport::Transport; /// The device name field is 32 bytes on every BMAP device seen so far. pub const MAX_NAME_BYTES: usize = 31; +/// Fall back to `default` for an optional status field, except on a desync: +/// every later read on the same socket would be wrong too, so a snapshot +/// built from defaults would hide it. +fn or_default(result: BmapResult, default: V) -> BmapResult { + match result { + Ok(v) => Ok(v), + Err(e @ BmapError::Desync(_)) => Err(e), + Err(_) => Ok(default), + } +} + pub struct BmapConnection { transport: T, config: DeviceConfig, @@ -34,20 +45,31 @@ impl BmapConnection { )) } - /// Reject a response that came from a different address. + /// Pick the frame answering `addr` out of a received buffer. + /// + /// The buffer can hold more than the reply: an unsolicited STATUS, or a + /// late frame such as the STATUS prince sends after acking START [31.3] + /// with PROCESSING. Those are skipped. If frames arrived but none came + /// from the requested address, the socket is out of step with the + /// requests: after a reconnect it can still hold answers queued before + /// the drop, and every read returns the previous request's answer. + /// Parsing that as the right reply would surface plausible-looking wrong + /// data, so return Desync instead. /// - /// The firmware answers in order, so after a reconnect the socket can - /// still hold responses queued before the drop and every read returns - /// the previous request's answer. Without this check the payload is - /// handed to the wrong parser and surfaces as plausible-looking data. - fn check_address(&self, resp: &BmapResponse, addr: Addr) -> BmapResult<()> { - if resp.fblock != addr.0 || resp.func != addr.1 { - return Err(BmapError::Desync(format!( + /// Returns `Ok(None)` when the buffer holds no valid frame at all. + fn select_reply(&self, data: &[u8], addr: Addr) -> BmapResult> { + let frames = parse_all_responses(data); + let first = frames.first().map(|f| (f.fblock, f.func)); + if let Some(frame) = frames.into_iter().find(|f| f.fblock == addr.0 && f.func == addr.1) { + return Ok(Some(frame)); + } + match first { + Some((fblock, func)) => Err(BmapError::Desync(format!( "Response came from [{}.{}], expected [{}.{}]. Reopen the connection.", - resp.fblock, resp.func, addr.0, addr.1 - ))); + fblock, func, addr.0, addr.1 + ))), + None => Ok(None), } - Ok(()) } /// Validate a single reply: present, from the right address, not ERROR. @@ -56,11 +78,10 @@ impl BmapConnection { /// truncated frame, a desynced socket or a device error surface the same /// way in all of them. fn check_reply(&self, data: &[u8], addr: Addr) -> BmapResult { - let resp = parse_response(data).ok_or_else(|| BmapError::Device { + let resp = self.select_reply(data, addr)?.ok_or_else(|| BmapError::Device { message: "Invalid or empty response".into(), code: 0, })?; - self.check_address(&resp, addr)?; self.check_error(&resp)?; Ok(resp) } @@ -284,10 +305,12 @@ impl BmapConnection { // Single GET for mode index, derive name without extra round trip. let (current_idx, current_name) = match self.mode_idx() { Ok(idx) => (idx, self.mode_name_from_idx(idx)), + Err(e @ BmapError::Desync(_)) => return Err(e), Err(_) => (0, String::new()), }; - let (cnc_level, cnc_max) = self.cnc().unwrap_or((0, 10)); - let (prompts_enabled, prompts_language) = self.prompts().unwrap_or((false, "Unknown")); + let (cnc_level, cnc_max) = or_default(self.cnc(), (0, 10))?; + let (prompts_enabled, prompts_language) = + or_default(self.prompts(), (false, "Unknown"))?; let battery = self.battery_status()?; Ok(DeviceStatus { @@ -297,12 +320,12 @@ impl BmapConnection { mode_idx: current_idx, cnc_level, cnc_max, - eq: self.eq().unwrap_or_default(), - name: self.name().unwrap_or_default(), - firmware: self.firmware().unwrap_or_default(), - sidetone: self.sidetone().unwrap_or("off").to_string(), - multipoint: self.multipoint().unwrap_or(false), - auto_pause: self.auto_pause().unwrap_or(false), + eq: or_default(self.eq(), Vec::new())?, + name: or_default(self.name(), String::new())?, + firmware: or_default(self.firmware(), String::new())?, + sidetone: or_default(self.sidetone(), "off")?.to_string(), + multipoint: or_default(self.multipoint(), false)?, + auto_pause: or_default(self.auto_pause(), false)?, prompts_enabled, prompts_language: prompts_language.to_string(), }) @@ -1323,4 +1346,40 @@ mod tests { assert!(empty(dev.set_mode("aware", false))); assert!(empty(dev.set_eq(0, 0, 0))); } + + #[test] + fn test_late_status_ahead_of_reply_is_skipped() { + // prince sends STATUS [31.3] after acking START with PROCESSING. + let mut t = MockTransport::new(); + t.responses.insert((2, 2), vec![31, 3, 0x03, 1, 0x01, 2, 2, 0x03, 4, 80, 0xff, 0xff, 0x00]); + let dev = BmapConnection::new(t, devices::qc_ultra2()); + assert_eq!(dev.battery().unwrap(), 80); + } + + #[test] + fn test_only_foreign_frames_is_desync() { + let mut t = MockTransport::new(); + t.responses.insert((2, 2), vec![31, 3, 0x03, 1, 0x01, 0, 5, 0x03, 1, 0x34]); + let dev = BmapConnection::new(t, devices::qc_ultra2()); + assert!(matches!(dev.battery(), + Err(BmapError::Desync(m)) if m.contains("[31.3], expected [2.2]"))); + } + + #[test] + fn test_setget_skips_stray_frame() { + let mut t = MockTransport::new(); + t.responses.insert((1, 10), vec![31, 3, 0x03, 1, 0x01, 1, 10, 0x03, 1, 0x07]); + let dev = BmapConnection::new(t, devices::qc_ultra2()); + dev.set_multipoint(true).unwrap(); + } + + #[test] + fn test_status_does_not_swallow_desync() { + let mut t = MockTransport::new(); + t.add(2, 2, 0x03, &[80, 0xff, 0xff, 0x00]); + t.add(31, 3, 0x03, &[0x00]); + t.responses.insert((1, 7), vec![0, 5, 0x03, 1, 0x34]); + let dev = BmapConnection::new(t, devices::qc_ultra2()); + assert!(matches!(dev.status(), Err(BmapError::Desync(_)))); + } } diff --git a/rust/src/transport.rs b/rust/src/transport.rs index c2987f2..05c62cc 100644 --- a/rust/src/transport.rs +++ b/rust/src/transport.rs @@ -13,6 +13,9 @@ use crate::error::{BmapError, BmapResult}; const AF_BLUETOOTH: i32 = 31; const BTPROTO_RFCOMM: i32 = 3; +/// Upper bound on chunks discarded before a send. +const MAX_STALE_CHUNKS: usize = 64; + /// RFCOMM channel for BMAP protocol. pub const BMAP_CHANNEL: u8 = 2; @@ -97,8 +100,29 @@ impl RfcommTransport { } } + /// Drop bytes already waiting on the socket before a new request. + /// + /// Late replies and unsolicited STATUS notifications would otherwise be + /// read as the answer to the next request. Non-blocking and bounded, so + /// a device that streams notifications cannot stall the request; a dead + /// socket is left for the send to report. + fn discard_pending(&self) { + let fd = self.fd.as_raw_fd(); + let mut buf = [0u8; 4096]; + for _ in 0..MAX_STALE_CHUNKS { + let n = unsafe { + libc::recv(fd, buf.as_mut_ptr() as *mut libc::c_void, buf.len(), + libc::MSG_DONTWAIT) + }; + if n <= 0 { + break; + } + } + } + fn send_recv_inner(&self, packet: &[u8], drain: bool) -> BmapResult> { let fd = self.fd.as_raw_fd(); + self.discard_pending(); // Send — use libc::send directly to avoid fd ownership issues. let sent = unsafe {