Merge pull request #18 from OEDO-Dynamics/docs/can-tools-d-operation

docs: Add Operation section and update CI / config examples (#9 PR-D)

Author: RyodoTanaka

.github/workflows/shellcheck.yml

@@ -1,6 +1,12 @@
 name: shellcheck
 
-on: [push, pull_request]
+on:
+  push:
+    branches: [develop, main]
+  pull_request:
+
+permissions:
+  contents: read
 
 jobs:
   shellcheck:
@@ -18,48 +24,39 @@ jobs:
 
           # Run shellcheck on repo-root *.sh and *.bash files, tests/
           # Exclude can-utils/, openimu/
-          # Exclude files that will be modified in later PRs (PR-A2, PR-B):
-          #   - canhat_fd_setup.bash
-          #   - canhat_fd_rename.bash
-          #   - enable_canhat_fd.sh
-          #   - can_monitor.sh
-          # (These exclusions will be removed in PR-D / #9)
 
           echo "=== Running shellcheck -S warning ==="
 
           # Build file list
           files=()
 
-          # Add repo-root *.sh and *.bash (excluding excluded patterns)
+          # Add repo-root *.sh and *.bash (can-utils/ and openimu/ are not
+          # matched by -maxdepth 1 in the first place, so no further
+          # exclusion is needed for them).
           while IFS= read -r f; do
-              basename="$(basename "$f")"
-              # Skip files to be modified in later PRs (PR-A2, PR-B)
-              if [[ "$basename" == "canhat_fd_setup.bash" ]] || \
-                 [[ "$basename" == "canhat_fd_rename.bash" ]] || \
-                 [[ "$basename" == "enable_canhat_fd.sh" ]] || \
-                 [[ "$basename" == "can_monitor.sh" ]]; then
-                  continue
-              fi
               files+=("$f")
-          done < <(find . -maxdepth 1 -type f \( -name "*.sh" -o -name "*.bash" \) ! -path "./can-utils/*" ! -path "./openimu/*" | sort)
+          done < <(find . -maxdepth 1 -type f \( -name "*.sh" -o -name "*.bash" \) | sort)
 
-          # Add tests/ files: test_*.sh/bash and tests/fakes/* (bash scripts without extension)
+          # Add tests/ files: tests/run.sh, test_*.sh/bash, fakes/*, diag_fakes/* (all shell scripts)
           while IFS= read -r f; do
-              if [[ "$f" == "tests/test_"* ]] || [[ "$f" == "tests/fakes/"* ]]; then
-                  files+=("$f")
+              if [[ -f "$f" ]] && {
+                  [[ "$f" == "tests/run.sh" ]] || \
+                  [[ "$f" == "tests/test_"* ]] || \
+                  [[ "$f" == "tests/fakes/"* ]] || \
+                  [[ "$f" == "tests/diag_fakes/"* ]]; }; then
+                  # Only add if it's a shell script (bash/sh shebang, or *.sh/*.bash)
+                  if head -1 "$f" 2>/dev/null | grep -qE '^#!.*\b(ba)?sh\b' || [[ "$f" =~ \.(sh|bash)$ ]]; then
+                      files+=("$f")
+                  fi
               fi
-          done < <(find tests/ -type f \( -name "*.sh" -o -name "*.bash" -o -path "tests/fakes/*" \) | sort)
+          done < <(find tests/ -type f 2>/dev/null | sort)
 
           if (( ${#files[@]} > 0 )); then
               shellcheck -S warning "${files[@]}"
-              rc=$?
           else
               echo "No shell files found to check"
-              rc=0
           fi
 
-          exit $rc
-
       - name: Run tests
         run: |
           cd tests

README.md

@@ -73,17 +73,20 @@ For full option details, see `/opt/can_tools/can_tools.conf.example`.
 
 #### Applying configuration changes
 
-A conf edit (or a reinstall that updates a script) only takes effect:
-- at the **next reboot**, or
-- by running `sudo systemctl restart canhat_fd_setup.service` while the
-  **machine is stopped** (hydraulics locked, implement down, key state
-  decided).
-
-Never restart it, or run `ip link set ... down/up`, while the machine is
-live - bringing CAN down/up while the controller is sending/receiving is
-what caused the 2026-10-14 incident. Until the next reboot (or that manual,
-machine-stopped restart), the running `canhat_fd_setup.service` keeps using
-the previously-applied settings.
+> [!WARNING]
+> **Never edit `/opt/can_tools/can_tools.conf` while the machine is live, and
+> never run `systemctl restart canhat_fd_setup.service` or `ip link set ...
+> down/up` by hand while it is live.** `can_watchdog.service` re-reads the
+> conf file on **every cycle** (every `WD_LOOP_S`, 1 s by default) - it does
+> **not** wait for a reboot the way this used to be documented. See
+> [`docs/can_watchdog.md`](docs/can_watchdog.md#configuration-is-re-read-live-the-dangerous-part)
+> for exactly which keys take effect immediately, which ones only pause the
+> watchdog (safe), and which ones get applied live to a running interface the
+> next time the watchdog happens to flush/recover it (dangerous - this is how
+> the 2026-10-14 incident happened). Only edit the conf with the machine
+> mechanically stopped (hydraulics locked, implement lowered, key state
+> decided), and reboot (or restart `canhat_fd_setup.service`, still with the
+> machine stopped) right after.
 
 ### Systemd Services
 
@@ -126,6 +129,194 @@ disable` above and put the machine back into the FD/Plus race this section
 exists to avoid. Update a CAN HAT Plus machine by hand too - copy over the
 changed files/units yourself instead of running `install.sh`.
 
+## Operation
+
+This section is an overview; the full operator runbook (complete reason-code
+table, CRIT triage, `--clear-degraded` caveats, the exact `can_pause`/
+`can_diag.sh` invocations, and the manual unbind/bind procedure) is in
+[`docs/can_watchdog.md`](docs/can_watchdog.md). Read that before touching a
+machine that is in operation.
+
+### System Architecture
+
+The CAN subsystem starts in this boot sequence order:
+
+1. `canhat_fd_rename.service` (`Type=oneshot`,
+   `After=sys-subsystem-net-devices-can0.device sys-subsystem-net-devices-can1.device`,
+   `Before=canhat_fd_setup.service docker.service`) - pins `can0`/`can1` to
+   their physical SPI devices per `CAN_SPI_MAP` (default
+   `spi0.0:can0 spi1.0:can1`) using the lib's `can_pin_name` (`ip link set
+   <iface> down` then `... name <want>`, with a probe-order swap if the two
+   SPI devices enumerate in the opposite order).
+2. `canhat_fd_setup.service` (`Type=oneshot`, `After=canhat_fd_rename.service`)
+   - brings each interface up via the lib's `can_setup_iface`: `ip link set
+   <if> down` then `up type can bitrate ... restart-ms ... berr-reporting ...
+   [dbitrate ... fd on | fd off]`, then `ip link set <if> txqueuelen ...`.
+   **No `ethtool` is used or installed** on the machine - everything goes
+   through `ip link set` / sysfs.
+3. `can_watchdog.service` (`Type=simple`, `Restart=always`,
+   `Wants=canhat_fd_setup.service`, `After=canhat_fd_setup.service`) - the
+   resident watchdog described below.
+
+`canhat_fd_rename.service` does declare `Before=canhat_fd_setup.service
+docker.service` (so renaming happens before both `canhat_fd_setup.service`
+and `docker.service` start), but there is **no further `Before=` ordering**
+from `canhat_fd_setup.service` or `can_watchdog.service` onto whatever
+application or container speaks CAN - beyond that one `Before=`, only the
+`After=` chain above between the three can_tools units themselves.
+If an app or container needs to start strictly after the interfaces are up
+and the watchdog is running, that has to be arranged on the consumer's own
+unit (or handled by the app's own reconnect-on-ENETDOWN logic; see
+`CAN_DEPENDENT_UNITS` in `docs/can_watchdog.md`).
+
+`can_watchdog` is **FD-only**: `install.sh` only ever installs/enables it
+alongside the FD units, and it is never enabled on a CAN HAT Plus machine
+(see "CAN HAT Plus" below).
+
+### Deployment Workflow
+
+**Step 1: Verify what would change (optional but recommended)**
+
+```bash
+sudo ./install.sh --check
+```
+
+Prints a unified diff for every file that would change and exits non-zero if
+there is anything to do. `install.sh --check` run again right after a plain
+`install.sh` should show no differences and exit 0 (running `--check` twice
+in a row with **no** `install.sh` in between shows the same diff both times -
+`--check` itself never changes anything).
+
+**Step 2: Install or upgrade**
+
+```bash
+sudo ./install.sh
+```
+
+Idempotent and non-destructive: files are only written if their contents
+differ, running services are never stopped/restarted/bounced, and an
+existing `/opt/can_tools/can_tools.conf` is never overwritten. The legacy
+`can_monitor.timer`/`can_monitor.service` units **are** stopped, disabled and
+removed by this step if still present - that is the one "stops a service"
+exception, and it is intentional (`can_monitor` is retired).
+
+**Step 3: Apply new settings - machine stopped, not live**
+
+See the warning in "Applying configuration changes" above and
+[`docs/can_watchdog.md`](docs/can_watchdog.md#configuration-is-re-read-live-the-dangerous-part):
+`can_watchdog` re-reads the conf **every cycle**, not at reboot, so **never
+edit the conf while the machine is running** - no key is safe to change live
+(e.g. adding `can1` to `CAN_FLUSH_IFACES` can down/up a live `can1`). Changing `CAN_RESTART_MS` / `CAN_BERR_REPORTING` / `CAN_FD` makes
+the watchdog detect a mismatch against the still-running kernel settings and
+switch that interface to **observe-only** (`reason=config_mismatch_reboot_required`)
+until a reboot (or a machine-stopped `canhat_fd_setup.service` restart)
+actually applies them. Changing `CAN_BITRATE` / `CAN_DBITRATE` /
+`CAN_TXQUEUELEN` is **not** checked for a mismatch at all, so it is applied
+live, to a running interface, the next time the watchdog calls
+`can_setup_iface` for any reason (a TX flush, a bus-off recovery, a stall
+rebind) - with the machine still operating. Only ever edit the conf with the
+machine mechanically stopped, then reboot (or restart
+`canhat_fd_setup.service`, still stopped) right away.
+
+### Watchdog Behavior and Logging
+
+Once running, `can_watchdog.service` monitors each CAN interface every
+`WD_LOOP_S` (1 s default) and recovers autonomously from most error
+conditions. **It does not automatically reboot the machine** (see Issue #10).
+A one-paragraph summary of each mechanism follows; see
+[`docs/can_watchdog.md`](docs/can_watchdog.md) for the exact conditions,
+backoff schedules and the complete reason-code table.
+
+- **TX flush** (ERROR-WARNING/ERROR-PASSIVE, `CAN_FLUSH_IFACES`, default
+  `can0` only): if all of rx/tx/rx_fifo/tx_fifo stay unchanged for 2
+  consecutive cycles, the watchdog re-runs `can_setup_iface` (a down/up) to
+  discard stale queued TX frames so they are not resent once the ECU comes
+  back. Any other time the interface is in ERROR-WARNING/ERROR-PASSIVE, the
+  watchdog does not touch it at all.
+- **Bus-off recovery**: on `BUS-OFF`, the watchdog waits for the rx-offload
+  queue to settle, then re-runs `can_setup_iface` (down/up; the ifindex does
+  not change, so application sockets stay valid). Repeated bus-off within a
+  short window backs off exponentially (1 s up to 30 s); a storm (5+ within
+  60 s) logs one CRIT but recovery keeps running.
+- **rx-offload stall detection**: if `rx_packets` is flat while
+  `rx_fifo_errors`/`tx_fifo_errors` keep growing for 3 consecutive cycles
+  (or 2 cycles right after a bus-off recovery, or 3 bus-off recoveries in a
+  row with no rx progress), the watchdog performs an **unbind/bind** cycle
+  (not a down/up - the rx-offload lock this works around survives a plain
+  down/up). This changes the ifindex, so any application holding an open CAN
+  socket must reopen it; see `CAN_DEPENDENT_UNITS` in
+  `docs/can_watchdog.md` to restart specific units/containers afterward.
+- **Degraded mode**: if a step above cannot complete (Oops/BUG detected in
+  the kernel log, an unbind/bind timeout, a rebind-quota overrun, 3
+  consecutive `ip` timeouts system-wide, ...), the affected interface(s) are
+  marked degraded (`/run/can_tools/degraded_<iface>`) and the watchdog stops
+  touching them - it only re-logs a CRIT every 10 minutes
+  (`reason=manual_reboot_required`). See `docs/can_watchdog.md` for which
+  reasons are safe to clear with `can_watchdog.sh --clear-degraded <if>` and
+  which genuinely require a reboot.
+
+#### Reading Watchdog Logs
+
+```bash
+journalctl -u can_watchdog -n 50 -f      # last 50 lines, follow
+journalctl -u can_watchdog -p crit       # CRIT lines only (degraded/storm/Oops/etc.)
+```
+
+Common log line shape: `iface=<if> reason=<reason> <details...>` (a repeated
+`iface+reason` pair is rate-limited to one line per `WD_LOG_RATE_S`, with
+`suppressed=N` on the next line that does get through). The 60 s periodic
+INFO summary line (`iface=.. rx=.. tx=.. rxfifo=.. txfifo=.. ifindex=..
+degraded=0|1`) has no `reason=` field - that is the only line with
+`degraded=`. See `docs/can_watchdog.md` for the full reason-code table and
+what each one means operationally.
+
+#### Diagnosing a Stopped Interface
+
+```bash
+cat /sys/class/net/can0/statistics/rx_packets     # should be increasing
+cat /sys/class/net/can0/statistics/rx_fifo_errors # usually 0 / not growing
+cat /sys/class/net/can0/statistics/tx_fifo_errors # usually 0 / not growing
+```
+
+`rx_packets` flat while `rx_fifo_errors`/`tx_fifo_errors` grow, for 3
+consecutive watchdog cycles, is exactly the rx-offload stall signal above -
+the watchdog is already acting on it; you do not need to intervene unless it
+shows up as degraded. `ip -s link show can0` does not expose the fifo
+counters - use sysfs as above.
+
+### Pause and Diagnostics
+
+See [`docs/can_watchdog.md`](docs/can_watchdog.md#pause) for the working
+`can_pause`/`can_unpause`/`can_diag.sh` invocations and for `can_diag.sh`'s
+actual argument (a duration in seconds, not an interface name), output path,
+and what to do if it was killed with SIGKILL mid-capture. In short:
+- `can_pause`/`can_unpause` are **shell functions in `can_tools_lib.bash`**,
+  not standalone commands - there is nothing on `$PATH` called `can_pause`.
+- `can_diag.sh` is **bench / machine-stopped use only**: it pauses the
+  watchdog and turns on bus-error reporting on every interface while it
+  captures.
+
+### Netlink Hangs (rtnl_hung)
+
+`rtnl_hung` is a **global, all-interface** condition, not a per-interface
+one: every watchdog cycle calls one bounded `ip -d link show type can` to
+read interface state; if *that call itself* (not an unbind/bind) times out
+for `WD_RTNL_HUNG_CONSEC` (3) consecutive cycles, **every** configured
+interface is marked degraded with `reason=rtnl_hung`, and the watchdog stops
+calling `ip` at all - it falls back to sysfs-only observation for everything
+until a reboot. A single interface's unbind hanging instead produces
+`reason=unbind_timeout` on just that interface (see
+`docs/can_watchdog.md`). Either way, recovery is **manual reboot only** -
+there is no live recovery from a genuinely hung netlink socket.
+
+### CAN HAT Plus (Standard CAN)
+
+The notes in the "Systemd Services" section about CAN HAT Plus still apply:
+
+- `install.sh` does not install or enable `canhat_plus_*` units.
+- `can_watchdog` is FD-only and will not run on a CAN HAT Plus machine.
+- Set up CAN HAT Plus by hand (see the "Systemd Services" section).
+
 ## CAN Interface Setup
 
 ### PCAN setup

can_tools.conf.example

@@ -21,9 +21,13 @@
 # CAN_IFACES=can0 can1
 # CAN_SPI_MAP=spi0.0:can0 spi1.0:can1
 # CAN_BITRATE=250000
-# CAN_RESTART_MS=100
+# CAN_RESTART_MS=0
+#   WARNING: Do NOT set CAN_RESTART_MS to a value other than 0. The kernel's
+#   automatic bus-off restart (via echo_skb, see raspberrypi/linux#6916) will
+#   race with can_watchdog's recovery logic, leading to unpredictable behavior.
+#   The watchdog owns bus-off recovery; leave this at 0.
 # CAN_BERR_REPORTING=off
-# CAN_FD=on
+# CAN_FD=off
 # CAN_DBITRATE=2000000
 # CAN_TXQUEUELEN=10