more fixes

Author: kfafard

backend/app.py

@@ -5,23 +5,20 @@
 import json
 import sys
 import re
-import os
-import shlex
 import shutil
 import subprocess
 from pathlib import Path
 from typing import Any, Dict, List, Optional
 
-from fastapi import Body, FastAPI, WebSocket, WebSocketDisconnect, HTTPException
+from fastapi import FastAPI, WebSocket, WebSocketDisconnect, HTTPException
 from fastapi.middleware.cors import CORSMiddleware
 from fastapi.staticfiles import StaticFiles
-from fastapi import HTTPException
 from pydantic import BaseModel 
 from paths import ensure_user_file
 from auto_setup import ensure_can_environment, log_env_summary
 
 # Local modules
-from bus import BusManager, Frame
+from bus import BusManager
 from decoder import decode_frame, safe_hex
 from j1939_maps import PGN_NAME_MAP
 from models import ConnectRequest, SendRequest, LogStartRequest
@@ -41,23 +38,32 @@
 
 # ---------------- Privileged runner and link helpers ----------------
 
+# backend/app.py  (replace _run_priv)
 def _run_priv(cmd: list[str], check: bool = True) -> subprocess.CompletedProcess:
     """
-    Run a privileged netlink command. We try directly first (works if the Linux
-    binary has cap_net_admin/cap_net_raw). If that fails for permissions and
-    pkexec is available, we transparently retry with pkexec (GUI password).
+    Run a privileged netlink command.
+
+    Behavior:
+    - Try normally first.
+    - If it fails (non-zero exit), and it *looks* like a permission problem,
+      retry via pkexec (GUI password) when available.
+    - If check=True, raise CalledProcessError for a non-zero final result.
     """
-    try:
-        return subprocess.run(cmd, text=True, capture_output=True, check=check)
-    except subprocess.CalledProcessError as e:
-        # If permission problem, try pkexec if present
-        if ("Operation not permitted" in (e.stderr or "") or e.returncode in (1, 126)) and shutil.which("pkexec"):
-            return subprocess.run(["pkexec", *cmd], text=True, capture_output=True, check=check)
-        raise
-    except PermissionError:
-        if shutil.which("pkexec"):
-            return subprocess.run(["pkexec", *cmd], text=True, capture_output=True, check=check)
-        raise
+    proc = subprocess.run(cmd, text=True, capture_output=True)
+    if proc.returncode != 0:
+        stderr = (proc.stderr or "")
+        looks_perm = (
+            "Operation not permitted" in stderr
+            or "permission denied" in stderr.lower()
+            or proc.returncode in (1, 126, 127)
+        )
+        if looks_perm and shutil.which("pkexec"):
+            proc = subprocess.run(["pkexec", *cmd], text=True, capture_output=True)
+
+    if check and proc.returncode != 0:
+        raise subprocess.CalledProcessError(proc.returncode, cmd, output=proc.stdout, stderr=proc.stderr)
+
+    return proc
 
 def _ip_exists(iface: str) -> bool:
     r = subprocess.run(["ip", "-br", "link", "show", iface], text=True, capture_output=True)
@@ -107,26 +113,12 @@ def bus_health_snapshot_safe() -> Dict[str, Any]:
 # -----------------------------------------------------------------------------
 # Helpers for bring-up
 # -----------------------------------------------------------------------------
-def _which_any(*names: str) -> str:
-    """Return first existing absolute path for a binary name; raise if none."""
-    for n in names:
-        p = shutil.which(n)
-        if p:
-            return p
-    raise FileNotFoundError(f"Missing required tool: {', '.join(names)}")
-
-def _safe_bitrate(bps: int) -> int:
-    allowed = {125000, 250000, 500000, 1000000}
-    if bps in allowed:
-        return bps
-    raise HTTPException(status_code=400, detail=f"Unsupported bitrate {bps}")
-
 def _safe_iface(name: str) -> str:
-    import re
     if re.fullmatch(r"(v?can)\d{1,3}", name):
         return name
     raise HTTPException(status_code=400, detail=f"Bad interface name {name}")
 
+
 # -----------------------------------------------------------------------------
 # API routes
 # -----------------------------------------------------------------------------
@@ -223,7 +215,11 @@ def api_can_bringup(req: BringUpReq):
 
         # Physical SocketCAN device: DOWN -> type can bitrate -> UP
         # Bring it down first (ignore error if it's already down)
-        _run_priv(["ip", "link", "set", iface, "down"], check=False)
+        try:
+            _run_priv(["ip", "link", "set", iface, "down"], check=True)
+        except subprocess.CalledProcessError:
+            # Ignore errors like "Cannot find device" — the next steps will clarify state
+            pass
         # Configure bitrate/type (this is what fails if you try it on vcan or while UP)
         _run_priv(["ip", "link", "set", iface, "type", "can", "bitrate", str(bitrate)], check=True)
         # Bring it up
@@ -294,7 +290,6 @@ async def api_platform():
     Report the server's platform: 'linux', 'win32', 'darwin', etc.
     Frontend uses this to hide Bring Up on Windows (not needed for Kvaser).
     """
-    import sys
     return {"platform": sys.platform}
 
 # ----------------------------- Logging control -------------------------------

backend/bus.py

@@ -78,7 +78,7 @@ def open(self):
         self.bus = can.interface.Bus(
             channel=self.channel,
             bustype="socketcan",
-            receive_own_messages=True  # <— this is the key
+            receive_own_messages=True  # <— key for loopback echo on vcan
         )
         self._start_rx()
 
@@ -254,52 +254,31 @@ def health(self) -> Dict[str, Any]:
             "frames_total": self.frames_total,
         }
 
-# --- Kvaser backend (Windows) -----------------------------------------------
-# This implements a minimal Kvaser backend via python-can. It expects that
-# Kvaser CANlib drivers are installed on the machine (so that python-can can
-# load canlib32.dll). Channel naming follows "kvaser<N>" where N is an integer.
+
+# --- Kvaser backend (Windows) ------------------------------------------------
+# Present but not wired into BusManager I/O; kept for future Windows work.
 class _KvaserBus:
     def __init__(self):
         self._bus = None
         self._last_info = {}
 
     async def discover_interfaces(self):
-        """
-        Return a small set of candidate channel names the UI can offer.
-        We can't reliably enumerate Kvaser channels portably without extra deps,
-        so provide a few common indices. The user will usually pick kvaser0.
-        """
         return [f"kvaser{i}" for i in range(4)]
 
     async def connect(self, channel: str, bitrate: int):
-        """
-        Open a Kvaser channel: interface='kvaser', channel=<index>, bitrate=<bps>.
-        'channel' is expected as 'kvaser0', 'kvaser1', etc.
-        """
         import can  # python-can
         try:
             if not channel.lower().startswith("kvaser"):
                 return False, f"invalid channel name '{channel}'. use 'kvaser0', 'kvaser1', etc."
             idx = int(channel.replace("kvaser", ""))
-
-            # Close previous if any
             if self._bus is not None:
                 try:
                     self._bus.shutdown()
                 except Exception:
                     pass
                 self._bus = None
-
-            self._bus = can.interface.Bus(
-                interface="kvaser",
-                channel=idx,
-                bitrate=bitrate,
-            )
-            self._last_info = {
-                "backend": "kvaser",
-                "channel": idx,
-                "bitrate": bitrate,
-            }
+            self._bus = can.interface.Bus(interface="kvaser", channel=idx, bitrate=bitrate)
+            self._last_info = {"backend": "kvaser", "channel": idx, "bitrate": bitrate}
             return True, f"connected to {channel} @ {bitrate} bps"
         except Exception as e:
             return False, f"Failed to open {channel}: {e}"
@@ -367,43 +346,17 @@ def _list_socketcan_names() -> List[str]:
 
 
 # ──────────────────────────────────────────────────────────────────────────────
-# Front-end facing manager (fixed deadlock + offloaded blocking calls)
+# Front-end facing manager (adds _lock/_bus/_info and avoids pre-instantiation)
 # ──────────────────────────────────────────────────────────────────────────────
 
 class BusManager:
     def __init__(self):
-        self._impl = None
-        self._impl_name = None
+        # Runtime state guarded by _lock
+        self._bus = None                      # active low-level bus
+        self._info: Dict[str, Any] = {}       # metadata for /health
+        self._lock = asyncio.Lock()           # prevents concurrent connect/disconnect
 
-        # Prefer SocketCAN on Linux
-        if sys.platform.startswith("linux"):
-            try:
-                self._impl = _SocketCANBus()
-                self._impl_name = "socketcan"
-            except Exception:
-                pass
-
-        # If Intrepid (ics) is installed, allow that to override on any platform.
-        # We probe for the module without importing it to keep Pylance happy.
-        try:
-            if importlib.util.find_spec("ics") is not None:
-                self._impl = _IntrepidBus()
-                self._impl_name = "intrepid"
-        except Exception:
-            pass
-
-        # On Windows, prefer Kvaser if python-can is present.
-        if sys.platform.startswith("win"):
-            try:
-                if importlib.util.find_spec("can") is not None:
-                    self._impl = _KvaserBus()
-                    self._impl_name = "kvaser"
-            except Exception:
-                # Leave whatever impl we already selected (e.g., Intrepid)
-                pass
-
-
-# ---- Discovery -----------------------------------------------------------
+    # ---- Discovery -----------------------------------------------------------
 
     async def discover_interfaces(self) -> List[str]:
         tasks = [
@@ -416,6 +369,7 @@ async def discover_interfaces(self) -> List[str]:
                 results.append(await t)  # type: ignore[arg-type]
             except Exception:
                 results.append([])
+
         out: List[str] = []
         seen: set[str] = set()
         for group in results:
@@ -427,11 +381,10 @@ async def discover_interfaces(self) -> List[str]:
 
     # ---- Connect / Disconnect ----------------------------------------------
 
-    # INTERNAL: do not call without holding self._lock
     async def _disconnect_no_lock(self) -> None:
+        """Close current bus without acquiring _lock (caller must hold it)."""
         if self._bus is not None:
             try:
-                # offload potential blocking close
                 await asyncio.to_thread(self._bus.close)  # type: ignore[attr-defined]
             except Exception:
                 pass
@@ -444,13 +397,13 @@ async def connect(self, channel: str, bitrate: Optional[int] = None) -> Tuple[bo
         Offloads hardware open to a thread to avoid blocking the event loop.
         """
         async with self._lock:
-            # FIX: avoid deadlock by calling the no-lock variant
             await self._disconnect_no_lock()
             try:
                 if channel.startswith("intrepid"):
+                    if not HAS_INTREPID:
+                        return False, "Intrepid library not available"
                     idx = int(channel.replace("intrepid", "") or "0")
                     b = _IntrepidBus(device_index=idx, bitrate=bitrate)
-                    # offload blocking open
                     await asyncio.to_thread(b.open)
                     self._bus = b
                     name = ""
@@ -469,7 +422,6 @@ async def connect(self, channel: str, bitrate: Optional[int] = None) -> Tuple[bo
                     if not HAS_PYCAN:
                         return False, "python-can not available"
                     b = _SocketCANBus(channel=channel, bitrate=bitrate)
-                    # offload blocking open
                     await asyncio.to_thread(b.open)
                     self._bus = b
                     self._info = {