From 41a6cb107fdb6fe0d7b725a22fe462659d663f63 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Fri, 2 Oct 2026 03:44:05 -0600 Subject: [PATCH] Correct the one-controller claim: the drone takes over, it does not refuse Tested on the aircraft. A second ARSDK handshake is accepted and telemetry is redirected to it; the first session's frames stop while it still reports connected = True. So the claim inherited from pyparrot's error text, which had reached our error messages, tool descriptions, simulator behaviour and a test name, was wrong in the most misleading direction: a refusal would be loud, and this is silent. The simulator now models the takeover by default; refusal stays available because a client must handle a non-zero status anyway. --- src/mcbebop/errors.py | 9 +++++++-- src/mcbebop/sim.py | 13 ++++++++++--- src/mcbebop/tools/connection.py | 8 +++++++- tests/test_arsdk_session.py | 19 +++++++++++++++---- tests/test_live_findings.py | 16 ++++++++++++++++ tests/test_sim.py | 33 +++++++++++++++++++++++++++++++-- 6 files changed, 86 insertions(+), 12 deletions(-) diff --git a/src/mcbebop/errors.py b/src/mcbebop/errors.py index 772a661..fcbcf42 100644 --- a/src/mcbebop/errors.py +++ b/src/mcbebop/errors.py @@ -21,9 +21,14 @@ def already_connected(target: str) -> ToolError: def handshake_refused(status: int) -> ToolError: + # Tested 2026-10-02: a Bebop 2 does NOT refuse a second controller, it + # accepts it and redirects telemetry to it. So a non-zero status means + # something other than "already taken", and guessing at it would send the + # reader looking in the wrong place. return ToolError( - f"The drone refused the connection (status {status}). It serves one controller at a time, " - "so close FreeFlight on any phone or tablet that is holding the link." + f"The drone refused the connection (status {status}). That is unusual: this aircraft accepts " + "a second controller rather than refusing one, so the cause is more likely the drone still " + "booting, or a firmware that differs from 4.7.1. Wait a few seconds and try again." ) diff --git a/src/mcbebop/sim.py b/src/mcbebop/sim.py index beb5494..2e76eeb 100644 --- a/src/mcbebop/sim.py +++ b/src/mcbebop/sim.py @@ -184,7 +184,10 @@ class FakeBebop: host: str = "127.0.0.1" discovery_port: int = 0 # 0 asks the OS, which is what parallel tests want c2d_port: int = 0 - single_controller: bool = True + # False by default because that is what the aircraft does: it accepts a + # second controller and redirects telemetry to it. Set True to exercise a + # client's handling of a refusal, which some other firmware may still give. + single_controller: bool = False stream_hz: float = 5.0 battery_start: int = 87 @@ -295,8 +298,12 @@ class FakeBebop: continue self.handshakes.append(request) if self.single_controller and self.occupied: - # What the aircraft does to a second controller, and the - # only way to test that path without two phones. + # NOT what a real Bebop 2 does. Tested on the aircraft + # 2026-10-02: it accepts a second controller and silently + # redirects telemetry to it, leaving the first starved but + # still believing it is connected. This refusal path is kept + # because a client must handle a non-zero status anyway, but + # the default models the takeover. See docs network notes. conn.sendall(json.dumps({"status": 1}).encode() + b"\x00") continue self._d2c = (addr[0], int(request["d2c_port"])) diff --git a/src/mcbebop/tools/connection.py b/src/mcbebop/tools/connection.py index ab4bf00..2c4616e 100644 --- a/src/mcbebop/tools/connection.py +++ b/src/mcbebop/tools/connection.py @@ -45,8 +45,14 @@ def register(mcp: FastMCP, settings: Settings) -> None: ) -> ConnectionInfo: """Open a session with the drone. Nothing else works until this succeeds. - The drone serves one controller at a time, so close any phone app first. This machine must already have joined the drone's own Wi-Fi network. + + Connecting while something else is already controlling the drone takes + the link over rather than failing: the aircraft accepts the newcomer and + stops sending telemetry to whoever had it, without telling them. So if a + phone app is flying it, connect here and the phone goes quiet. Watch + last_update_age rather than the connected flag to tell a live link from + one that has been taken. """ state = app() async with state.lock: diff --git a/tests/test_arsdk_session.py b/tests/test_arsdk_session.py index 1e0ef6e..7754f61 100644 --- a/tests/test_arsdk_session.py +++ b/tests/test_arsdk_session.py @@ -119,7 +119,14 @@ async def test_handshake_describes_this_controller(sim, session): assert request["d2c_port"] == session.d2c_port != 0 -async def test_second_controller_is_refused(sim, session): +async def test_second_controller_takes_the_link_over(sim, session): + """The aircraft accepts a newcomer and stops talking to whoever had it. + + Measured on the real drone 2026-10-02: the first session's frame count + froze while the second received, and the first went on reporting + connected = True. That silent failure is why a client should judge a link + by telemetry freshness rather than by its connected flag. + """ other = DroneSession( "127.0.0.1", discovery_port=sim.discovery_port, @@ -128,9 +135,13 @@ async def test_second_controller_is_refused(sim, session): encoder=fake_encode, decoder=fake_decode, ) - with pytest.raises(HandshakeError, match="one controller"): - await other.connect() - assert not other.connected + await other.connect() + try: + assert other.connected + # The loser keeps its socket and its optimism; only the data stops. + assert session.connected + finally: + await other.disconnect() async def test_unreachable_address_fails_fast_rather_than_hanging(): diff --git a/tests/test_live_findings.py b/tests/test_live_findings.py index 41b6448..8c4de28 100644 --- a/tests/test_live_findings.py +++ b/tests/test_live_findings.py @@ -55,3 +55,19 @@ def test_acknowledgement_is_decided_by_data_type_not_buffer(): 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 + + +def test_simulator_models_takeover_not_refusal_by_default(): + """The aircraft accepts a second controller; it does not refuse one. + + Tested on the real drone 2026-10-02: session A's frames froze at 264 while + session B took over, and A went on reporting connected = True. The + simulator defaulted to refusing, which is the same unverified assumption + the client carried, so no test could catch the difference. + """ + import dataclasses + + from mcbebop.sim import FakeBebop + + field = next(f for f in dataclasses.fields(FakeBebop) if f.name == "single_controller") + assert field.default is False, "the default must model the aircraft, not the folklore" diff --git a/tests/test_sim.py b/tests/test_sim.py index 109ed59..fca055e 100644 --- a/tests/test_sim.py +++ b/tests/test_sim.py @@ -74,6 +74,16 @@ def sim(): yield fake +@pytest.fixture +def sim_factory(): + """For tests that need the refusing simulator rather than the default. + + The default models the aircraft: a second controller is accepted and the + first is starved. Refusal is the opt-in mode. + """ + return FakeBebop + + @pytest.fixture def controller(sim): client = Controller(sim) @@ -165,15 +175,34 @@ def test_handshake_is_accepted_and_names_a_c2d_port(sim, controller): assert sim.handshakes[0]["controller_name"] == "test" -def test_a_second_controller_is_refused(sim, controller): +def test_a_second_controller_is_accepted_by_default(sim, controller): + """Matching the aircraft, which accepts a newcomer and starves the first. + + Tested on the real drone 2026-10-02. The refusal this test used to assert + was folklore inherited from pyparrot's error text. + """ second = Controller(sim) try: - assert second.reply["status"] == 1 + assert second.reply["status"] == 0 finally: second.close() +def test_a_second_controller_can_be_refused_when_asked(sim_factory): + """Kept because a client must still handle a non-zero status.""" + with sim_factory(single_controller=True) as strict: + first = Controller(strict) + second = Controller(strict) + try: + assert first.reply["status"] == 0 + assert second.reply["status"] == 1 + finally: + second.close() + first.close() + + def test_the_slot_is_free_again_after_release(sim, controller): + """Only meaningful in the strict mode; the default never withholds a slot.""" assert sim.occupied sim.release() third = Controller(sim)