Only allow one FTP operation at a time (#941)

* Only allow one FTP operation at a time

* Address copilot review comments

* Fix bug in tests

Author: 1Blademaster

radio/app/controllers/ftpController.py

@@ -26,6 +26,7 @@ def __init__(self, drone: Drone) -> None:
         self.seq: int = 0
         self.session: int = 0
         self.last_op: Optional[mavftp_op.FTP_OP] = None
+        self.current_op: Optional[str] = None
 
         # Directory listing state
         self.dir_offset: int = 0
@@ -413,46 +414,58 @@ def listFiles(self, path: str) -> Response:
         Returns:
             Response: A response object containing the status and data or error message.
         """
-        if path == "":
+        # Check if another operation is in progress
+        if self.current_op is not None:
             return {
                 "success": False,
-                "message": "Path cannot be empty",
+                "message": f"FTP operation already in progress: {self.current_op}",
             }
 
-        encoded_path = bytearray(path, "ascii")
-        directory_offset = 0
-        op = mavftp_op.FTP_OP(
-            self.seq,
-            self.session,
-            mavftp_op.OP_ListDirectory,
-            len(encoded_path),
-            0,
-            0,
-            directory_offset,
-            encoded_path,
-        )
+        try:
+            self.current_op = "list_files"
 
-        self.dir_offset = 0
-        self.list_result = []
-        self.list_temp_result = []
+            if path == "":
+                return {
+                    "success": False,
+                    "message": "Path cannot be empty",
+                }
 
-        self.drone.logger.info(f"Listing files in directory: {path}")
+            encoded_path = bytearray(path, "ascii")
+            directory_offset = 0
+            op = mavftp_op.FTP_OP(
+                self.seq,
+                self.session,
+                mavftp_op.OP_ListDirectory,
+                len(encoded_path),
+                0,
+                0,
+                directory_offset,
+                encoded_path,
+            )
 
-        self._sendFtpCommand(op)
-        response = self._processFtpResponse("list_files")
+            self.dir_offset = 0
+            self.list_result = []
+            self.list_temp_result = []
 
-        if response.get("success", False) is False:
-            return response
+            self.drone.logger.info(f"Listing files in directory: {path}")
 
-        self.drone.logger.info(
-            f"Successfully listed {len(self.list_result)} files in directory: {path}"
-        )
+            self._sendFtpCommand(op)
+            response = self._processFtpResponse("list_files")
+
+            if response.get("success", False) is False:
+                return response
 
-        return {
-            "success": True,
-            "message": "Directory listing retrieved successfully",
-            "data": self._convertDirectoryEntriesToDicts(self.list_result, path),
-        }
+            self.drone.logger.info(
+                f"Successfully listed {len(self.list_result)} files in directory: {path}"
+            )
+
+            return {
+                "success": True,
+                "message": "Directory listing retrieved successfully",
+                "data": self._convertDirectoryEntriesToDicts(self.list_result, path),
+            }
+        finally:
+            self.current_op = None
 
     def readFile(
         self, path: str, size: Optional[int] = None, offset: int = 0
@@ -468,69 +481,81 @@ def readFile(
         Returns:
             Response: A response object containing the file data or error message.
         """
-        if not path:
+        # Check if another operation is in progress
+        if self.current_op is not None:
             return {
                 "success": False,
-                "message": "File path cannot be empty",
+                "message": f"FTP operation already in progress: {self.current_op}",
             }
 
-        # Reset read state
-        self.read_buffer = BytesIO()
-        self.read_total = 0
-        self.read_gaps = []
-        self.reached_eof = False
-        self.requested_offset = offset
-        self.requested_size = (
-            size if size is not None else 0
-        )  # 0 means read entire file
-        self.remote_file_size = None
-        self.last_burst_read = None
-
-        # Send OpenFileRO command
-        encoded_path = bytearray(path, "ascii")
-        op = mavftp_op.FTP_OP(
-            self.seq,
-            self.session,
-            mavftp_op.OP_OpenFileRO,
-            len(encoded_path),
-            0,
-            0,
-            0,
-            encoded_path,
-        )
+        try:
+            self.current_op = "read_file"
 
-        self.drone.logger.info(
-            f"Reading file: {path} (offset={offset}, size={size if size else 'entire file'})"
-        )
+            if not path:
+                return {
+                    "success": False,
+                    "message": "File path cannot be empty",
+                }
 
-        self._sendFtpCommand(op)
-        response = self._processFtpResponse("read_file", timeout=30)
+            # Reset read state
+            self.read_buffer = BytesIO()
+            self.read_total = 0
+            self.read_gaps = []
+            self.reached_eof = False
+            self.requested_offset = offset
+            self.requested_size = (
+                size if size is not None else 0
+            )  # 0 means read entire file
+            self.remote_file_size = None
+            self.last_burst_read = None
+
+            # Send OpenFileRO command
+            encoded_path = bytearray(path, "ascii")
+            op = mavftp_op.FTP_OP(
+                self.seq,
+                self.session,
+                mavftp_op.OP_OpenFileRO,
+                len(encoded_path),
+                0,
+                0,
+                0,
+                encoded_path,
+            )
 
-        if response.get("success", False) is False:
-            return response
+            self.drone.logger.info(
+                f"Reading file: {path} (offset={offset}, size={size if size else 'entire file'})"
+            )
 
-        # Extract the requested portion of the data
-        self.read_buffer.seek(0)
-        all_data = self.read_buffer.read()
+            self._sendFtpCommand(op)
+            response = self._processFtpResponse("read_file", timeout=30)
 
-        if self.requested_size > 0:
-            # Return only the requested size from the requested offset
-            result_data = all_data[
-                self.requested_offset : self.requested_offset + self.requested_size
-            ]
-        else:
-            # Return entire file
-            result_data = all_data
+            if response.get("success", False) is False:
+                return response
 
-        self.drone.logger.info(
-            f"Successfully read {len(result_data)} bytes from file: {path}"
-        )
+            # Extract the requested portion of the data
+            self.read_buffer.seek(0)
+            all_data = self.read_buffer.read()
 
-        return {
-            "success": True,
-            "message": "File read successfully",
-            "data": {"file_data": result_data, "file_name": path.split("/")[-1]},
-        }
+            if self.requested_size > 0:
+                # Return only the requested size from the requested offset
+                result_data = all_data[
+                    self.requested_offset : self.requested_offset + self.requested_size
+                ]
+            else:
+                # Return entire file
+                result_data = all_data
+
+            self.drone.logger.info(
+                f"Successfully read {len(result_data)} bytes from file: {path}"
+            )
+
+            return {
+                "success": True,
+                "message": "File read successfully",
+                "data": {"file_data": result_data, "file_name": path.split("/")[-1]},
+            }
+        finally:
+            self.current_op = None
 
     def _handleOpenFileReadOnlyResponse(self, response_op: mavftp_op.FTP_OP) -> bool:
         """

radio/tests/test_ftpController.py

@@ -79,3 +79,80 @@ def test_convertDirectoryEntriesToDicts_emptyEntries_success(
     )
 
     assert result == []
+
+
+@falcon_test(pass_drone_status=True)
+def test_listFiles_blockedByCurrentOp(client: SocketIOTestClient, droneStatus):
+    """Test that listFiles is blocked when another operation is in progress"""
+    droneStatus.drone.ftpController.current_op = "read_file"
+
+    result = droneStatus.drone.ftpController.listFiles("/")
+
+    assert result == {
+        "success": False,
+        "message": "FTP operation already in progress: read_file",
+    }
+
+    droneStatus.drone.ftpController.current_op = None
+
+
+@falcon_test(pass_drone_status=True)
+def test_readFile_blockedByCurrentOp(client: SocketIOTestClient, droneStatus):
+    """Test that readFile is blocked when another operation is in progress"""
+    droneStatus.drone.ftpController.current_op = "list_files"
+
+    result = droneStatus.drone.ftpController.readFile("/test.txt")
+
+    assert result == {
+        "success": False,
+        "message": "FTP operation already in progress: list_files",
+    }
+
+    droneStatus.drone.ftpController.current_op = None
+
+
+@falcon_test(pass_drone_status=True)
+def test_currentOp_clearedOnListFilesError(client: SocketIOTestClient, droneStatus):
+    """Test that current_op is cleared even when listFiles fails"""
+    assert droneStatus.drone.ftpController.current_op is None
+
+    result = droneStatus.drone.ftpController.listFiles("")
+
+    assert result == {"success": False, "message": "Path cannot be empty"}
+    # Verify current_op is still None after error
+    assert droneStatus.drone.ftpController.current_op is None
+
+
+@falcon_test(pass_drone_status=True)
+def test_currentOp_clearedOnReadFileError(client: SocketIOTestClient, droneStatus):
+    """Test that current_op is cleared even when readFile fails"""
+    assert droneStatus.drone.ftpController.current_op is None
+
+    result = droneStatus.drone.ftpController.readFile("")
+
+    assert result == {"success": False, "message": "File path cannot be empty"}
+
+    # Verify current_op is still None after error
+    assert droneStatus.drone.ftpController.current_op is None
+
+
+@falcon_test(pass_drone_status=True)
+def test_multipleOperations_sequential(client: SocketIOTestClient, droneStatus):
+    """Test that operations can run sequentially after previous one completes"""
+    # Verify current_op is None initially
+    assert droneStatus.drone.ftpController.current_op is None
+
+    # First operation with empty path (will fail but clear current_op)
+    result1 = droneStatus.drone.ftpController.listFiles("")
+    assert result1 == {"success": False, "message": "Path cannot be empty"}
+    assert droneStatus.drone.ftpController.current_op is None
+
+    # Second operation should be allowed since first is complete
+    result2 = droneStatus.drone.ftpController.readFile("")
+    assert result2 == {"success": False, "message": "File path cannot be empty"}
+    assert droneStatus.drone.ftpController.current_op is None
+
+    # Third operation should also be allowed
+    result3 = droneStatus.drone.ftpController.listFiles("")
+    assert result3 == {"success": False, "message": "Path cannot be empty"}
+    assert droneStatus.drone.ftpController.current_op is None