Three fixes the real aircraft found, that the simulator could not
Acknowledge on data type alone, never on buffer id. A live Bebop 2 sends DATA_WITH_ACK on buffer 126 and plain DATA on 127, the opposite of what the buffer names imply, so requiring both to agree meant nothing was ever acknowledged. The drone resent its state instead of continuing: 35 telemetry keys where there should be 192, and a connect that took 4 s instead of 1.7. Record argument-less events. Eight events carry no arguments and their arrival is the whole message, including AllStatesChanged and AllSettingsChanged, which mark the end of a state dump. Keying only by argument decoded them to an empty dict and lost them. Wait for those terminators instead of a quiet period. The drone streams attitude at about 5 Hz throughout, so the link is never quiet and the wait always ran to its timeout with a partial burst. Also: preflight no longer reports ready when a blocking check has no data. It answered 'ready' on an aircraft it knew almost nothing about, which is worse than refusing to answer.
This commit is contained in:
@@ -299,9 +299,17 @@ class Connection:
|
|||||||
event.set()
|
event.set()
|
||||||
return
|
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
|
# Acknowledge before dispatching: the drone retransmits until it
|
||||||
# hears back, and whatever the callback does could be slow.
|
# 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]))
|
self.send_frame(DataType.ACK, BufferId.ack_for(frame.buffer_id), bytes([frame.seq]))
|
||||||
|
|
||||||
if self.on_frame is not None:
|
if self.on_frame is not None:
|
||||||
|
|||||||
@@ -492,22 +492,24 @@ class DroneSession:
|
|||||||
self._waiters.remove(waiter)
|
self._waiters.remove(waiter)
|
||||||
|
|
||||||
async def request_full_state(self) -> dict[str, Any]:
|
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
|
The drone does mark the end: `AllStatesChanged` and `AllSettingsChanged`
|
||||||
there is no "that's all" marker, so the only honest finish condition
|
arrive once the respective burst is complete. Waiting for a quiet period
|
||||||
is a short 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 = [
|
sent = [
|
||||||
await self.send(ALL_STATES, {}, confirm=False),
|
await self.send(ALL_STATES, {}, confirm=False),
|
||||||
await self.send(ALL_SETTINGS, {}, confirm=False),
|
await self.send(ALL_SETTINGS, {}, confirm=False),
|
||||||
]
|
]
|
||||||
|
terminators = ("AllStatesChanged", "AllSettingsChanged")
|
||||||
deadline = time.monotonic() + self.timeouts.full_state
|
deadline = time.monotonic() + self.timeouts.full_state
|
||||||
quiet_for = 0.4
|
|
||||||
while time.monotonic() < deadline:
|
while time.monotonic() < deadline:
|
||||||
with self._lock:
|
with self._lock:
|
||||||
last = max((r.at for r in self._store.values()), default=0.0)
|
seen = all(t in self._store for t in terminators)
|
||||||
if last and time.monotonic() - last > quiet_for:
|
if seen:
|
||||||
break
|
break
|
||||||
await asyncio.sleep(0.1)
|
await asyncio.sleep(0.1)
|
||||||
with self._lock:
|
with self._lock:
|
||||||
|
|||||||
@@ -204,6 +204,13 @@ def decode_args(spec: CommandSpec, payload: bytes, offset: int = 0) -> dict[str,
|
|||||||
for arg in spec.args:
|
for arg in spec.args:
|
||||||
value, offset = _decode_one(arg, payload, offset)
|
value, offset = _decode_one(arg, payload, offset)
|
||||||
values[f"{spec.event_key_prefix}_{arg.name}"] = value
|
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
|
return values
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -90,7 +90,11 @@ def register(mcp: FastMCP, settings: Settings) -> None:
|
|||||||
if ok is False:
|
if ok is False:
|
||||||
(blocking if blocks else advisory).append(f"{name}: {detail}")
|
(blocking if blocks else advisory).append(f"{name}: {detail}")
|
||||||
elif ok is None:
|
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")
|
battery = v.get("BatteryStateChanged_percent")
|
||||||
add(
|
add(
|
||||||
|
|||||||
@@ -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
|
||||||
Reference in New Issue
Block a user