Skip to content

core: services: ardupilot_manager: Return none if board path is gone - #4351

Open
patrickelectric wants to merge 1 commit into
bluerobotics:masterfrom
patrickelectric:fix-unplugged-board
Open

core: services: ardupilot_manager: Return none if board path is gone#4351
patrickelectric wants to merge 1 commit into
bluerobotics:masterfrom
patrickelectric:fix-unplugged-board

Conversation

@patrickelectric

Copy link
Copy Markdown
Member

GET /board keeps the last USB board after unplug.
Fix #1974

@github-actions

Copy link
Copy Markdown

Automated PR Review

0. Summary

  • Verdict: MINOR SUGGESTIONS ✏️

Fixes issue #1974 by making GET /board return None when the currently-tracked board is a serial device whose path (e.g. /dev/ttyACM0) no longer exists on the filesystem — i.e. the USB flight controller has been unplugged. The change is minimal, correctly guards against board is None and board.path is None, and works for the three relevant FlightController variants (serial has a path, SITL/Manual/Linux don't).

1. Correctness & Implementation Bugs

  • 1.1 [minor] core/services/ardupilot_manager/api/v1/routers/index.py:223 — the fix only masks the stale value at this endpoint; autopilot.current_board still holds the unplugged board internally. Any other consumer of autopilot.current_board (e.g. set_board, restart flows, or the internal current_sitl_frame/change_board logic in autopilot_manager.py:566) will still see the ghost board until something else clears it. Consider clearing autopilot.current_board at the point of detection (either here, or — better — from the detector loop that already knows a board disappeared), so the fix is a real state reconciliation rather than a per-endpoint filter. Not a blocker for [autopilot] No board connected, but it says Pixhawk #1974, but worth a follow-up.

6. Code Quality & Style

  • 6.1 [nit] core/services/ardupilot_manager/api/v1/routers/index.py:223-226 — the temporary board local is only used to avoid repeating autopilot.current_board, which reads fine as-is. A one-liner such as
    board = autopilot.current_board
    if board and board.path and not Path(board.path).exists():
        return None
    return board
    is what you already have — this is fine; flagging only that a short explanatory comment (e.g. # USB flight controller was unplugged) would help a future reader understand why an existing board is being suppressed, since without context the .exists() check looks arbitrary.

7. Tests

  • 7.1 [minor] No unit test added for the new branch. A small test that stubs autopilot.current_board with a FlightController(path="/nonexistent/tty") and asserts get_board() returns None would lock the behavior in and cost almost nothing. ./.hooks/pre-push runs pytest with coverage; a regression here would otherwise slip by silently.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.

@patrickelectric
patrickelectric requested a review from a team August 29, 2026 02:41
@patrickelectric patrickelectric added the move-to-stable Needs to be cherry-picked and move to stable label Aug 31, 2026

@joaoantoniocardoso joaoantoniocardoso left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this does produce the desired effect for the client (BlueOS UI), I'm not happy with how/where it is being implemented (in the API layer): it creates a gap between the internal state and the public representation, which usually is not the best move.

I'd say we should correct the internal state instead of the API.

@patrickelectric
patrickelectric force-pushed the fix-unplugged-board branch 2 times, most recently from 9b49932 to 67c6a01 Compare August 31, 2026 20:51
@patrickelectric

Copy link
Copy Markdown
Member Author

@joaoantoniocardoso Updated.

@patrickelectric
patrickelectric requested a review from a team August 31, 2026 20:54
GET /board keeps the last USB board after unplug.

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
@joaoantoniocardoso

Copy link
Copy Markdown
Member

Right. You basically moved the code around. We are increasing the state size by adding a layer on top of the current_board state, which is fine if needed... but I wonder if that's needed? Shouldn't we work on the input side instead of working on the output side? Like... Shouldn't the internal state know and model the fact that the board is not present instead of filtering it out through an additional state?

@patrickelectric

patrickelectric commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Right. You basically moved the code around. We are increasing the state size by adding a layer on top of the current_board state, which is fine if needed... but I wonder if that's needed? Shouldn't we work on the input side instead of working on the output side? Like... Shouldn't the internal state know and model the fact that the board is not present instead of filtering it out through an additional state?

I'm open for suggestions.
Inner logic is different from visualization logic. You can't have user visual information to match 1x1 to the internal state, they are not the same thing. Currently, the autopilot manager needs to check multiple things and current board can be different things. Is a generic information about the boar that is configured, not to the board to be visualized by the user. A board can exist internally and not be available hardware-wise, can be available but with a linux bind issue, can have multiple different states, that are not tracked by the frontend. You asked for it to exist in the backend, and now it exist. Now, is this necessary for the backend ? Currently no. Is it necessary for the front ? Yes.

@joaoantoniocardoso

joaoantoniocardoso commented Aug 31, 2026

Copy link
Copy Markdown
Member

Right. You basically moved the code around. We are increasing the state size by adding a layer on top of the current_board state, which is fine if needed... but I wonder if that's needed? Shouldn't we work on the input side instead of working on the output side? Like... Shouldn't the internal state know and model the fact that the board is not present instead of filtering it out through an additional state?

I'm open for suggestions. Inner logic is different from visualization logic. You can't have user visual information to match 1x1 to the internal state, they are not the same thing. Currently, the autopilot manager needs to check multiple things and current board can be different things. Is a generic information about the boar that is configured, not to the board to be visualized by the user. A board can exist internally and not be available hardware-wise, can be available but with a linux bind issue, can have multiple different states, that are not tracked by the frontend. You asked for it to exist in the backend, and now it exist. Now, is this necessary for the backend ? Currently no. Is it necessary for the front ? Yes.

Agreed, my stance is to check whether we are adding a state because it's now easy or adding a state because it's the representation model we want.

My suggestion is to study the input side, because of the following experiment:

  1. Connect Pixhawk via USB, configure it, and ensure it's running.
  2. Disconnect it; the UI would show the Pixhawk as the current board.
  3. Manually reboot the service.
  4. The UI now shows no board, and the service explicitly says:
2026-08-31T21:54:32.077Z | DEBUG    | autopilot_manager:get_board_to_be_used:494 - Preferred flight-controller is Pixhawk1.
2026-08-31T21:54:32.080Z | DEBUG    | autopilot_manager:get_board_to_be_used:499 - Flight-controller Pixhawk1 not connected.

Which to me means we do have the mechanism in place, just not wired to detect the board removal at runtime.

@patrickelectric

Copy link
Copy Markdown
Member Author

Right. You basically moved the code around. We are increasing the state size by adding a layer on top of the current_board state, which is fine if needed... but I wonder if that's needed? Shouldn't we work on the input side instead of working on the output side? Like... Shouldn't the internal state know and model the fact that the board is not present instead of filtering it out through an additional state?

I'm open for suggestions. Inner logic is different from visualization logic. You can't have user visual information to match 1x1 to the internal state, they are not the same thing. Currently, the autopilot manager needs to check multiple things and current board can be different things. Is a generic information about the boar that is configured, not to the board to be visualized by the user. A board can exist internally and not be available hardware-wise, can be available but with a linux bind issue, can have multiple different states, that are not tracked by the frontend. You asked for it to exist in the backend, and now it exist. Now, is this necessary for the backend ? Currently no. Is it necessary for the front ? Yes.

Agreed, my stance is to check whether we are adding a state because it's now easy or adding a state because it's the representation model we want.

My suggestion is to study the input side, because of the following experiment:

  1. Connect Pixhawk via USB, configure it, and ensure it's running.
  2. Disconnect it; the UI would show the Pixhawk as the current board.
  3. Manually reboot the service.
  4. The UI now shows no board, and the service explicitly says:
2026-08-31T21:54:32.077Z | DEBUG    | autopilot_manager:get_board_to_be_used:494 - Preferred flight-controller is Pixhawk1.
2026-08-31T21:54:32.080Z | DEBUG    | autopilot_manager:get_board_to_be_used:499 - Flight-controller Pixhawk1 not connected.

Which to me means we do have the mechanism in place, just not wired to detect the board removal at runtime.

The message that you are pointing just check if the selected board exist in the listed boards, not that's truly connected.

@joaoantoniocardoso

Copy link
Copy Markdown
Member

Ok, and it doesn't show Pixhawk on the UI after restarting the service? The model seems to know when to offer Pixhawk, just not at runtime.

@patrickelectric

Copy link
Copy Markdown
Member Author

Ok, and it doesn't show Pixhawk on the UI after restarting the service? The model seems to know when to offer Pixhawk, just not at runtime.

no, this is the current_board, if the board does not exist anymore, we don't change the current_board.

@joaoantoniocardoso

Copy link
Copy Markdown
Member

It actually offers Pixhawk at runtime if I connect the Pixhawk after initiating the service. So what it is lacking is to "unoffer"? It should, though 🤔

@joaoantoniocardoso

Copy link
Copy Markdown
Member

Perhaps:

diff --git a/core/services/ardupilot_manager/autopilot_manager.py b/core/services/ardupilot_manager/autopilot_manager.py
index daea287f9..9ea828a4c 100644
--- a/core/services/ardupilot_manager/autopilot_manager.py
+++ b/core/services/ardupilot_manager/autopilot_manager.py
@@ -195,7 +195,25 @@ class AutoPilotManager(metaclass=Singleton):
     async def auto_restart_ardupilot(self) -> None:
         """Auto-restart Ardupilot when it's not running but was supposed to."""
         while True:
-            needs_restart = self.should_be_running and not self.is_running()
+            detected = None
+            if not self._restart_lock.locked():
+                detected = await self.available_boards()
+                board = self._current_board
+                if board is not None and self._listed_as(board, detected) is None:
+                    logger.info(f"{board.name} is no longer detected.")
+                    self._current_board = None
+                    try:
+                        await self.mavlink_manager.stop()
+                    except Exception as error:
+                        logger.warning(f"Failed to stop Mavlink manager after board disconnect: {error}")
+
+            needs_restart = False
+            if detected is not None and self.should_be_running and not self.is_running():
+                try:
+                    self.get_board_to_be_used(detected)
+                    needs_restart = True
+                except RuntimeError:
+                    pass
             if needs_restart:
                 logger.debug("Restarting ardupilot...")
                 try:
@@ -487,16 +505,24 @@ class AutoPilotManager(metaclass=Singleton):
             raise NoPreferredBoardSet("Preferred board not set yet.")
         return FlightController(**preferred_board)

+    @staticmethod
+    def _listed_as(reference: FlightController, boards: List[FlightController]) -> Optional[FlightController]:
+        # Compare connected boards with saved board, excluding path (which can change between sessions)
+        for board in boards:
+            if reference.dict(exclude={"path"}).items() <= board.dict().items():
+                return board
+        return None
+
     def get_board_to_be_used(self, boards: List[FlightController]) -> FlightController:
         """Check if preferred board exists and is connected. If so, use it, otherwise, choose by priority."""
         try:
             preferred_board = self.get_preferred_board()
             logger.debug(f"Preferred flight-controller is {preferred_board.name}.")
-            for board in boards:
-                # Compare connected boards with saved board, excluding path (which can change between sessions)
-                if preferred_board.dict(exclude={"path"}).items() <= board.dict().items():
-                    return board
+            listed = self._listed_as(preferred_board, boards)
+            if listed is not None:
+                return listed
             logger.debug(f"Flight-controller {preferred_board.name} not connected.")
+            raise RuntimeError(f"Flight-controller {preferred_board.name} not connected.")
         except NoPreferredBoardSet as error:
             logger.warning(error)

~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

move-to-stable Needs to be cherry-picked and move to stable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[autopilot] No board connected, but it says Pixhawk

2 participants