diff --git a/.claude/skills/README.md b/.claude/skills/README.md new file mode 100644 index 00000000..488ff2e2 --- /dev/null +++ b/.claude/skills/README.md @@ -0,0 +1,33 @@ +# CrossPoint Reader: Claude Code skills + +Project skills for Claude Code. Claude auto-discovers them and loads one when the +task matches its `description`; you do not invoke them by hand. They encode how +this project wants C/C++ written: the judgment calls and self-review gates that +keep the firmware small, stable, and reviewable. + +These are written for capable agents, not beginners. They are principle- and +decision-focused on purpose. They deliberately avoid line-number citations, +which drift; they anchor on durable names (APIs, types, macros, files). + +This is separate from `.skills/SKILL.md`, the GitHub coding-agent guide that +mirrors CLAUDE.md. CLAUDE.md stays the always-loaded rule set; these skills are +the applied decision procedures that load on demand and add the judgment layer +CLAUDE.md does not carry. + +| Skill | Loads when you are... | +|---|---| +| `heap-discipline` | allocating memory: new/malloc/vector/string, buffers, caches | +| `control-flow-clarity` | writing branching logic, state flags, modes, if/else ladders | +| `hal-and-abstractions` | touching storage, input, display, settings, i18n, rendering | +| `scope-discipline` | adding a feature, activity, lib, setting, or dependency | +| `refactor-for-review` | refactoring, cleaning up, or preparing a change for PR | + +Each skill ends with a self-review checklist Claude runs against its own diff +before handing it back. Reviewing a PR? Those checklists double as a fast rubric. + +## Maintaining these + +Edit the `SKILL.md` under each directory. Keep them tight. Do not restate +CLAUDE.md; add the judgment CLAUDE.md cannot afford to carry. Trigger quality +lives in the `description` field: it must name the situations that should pull +the skill in, in the words a contributor's task would use. diff --git a/.claude/skills/control-flow-clarity/SKILL.md b/.claude/skills/control-flow-clarity/SKILL.md new file mode 100644 index 00000000..f18fc3b9 --- /dev/null +++ b/.claude/skills/control-flow-clarity/SKILL.md @@ -0,0 +1,56 @@ +--- +name: control-flow-clarity +description: Branching and state-modeling clarity in C/C++. Use when writing or refactoring logic that branches on discrete values: if/else-if ladders, status flags, mode or state ints, or anything that should be an enum plus an exhaustive switch. Covers enum class over magic ints, exhaustive switch over nested if, early-return guard clauses, table dispatch, and when each is the right call. +--- + +# Control-Flow Clarity + +The goal: a reviewer verifies correctness by reading, not by tracing. Branching +that mirrors the problem's shape is self-evident; branching that encodes it in +ad-hoc ints and nesting forces the reader to reconstruct intent. + +## Core moves + +- **Model a closed set of states/modes as an `enum class`, not ints or bools.** + A variable kept honest by a comment ("0 = hidden, 1 = showing, 2 = confirm") + is a latent bug. Make it an enum and the comment becomes the type. +- **Dispatch on an enum with an exhaustive `switch`, no `default`.** This + codebase relies on it: omitting `default` lets the compiler flag the + unhandled case when someone adds an enum value. A `default:` that swallows the + unknown case throws that safety away. Add `default` only when "every other + value does nothing" is a deliberate, documented decision. +- **Replace nested `if/else-if` ladders that branch on one discriminant with a + `switch`.** If each branch only maps input to a value, prefer a lookup table + (`static constexpr` array) over both. +- **Prefer early-return guard clauses over nested success bodies.** Handle the + error/empty/skip cases first and return; keep the main path at the left + margin. + +## When NOT to switch + +- Branches test unrelated conditions, not one discriminant: a guarded `if` + sequence is honest; a switch would be forced. +- Two outcomes on a genuine boolean: keep the `if`. +- The discriminant is an open or unbounded set (arbitrary ints, strings): table + or map, not a switch. + +## Enum hygiene + +- `enum class` by default for type safety. Plain `enum` only when values must + implicitly convert (e.g. a value that doubles as a UI dropdown index), and + then give it a trailing `_COUNT` sentinel for safe bounds/iteration, matching + the existing settings enums. +- Name the discriminant after what it selects, not its storage: + `Orientation orientation`, not `uint8_t mode`. +- No magic numeric codes for states. If you write a comment mapping numbers to + meanings, you owe an enum. + +## Self-review + +- [ ] No int/bool standing in for a closed set of modes; it is an `enum class`. +- [ ] Enum dispatch is an exhaustive `switch` with no catch-all `default` (or + the `default` is a documented deliberate choice). +- [ ] No nested if/else-if ladder on a single discriminant that should be a + switch or a table. +- [ ] Error/skip cases are early-return guards; the happy path is not buried. +- [ ] No magic numbers where a named enum or `constexpr` would state the intent. diff --git a/.claude/skills/hal-and-abstractions/SKILL.md b/.claude/skills/hal-and-abstractions/SKILL.md new file mode 100644 index 00000000..bca92f2d --- /dev/null +++ b/.claude/skills/hal-and-abstractions/SKILL.md @@ -0,0 +1,59 @@ +--- +name: hal-and-abstractions +description: Layering and abstraction discipline for the firmware. Use when touching storage, input, display, settings, i18n, or rendering, or any code that could reach into the SDK. Covers routing through the HAL (HalStorage / HalGPIO / HalDisplay) instead of raw SDK classes, MappedInputManager logical buttons instead of raw GPIO indices, UITheme/GUI for all rendering, the singleton macros, tr() for user-facing text, and where a new abstraction boundary belongs. +--- + +# HAL and Abstractions + +CLAUDE.md lists the HAL classes and the SdFat-concurrency reason they exist. +This is when and how to route through them, and where to draw a new boundary. + +## Route through the layer, always + +- **SD card I/O:** `Storage` (HalStorage) and `HalFile`. Never `SdFat`, + `FsFile`, `SdSpiCard`, `FsBaseFile`, or `SDCardManager` directly. The HAL + serializes every SD access through one mutex; bypassing it races the SPI state + machine and panics FreeRTOS (CLAUDE.md has the failure mode). This is a + correctness boundary, not a style preference. +- **Display:** `HalDisplay` over `EInkDisplay`. **Input:** `HalGPIO` over + `InputManager`. +- **Rendering:** everything through the `GUI` macro (UITheme) and the renderer's + oriented metrics. No hardcoded fonts, colors, coordinates, or 800/480 + literals; ask the renderer for width/height and use the oriented viewable + area. +- **Input in activities:** `MappedInputManager::Button` logical enums + (`Button::Confirm`, `Button::PageForward`, ...). Never raw `HalGPIO::BTN_*` + indices outside `ButtonRemapActivity`. Logical buttons survive user remapping + and orientation; raw indices do not. +- **Shared state:** the singleton macros (`SETTINGS`, `APP_STATE`, `GUI`, + `Storage`, `I18N`), not threaded pointers. + +## User-facing text + +Every string a user reads goes through `tr(STR_*)`. Add the key to the English +YAML, regenerate with `scripts/gen_i18n.py`, then use the `StrId`. Log lines +(`LOG_*`) stay hardcoded. + +## Drawing a new boundary + +When you need an SDK capability the HAL does not expose yet, **add the method to +the HAL; do not reach around it.** The new method inherits the mutex, logging, +and error contract the rest of the HAL carries. A one-off direct SDK call in an +activity is exactly the layering violation the mutex discipline cannot tolerate. + +Keep abstractions thin. A wrapper that only renames an SDK call without adding +the mutex, logging, or an error contract is dead weight. Add a layer only when +it carries one of those contracts or hides a real implementation choice. + +## Self-review + +- [ ] No direct SdFat / FsFile / SDCardManager / EInkDisplay / InputManager use + outside `lib/hal`. +- [ ] File access uses `HalFile`; no `.close()` on a local handle + (DESTRUCTOR_CLOSES_FILE); members closed in `onExit`. +- [ ] Input uses `MappedInputManager::Button`, not raw `BTN_*` indices. +- [ ] Rendering goes through GUI/UITheme and oriented metrics; no 800/480 or + hardcoded fonts/coords. +- [ ] User-facing strings use `tr(STR_*)`; new keys added to YAML and + regenerated. +- [ ] Any new SDK capability is exposed as a HAL method, not called inline. diff --git a/.claude/skills/heap-discipline/SKILL.md b/.claude/skills/heap-discipline/SKILL.md new file mode 100644 index 00000000..9b5ea64d --- /dev/null +++ b/.claude/skills/heap-discipline/SKILL.md @@ -0,0 +1,65 @@ +--- +name: heap-discipline +description: Memory allocation discipline for the ESP32-C3 (~380KB RAM, no PSRAM, single 48KB framebuffer). Use whenever writing or reviewing code that allocates: new / malloc / std::vector / std::string, buffers, caches, or anything held across a loop or an activity lifecycle. Covers makeUniqueNoThrow vs raw new/malloc, fragmentation avoidance, reserve-before-push_back, alloc-once-reuse, stack vs heap sizing, and the chunked grayscale buffer pattern. +--- + +# Heap Discipline (ESP32-C3) + +CLAUDE.md states the allocation rules. This is the procedure you run while +writing the code and the gate you run before handing it back. + +The constraint that makes every call matter: ~380KB RAM, no PSRAM, one 48KB +framebuffer. **Fragmentation, not total usage, is what kills this device.** +Free-heap can read fine while the largest free block is too small for the next +allocation. Optimize for not leaving holes, not just for using fewer bytes. + +## Allocation decision procedure + +Ask in order; stop at the first yes. + +1. **Stack?** Local, bounded, under ~256 bytes total: plain array/struct. No + heap, no fragmentation. Keep frames lean; the task stack is small. +2. **Compile-time constant?** `static constexpr` lives in flash, costs zero DRAM. +3. **Allocated once and reused for an activity's lifetime?** Allocate in + `onEnter`, hold in a member, release in `onExit`. Never per-frame, never + per-iteration. +4. **Dynamic and fallible?** `makeUniqueNoThrow(...)` / + `makeUniqueNoThrow(n)` from `lib/Memory/Memory.h`. Null-check, `LOG_ERR` + with the size, return false. It frees on every exit path. +5. **A C/SDK API takes ownership and frees it itself?** Only then raw + `new (std::nothrow)` / `malloc`, with a comment naming who frees it. + +Bare `new` / `new[]` is never correct here: under `-fno-exceptions` it calls +`abort()` on OOM instead of returning null. + +## Fragmentation rules + +- `std::vector`: `reserve(n)` before any `push_back` loop. Each growth is + alloc-copy-free (three heap ops) and leaves a hole. Unknown n: estimate high. +- No repeated `new`/`delete` or growing containers inside a loop or render path. + Hoist the allocation out of the loop. +- Large contiguous blocks fragment worst. Full-screen-class buffers use the + chunked `storeBwBuffer` / `restoreBwBuffer` path in `GfxRenderer` so they + never demand one contiguous 48KB block. Reuse that path. Do not malloc a + second full-screen buffer. +- `std::string` / Arduino `String`: acceptable on cold paths (file I/O, one-shot + setup). Banned on hot/render paths. Build text with a stack `char[]` + + `snprintf`; if a `std::string` is unavoidable, `reserve` it first. + +## Justify every allocation + +Per CLAUDE.md's evidence rule: when you add a heap allocation, state in one line +why stack/static/reuse was rejected and the worst-case size. If you cannot name +the size, you cannot budget it, and you should not allocate it. + +## Self-review before handoff + +- [ ] No bare `new`/`new[]`. Every fallible alloc is `makeUniqueNoThrow`, or a + raw alloc with an explicit owner comment. +- [ ] Every allocation is null-checked with `LOG_ERR` before the error return. +- [ ] No allocation inside a loop or render path that could be hoisted. +- [ ] Every `push_back` loop has a preceding `reserve`. +- [ ] Anything allocated in `onEnter` is released in `onExit`; member `HalFile` + closed there too. +- [ ] No second full-screen buffer; grayscale uses store/restoreBwBuffer. +- [ ] Each new allocation carries a one-line size + why-not-stack/static note. diff --git a/.claude/skills/refactor-for-review/SKILL.md b/.claude/skills/refactor-for-review/SKILL.md new file mode 100644 index 00000000..56f43f66 --- /dev/null +++ b/.claude/skills/refactor-for-review/SKILL.md @@ -0,0 +1,59 @@ +--- +name: refactor-for-review +description: Producing small, single-concern, reviewable changes. Use when refactoring, cleaning up, restructuring, decomposing, or preparing a change for PR, especially in this multi-contributor AI-assisted codebase that is prone to sprawl diffs. Covers one-concern-per-commit, extracting helpers without widening scope, not bundling unrelated edits, decomposing oversized activities, comment hygiene, and a pre-handoff self-review checklist. +--- + +# Refactor for Review + +This is a multi-contributor, AI-assisted codebase, and the dominant failure mode +is the sprawl diff: a one-line intent that touches thirty files. The goal is a +change a reviewer can verify in one sitting. Cleaner structure that makes the +next change easier is the win, not lines added. + +## One concern per change + +- A commit/PR does one thing. A bug fix is not also a rename is not also a + reformat. If you spot an unrelated improvement mid-change, leave it or capture + it separately; do not fold it in. +- When the working tree has bundled two changes, separate them with the + copy-affected-files-aside, reset, re-apply one concern, restore the rest + pattern, not by committing the tangle. +- Refactor and behavior change do not ride together. A pure refactor must not + alter behavior; a behavior change should not drag a refactor along. If both + are needed: two commits, refactor first. + +## Keep the diff narrow + +- Extract a helper to remove real duplication or to name a concept, not to chase + abstraction. Three-plus copies, or a block that needs a name to be understood: + extract. Two similar lines: leave them. +- No "while I'm here" scope creep. A signature or type change that ripples to + many call sites is its own PR: map every caller first, update them in one + topological pass, and land it separately, not as a rider on a feature. +- Match the surrounding code: comment density, naming, idiom. The diff should + read like the file, not like a different author. + +## Decompose oversized units + +An activity or function that has outgrown one screen of responsibility (multiple +unrelated state machines, or a file far larger than its siblings) is a +decomposition candidate. Extract a cohesive sub-responsibility into its own +unit, as a standalone behavior-preserving refactor, verified on its own, never +mixed into a feature change. + +## Comments earn their place + +Comments explain why: an invariant, a defense, a past incident, a non-obvious +constraint. Never what the next line already says. Delete narration, phase-marker +comments ("now we loop over..."), and restated function names. If a comment and +the code it sits on say the same thing, the comment is the thing to cut. + +## Self-review before handoff + +- [ ] The change does exactly one thing; nothing unrelated rode along. +- [ ] Refactor and behavior change are not mixed in one commit. +- [ ] No "while I'm here" creep; rename/signature ripples are split out. +- [ ] Extractions remove real duplication or name a real concept, not + speculative abstraction. +- [ ] New comments say why, not what; no narration or phase markers. +- [ ] A reviewer can understand the diff without running it. diff --git a/.claude/skills/scope-discipline/SKILL.md b/.claude/skills/scope-discipline/SKILL.md new file mode 100644 index 00000000..3c262ccc --- /dev/null +++ b/.claude/skills/scope-discipline/SKILL.md @@ -0,0 +1,54 @@ +--- +name: scope-discipline +description: Feature-scope discipline for a dedicated e-reader (not a Swiss Army knife). Use when adding a feature, a new activity, a new lib, a setting, or a dependency, or when a request would grow the firmware's surface. Covers the SCOPE.md test, the RAM-cost vs reading-benefit gate, preferring no-code or existing-mechanism solutions, awareness of the existing activity surface, and how to push back on out-of-scope asks. +--- + +# Scope Discipline + +The mission: do one thing exceptionally well, focused reading on constrained +hardware. `SCOPE.md` is the source of truth for what is in and out. Read it +before adding surface. This is the gate to run before writing a new feature. + +## The gate + +Before adding a feature, activity, lib, setting, or dependency, answer in order: + +1. **Is it in `SCOPE.md`?** Explicitly out: interactive apps (notepad, + calculator, games), active connectivity (RSS, news, browser), media/audio + playback. If it is out, say so and stop. +2. **Does it materially improve focused reading?** If the benefit is + "nice to have" or serves a different use case, it is out. This is not a PDA. +3. **What does it cost in RAM and in the largest-free-block budget?** A feature + that adds steady-state RAM or a large transient allocation needs a reading + benefit that clearly outweighs it. Quantify with `firmware_size_history.py` + and `script_profile_mem.sh` rather than guessing. +4. **Can it be done with no new code?** Prefer an existing activity, an existing + setting, or a doc over a new code path. The cheapest feature is the one + already built. + +If a request fails the gate, push back with the specific reason and the +`SCOPE.md` basis, and offer the in-scope alternative. Make the call and say why; +do not just hand over a menu. + +## Surface awareness + +The firmware already carries dozens of activities. Each new one is permanent +RAM, permanent maintenance, and another thing every future refactor must not +break. Default to extending an existing activity or setting before adding a new +screen. New top-level surface needs a real justification, not "it would be +convenient." + +## Settings are not free + +A new setting is a field to persist, migrate, validate, translate, and render, +plus combinatorial test burden. Add one only when users genuinely need the +choice; otherwise pick a sensible fixed default. + +## Self-review + +- [ ] Checked against `SCOPE.md`; not on the out-of-scope list. +- [ ] Stated the concrete reading benefit, not a generic "useful." +- [ ] Named the RAM/size cost (measured, not guessed) and why the benefit wins. +- [ ] Checked whether an existing activity/setting/doc already covers it. +- [ ] New setting (if any) is justified by a real user need, not added + "just in case." diff --git a/.gitignore b/.gitignore index 088d756c..6bd12537 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,7 @@ build lib/EpdFont/scripts/downloaded_fonts/ lib/EpdFont/scripts/instanced_fonts/ lib/EpdFont/scripts/output/ +# Claude Code: share project skills with the team, keep local agent state +# (worktrees, scheduled-task locks, settings.local, scout CLEANUP.md) out. +.claude/* +!.claude/skills/