diff --git a/README.md b/README.md index 7591e4d..2c84f72 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ connection to the headphones. | **QuietComfort Headphones** | CNC 0-10 + Wind Block via ModeConfig | 3-band | 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 @@ -119,8 +119,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/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 9736c93..b34c1b9 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,16 +172,21 @@ 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); - start(addr, {idx, static_cast(announce ? 1 : 0)}); + if (idx == 255) throw std::invalid_argument("Unknown mode: " + name); + 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) { @@ -203,8 +232,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 +287,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_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 +306,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; @@ -296,17 +321,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 (!detail::iequals(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::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; + mc.name = "None"; + write_mode(match->mode_idx, mc); } std::vector send_raw(const std::vector& data) { @@ -319,33 +348,59 @@ 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; } + // 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. + // + // 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. + // 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 = select_reply(data, addr); + if (!resp) throw device_error("Invalid or empty response"); + check_error(*resp); + return *resp; + } + 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("Invalid or empty response"); - check_error(*resp); - return resp->payload; + 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_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_error(*resp); - return *resp; + return check_reply(transport_->send_recv(pkt), addr); } std::vector start_drain(Addr addr, const std::vector& payload) { @@ -356,7 +411,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]); } } @@ -373,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. @@ -419,18 +479,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..bf28d2c --- /dev/null +++ b/cpp/src/errors.h @@ -0,0 +1,43 @@ +// Exception types for BMAP protocol errors. +// +// All 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_; +}; + +/// 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 +/// 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/src/main.cpp b/cpp/src/main.cpp index 070bd70..033dee0 100644 --- a/cpp/src/main.cpp +++ b/cpp/src/main.cpp @@ -247,7 +247,12 @@ int main(int argc, char** argv) { try { dev.set_mode(cmd); std::cout << "OK: " << cmd << "\n"; - } 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 7f55523..6ee2a15 100644 --- a/cpp/tests/test_connection.cpp +++ b/cpp/tests/test_connection.cpp @@ -392,3 +392,206 @@ 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)); +} + +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 e083a43..1fc6248 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -457,8 +457,12 @@ This makes feature dispatch a simple dict lookup at runtime. ``` BmapError ├── BmapConnectionError — socket/transport failures +│ └── BmapDesyncError — no frame in the reply came from the +│ requested [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 +527,7 @@ pub enum BmapError { NotFound(String), Unsupported(String), InvalidArg(String), + Desync(String), // no reply frame came from the requested [fblock.func] } ``` @@ -562,13 +567,17 @@ 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 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), `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/__init__.py b/python/pybmap/__init__.py index 1e09f27..bd689c3 100644 --- a/python/pybmap/__init__.py +++ b/python/pybmap/__init__.py @@ -26,6 +26,7 @@ from .errors import ( BmapError, BmapConnectionError, BmapAuthError, BmapDeviceError, BmapTimeoutError, BmapNotFoundError, BmapInvalidArgError, + BmapDesyncError, ) from .types import ( BatteryReading, BatteryStatus, BmapResponse, ButtonMapping, DeviceStatus, 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/pybmap/connection.py b/python/pybmap/connection.py index 60b6071..198be73 100644 --- a/python/pybmap/connection.py +++ b/python/pybmap/connection.py @@ -16,8 +16,11 @@ OP_PROCESSING, SIDETONE_NAMES, SIDETONE_VALUES, SPATIAL_VALUES, VOICE_LANGUAGES, ) -from .protocol import bmap_packet, parse_response, parse_all_responses, fmt_response -from .errors import BmapError, BmapAuthError, BmapDeviceError +from .protocol import bmap_packet, parse_all_responses, fmt_response +from .errors import ( + BmapError, BmapAuthError, BmapDeviceError, BmapDesyncError, + BmapInvalidArgError, +) from .types import AudioSettings, BatteryStatus, DeviceStatus from .devices import parsers @@ -54,17 +57,52 @@ def _feature(self, name): ) return features[name] + def _select_reply(self, data, fblock, func): + """Pick the frame answering [fblock.func] 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 raise instead. + + Returns None when the buffer holds no valid frame at all. + """ + 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." + % (frames[0].fblock, frames[0].func, fblock, func) + ) + return None + + 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._select_reply(data, 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 = parse_response(resp) - 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(resp, fblock, func).payload def _get(self, feature_name): """Send a GET request and return its parsed payload.""" @@ -82,10 +120,7 @@ def _setget(self, feature_name, payload): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_SETGET, payload) ) - parsed = parse_response(resp) - if parsed and parsed.op == OP_ERROR: - self._raise_error(parsed) - return parsed + return self._check_reply(resp, fblock, func) def _start(self, feature_name, payload=b""): """Send a START request and return the parsed response.""" @@ -94,10 +129,7 @@ def _start(self, feature_name, payload=b""): resp = self._transport.send_recv( bmap_packet(fblock, func, OP_START, payload) ) - parsed = parse_response(resp) - if parsed and parsed.op == OP_ERROR: - self._raise_error(parsed) - return parsed + return self._check_reply(resp, fblock, func) def _start_drain(self, feature_name, payload=b""): """Send a START request and drain all responses.""" @@ -109,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 @@ -197,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 = parse_response(resp) + parsed = self._select_reply(resp, fblock, func) if parsed and parsed.payload: return parsed.payload[0] return None @@ -352,7 +390,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): @@ -395,14 +433,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").""" @@ -512,7 +547,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 @@ -528,9 +563,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.""" @@ -545,6 +578,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") @@ -552,29 +586,60 @@ 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.""" - modes = self.modes() - found = None - for idx, config in modes.items(): - if config.name.lower() == name.lower(): - found = (idx, config) - break + 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. + + 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 @@ -670,13 +735,18 @@ 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. Same rule as find_free_slot() in the Rust + and C++ libraries. + """ 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/pybmap/errors.py b/python/pybmap/errors.py index ae349a3..72a0983 100644 --- a/python/pybmap/errors.py +++ b/python/pybmap/errors.py @@ -31,3 +31,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/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_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/python/tests/test_connection.py b/python/tests/test_connection.py index fa726ad..ce3f466 100644 --- a/python/tests/test_connection.py +++ b/python/tests/test_connection.py @@ -7,8 +7,12 @@ 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.devices import qc_ultra2, qc_ultra2_earbuds, qc_prince +from pybmap.errors import ( + BmapError, BmapAuthError, BmapDeviceError, + BmapDesyncError, BmapConnectionError, BmapInvalidArgError, +) +from pybmap.devices import qc_ultra2, qc_ultra2_earbuds, qc_prince, qc45 +from pybmap.types import ModeConfig EARBUDS_BATTERY_FIXTURE = bytes.fromhex( @@ -434,3 +438,198 @@ 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") + + +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) + + +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")] + + +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 5af5b54..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,33 +45,63 @@ impl BmapConnection { )) } - 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 { + /// 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. + /// + /// 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.", + fblock, func, addr.0, addr.1 + ))), + None => Ok(None), + } + } + + /// 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 = self.select_reply(data, addr)?.ok_or_else(|| BmapError::Device { message: "Invalid or empty response".into(), code: 0, })?; 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_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_error(&resp)?; - Ok(resp) + self.check_reply(&data, addr) } /// Send START and drain all async responses. @@ -264,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 { @@ -277,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(), }) @@ -351,12 +394,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) { @@ -483,9 +521,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(()) } @@ -568,6 +604,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) @@ -576,8 +613,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))); @@ -594,11 +634,32 @@ 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. 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.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, } @@ -1120,4 +1181,205 @@ 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))); + } + + #[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/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), 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(); 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 {