Improve remote execution in `hil_ci.sh`

4 files changed, 158 insertions, 61 deletions

diff --git a/.claude/skills/code-size/SKILL.md b/.claude/skills/code-size/SKILL.md
index f10380374..e12a30d86 100644
--- a/.claude/skills/code-size/SKILL.md
+++ b/.claude/skills/code-size/SKILL.md
@@ -64,7 +64,7 @@ python3 tools/metrics_compare_base.py -b raspberry_pi_pico --base-branch v0.18.0
 
 - Single example, single board: ~30 s
 - All examples, single board: ~60-90 s
-- `--ci` (all arm-gcc families, first board each): 4-8 minutes (parallel build)
+- `--ci` (all arm-gcc families, first board each): 4-8 minutes — sequential sweep across boards (Ninja parallelizes within each board, not across)
 
 Use timeouts ≥ 10 minutes (600000 ms) for `--ci`.
 
diff --git a/AGENTS.md b/AGENTS.md
index eefe9dde1..5c9908d19 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -103,10 +103,10 @@ openocd -f interface/jlink.cfg -f target/stm32h7x.cfg
 openocd -f interface/cmsis-dap.cfg -f target/rp2040.cfg -c "adapter speed 5000"
 ```
 
-**Terminal 2 — connect GDB** (JLink :2331, OpenOCD :3333):
+**Terminal 2 — connect GDB** (replace `<port>` with `2331` for JLinkGDBServer or `3333` for OpenOCD):
 ```bash
 arm-none-eabi-gdb /tmp/build/firmware.elf
-(gdb) target remote :2331
+(gdb) target remote :<port>
 (gdb) monitor reset halt
 (gdb) load
 (gdb) break main         # optional, to stop at entry
diff --git a/test/hil/hil_ci.sh b/test/hil/hil_ci.sh
index d1b5f7def..96872e2e1 100644
--- a/test/hil/hil_ci.sh
+++ b/test/hil/hil_ci.sh
@@ -26,6 +26,7 @@ ARGS=()
 while [[ $# -gt 0 ]]; do
   case "$1" in
     -b)
+      [[ $# -ge 2 ]] || { echo "error: -b requires a BOARD argument" >&2; exit 1; }
       BOARD="$2"
       ARGS+=("$1" "$2")
       shift 2
@@ -37,9 +38,14 @@ while [[ $# -gt 0 ]]; do
   esac
 done
 
-# Setup remote directory
+# Setup remote directory. Use `bash -s` + heredoc so REMOTE_DIR (user-overridable)
+# is passed as a positional parameter and never reinterpreted by the remote shell.
 echo "==> Setting up remote $REMOTE:$REMOTE_DIR"
-ssh "$REMOTE" "rm -rf $REMOTE_DIR && mkdir -p $REMOTE_DIR/test/hil $REMOTE_DIR/examples"
+ssh "$REMOTE" bash -s -- "$REMOTE_DIR" <<'REMOTE'
+set -e
+rm -rf -- "$1"
+mkdir -p -- "$1/test/hil" "$1/examples"
+REMOTE
 
 # Copy HIL test script and config
 echo "==> Copying test scripts"
@@ -60,7 +66,7 @@ if [ -n "$BOARD" ]; then
   BUILD_DIR="$ROOT_DIR/examples/cmake-build-$BOARD"
   if [ ! -d "$BUILD_DIR" ]; then
     echo "Error: build directory not found: $BUILD_DIR"
-    echo "Build first with: cd examples && cmake -DBOARD=$BOARD -G Ninja -B cmake-build-$BOARD .. && cmake --build cmake-build-$BOARD"
+    echo "Build first with: cd examples && cmake -DBOARD=$BOARD -G Ninja -B cmake-build-$BOARD . && cmake --build cmake-build-$BOARD"
     exit 1
   fi
   echo "==> Copying binaries for $BOARD"
@@ -72,7 +78,12 @@ else
   done
 fi
 
-# Run test
+# Run test. Use `bash -s` so REMOTE_DIR + ARGS reach the remote shell as positional
+# parameters; quoting and metacharacters in args are preserved.
 CONFIG_BASENAME="$(basename "$CONFIG")"
 echo "==> Running HIL test on $REMOTE"
-ssh -t "$REMOTE" "cd $REMOTE_DIR && python3 -u test/hil/hil_test.py -B examples ${ARGS[*]} test/hil/$CONFIG_BASENAME"
+ssh "$REMOTE" bash -s -- "$REMOTE_DIR" "${ARGS[@]}" "test/hil/$CONFIG_BASENAME" <<'REMOTE'
+cd -- "$1"
+shift
+exec python3 -u test/hil/hil_test.py -B examples "$@"
+REMOTE
diff --git a/tools/metrics_compare_base.py b/tools/metrics_compare_base.py
index a189e3143..0fb767bb7 100644
--- a/tools/metrics_compare_base.py
+++ b/tools/metrics_compare_base.py
@@ -18,19 +18,57 @@ import argparse
 import glob
 import json
 import os
+import re
+import shlex
 import subprocess
 import sys
 
 TINYUSB_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 METRICS_DIR = os.path.join(TINYUSB_ROOT, 'cmake-metrics')
 
+def tinyusb_src_filter(checkout_dir):
+    """Return a path-substring filter that uniquely matches TinyUSB stack source files
+    in `checkout_dir`. The substring is the absolute path to the checkout's `src/`
+    dir — collision-free with vendored deps (pico-sdk, lwip, FreeRTOS, etc.) which
+    live at unrelated paths."""
+    return os.path.realpath(os.path.join(checkout_dir, 'src')) + os.sep
+
 verbose = False
 
 
 def run(cmd, **kwargs):
+    """Run a command. cmd must be a list (no shell=True)."""
+    if not isinstance(cmd, list):
+        raise TypeError('run() requires a list, got str — fix the caller')
     if verbose:
-        print(f'  $ {cmd}')
-    return subprocess.run(cmd, shell=True, capture_output=True, text=True, **kwargs)
+        print(f'  $ {" ".join(shlex.quote(str(c)) for c in cmd)}')
+    return subprocess.run(cmd, capture_output=True, text=True, **kwargs)
+
+
+def symlink_deps(main_root, worktree_dir):
+    """Symlink dependency directories (fetched by tools/get_deps.py) from the main
+    checkout into the temporary worktree. Without this, the base build fails because
+    the worktree doesn't have the untracked deps."""
+    def link_subdirs(rel_parent):
+        src_parent = os.path.join(main_root, rel_parent)
+        dst_parent = os.path.join(worktree_dir, rel_parent)
+        if not os.path.isdir(src_parent):
+            return
+        os.makedirs(dst_parent, exist_ok=True)
+        for entry in os.listdir(src_parent):
+            src = os.path.join(src_parent, entry)
+            dst = os.path.join(dst_parent, entry)
+            if os.path.isdir(src) and not os.path.exists(dst):
+                os.symlink(src, dst)
+
+    # lib/* and tools/* deps (e.g. lib/lwip, tools/linkermap)
+    link_subdirs('lib')
+    link_subdirs('tools')
+    # hw/mcu/<vendor>/<dep> (e.g. hw/mcu/raspberry_pi/Pico-PIO-USB)
+    hw_mcu = os.path.join(main_root, 'hw', 'mcu')
+    if os.path.isdir(hw_mcu):
+        for vendor in os.listdir(hw_mcu):
+            link_subdirs(os.path.join('hw', 'mcu', vendor))
 
 
 def ci_first_boards():
@@ -38,7 +76,7 @@ def ci_first_boards():
     matrix_py = os.path.join(TINYUSB_ROOT, '.github', 'workflows', 'ci_set_matrix.py')
     if not os.path.isfile(matrix_py):
         return []
-    ret = run(f'{sys.executable} {matrix_py}')
+    ret = run([sys.executable, matrix_py])
     if ret.returncode != 0:
         return []
     try:
@@ -59,23 +97,34 @@ def ci_first_boards():
 
 
 def build_board(src_dir, build_dir, board, example=None):
-    """Configure and build examples for a board. Returns True on success."""
+    """Configure and build examples for a board. Returns True on success.
+
+    When `example` is given, only that target is built (`cmake --build --target NAME`),
+    keeping single-example workflows fast.
+    """
     os.makedirs(build_dir, exist_ok=True)
-    ret = run(f'cmake -B {build_dir} -G Ninja -DBOARD={board} -DCMAKE_BUILD_TYPE=MinSizeRel '
-              f'{os.path.join(src_dir, "examples")}')
+    ret = run(['cmake', '-B', build_dir, '-G', 'Ninja',
+               f'-DBOARD={board}', '-DCMAKE_BUILD_TYPE=MinSizeRel',
+               os.path.join(src_dir, 'examples')])
     if ret.returncode != 0:
         print(f'  Error configuring {board}: {ret.stderr}')
         return False
-    target = f'--target {os.path.basename(example)}' if example else ''
-    ret = run(f'cmake --build {build_dir} {target}', timeout=600)
+    cmd = ['cmake', '--build', build_dir]
+    if example:
+        cmd += ['--target', os.path.basename(example)]
+    ret = run(cmd, timeout=600)
     if ret.returncode != 0:
         print(f'  Error building {board}: {ret.stderr}')
         return False
     return True
 
 
-def generate_metrics(build_dir, out_basename, filter_str, example=None):
-    """Run metrics.py combine on .map.json files. Returns metrics json path or None."""
+def generate_metrics(build_dir, out_basename, filters, example=None):
+    """Run metrics.py combine on .map.json files. Returns metrics json path or None.
+
+    `filters` is a list of substrings; metrics.py keeps a compile unit if its path
+    contains any of them.
+    """
     if example:
         patterns = glob.glob(f'{build_dir}/{example}/*.map.json')
     else:
@@ -85,8 +134,11 @@ def generate_metrics(build_dir, out_basename, filter_str, example=None):
         return None
 
     metrics_py = os.path.join(TINYUSB_ROOT, 'tools', 'metrics.py')
-    ret = run(f'{sys.executable} {metrics_py} combine -f {filter_str} -j -q '
-              f'-o {out_basename} {" ".join(patterns)}')
+    cmd = [sys.executable, metrics_py, 'combine']
+    for f in filters:
+        cmd += ['-f', f]
+    cmd += ['-j', '-q', '-o', out_basename, *patterns]
+    ret = run(cmd)
     if ret.returncode != 0:
         print(f'  Error: {ret.stderr}')
         return None
@@ -99,8 +151,12 @@ def main():
     parser = argparse.ArgumentParser(description='Compare code size metrics with base branch')
     parser.add_argument('-b', '--board', action='append', default=[],
                         help='Board name (repeatable). Required unless --ci is given.')
-    parser.add_argument('-f', '--filter', default='tinyusb/src',
-                        help='Path filter for metrics (default: tinyusb/src)')
+    parser.add_argument('-f', '--filter', action='append', default=None,
+                        help='Path-substring filter (repeatable). When given, '
+                             'overrides the default and is applied to BOTH base and '
+                             'current builds. Default: each side\'s own absolute '
+                             '<checkout>/src/ path, which uniquely matches TinyUSB '
+                             'stack code without colliding with vendored deps.')
     parser.add_argument('--base-branch', default='master',
                         help='Base branch to compare against (default: master)')
     parser.add_argument('-e', '--example', action='append', default=None,
@@ -136,22 +192,28 @@ def main():
         parser.error('at least one -b BOARD is required (or pass --ci)')
 
     metrics_py = os.path.join(TINYUSB_ROOT, 'tools', 'metrics.py')
-    linkermap_dir = os.path.join(TINYUSB_ROOT, 'tools', 'linkermap')
     worktree_dir = os.path.join(METRICS_DIR, '_worktree')
 
+    # Per-side filters: when no override is given, each build uses its own
+    # absolute <checkout>/src/ path so we only match TinyUSB stack code from that
+    # checkout (and never vendored-dep `src/` like pico-sdk/src/...).
+    if args.filter:
+        base_filters = cur_filters = list(args.filter)
+    else:
+        base_filters = [tinyusb_src_filter(worktree_dir)]
+        cur_filters = [tinyusb_src_filter(TINYUSB_ROOT)]
+
     # Step 1: Create worktree for base branch
     print(f'[1/5] Setting up {args.base_branch} worktree...')
     if os.path.isdir(worktree_dir):
-        run(f'git -C {TINYUSB_ROOT} worktree remove --force {worktree_dir}')
-    ret = run(f'git -C {TINYUSB_ROOT} worktree add {worktree_dir} {args.base_branch}')
+        run(['git', '-C', TINYUSB_ROOT, 'worktree', 'remove', '--force', worktree_dir])
+    ret = run(['git', '-C', TINYUSB_ROOT, 'worktree', 'add', worktree_dir, args.base_branch])
     if ret.returncode != 0:
         print(f'Error creating worktree: {ret.stderr}')
         sys.exit(1)
 
-    # Ensure linkermap is available
-    wt_linkermap = os.path.join(worktree_dir, 'tools', 'linkermap')
-    if not os.path.exists(wt_linkermap) and os.path.exists(linkermap_dir):
-        os.symlink(linkermap_dir, wt_linkermap)
+    # Symlink dependency dirs (lib/*, hw/mcu/*/*, tools/*) so the worktree builds.
+    symlink_deps(TINYUSB_ROOT, worktree_dir)
 
     try:
         examples = args.example or [None]
@@ -164,18 +226,23 @@ def main():
             base_build = os.path.join(board_dir, 'base')
             cur_build = os.path.join(board_dir, 'build')
 
-            # Step 2: Build base (all examples, cmake will skip already-built)
-            print(f'[2/5] Building {args.base_branch} for {board}...')
-            if not build_board(worktree_dir, base_build, board):
-                continue
-
-            # Step 3: Build current
-            print(f'[3/5] Building current for {board}...')
-            if not build_board(TINYUSB_ROOT, cur_build, board):
+            # Build only the requested examples (or all if -e not given). Single-example
+            # mode used to build everything and filter at metric time — that was wasted work.
+            board_failed = False
+            for example in examples:
+                build_label = f' --target {os.path.basename(example)}' if example else ''
+                print(f'[2/5] Building {args.base_branch} for {board}{build_label}...')
+                if not build_board(worktree_dir, base_build, board, example):
+                    board_failed = True
+                    break
+                print(f'[3/5] Building current for {board}{build_label}...')
+                if not build_board(TINYUSB_ROOT, cur_build, board, example):
+                    board_failed = True
+                    break
+            if board_failed:
                 continue
 
             built_pairs.append((board, base_build, cur_build))
-            base_filter = args.filter.replace('tinyusb/', '', 1) if args.filter.startswith('tinyusb/') else args.filter
 
             for example in examples:
                 suffix = f'_{example.replace("/", "_")}' if example else ''
@@ -184,16 +251,16 @@ def main():
                 # Step 4: Generate metrics
                 print(f'[4/5] Generating metrics for {board}{label}...')
                 base_json = generate_metrics(base_build, os.path.join(board_dir, f'base_metrics{suffix}'),
-                                             base_filter, example)
+                                             base_filters, example)
                 cur_json = generate_metrics(cur_build, os.path.join(board_dir, f'build_metrics{suffix}'),
-                                            args.filter, example)
+                                            cur_filters, example)
                 if not base_json or not cur_json:
                     continue
 
                 # Step 5: Compare
                 out_base = os.path.join(board_dir, f'metrics_compare{suffix}')
                 print(f'[5/5] Comparing {board}{label}...')
-                ret = run(f'{sys.executable} {metrics_py} compare -m -o {out_base} {base_json} {cur_json}')
+                ret = run([sys.executable, metrics_py, 'compare', '-m', '-o', out_base, base_json, cur_json])
                 print(ret.stdout)
 
                 # Optional: bloaty diff
@@ -202,50 +269,69 @@ def main():
                     base_elf = os.path.join(base_build, example, f'{elf_name}.elf')
                     cur_elf = os.path.join(cur_build, example, f'{elf_name}.elf')
                     if os.path.exists(base_elf) and os.path.exists(cur_elf):
-                        src_filter = f'--source-filter={args.filter}' if args.filter else ''
+                        # Bloaty expects one regex; OR-join all filters (current side
+                        # for the new ELF, base side for the base ELF).
+                        bloaty_regex = '(' + '|'.join(
+                            re.escape(f) for f in (cur_filters + base_filters)
+                        ) + ')'
+                        bloaty_common = ['bloaty', '--domain=vm', f'--source-filter={bloaty_regex}']
                         print(f'--- bloaty sections ---')
-                        ret = run(f'bloaty --domain=vm -d compileunits,sections {src_filter} {cur_elf} -- {base_elf}')
+                        ret = run(bloaty_common + ['-d', 'compileunits,sections', cur_elf, '--', base_elf])
                         print(ret.stdout)
                         print(f'--- bloaty symbols ---')
-                        ret = run(f'bloaty --domain=vm -d compileunits,symbols -s vm {src_filter} {cur_elf} -- {base_elf}')
+                        ret = run(bloaty_common + ['-d', 'compileunits,symbols', '-s', 'vm',
+                                                    cur_elf, '--', base_elf])
                         print(ret.stdout)
                     else:
                         print(f'  bloaty: ELF not found')
 
-        # Optional combined comparison across all boards
+        # Optional combined comparison across all boards.
+        # Aggregates the per-board metrics JSONs (not raw map.json globs) so the argv
+        # stays small even with --ci spanning many boards.
         if args.combined and built_pairs:
             combined_dir = os.path.join(METRICS_DIR, '_combined')
             os.makedirs(combined_dir, exist_ok=True)
-            base_filter = args.filter.replace('tinyusb/', '', 1) if args.filter.startswith('tinyusb/') else args.filter
-            base_maps = []
-            cur_maps = []
-            for _board, base_build, cur_build in built_pairs:
-                base_maps += glob.glob(f'{base_build}/**/*.map.json', recursive=True)
-                cur_maps += glob.glob(f'{cur_build}/**/*.map.json', recursive=True)
-            if not base_maps or not cur_maps:
-                print('  combined: no map.json files collected, skipping')
+
+            # Use the no-suffix per-board JSONs (whole-board metrics). Combined mode
+            # is meant for board-level sweeps; -e/--example combinations skip combined.
+            base_jsons, cur_jsons = [], []
+            for board, _, _ in built_pairs:
+                bj = os.path.join(METRICS_DIR, board, 'base_metrics.json')
+                cj = os.path.join(METRICS_DIR, board, 'build_metrics.json')
+                if os.path.isfile(bj) and os.path.isfile(cj):
+                    base_jsons.append(bj)
+                    cur_jsons.append(cj)
+
+            if not base_jsons or not cur_jsons:
+                print('  combined: no per-board metrics found (did you pass -e? skip --combined with -e)')
             else:
-                print(f'\n=== combined ({len(args.board)} boards) ===')
+                print(f'\n=== combined ({len(base_jsons)} boards) ===')
                 base_out = os.path.join(combined_dir, 'base_metrics')
                 cur_out = os.path.join(combined_dir, 'build_metrics')
-                ret = run(f'{sys.executable} {metrics_py} combine -f {base_filter} -j -q '
-                          f'-o {base_out} {" ".join(base_maps)}')
+
+                # Per-board JSONs are already filtered to TinyUSB-only files; combine
+                # without re-filtering so we don't accidentally drop entries.
+                def _combine(out_basename, inputs):
+                    cmd = [sys.executable, metrics_py, 'combine',
+                           '-j', '-q', '-o', out_basename, *inputs]
+                    return run(cmd)
+
+                ret = _combine(base_out, base_jsons)
                 if ret.returncode != 0:
                     print(f'  combined base error: {ret.stderr}')
                 else:
-                    ret = run(f'{sys.executable} {metrics_py} combine -f {args.filter} -j -q '
-                              f'-o {cur_out} {" ".join(cur_maps)}')
+                    ret = _combine(cur_out, cur_jsons)
                     if ret.returncode != 0:
                         print(f'  combined current error: {ret.stderr}')
                     else:
                         out_combined = os.path.join(combined_dir, 'metrics_compare')
-                        ret = run(f'{sys.executable} {metrics_py} compare -m '
-                                  f'-o {out_combined} {base_out}.json {cur_out}.json')
+                        ret = run([sys.executable, metrics_py, 'compare', '-m',
+                                   '-o', out_combined, f'{base_out}.json', f'{cur_out}.json'])
                         print(ret.stdout)
                         print(f'  combined report: {out_combined}.md')
     finally:
         print(f'\nCleaning up worktree...')
-        run(f'git -C {TINYUSB_ROOT} worktree remove --force {worktree_dir}')
+        run(['git', '-C', TINYUSB_ROOT, 'worktree', 'remove', '--force', worktree_dir])
 
 
 if __name__ == '__main__':