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.
This commit is contained in:
@@ -23,7 +23,7 @@ import logging
|
|||||||
import threading
|
import threading
|
||||||
import time
|
import time
|
||||||
from collections.abc import Callable, Iterable
|
from collections.abc import Callable, Iterable
|
||||||
from dataclasses import dataclass, field
|
from dataclasses import dataclass, field, replace
|
||||||
from functools import cache
|
from functools import cache
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
@@ -70,15 +70,27 @@ class Timeouts:
|
|||||||
|
|
||||||
@dataclass
|
@dataclass
|
||||||
class Reading:
|
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
|
value: Any
|
||||||
at: float # time.monotonic()
|
at: float # time.monotonic()
|
||||||
count: int = 1
|
count: int = 1
|
||||||
|
sources: dict[int, Any] = field(default_factory=dict)
|
||||||
|
|
||||||
def age(self, now: float | None = None) -> float:
|
def age(self, now: float | None = None) -> float:
|
||||||
return (now if now is not None else time.monotonic()) - self.at
|
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
|
@dataclass
|
||||||
class _Waiter:
|
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]}"
|
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`.
|
"""Normalise whatever the codec returned into an `Event`.
|
||||||
|
|
||||||
`decode_event` returns `(ids, values)`, which is the second branch. The
|
`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:
|
if decoded is None:
|
||||||
return None
|
return None
|
||||||
if isinstance(decoded, Event):
|
if isinstance(decoded, Event):
|
||||||
return decoded
|
return decoded if decoded.buffer_id else replace(decoded, buffer_id=buffer_id)
|
||||||
ids = getattr(decoded, "ids", None)
|
ids = getattr(decoded, "ids", None)
|
||||||
name = getattr(decoded, "name", None)
|
name = getattr(decoded, "name", None)
|
||||||
values = getattr(decoded, "values", 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]
|
name=str(name) if name else _name_for(ids), # type: ignore[arg-type]
|
||||||
values=dict(values or {}),
|
values=dict(values or {}),
|
||||||
at=getattr(decoded, "at", None) or time.monotonic(),
|
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:
|
if len(frame.payload) < COMMAND_HEADER.size:
|
||||||
return
|
return
|
||||||
try:
|
try:
|
||||||
event = _as_event(self._decode(frame.payload), frame.payload)
|
event = _as_event(self._decode(frame.payload), frame.payload, frame.buffer_id)
|
||||||
except Exception:
|
except Exception:
|
||||||
# An event we cannot decode is a gap in coverage, not a reason to
|
# 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.
|
# drop the link: the next frame may be the one that matters.
|
||||||
@@ -282,7 +295,9 @@ class DroneSession:
|
|||||||
with self._lock:
|
with self._lock:
|
||||||
for key, value in values.items():
|
for key, value in values.items():
|
||||||
prior = self._store.get(key)
|
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)]
|
hit = [w for w in self._waiters if event.ids in w.ids and w.predicate(event)]
|
||||||
for waiter in hit:
|
for waiter in hit:
|
||||||
waiter.hit = event
|
waiter.hit = event
|
||||||
@@ -309,7 +324,16 @@ class DroneSession:
|
|||||||
if keys is not None:
|
if keys is not None:
|
||||||
wanted = list(keys)
|
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)}
|
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]:
|
def values(self, keys: Iterable[str] | None = None) -> dict[str, Any]:
|
||||||
"""The same thing without the staleness wrapper, for internal checks."""
|
"""The same thing without the staleness wrapper, for internal checks."""
|
||||||
|
|||||||
@@ -90,6 +90,14 @@ class Event:
|
|||||||
name: str # the command name, e.g. "BatteryStateChanged"
|
name: str # the command name, e.g. "BatteryStateChanged"
|
||||||
values: dict[str, Any] # keyed `<Command>_<arg>`, as our captures are
|
values: dict[str, Any] # keyed `<Command>_<arg>`, as our captures are
|
||||||
at: float # time.monotonic()
|
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):
|
class HandshakeError(RuntimeError):
|
||||||
|
|||||||
@@ -64,6 +64,14 @@ class SendResult(BaseModel):
|
|||||||
class StateValue(BaseModel):
|
class StateValue(BaseModel):
|
||||||
value: object
|
value: object
|
||||||
age: float = Field(description="Seconds since the drone last reported this. Old values may be stale.")
|
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):
|
class Check(BaseModel):
|
||||||
|
|||||||
@@ -28,6 +28,40 @@ def _sensor_results(values: dict) -> dict[str, bool]:
|
|||||||
return out
|
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:
|
def register(mcp: FastMCP, settings: Settings) -> None:
|
||||||
@mcp.tool(annotations={"readOnlyHint": True, "openWorldHint": False})
|
@mcp.tool(annotations={"readOnlyHint": True, "openWorldHint": False})
|
||||||
async def get_state(
|
async def get_state(
|
||||||
@@ -49,7 +83,10 @@ def register(mcp: FastMCP, settings: Settings) -> None:
|
|||||||
"""
|
"""
|
||||||
session = require_session()
|
session = require_session()
|
||||||
raw = session.state(keys or None)
|
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})
|
@mcp.tool(annotations={"readOnlyHint": True, "openWorldHint": False})
|
||||||
async def watch_state(
|
async def watch_state(
|
||||||
@@ -96,13 +133,8 @@ def register(mcp: FastMCP, settings: Settings) -> None:
|
|||||||
# one that says nothing at all.
|
# one that says nothing at all.
|
||||||
(blocking if blocks else advisory).append(f"{name}: not reported")
|
(blocking if blocks else advisory).append(f"{name}: not reported")
|
||||||
|
|
||||||
battery = v.get("BatteryStateChanged_percent")
|
battery, battery_detail = _battery(session)
|
||||||
add(
|
add("battery", None if battery is None else battery >= 30, battery_detail, blocks=True)
|
||||||
"battery",
|
|
||||||
None if battery is None else battery >= 30,
|
|
||||||
"not reported" if battery is None else f"{battery}%",
|
|
||||||
blocks=True,
|
|
||||||
)
|
|
||||||
|
|
||||||
rssi = v.get("WifiSignalChanged_rssi")
|
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")
|
add("link", None if rssi is None else rssi > -75, "not reported" if rssi is None else f"{rssi} dBm")
|
||||||
|
|||||||
@@ -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
|
||||||
Reference in New Issue
Block a user