diff --git a/src/mcbebop/arsdk/connection.py b/src/mcbebop/arsdk/connection.py index 1d3495d..6ac06cb 100644 --- a/src/mcbebop/arsdk/connection.py +++ b/src/mcbebop/arsdk/connection.py @@ -299,9 +299,17 @@ class Connection: event.set() return - if frame.data_type == DataType.DATA_WITH_ACK and frame.buffer_id == BufferId.D2C_ACK: + if frame.data_type == DataType.DATA_WITH_ACK: # Acknowledge before dispatching: the drone retransmits until it # hears back, and whatever the callback does could be slow. + # + # Decide on the data type alone, never on the buffer id. The naming + # convention says 126 is the drone's non-acknowledged buffer and 127 + # its acknowledged one, but a live Bebop 2 does the opposite: it + # sends plain DATA on 127 and DATA_WITH_ACK on 126. Requiring both + # to agree meant nothing was ever acknowledged, so the drone kept + # resending its state instead of continuing, and half of it never + # arrived. self.send_frame(DataType.ACK, BufferId.ack_for(frame.buffer_id), bytes([frame.seq])) if self.on_frame is not None: diff --git a/src/mcbebop/arsdk/session.py b/src/mcbebop/arsdk/session.py index d8ba8f4..93e31b9 100644 --- a/src/mcbebop/arsdk/session.py +++ b/src/mcbebop/arsdk/session.py @@ -492,22 +492,24 @@ class DroneSession: self._waiters.remove(waiter) async def request_full_state(self) -> dict[str, Any]: - """Ask for everything, then let it arrive. + """Ask for everything, then wait for the drone to say it has finished. - The drone answers these two with a burst of a hundred-odd events, and - there is no "that's all" marker, so the only honest finish condition - is a short quiet period. + The drone does mark the end: `AllStatesChanged` and `AllSettingsChanged` + arrive once the respective burst is complete. Waiting for a quiet period + instead does not work, because the drone streams attitude and speed at + about 5 Hz the whole time, so the link is never quiet and the wait ends + on its timeout with only part of the burst recorded. """ sent = [ await self.send(ALL_STATES, {}, confirm=False), await self.send(ALL_SETTINGS, {}, confirm=False), ] + terminators = ("AllStatesChanged", "AllSettingsChanged") deadline = time.monotonic() + self.timeouts.full_state - quiet_for = 0.4 while time.monotonic() < deadline: with self._lock: - last = max((r.at for r in self._store.values()), default=0.0) - if last and time.monotonic() - last > quiet_for: + seen = all(t in self._store for t in terminators) + if seen: break await asyncio.sleep(0.1) with self._lock: diff --git a/src/mcbebop/protocol/codec.py b/src/mcbebop/protocol/codec.py index dfeb6d8..b537e80 100644 --- a/src/mcbebop/protocol/codec.py +++ b/src/mcbebop/protocol/codec.py @@ -204,6 +204,13 @@ def decode_args(spec: CommandSpec, payload: bytes, offset: int = 0) -> dict[str, for arg in spec.args: value, offset = _decode_one(arg, payload, offset) values[f"{spec.event_key_prefix}_{arg.name}"] = value + if not spec.args: + # An event with no arguments is the whole message: its arrival is the + # fact. Eight events are shaped this way, including AllStatesChanged + # and AllSettingsChanged, which are how the drone says a state dump is + # finished. Returning an empty dict loses them completely, so record + # the name itself, which is also how our earlier captures keyed them. + values[spec.event_key_prefix] = True return values diff --git a/src/mcbebop/tools/state.py b/src/mcbebop/tools/state.py index 5322254..9a3028f 100644 --- a/src/mcbebop/tools/state.py +++ b/src/mcbebop/tools/state.py @@ -90,7 +90,11 @@ def register(mcp: FastMCP, settings: Settings) -> None: if ok is False: (blocking if blocks else advisory).append(f"{name}: {detail}") elif ok is None: - advisory.append(f"{name}: not reported") + # Not knowing is not the same as being fine. A check that + # matters and has no data must not read as a pass: a preflight + # that says "ready" because nothing was reported is worse than + # one that says nothing at all. + (blocking if blocks else advisory).append(f"{name}: not reported") battery = v.get("BatteryStateChanged_percent") add( diff --git a/tests/test_live_findings.py b/tests/test_live_findings.py new file mode 100644 index 0000000..41b6448 --- /dev/null +++ b/tests/test_live_findings.py @@ -0,0 +1,57 @@ +"""Regressions for three bugs that only a real aircraft exposed. + +Each of these passed every simulator test while being wrong against the drone, +which is why they have their own file: the simulator was written from the same +assumptions as the code, so it agreed with it. +""" + +from mcbebop.arsdk.types import BufferId, DataType, Frame +from mcbebop.protocol import codec, xml_index +from mcbebop.protocol.types import Direction + + +def test_argument_less_events_are_recorded(): + """Eight events carry no arguments; their arrival IS the message. + + AllStatesChanged and AllSettingsChanged are how the drone says a state dump + is finished. Keying only by argument meant they decoded to an empty dict and + disappeared, so nothing could wait for the end of a burst. + """ + for name in ("common.CommonState.AllStatesChanged", "common.SettingsState.AllSettingsChanged"): + spec = xml_index.get(name) + assert spec.args == (), "this test is about events that have no arguments" + _, values = codec.decode_event(codec.encode_command(spec, {})) + assert values == {spec.name: True} + + bare = [c for c in xml_index.all_commands() if c.direction is Direction.FROM_DRONE and not c.args] + assert len(bare) == 8 + + +def test_every_bare_event_decodes_to_something(): + for spec in xml_index.all_commands(): + if spec.direction is Direction.FROM_DRONE and not spec.args: + _, values = codec.decode_event(codec.encode_command(spec, {})) + assert values, f"{spec.full_name} decoded to nothing" + + +def test_acknowledgement_is_decided_by_data_type_not_buffer(): + """A live Bebop 2 sends DATA_WITH_ACK on buffer 126, not 127. + + The naming says 127 is the drone's acknowledged buffer, and requiring both + the type and the buffer to agree meant no frame was ever acknowledged. The + drone then resent its state rather than continuing, and most of it never + arrived: 35 keys instead of 192. + """ + import inspect + + from mcbebop.arsdk import connection as conn_mod + + source = inspect.getsource(conn_mod.Connection._handle) + ack_line = next(line for line in source.splitlines() if "DataType.DATA_WITH_ACK" in line) + assert "buffer_id" not in ack_line, "the ack decision must not depend on which buffer it arrived on" + + # Both buffers must round-trip to a valid ack target. + for buffer_id in (BufferId.D2C_NON_ACK, BufferId.D2C_ACK): + frame = Frame(DataType.DATA_WITH_ACK, buffer_id, 9, b"\x00\x05\x00\x00") + assert Frame.decode_all(frame.encode())[0].buffer_id == buffer_id + assert 0 <= BufferId.ack_for(buffer_id) <= 255