From 907f8767731a2dcb007f4642f12205b2b97cf7b1 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Fri, 2 Oct 2026 07:00:10 -0600 Subject: [PATCH] Track which part of the aircraft reported a value, and resolve the battery The drone sends BatteryStateChanged from two places with different values: the true figure on buffer 126, corroborated by its own log and tracking a smooth discharge, and a constant 0 on buffer 127 that appears in no log line. Keeping the newest frame meant a healthy aircraft read as flat and preflight refused. Events now carry their buffer and the store keeps the last value per source, so a disagreement is visible instead of silently resolved. get_state reports the sources whenever they differ, rather than handing over a winner. preflight breaks the tie on physical grounds, not preference: an aircraft that is powered and holding a link is not at 0%, so a zero from a live drone is not credible while a non-zero one is. It says the number was contested either way, and a genuine zero from every source still reports zero and still blocks. --- src/mcbebop/arsdk/session.py | 38 +++++++++++++++++---- src/mcbebop/arsdk/types.py | 8 +++++ src/mcbebop/models.py | 8 +++++ src/mcbebop/tools/state.py | 48 ++++++++++++++++++++++----- tests/test_battery_conflict.py | 60 ++++++++++++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 15 deletions(-) create mode 100644 tests/test_battery_conflict.py diff --git a/src/mcbebop/arsdk/session.py b/src/mcbebop/arsdk/session.py index 93e31b9..3273c47 100644 --- a/src/mcbebop/arsdk/session.py +++ b/src/mcbebop/arsdk/session.py @@ -23,7 +23,7 @@ import logging import threading import time from collections.abc import Callable, Iterable -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from functools import cache from typing import Any @@ -70,15 +70,27 @@ class Timeouts: @dataclass class Reading: - """One telemetry key: the value, and when the drone last said so.""" + """One telemetry key: the value, and when the drone last said so. + + `sources` holds the last value seen from each buffer. It exists because + this aircraft sends `BatteryStateChanged` from two places with different + values, so "the newest frame" and "the truth" are not the same thing, and + a consumer needs to be able to see that rather than be handed a winner. + """ value: Any at: float # time.monotonic() count: int = 1 + sources: dict[int, Any] = field(default_factory=dict) def age(self, now: float | None = None) -> float: return (now if now is not None else time.monotonic()) - self.at + @property + def conflicting(self) -> bool: + """True when two buffers are currently reporting different values.""" + return len(set(map(repr, self.sources.values()))) > 1 + @dataclass class _Waiter: @@ -119,7 +131,7 @@ def _name_for(ids: tuple[int, int, int]) -> str: return spec.name if spec is not None else f"cmd_{ids[0]}_{ids[1]}_{ids[2]}" -def _as_event(decoded: Any, payload: bytes) -> Event | None: +def _as_event(decoded: Any, payload: bytes, buffer_id: int = 0) -> Event | None: """Normalise whatever the codec returned into an `Event`. `decode_event` returns `(ids, values)`, which is the second branch. The @@ -131,7 +143,7 @@ def _as_event(decoded: Any, payload: bytes) -> Event | None: if decoded is None: return None if isinstance(decoded, Event): - return decoded + return decoded if decoded.buffer_id else replace(decoded, buffer_id=buffer_id) ids = getattr(decoded, "ids", None) name = getattr(decoded, "name", None) values = getattr(decoded, "values", None) @@ -151,6 +163,7 @@ def _as_event(decoded: Any, payload: bytes) -> Event | None: name=str(name) if name else _name_for(ids), # type: ignore[arg-type] values=dict(values or {}), at=getattr(decoded, "at", None) or time.monotonic(), + buffer_id=buffer_id, ) @@ -265,7 +278,7 @@ class DroneSession: if len(frame.payload) < COMMAND_HEADER.size: return try: - event = _as_event(self._decode(frame.payload), frame.payload) + event = _as_event(self._decode(frame.payload), frame.payload, frame.buffer_id) except Exception: # An event we cannot decode is a gap in coverage, not a reason to # drop the link: the next frame may be the one that matters. @@ -282,7 +295,9 @@ class DroneSession: with self._lock: for key, value in values.items(): prior = self._store.get(key) - self._store[key] = Reading(value, event.at, (prior.count + 1) if prior else 1) + sources = dict(prior.sources) if prior else {} + sources[event.buffer_id] = value + self._store[key] = Reading(value, event.at, (prior.count + 1) if prior else 1, sources) hit = [w for w in self._waiters if event.ids in w.ids and w.predicate(event)] for waiter in hit: waiter.hit = event @@ -309,7 +324,16 @@ class DroneSession: if keys is not None: wanted = list(keys) items = {k: v for k, v in items.items() if k in wanted or any(k.startswith(w) for w in wanted)} - return {k: {"value": v.value, "age": round(v.age(now), 3)} for k, v in sorted(items.items())} + out: dict[str, Any] = {} + for key, reading in sorted(items.items()): + entry: dict[str, Any] = {"value": reading.value, "age": round(reading.age(now), 3)} + if reading.conflicting: + # Say so rather than hand over a winner. The newest frame is not + # necessarily the true one when two parts of the aircraft report + # the same thing differently. + entry["sources"] = dict(reading.sources) + out[key] = entry + return out def values(self, keys: Iterable[str] | None = None) -> dict[str, Any]: """The same thing without the staleness wrapper, for internal checks.""" diff --git a/src/mcbebop/arsdk/types.py b/src/mcbebop/arsdk/types.py index 37e14d1..68fbb68 100644 --- a/src/mcbebop/arsdk/types.py +++ b/src/mcbebop/arsdk/types.py @@ -90,6 +90,14 @@ class Event: name: str # the command name, e.g. "BatteryStateChanged" values: dict[str, Any] # keyed `_`, as our captures are at: float # time.monotonic() + buffer_id: int = 0 + """Which buffer carried it. + + Kept because this aircraft sends the same command from more than one place + with different values: `BatteryStateChanged` arrives as the true figure on + one buffer and as a constant 0 on another. Without knowing the source, a + store that keeps the newest value silently reports whichever spoke last. + """ class HandshakeError(RuntimeError): diff --git a/src/mcbebop/models.py b/src/mcbebop/models.py index a4b5c46..b87413e 100644 --- a/src/mcbebop/models.py +++ b/src/mcbebop/models.py @@ -64,6 +64,14 @@ class SendResult(BaseModel): class StateValue(BaseModel): value: object age: float = Field(description="Seconds since the drone last reported this. Old values may be stale.") + sources: dict[int, object] | None = Field( + default=None, + description=( + "Present only when parts of the aircraft disagree about this value, mapping the link " + "buffer each report arrived on to what it said. `value` is then merely the most recent " + "of them, not a decision about which is right." + ), + ) class Check(BaseModel): diff --git a/src/mcbebop/tools/state.py b/src/mcbebop/tools/state.py index 9a3028f..d2b8c75 100644 --- a/src/mcbebop/tools/state.py +++ b/src/mcbebop/tools/state.py @@ -28,6 +28,40 @@ def _sensor_results(values: dict) -> dict[str, bool]: return out +def _battery(session) -> tuple[int | None, str]: + """The battery percentage, and how much to trust it. + + This aircraft sends `BatteryStateChanged` from two places with different + values: the real figure, which its own log corroborates and which tracks a + smooth discharge, and a constant 0 that appears in no log line. Taking the + newest frame therefore reports a healthy drone as flat. + + The tie is broken on physical grounds rather than preference: an aircraft + that is powered, holding a link and streaming telemetry is not at 0%, so a + zero from a live drone is not a credible reading while a non-zero one is. + The disagreement is reported either way, because a caller should know the + number was contested. + """ + raw = session.state(["BatteryStateChanged_percent"]).get("BatteryStateChanged_percent") + if raw is None: + return None, "not reported" + + sources = raw.get("sources") or {} + distinct = {v for v in sources.values()} + if len(distinct) < 2: + return raw["value"], f"{raw['value']}%" + + credible = sorted(v for v in distinct if isinstance(v, int) and v > 0) + if not credible: + return None, f"sources disagree but none is credible: {sources}" + best = credible[-1] + others = sorted(distinct - {best}) + return best, ( + f"{best}%, contested: the aircraft also reports {others} on another channel. " + "A drone that is powered and holding a link is not at 0%, so that reading is disregarded." + ) + + def register(mcp: FastMCP, settings: Settings) -> None: @mcp.tool(annotations={"readOnlyHint": True, "openWorldHint": False}) async def get_state( @@ -49,7 +83,10 @@ def register(mcp: FastMCP, settings: Settings) -> None: """ session = require_session() raw = session.state(keys or None) - return {k: StateValue(value=v["value"], age=round(v["age"], 2)) for k, v in raw.items()} + return { + k: StateValue(value=v["value"], age=round(v["age"], 2), sources=v.get("sources")) + for k, v in raw.items() + } @mcp.tool(annotations={"readOnlyHint": True, "openWorldHint": False}) async def watch_state( @@ -96,13 +133,8 @@ def register(mcp: FastMCP, settings: Settings) -> None: # one that says nothing at all. (blocking if blocks else advisory).append(f"{name}: not reported") - battery = v.get("BatteryStateChanged_percent") - add( - "battery", - None if battery is None else battery >= 30, - "not reported" if battery is None else f"{battery}%", - blocks=True, - ) + battery, battery_detail = _battery(session) + add("battery", None if battery is None else battery >= 30, battery_detail, blocks=True) rssi = v.get("WifiSignalChanged_rssi") add("link", None if rssi is None else rssi > -75, "not reported" if rssi is None else f"{rssi} dBm") diff --git a/tests/test_battery_conflict.py b/tests/test_battery_conflict.py new file mode 100644 index 0000000..42277b5 --- /dev/null +++ b/tests/test_battery_conflict.py @@ -0,0 +1,60 @@ +"""The aircraft reports its battery twice, with different values. + +Observed on the live drone 2026-10-02: `BatteryStateChanged` (0-5-1) arrives as +`00 05 01 00 40` on buffer 126 and `00 05 01 00 00` on buffer 127, repeatedly. +The 64 is corroborated by the drone's own log and tracks a smooth discharge; the +zeros appear in no log line. A store that keeps the newest frame therefore +reported a healthy aircraft as flat, and preflight refused on it. +""" + +import pytest + +from mcbebop.arsdk.session import Reading +from mcbebop.arsdk.types import Event +from mcbebop.tools.state import _battery + +KEY = "BatteryStateChanged_percent" + + +class FakeSession: + def __init__(self, entry): + self.entry = entry + + def state(self, keys): + return {KEY: self.entry} if self.entry is not None else {} + + +def test_reading_detects_disagreement(): + assert Reading(0, 1.0, 4, {126: 64, 127: 0}).conflicting + assert not Reading(64, 1.0, 1, {126: 64}).conflicting + assert not Reading(64, 1.0, 2, {126: 64, 127: 64}).conflicting + + +def test_event_carries_its_buffer(): + """Without the source, the store cannot tell two reporters apart.""" + assert Event(ids=(0, 5, 1), name="x", values={}, at=0.0).buffer_id == 0 + assert Event(ids=(0, 5, 1), name="x", values={}, at=0.0, buffer_id=127).buffer_id == 127 + + +def test_the_real_conflict_resolves_to_the_credible_value(): + pct, detail = _battery(FakeSession({"value": 0, "age": 0.1, "sources": {126: 64, 127: 0}})) + assert pct == 64, "the newest frame said 0; the credible one is 64" + assert "contested" in detail and "0" in detail, "the caller must learn the number was disputed" + + +def test_a_lone_reading_is_not_described_as_contested(): + pct, detail = _battery(FakeSession({"value": 64, "age": 0.1, "sources": {126: 64}})) + assert pct == 64 and detail == "64%" + + +@pytest.mark.parametrize( + ("entry", "expected"), + [ + ({"value": 58, "age": 0.1}, 58), + ({"value": 0, "age": 0.1, "sources": {126: 0, 127: 0}}, 0), + (None, None), + ], +) +def test_other_shapes(entry, expected): + """A genuine zero from every source still reports zero, and still blocks.""" + assert _battery(FakeSession(entry))[0] == expected