From 5a2f9caf0beb3f32d6426dfeca7a32f486b8e586 Mon Sep 17 00:00:00 2001 From: Halbo <86326485+halbothpa@users.noreply.github.com> Date: Thu, 14 May 2026 12:11:03 +0200 Subject: [PATCH] foundation: fix broken first-run + storage --json + reexec_in_venv The plugin's first-run experience was broken end-to-end on a fresh install. Six bugs (BUGS.md #1-6) chained into a confusing failure mode where bootstrap exited 0, then every RpcSession() raised "bindings not generated". This commit fixes all six in one pass so a clean install just works. * bootstrap.py: - Track upstream rename: app.proto -> application.proto. Drop subghz.proto which 404s from the dev branch and isn't imported. - Hold the .downloaded marker until ALL files download successfully; raise on any failure instead of warning-and-continuing. - Markers are now JSON recording the exact file set, so an upstream rename forces a re-download instead of trusting a stale marker. - Make protoc failures fatal (was: silent warning). - Add verify_proto_imports() self-check after generation. - Skip reexec_in_venv() when sys.argv[0] is "" / "-c" / "-" so ad-hoc `python -c "...bootstrap.reexec_in_venv()"` no longer crashes with `Argument expected for the -c option`. * flipper_core.py: - Alias `application_pb2 as app_pb2` so existing references in this file keep working without a sweeping rename. - Surface the real ImportError in RpcSession.__init__ instead of the misleading "bindings not generated" message. * flipper_storage.py: - Move --port / --json to a parent parser so they parse correctly whether placed before or after the subcommand (was: rejected after). * BUGS.md / .gitignore: mark fixed bugs, ignore renamed marker files. Unblocks PR 2 (reader-thread dispatcher) which needs working protos. Co-Authored-By: Claude Sonnet 4.6 --- .gitignore | 6 +- BUGS.md | 198 +++++++++++++++++++++++++++++++++++++ scripts/bootstrap.py | 148 ++++++++++++++++++++++----- scripts/flipper_core.py | 12 ++- scripts/flipper_storage.py | 26 +++-- 5 files changed, 349 insertions(+), 41 deletions(-) create mode 100644 BUGS.md diff --git a/.gitignore b/.gitignore index 2c489e8..fa9df04 100644 --- a/.gitignore +++ b/.gitignore @@ -5,9 +5,11 @@ __pycache__/ *.pyd .pytest_cache/ -# Generated protobuf bindings (regenerated on first use) -scripts/protobuf/generated/ +# Downloaded + generated protobuf bindings (regenerated on first use) +scripts/protobuf/*.proto scripts/protobuf/.downloaded +scripts/protobuf/.downloaded.json +scripts/protobuf/generated/ # Test artifacts flipper-test-artifacts/ diff --git a/BUGS.md b/BUGS.md new file mode 100644 index 0000000..9b7c93f --- /dev/null +++ b/BUGS.md @@ -0,0 +1,198 @@ +# Bug List + +Issues discovered while testing this project with the `flipper-dev-toolkit` plugin. +All entries below are bugs **in the `flipper-dev-toolkit` plugin itself** (cached at +`~/.claude/plugins/cache/flipper-dev-toolkit/`), not in the FlipperZeroPlugIn code. +Once verified, file them upstream against that plugin. + +Status legend: `[ ]` open · `[x]` fixed · `[~]` worked around locally + +--- + +## 1. `[x]` `bootstrap.py` downloads `app.proto`, which no longer exists upstream + +**File:** `scripts/bootstrap.py:20-30` + +The hardcoded `PROTO_FILES` list contains `app.proto` and `subghz.proto`. Both 404 from +`https://raw.githubusercontent.com/flipperdevices/flipperzero-protobuf/dev/`. +Flipper renamed `app.proto` to `application.proto` upstream; `subghz.proto` was removed +(or moved). + +**Symptom:** Bootstrap prints `WARN: could not fetch app.proto: HTTP Error 404` +and `WARN: could not fetch subghz.proto: HTTP Error 404`, then proceeds to a broken +proto-generation step. + +**Fix:** Replace `app.proto` with `application.proto` in `PROTO_FILES`. Drop +`subghz.proto` or relocate the source. + +**Local workaround:** Manually fetched `application.proto`; left `subghz.proto` missing +(no current consumer of it observed). + +--- + +## 2. `[x]` `flipper_core.py` imports `app_pb2`, but generator produces `application_pb2` + +**File:** `scripts/flipper_core.py:37` + +After fix #1, `grpc_tools.protoc` generates `application_pb2.py` (matching the new +proto filename). But `flipper_core.py` still does `import app_pb2`, so every script +that touches RPC fails with `ModuleNotFoundError: No module named 'app_pb2'` — which +the code re-raises as the misleading +`RuntimeError: Flipper protobuf bindings not generated. Run: python scripts/bootstrap.py`. +Running bootstrap doesn't help; the import name is the real problem. + +**Fix:** Change `import app_pb2` → `import application_pb2 as app_pb2` (or rename +references). Update the `try/except ImportError` to log the actual missing module +instead of claiming bindings are ungenerated. + +**Local workaround:** Added shim `protobuf/generated/app_pb2.py` containing +`from application_pb2 import *`. + +--- + +## 3. `[x]` `bootstrap.py` writes `.downloaded` marker even on partial failure + +**File:** `scripts/bootstrap.py:63-77` (`download_protos`) + +`marker.touch()` runs unconditionally after the download loop, even if individual +files 404. A re-run of bootstrap won't retry the missing protos — user must manually +delete `scripts/protobuf/.downloaded`. + +**Fix:** Only write the marker if all downloads succeeded. Or use per-file markers / +file existence checks. + +--- + +## 4. `[x]` `bootstrap.py` swallows protoc failures with only a warning + +**File:** `scripts/bootstrap.py:107-108` + +`generate_proto_bindings` catches every exception and logs `WARN`, then the +top-level `main()` exits 0 and prints `[bootstrap] Ready`. Downstream scripts then +crash with the misleading "Flipper protobuf bindings not generated" error. + +**Fix:** Either fail loudly (non-zero exit) when protoc fails, or check for the +generated `*_pb2.py` files and fail if they're missing. + +--- + +## 5. `[x]` `connect-flipper` skill references a `--json` flag `flipper_storage.py` doesn't implement + +**File:** `skills/connect-flipper/SKILL.md` (the skill instructions), vs. +`scripts/flipper_storage.py` argparse setup. + +The skill's documented probe command is: + +``` +python scripts/flipper_storage.py list /ext --json +``` + +But `flipper_storage.py` errors with +`unrecognized arguments: --json`. Either the flag was dropped or never added. + +**Fix:** Add `--json` to `flipper_storage.py` argparse (and serialize the list output +as JSON), or update the skill to use a command that actually works (e.g. the plain +`list /ext` output, or `flipper_screen.py --json` which does support `--json`). + +--- + +## 6. `[x]` `bootstrap.reexec_in_venv()` corrupts argv when called via `python -c` + +**File:** `scripts/bootstrap.py:120-136` + +When invoked from `python -c "...; bootstrap.reexec_in_venv()"`, `sys.argv` is just +`['-c']` (or empty), so `os.execve(py, [py, *sys.argv], env)` re-execs the venv Python +with no script path — producing +`Argument expected for the -c option`. + +**Fix:** Skip the re-exec when there's no script to run (e.g. `if len(sys.argv) < 1 +or sys.argv[0] in ('', '-c'): return`), or document that `reexec_in_venv()` must only +be imported from a script entry point, not invoked ad-hoc. + +This makes the documented "explicit connect probe" in the `connect-flipper` skill +fail. + +--- + +## 7. `[x]` First-run experience is broken end-to-end on a fresh Windows install + +Cumulative effect of #1–#4: a brand-new user running `/flipper-dev-toolkit:connect-flipper` +hits a confusing chain of failures (404s → silent protoc failure → misleading +"bindings not generated" error → import error on the real cause). Required +non-obvious manual recovery: fetch missing proto, regenerate bindings, create +import shim. + +**Fix:** Address #1–#4. Add a self-test step at the end of bootstrap that imports +each `*_pb2` module the codebase actually uses, and fails the bootstrap if any +import raises. + +--- + +# Feature Requests / Enhancements + +Status legend: `[ ]` open · `[x]` shipped · `[~]` partial / prototype exists + +## F1. `[ ]` Live streaming display viewer with interactive controls + +**Motivation:** `/capture-screen` produces a single still PNG. For UI development, +debugging menus, or stepping through a FAP, a one-shot snapshot forces a tedious +loop of `capture → look → send-input → capture`. A live viewer would collapse that +to a single window the developer keeps open while iterating. + +**Proposed behavior:** + +- Continuously stream the 128×64 framebuffer over RPC (Gui `StartScreenStream` / + `ScreenFrame` push events) at ≥10 FPS. +- Render to a local window — options in rough order of effort: + 1. **Web UI** served by the plugin (Flask/FastAPI + WebSocket). Easiest to wrap + with buttons; works cross-platform; can be opened in the user's browser. + 2. **Tk/Pillow window** launched from the script. No browser needed but worse + for adding rich controls. + 3. **Terminal viewer** rendering ASCII / Unicode half-blocks in place. Useful + in headless / SSH contexts. +- On-screen control buttons that map to Flipper's input RPC: **Up, Down, Left, + Right, OK, Back**, each supporting **short / long / press / release**. Mirror + the existing `send-input` skill semantics. +- Optional: keyboard shortcuts in the viewer (arrows, Enter, Esc) for hands-free + control. +- Optional: record button → dump a sequence of PNGs (or animated GIF/APNG) for + bug reports and visual regression baselines. + +**Surfacing:** New skill `flipper-dev-toolkit:live-view` (or extend +`/capture-screen` with a `--live` flag) and a corresponding MCP tool +`flipper_live_view` that returns a local URL. + +**Notes / risks:** + +- `RpcSession` in `flipper_core.py` already has a reader thread and notification + deque — streaming framebuffer events should plug into that. Verify how + Momentum/OFW emit `ScreenFrame` push events vs. polled `ScreenFrameRequest`. +- Sending input while streaming must share the serial port through the existing + `_io_lock`, otherwise frames and input commands will interleave and corrupt + the framing. +- Frame rate is limited by serial throughput (~115200 baud → ~1 KB framebuffer + per frame is fine for 10–15 FPS; faster needs delta encoding, which the + protocol may or may not support). + +## F2. `[ ]` `setup` skill / command that runs bootstrap idempotently and verifies it worked + +**Motivation:** Today the first run silently breaks (see bugs #1–#4 and #7). +There's no user-facing "set up the toolkit, tell me if anything is wrong" +entry point — `connect-flipper` is the de-facto setup trigger, and it hides +the bootstrap output. + +**Proposed behavior:** + +- New skill `flipper-dev-toolkit:setup` (or `/setup-toolkit`) that: + 1. Runs `bootstrap.py` with verbose output surfaced to the user. + 2. Verifies the venv is healthy (`pyserial`, `protobuf`, `Pillow`, `mcp`, + `grpcio-tools` all importable). + 3. Verifies every `*_pb2` module that `flipper_core.py` imports actually + loads (catches the `app_pb2` vs. `application_pb2` class of bug at + setup time instead of at first RPC call). + 4. Probes for a connected Flipper but does **not** require one — setup + should succeed offline so users can prepare before plugging in. + 5. Reports a checklist: venv ✓, deps ✓, protos ✓, generated bindings ✓, + device ✓/✗. +- A `--repair` flag that nukes `.venv/`, `protobuf/`, and `protobuf/generated/` + so the user can recover from a poisoned bootstrap state with one command. diff --git a/scripts/bootstrap.py b/scripts/bootstrap.py index 66f40e6..7fac42e 100644 --- a/scripts/bootstrap.py +++ b/scripts/bootstrap.py @@ -4,8 +4,8 @@ """ from __future__ import annotations +import json import os -import shutil import subprocess import sys import urllib.request @@ -17,16 +17,30 @@ PROTO_DIR = PLUGIN_ROOT / "scripts" / "protobuf" PROTO_BASE_URL = "https://raw.githubusercontent.com/flipperdevices/flipperzero-protobuf/dev" + +# Upstream renamed `app.proto` -> `application.proto` and dropped `subghz.proto` +# from the dev branch. Keep this list in sync with what `flipper_core.py` imports. PROTO_FILES = [ "flipper.proto", "system.proto", "storage.proto", "gui.proto", "gpio.proto", - "app.proto", + "application.proto", "desktop.proto", "property.proto", - "subghz.proto", +] + +# Proto modules `flipper_core.py` expects to be importable after generation. +# Used as the post-bootstrap self-check. +EXPECTED_PB2_MODULES = [ + "flipper_pb2", + "system_pb2", + "storage_pb2", + "gui_pb2", + "gpio_pb2", + "application_pb2", + "desktop_pb2", ] REQS = [ @@ -61,11 +75,25 @@ def pip_install(py: Path) -> None: def download_protos() -> None: + """Fetch every .proto in PROTO_FILES. Marker is written only on full success. + + The marker records the exact filename set it was downloaded for, so a later + edit to PROTO_FILES (e.g. an upstream rename) forces a re-download instead + of silently leaving stale files on disk. + """ PROTO_DIR.mkdir(parents=True, exist_ok=True) - marker = PROTO_DIR / ".downloaded" + marker = PROTO_DIR / ".downloaded.json" if marker.exists(): - return + try: + recorded = set(json.loads(marker.read_text()).get("files", [])) + if recorded == set(PROTO_FILES): + # Same set; trust the previous successful run. + return + except Exception: + pass # marker corrupt; fall through and re-download + print("[bootstrap] Downloading Flipper .proto definitions", file=sys.stderr) + failed: list[tuple[str, str]] = [] for name in PROTO_FILES: url = f"{PROTO_BASE_URL}/{name}" dst = PROTO_DIR / name @@ -73,39 +101,98 @@ def download_protos() -> None: with urllib.request.urlopen(url, timeout=30) as r: dst.write_bytes(r.read()) except Exception as e: - print(f"[bootstrap] WARN: could not fetch {name}: {e}", file=sys.stderr) - marker.touch() + failed.append((name, str(e))) + + if failed: + details = "; ".join(f"{n}: {e}" for n, e in failed) + raise RuntimeError( + f"[bootstrap] Failed to download {len(failed)} proto file(s): {details}. " + f"Check network access and upstream proto naming at {PROTO_BASE_URL}." + ) + + marker.write_text(json.dumps({"files": PROTO_FILES}, indent=2)) + + # Legacy marker from older bootstrap runs is no longer trusted; remove it + # so a downgrade doesn't get confused by both markers existing. + legacy = PROTO_DIR / ".downloaded" + if legacy.exists(): + try: + legacy.unlink() + except Exception: + pass def generate_proto_bindings(py: Path) -> None: + """Run protoc; raise on failure rather than silently warning. + + Marker records which .proto inputs were generated so a later proto-file + change forces a regeneration. + """ out_dir = PROTO_DIR / "generated" out_dir.mkdir(exist_ok=True) init = out_dir / "__init__.py" if not init.exists(): init.write_text("") - marker = out_dir / ".generated" + + marker = out_dir / ".generated.json" + proto_paths = sorted(PROTO_DIR.glob("*.proto")) + proto_names = [p.name for p in proto_paths] + if marker.exists(): - return + try: + recorded = set(json.loads(marker.read_text()).get("files", [])) + if recorded == set(proto_names): + return + except Exception: + pass # marker corrupt; regenerate + print("[bootstrap] Generating protobuf bindings", file=sys.stderr) - # protoc shipped with `grpcio-tools` via python -m grpc_tools.protoc, or use plain protoc. - # We use the `protoc` from `protobuf` package if available; fall back to grpcio-tools. - try: - subprocess.check_call( - [str(py), "-m", "pip", "install", "--quiet", "grpcio-tools"] + subprocess.check_call( + [str(py), "-m", "pip", "install", "--quiet", "grpcio-tools"] + ) + if not proto_paths: + raise RuntimeError( + "[bootstrap] No .proto files found in " + f"{PROTO_DIR}. Run download_protos() first." ) - protos = [str(p) for p in PROTO_DIR.glob("*.proto")] - if protos: - subprocess.check_call( - [ - str(py), "-m", "grpc_tools.protoc", - f"-I{PROTO_DIR}", - f"--python_out={out_dir}", - *protos, - ] - ) - marker.touch() - except Exception as e: - print(f"[bootstrap] WARN: protoc generation failed: {e}", file=sys.stderr) + subprocess.check_call( + [ + str(py), "-m", "grpc_tools.protoc", + f"-I{PROTO_DIR}", + f"--python_out={out_dir}", + *(str(p) for p in proto_paths), + ] + ) + + marker.write_text(json.dumps({"files": proto_names}, indent=2)) + + legacy = out_dir / ".generated" + if legacy.exists(): + try: + legacy.unlink() + except Exception: + pass + + +def verify_proto_imports(py: Path) -> None: + """Confirm every expected *_pb2 module loads in the venv interpreter. + + Catches the class of bug where upstream renames a .proto file but the + generated module name no longer matches what flipper_core.py imports. + """ + out_dir = PROTO_DIR / "generated" + probe = ( + f"import sys; sys.path.insert(0, r'{out_dir}'); " + + "; ".join(f"import {m}" for m in EXPECTED_PB2_MODULES) + ) + try: + subprocess.check_call([str(py), "-c", probe]) + except subprocess.CalledProcessError as e: + raise RuntimeError( + f"[bootstrap] Generated proto modules failed to import: {e}. " + f"Expected modules: {EXPECTED_PB2_MODULES}. " + f"Check that PROTO_FILES (in bootstrap.py) matches what flipper_core.py imports." + ) from e def main() -> int: @@ -113,6 +200,7 @@ def main() -> int: pip_install(py) download_protos() generate_proto_bindings(py) + verify_proto_imports(py) print(f"[bootstrap] Ready. Python: {py}", file=sys.stderr) return 0 @@ -121,9 +209,15 @@ def reexec_in_venv() -> None: """Re-run the calling script inside the plugin venv. Import this at the top of any CLI/MCP entry point. + + Skips when invoked via `python -c "..."` (no script to re-exec) so that + ad-hoc probes don't crash with `Argument expected for the -c option`. """ if os.environ.get("FLIPPER_IN_VENV") == "1": return + if not sys.argv or sys.argv[0] in ("", "-c", "-"): + # Called from `python -c` or stdin; nothing meaningful to re-exec. + return py = venv_python() if not py.exists(): ensure_venv() diff --git a/scripts/flipper_core.py b/scripts/flipper_core.py index 27dbbae..f8283d7 100644 --- a/scripts/flipper_core.py +++ b/scripts/flipper_core.py @@ -34,11 +34,16 @@ import storage_pb2 # type: ignore import gui_pb2 # type: ignore import gpio_pb2 # type: ignore - import app_pb2 # type: ignore + # Upstream renamed `app.proto` -> `application.proto`; the generated + # module name follows the proto filename. Alias locally to keep the + # rest of this file referring to `app_pb2.*`. + import application_pb2 as app_pb2 # type: ignore import desktop_pb2 # type: ignore PROTOS_AVAILABLE = True -except ImportError: + PROTO_IMPORT_ERROR: Exception | None = None +except ImportError as _e: PROTOS_AVAILABLE = False + PROTO_IMPORT_ERROR = _e CLI_PROMPT = b">: " @@ -111,7 +116,8 @@ class RpcSession: def __init__(self, port: str, timeout: float = 5.0): if not PROTOS_AVAILABLE: raise RuntimeError( - "Flipper protobuf bindings not generated. " + "Flipper protobuf bindings could not be imported " + f"({type(PROTO_IMPORT_ERROR).__name__}: {PROTO_IMPORT_ERROR}). " "Run: python scripts/bootstrap.py" ) self.port = port diff --git a/scripts/flipper_storage.py b/scripts/flipper_storage.py index d648a68..2a533df 100644 --- a/scripts/flipper_storage.py +++ b/scripts/flipper_storage.py @@ -13,30 +13,38 @@ def main() -> int: - ap = argparse.ArgumentParser(description="Flipper storage operations.") - ap.add_argument("--port") - ap.add_argument("--json", action="store_true") + # Shared flags are defined on a parent parser so they work both BEFORE + # the subcommand (`flipper_storage.py --json list /ext`) and AFTER it + # (`flipper_storage.py list /ext --json`). + common = argparse.ArgumentParser(add_help=False) + common.add_argument("--port") + common.add_argument("--json", action="store_true") + + ap = argparse.ArgumentParser( + description="Flipper storage operations.", + parents=[common], + ) sub = ap.add_subparsers(dest="cmd", required=True) - p_ls = sub.add_parser("list") + p_ls = sub.add_parser("list", parents=[common]) p_ls.add_argument("path") - p_read = sub.add_parser("read") + p_read = sub.add_parser("read", parents=[common]) p_read.add_argument("device_path") p_read.add_argument("--out", required=True, help="Local file to write") - p_write = sub.add_parser("write") + p_write = sub.add_parser("write", parents=[common]) p_write.add_argument("local_path") p_write.add_argument("device_path") - p_mk = sub.add_parser("mkdir") + p_mk = sub.add_parser("mkdir", parents=[common]) p_mk.add_argument("path") - p_rm = sub.add_parser("delete") + p_rm = sub.add_parser("delete", parents=[common]) p_rm.add_argument("path") p_rm.add_argument("--recursive", "-r", action="store_true") - p_stat = sub.add_parser("stat") + p_stat = sub.add_parser("stat", parents=[common]) p_stat.add_argument("path") args = ap.parse_args()