diff --git a/.clang-format b/.clang-format index bbfb6c9..60083af 100644 --- a/.clang-format +++ b/.clang-format @@ -2,13 +2,13 @@ # sources: Google, wrapped at 100 columns. # # It applies to src/pinside/firmware/templates/, which is where the C actually -# lives — a generated project is a copy of those files plus one table, so +# lives: a generated project is a copy of those files plus one table, so # formatting the templates is what keeps every generated fixture consistent. # There is nothing to format in a generated directory; regenerate instead. BasedOnStyle: Google ColumnLimit: 100 -# The compact lookup tables this code base uses throughout — one case label, -# one assignment, one return — read better than the exploded form Google +# The compact lookup tables this code base uses throughout (one case label, +# one assignment, one return) read better than the exploded form Google # defaults to. AllowShortCaseLabelsOnASingleLine: true diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md new file mode 100644 index 0000000..3472f1a --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -0,0 +1,29 @@ +--- +name: Bug report +about: Something pinside got wrong +labels: bug +--- + +## What happened + + + +``` +$ pinside ... +``` + +## What you expected + +## The board or config + +pinside reads local files, so the fastest fix usually starts from the input. +If the `.kicad_pcb` or the fixture config cannot be shared, the relevant part +usually can: a footprint, an `Edge.Cuts` shape, one channel out of the config. + +## Versions + +- pinside: +- Python: +- KiCad, if `project` is involved: +- OS: diff --git a/.github/ISSUE_TEMPLATE/check_request.md b/.github/ISSUE_TEMPLATE/check_request.md new file mode 100644 index 0000000..b85352e --- /dev/null +++ b/.github/ISSUE_TEMPLATE/check_request.md @@ -0,0 +1,29 @@ +--- +name: New check +about: A way a fixture goes wrong that pinside does not catch yet +labels: check +--- + +## The mistake + + + +## What the board file shows + + + +## The finding it should produce + +A finding says what is wrong, which references it applies to, and what to do. + +- Summary: +- References: +- What to do: +- Severity: error / warning / info + + diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 0000000..0086358 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1 @@ +blank_issues_enabled: true diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..9d868b8 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,25 @@ +## What this changes + + + +## Checks + +- [ ] `scripts/lint.sh && scripts/test.sh` pass. +- [ ] New or changed behaviour has a test. +- [ ] `CHANGELOG.md` has an entry under `[Unreleased]`, if this is user-visible. + +CONTRIBUTING.md asks two questions that CI cannot answer for you. Answer the +ones that apply, and delete the rest. + +- [ ] **Touched anything under `src/pinside/kicad/`?** The KiCad tests run in + CI in a container, but a real KiCad install is the only place `project` + output gets opened. Say which KiCad version you ran against. +- [ ] **Touched anything under `src/pinside/firmware/templates/`?** CI compiles + the host tests with the mock HAL and cross-compiles against the Pico SDK. + Say whether you flashed it and what the fixture did. +- [ ] **Added or changed a target in `targets.py`?** Name the datasheet and the + table you checked the function map against. A wrong map produces a config + that validates and does not work. +- [ ] **Changed a probe in `pogo.py`?** Cite the supplier drawing. These numbers + end up as drill sizes on a board somebody orders. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 108955d..6131b01 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -51,6 +51,21 @@ jobs: - name: Run tests run: scripts/test.sh --verbose + # Once, not on every interpreter: the number is about which code paths + # the suite reaches, and that does not vary by Python version. + - name: Coverage + if: matrix.python == '3.13' + run: scripts/test.sh --coverage + + # The client tests run against a fake port either way. With pyserial here + # two more run against pyserial's own loop:// port, which is what caught + # the client accepting an echoed request as a response. + - name: The client, against real pyserial + if: matrix.python == '3.13' + run: | + python -m pip install 'pyserial>=3.5' + scripts/test.sh --verbose + example: name: End to end runs-on: ubuntu-latest @@ -65,6 +80,20 @@ jobs: - name: Install run: python -m pip install . + # The core must work with nothing else installed. If this ever needs a + # dependency to run, the claim on the front of the README is false. + - name: Nothing but the standard library is installed + run: | + python - <<'PY' + import importlib.metadata as md + installed = {d.metadata["Name"].lower() for d in md.distributions()} + # pip and its own bundled bits are the venv, not pinside's dependencies. + allowed = {"pinside", "pip", "setuptools", "wheel", "pkg-resources"} + extra = sorted(installed - allowed) + assert not extra, f"pinside pulled in {extra}" + print("clean:", sorted(installed)) + PY + - name: Check the example board run: pinside check examples/demo-board.kicad_pcb --strict @@ -99,7 +128,27 @@ jobs: pinside generate examples/demo-fixture.json --out /tmp/demo-firmware /tmp/demo-firmware/test/run.sh - - name: The generated contract is valid JSON and names the config hash + # `probe` is the one command that needs an optional dependency. Both + # halves are worth asserting: that it says how to install it, and that it + # works once installed. + - name: probe explains its missing dependency, then works with it + run: | + set +e + pinside probe examples/demo-fixture.json 2>/tmp/probe.err + status=$? + set -e + cat /tmp/probe.err + test "$status" -ne 0 + grep -q "pinside\[client\]" /tmp/probe.err + python -m pip install '.[client]' + pinside probe examples/demo-fixture.json --port loop:// --timeout 1 2>/tmp/probe2.err \ + && { echo "a loopback should not pass as a fixture" >&2; exit 1; } + cat /tmp/probe2.err + + # The contract's real validation lives in tests/test_contract.py, which checks it against + # the firmware's own dispatch table. What is left for this job is the thing only an + # installed wheel can show: that openrpc.json is shipped and generated at all. + - name: The installed package generates a usable contract run: | python - <<'PY' import json, pathlib @@ -111,6 +160,142 @@ jobs: len(contract["x-pinside"]["channels"]), "channels") PY + kicad: + name: KiCad project generation + runs-on: ubuntu-latest + # tests/test_kicad.py generates a fixture project and runs KiCad's own ERC + # and DRC over it. Without KiCad it skips, which meant the whole emitter + # under src/pinside/kicad/ -- the code that has to produce a file KiCad will + # actually open -- was only ever checked on a developer's machine. + # + # --user root because the image declares USER kicad, and the runner mounts + # its own /__w into the container owned by root. As the kicad user, + # actions/checkout cannot write /__w/_temp/_runner_file_commands and dies + # with EACCES before a single step of ours runs. + container: + image: kicad/kicad:10.0 + options: --user root + steps: + - uses: actions/checkout@v7 + # Still needed: the checkout is owned by a different uid than git expects, + # and git refuses to look at a tree it considers someone else's. + - name: Trust the checkout + run: git config --global --add safe.directory "$GITHUB_WORKSPACE" + - name: Versions + run: | + kicad-cli version + python3 --version + + # A KiCad install has libraries on disk and a *profile* that maps library + # nicknames to them. The profile is created on the first GUI launch, from + # KiCad's own templates; kicad-cli never creates one. So in a fresh + # container every `MCU_Module:` and `power:` reference is unresolvable and + # ERC reports 44 warnings about configuration rather than about the + # schematic. + # + # Seeding the tables is what a first launch does. Verified by reproducing + # both halves on a workstation with KICAD_CONFIG_HOME pointed at an empty + # directory: 44 violations without them, 0 with. + - name: Seed the library tables a first launch would create + run: | + # The default tables sit directly in the template directory. KiCad + # also ships project templates one level below it, each with its own + # pair, and seeding from one of those configures nothing: an earlier + # `find | head -1` picked template/Edgeberry_Cartridge and died on its + # missing fp-lib-table. Requiring both files in one directory, from a + # known location, is what makes that unambiguous. + for dir in /usr/share/kicad/template /usr/local/share/kicad/template; do + if [ -f "$dir/sym-lib-table" ] && [ -f "$dir/fp-lib-table" ]; then + template="$dir" + break + fi + done + test -n "${template:-}" || { echo "no default library tables" >&2; exit 1; } + + version=$(kicad-cli version | grep -oE '[0-9]+\.[0-9]+' | head -1) + config="$HOME/.config/kicad/${version:-10.0}" + mkdir -p "$config" + cp "$template/sym-lib-table" "$template/fp-lib-table" "$config/" + + # A wrong table copies exactly as successfully as a right one, so + # check the result names the libraries the generated schematic uses. + for lib in MCU_Module power Device; do + grep -q "\"$lib\"" "$config/sym-lib-table" || + { echo "the seeded table does not name $lib" >&2; exit 1; } + done + echo "seeded $config from $template" \ + "($(grep -c '(lib ' "$config/sym-lib-table") libraries)" + # Not scripts/test.sh: the image's Python is the one KiCad ships with, and + # the script's venv handling is for the tooling, not the interpreter. + # + # PINSIDE_REQUIRE_KICAD turns this job's reason for existing into an + # assertion. Without it, a container that stopped shipping kicad-cli, or + # a symbol path that moved, would make the tests skip and the job pass: + # green, and covering nothing. + - name: Run the suite, KiCad tests included + env: + PINSIDE_REQUIRE_KICAD: "1" + run: PYTHONPATH=src python3 -m unittest discover -s tests --verbose + + firmware: + name: Firmware against the Pico SDK + runs-on: ubuntu-latest + # The host tests in the `test` job compile fixture_core.c against the mock + # HAL, which never touches fixture_hal_rp2350.c. That file is the half of + # the firmware that talks to real hardware, and until this job existed the + # only thing checking it was clang-format. + env: + PICO_SDK_VERSION: "2.1.1" + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-python@v7 + with: + python-version: "3.13" + + - name: Install the ARM toolchain + run: | + sudo apt-get update + sudo apt-get install -y gcc-arm-none-eabi cmake ninja-build + + - name: Cache the SDK + id: sdk + uses: actions/cache@v4 + with: + path: pico-sdk + key: pico-sdk-${{ env.PICO_SDK_VERSION }} + + # Pinned to a tag, not to master: an SDK that moves under the templates + # turns a template bug and an upstream change into the same red job. + - name: Fetch the SDK + if: steps.sdk.outputs.cache-hit != 'true' + run: | + git clone --branch "$PICO_SDK_VERSION" --depth 1 \ + https://github.com/raspberrypi/pico-sdk.git pico-sdk + git -C pico-sdk submodule update --init --depth 1 + + - name: Install + run: python -m pip install . + + - name: Generate the demo firmware + run: pinside generate examples/demo-fixture.json --out /tmp/fw + + - name: Cross-compile it + env: + PICO_SDK_PATH: ${{ github.workspace }}/pico-sdk + run: | + cmake -S /tmp/fw -B /tmp/fw/build -G Ninja + cmake --build /tmp/fw/build + # A UF2 is the artefact you actually flash. CMake will happily produce + # an ELF and no UF2 if the SDK's post-build step was never wired up. + find /tmp/fw/build -name '*.uf2' -print | grep -q . \ + || { echo "no .uf2 was produced" >&2; exit 1; } + + - uses: actions/upload-artifact@v7 + with: + name: demo-firmware-uf2 + path: /tmp/fw/build/**/*.uf2 + if-no-files-found: error + package: name: Package runs-on: ubuntu-latest diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1b7b192..e8ac3ca 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -6,11 +6,11 @@ on: workflow_dispatch: permissions: - contents: write + contents: read jobs: - release: - name: Build and publish + build: + name: Build runs-on: ubuntu-latest steps: - uses: actions/checkout@v7 @@ -37,7 +37,60 @@ jobs: python -m pip install --upgrade build python -m build - - name: Publish the release + # The same assertion CI makes on every push. Repeated here because this is + # the artefact people install, and a wheel that lost the templates still + # imports and fails at the moment somebody runs `generate`. + - name: The wheel carries the firmware templates + run: | + python - <<'PY' + import pathlib, zipfile + wheel = next(pathlib.Path("dist").glob("*.whl")) + names = zipfile.ZipFile(wheel).namelist() + templates = [n for n in names if "firmware/templates/" in n] + assert len(templates) >= 9, f"only {len(templates)} templates in {wheel.name}" + assert any(n.endswith("py.typed") for n in names), "py.typed missing" + PY + + - uses: actions/upload-artifact@v7 + with: + name: dist + path: dist/ + if-no-files-found: error + + pypi: + name: Publish to PyPI + needs: build + runs-on: ubuntu-latest + # A trusted publisher: PyPI verifies this workflow's OIDC identity, so there + # is no long-lived token in the repository to leak or rotate. Configure it at + # pypi.org/manage/project/pinside/settings/publishing with + # owner=bitcrushtesting, repository=pinside, workflow=release.yml, + # environment=pypi. + environment: + name: pypi + url: https://pypi.org/p/pinside + permissions: + id-token: write + steps: + - uses: actions/download-artifact@v7 + with: + name: dist + path: dist/ + - uses: pypa/gh-action-pypi-publish@release/v1 + + github: + name: Publish the GitHub release + needs: pypi + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - uses: actions/checkout@v7 + - uses: actions/download-artifact@v7 + with: + name: dist + path: dist/ + - name: Create the release env: GH_TOKEN: ${{ github.token }} run: | diff --git a/.gitignore b/.gitignore index ab8a2ac..93b38ec 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,27 @@ dist/ # scripts/test.sh and the generated firmware's own tests build here. /tmp-firmware/ + +# scripts/test.sh --coverage +.coverage +htmlcov/ + +# What KiCad leaves beside a board it has opened. Per-user state and automatic +# snapshots: they churn on every open, whether or not anything was edited. +# +# Not *.kicad_pro. That one carries the design rules, the net classes and the +# board stackup, so it belongs in the repository -- and `pinside project` +# generates one deliberately, as part of the project it writes. +*.kicad_prl +.history/ +*-backups/ +fp-info-cache +_autosave-* +*.kicad_pcb-bak +*.kicad_sch-bak +*.kicad_*.lck + +# The demo board is an input to pinside, not a KiCad project: it is a bare +# .kicad_pcb with no schematic, and the .kicad_pro is one KiCad invents the +# first time somebody opens it. +/examples/demo-board.kicad_pro diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b174be..326efd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,27 +13,89 @@ existing agents wrong. ## [Unreleased] +## [0.2.0] + +The fixture board, the firmware's host side, and the first release published to PyPI. + ### Added - `pinside project` writes a KiCad project for the fixture board: every probe at its DUT test - point's own coordinates, the DUT's outline and mounting holes, and a generated pogo receptacle - footprint. Routing is deliberately not generated. + point's own coordinates, the DUT's outline and mounting holes, a generated pogo receptacle + footprint, and a GND pour on both copper layers. Routing is deliberately not generated. - Carrier boards (`target.board`), defaulting to the **Raspberry Pi Pico 2**. A module exposes only some of its chip's pins, and configs are now checked against the board rather than the chip (`PF024`). Naming a board determines the chip. - A spring-pin catalogue (`fixture.probe`), defaulting to the Mill-Max 0985 receptacle. The probe - sets the minimum spacing, so a finer pin relaxes `PS021` without a second edit. -- `fixture.mirror`, defaulting to `x` — a bed-of-nails takes the DUT face-down. + sets the minimum spacing, so a finer pin relaxes `PS021` without a second edit. Each entry + records where its dimensions came from and whether anyone has checked them against the + supplier's drawing; the generated fixture README says which. +- `fixture.mirror`, defaulting to `x`, because a bed-of-nails takes the DUT face-down. +- **`pinside.client` and `pinside probe`**, an optional extra (`pip install 'pinside[client]'`). + A JSON-RPC client over USB CDC that refuses a fixture whose config hash disagrees with the + config it was handed, and keeps pushed notifications out of the response stream. `pinside + probe` is the bench smoke test: it names every channel and fails on a rail out of range. +- **Baselines.** `--write-baseline` records the findings a board has already been judged on; + `--baseline` accepts them. Suppression is by code *and* reference, so a new occurrence of an + accepted code still fails. Available on `check`, `generate` and `project`. +- `--json` on `generate` and `project`, so their findings can be parsed rather than scraped off + stderr. It reports on the refusal path too. +- New board checks: cutouts and second outlines (`PS003`, `PS004`), probes and mounting holes + placed over a cutout (`PS013`, `PS014`), a receptacle body fouling a neighbouring component + (`PS027`), supply rails or reset lines the fixture cannot reach (`PS033`, `PS034`), and net + numbering that KiCad will not preserve (`PS043`, `PS044`). +- The `rp2354a` target, and `targets.SAME_PINOUT` recording that the stacked-flash parts share + their plain counterparts' pin map. +- The generated fixture README turns the plate force into hardware: whether a thumbscrew or a + pneumatic clamp is needed, what each standoff carries, and how long they have to be. +- `scripts/test.sh --coverage`. +- CI now runs the KiCad tests inside the KiCad container and cross-compiles the generated + firmware against a pinned Pico SDK. Both paths were previously verified only on a developer's + machine. ### Fixed +- **`pinside init` did not work at all.** The positional `board` and the carrier option + `--board` shared an argparse destination, so every invocation looked the `.kicad_pcb` path up + in the module catalogue and refused. The carrier option is now `--carrier`. +- **`pinside project --board pico2w` produced a schematic KiCad could not open.** KiCad writes + the Pico W as a symbol deriving from the Pico, carrying properties and no pins; copied into a + schematic's `lib_symbols` that `extends` resolves to nothing. Derived symbols are now + flattened against their parent, units included. +- **A board with a slot or a cutout was reported as having an unclosed outline** (`PS002`, an + error). Edge.Cuts holds every edge a board has; the largest ring is now the perimeter and the + rest are cutouts. +- **`examples/demo-board.kicad_pcb` had a netlist KiCad could not keep.** All sixteen test points + were written with net ordinal `1` under sixteen different names. Through KiCad 9 a net is + identified by its ordinal, so KiCad read them as one net and the first save dropped fifteen of + them to no-net. `pinside check` called the board clean, because it read the name off each pad + and never compared the ordinals: that is what `PS043` now catches, and the board is renumbered. + Confirmed in both directions against `kicad-cli pcb export ipcd356`. +- **The generated firmware did not link against the real Pico SDK.** `main.c` calls + `set_sys_clock_khz`, a `static inline` in `hardware/clocks.h`, and included only + `pico/stdlib.h`, which does not pull that in. Calling an undeclared function is a warning + under C11, not an error, so every translation unit compiled and only the linker objected. The + host tests could not have caught it: they build `fixture_core.c` against the mock HAL and + never compile `main.c`. Found by the new cross-compile job on its first run. The generated + project now includes the header, links `hardware_clocks`, and builds with + `-Werror=implicit-function-declaration`, so the next missing header is a compile error naming + the function rather than a bare symbol at link time. - `resolve_board` now applies the fixture transform. It did not, so `fx`/`fy` were zero everywhere: not obviously wrong, just every probe at the origin. +- Findings printed by the CLI now go to the current `sys.stderr` rather than whichever stream it + was at import time. + +### Changed + +- `PS002` now fires only on segments that genuinely close into nothing, and reports how many. +- `check_placement` distinguishes "never placed" from "placed over a hole", so a probe in a + cutout no longer reports both. +- The probe body diameter is a limit of its own (`--probe-body`), taken from the chosen probe. ### Note Generating a project needs KiCad installed, because a schematic embeds a copy of every symbol it -places. `check` and `generate` still need nothing. +places. `check` and `generate` still need nothing, and `probe` is the only command with a +dependency at all. ## [0.1.0] @@ -65,5 +127,6 @@ First release. - RP2350A, RP2350B and RP2354B. -[Unreleased]: https://github.com/bitcrushtesting/pinside/compare/v0.1.0...HEAD +[Unreleased]: https://github.com/bitcrushtesting/pinside/compare/v0.2.0...HEAD +[0.2.0]: https://github.com/bitcrushtesting/pinside/compare/v0.1.0...v0.2.0 [0.1.0]: https://github.com/bitcrushtesting/pinside/releases/tag/v0.1.0 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2b32b12..4932887 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,13 +47,29 @@ identifier is derived from the config, so the result is internally consistent an an unchanged config is byte-identical. **Symbol definitions are copied verbatim, never re-serialised.** KiCad's format distinguishes a -bare token from a quoted string — `(type default)` and `(shape line)` are not strings — and a +bare token from a quoted string (`(type default)` and `(shape line)` are not strings) and a parse throws that away. Round-tripping a symbol produces a file KiCad silently refuses to open. **A finding must be actionable.** Each one says what is wrong, which references -it applies to, and what to do — "GPIO9 cannot be i2c0 sda" is only useful -because it goes on to name the pins that can. New codes are appended, never -renumbered: people put them in `--ignore` lists. +it applies to, and what to do. "GPIO9 cannot be i2c0 sda" is only useful +because it goes on to name the pins that can. + +**A finding code is a public interface.** People put them in `--ignore` lists, +in baseline files, and in CI configuration that nobody revisits. So: + +- **Codes are appended, never renumbered.** Pick the next free number in the + right family (`PS0[0-5]x` by subject, `PF0xx` by stage). Gaps are fine; + reordering is not. +- **A withdrawn code is retired, not reused.** Delete the check, leave the + number burnt, and say so in `CHANGELOG.md`. Reusing it silently repoints + every existing `--ignore PS0xx` at a different question, and the people + affected are the ones who will never read the release notes. +- **Widening a code needs a new one.** If a check starts reporting a case it + did not before, that is a new finding: somebody accepted the old meaning. + Narrowing it, or improving its wording or its detail, is not. +- **Every code must appear in the README's tables.** `tests/test_docs.py` + enforces this in both directions, so an undocumented code fails CI and so + does a documented code nothing emits. **The generated firmware is tested, not just generated.** `fixture_core.c` has no SDK headers precisely so it can be built against `mock_hal.c` on a host. A change @@ -81,9 +97,50 @@ refuses to write any file with a placeholder left in it. ## Adding a target `targets.py` holds pin capability, not a datasheet. Add a `Target`, and check -the function map against the vendor's own table — `tests/test_firmware.py` has +the function map against the vendor's own table. `tests/test_firmware.py` has spot checks for the RP2350 that exist because getting this wrong produces a -config that validates and does not work. +config that validates and does not work. `tests/test_kicad.py` checks the GPIO +count and the ADC map a second time against KiCad's own symbol library, which +is an independent transcription of the same pinout; give a new target an entry +in `TestTargetsAgainstKiCad.SYMBOLS` so it gets that check too. + +A part in the same family as one already here costs almost nothing: the RP2354s +are the RP2350s with flash in the package, so they share a pin map, and +`targets.SAME_PINOUT` records that rather than keeping two copies to drift. + +### A target from another vendor + +Every target so far is an RP2350, so one HAL covers them all. A part from +another vendor needs a second implementation beside `fixture_hal_rp2350.c`, and +that is the work worth scoping before starting. + +`fixture_hal.h` is the whole contract: **17 functions in 56 lines**, and +`fixture_hal_rp2350.c` implements them in 114. Nothing above it knows what chip +it is on. Grouped by what they need from the silicon: + +| Group | Functions | What it takes | +|---|---|---| +| Console | `fx_hal_write`, `fx_hal_millis` | A byte sink and a millisecond counter. USB CDC on the RP2350; a UART is fine. | +| GPIO | `configure`, `get`, `set`, `release` | Direction, pull, and a real high-Z for open-drain. `release` must actually float the pin, not drive it high. | +| ADC | `read`, `full_scale`, `reference_mv` | Raw counts, plus the two numbers that turn them into volts. Parts differ in both; do not hardcode 12 bits. | +| UART | `configure`, `write`, `read` | Non-blocking `read`: it is polled from `fx_core_poll` for streaming, and blocking there stalls every other channel. | +| I2C | `write`, `read` | Return a negative value on NAK. The scan depends on telling a NAK from a bus error. | +| SPI | `configure`, `transfer` | Mode and speed, and a full-duplex transfer. | + +Two of these are where an ill-fitting port shows up: + +- **`fx_hal_gpio_release` must be high-Z.** A HAL that drives high instead makes + every `open_drain` channel a short across whatever the DUT pulls up. Nothing + in the core can detect that; it just contends. +- **`fx_hal_uart_read` must not block.** `fx_core_poll` calls it for every + streaming bus on every pass. + +`mock_hal.c` is a complete implementation in 121 lines and is the thing to read +first: it is what the host tests run against, so it defines the behaviour a real +HAL has to match. A new HAL should compile against the same `test_core.c`. + +The generator also needs the new HAL wired into the emitted `CMakeLists.txt`, +which currently assumes the Pico SDK. ## Before opening a pull request @@ -91,13 +148,13 @@ config that validates and does not work. scripts/lint.sh && scripts/test.sh ``` -The KiCad-facing tests skip themselves without KiCad installed, so **CI does not cover them** — +The KiCad-facing tests skip themselves without KiCad installed, so **CI does not cover them**: run them locally before changing anything under `kicad/`. They were verified against KiCad 10.0.3. Both run in CI along with an end-to-end job that installs the wheel and generates firmware from `examples/`. If you changed anything under `firmware/templates/`, say in the pull request whether the generated project -still builds against the Pico SDK — CI compiles the host tests, but it has no +still builds against the Pico SDK; CI compiles the host tests, but it has no SDK and cannot build the firmware itself. ## Versioning diff --git a/README.md b/README.md index 092e3d2..a79453a 100644 --- a/README.md +++ b/README.md @@ -1,9 +1,16 @@ # Pinside +[![CI](https://github.com/bitcrushtesting/pinside/actions/workflows/ci.yml/badge.svg)](https://github.com/bitcrushtesting/pinside/actions/workflows/ci.yml) +[![PyPI](https://img.shields.io/pypi/v/pinside)](https://pypi.org/project/pinside/) +[![Python 3.10+](https://img.shields.io/badge/python-3.10%20%7C%203.11%20%7C%203.12%20%7C%203.13-blue)](https://www.python.org/downloads/) +[![License: MIT](https://img.shields.io/badge/license-MIT-green)](LICENSE) +[![Dependencies: none](https://img.shields.io/badge/dependencies-none-brightgreen)](pyproject.toml) +[![Ruff](https://img.shields.io/endpoint?url=https://raw.githubusercontent.com/astral-sh/ruff/main/assets/badge/v2.json)](https://github.com/astral-sh/ruff) + Read a KiCad board and answer three questions: **can a bed-of-nails fixture be built against it?**, **what does that fixture board look like?**, and **what firmware does it run?** -A pogo-pin fixture is three lists taken from the device under test — where the test pads are, +A pogo-pin fixture is three lists taken from the device under test: where the test pads are, where it can be bolted down, and how big it is. All three are already in the `.kicad_pcb`, so copying them by hand is how a fixture ends up 0.3 mm out with nobody able to say why. Pinside reads them instead, and then checks the things that only turn up after the fixture comes back @@ -14,10 +21,16 @@ The board file is **only ever read**. Pinside never writes KiCad sources. ## Install ```bash -pip install . +pip install pinside ``` -No dependencies; Python 3.10+. It also runs straight from a checkout: +No dependencies; Python 3.10+. Add `pinside[client]` for the one command that needs pyserial: + +```bash +pip install 'pinside[client]' # ... if you want `pinside probe` as well +``` + +It also runs straight from a checkout, with nothing installed: ```bash PYTHONPATH=src python3 -m pinside check board.kicad_pcb @@ -45,13 +58,19 @@ pinside check board.kicad_pcb -f svg > plan.svg # 1:1 drill plan; print and pinside check board.kicad_pcb --mirror x # the frame for a face-down DUT pinside init board.kicad_pcb -o fixture.json # draft a config covering every test point +pinside init board.kicad_pcb --carrier bare # ... for a fixture carrying the chip itself pinside project fixture.json --out fixture-board/ # a KiCad project for the fixture pinside generate fixture.json --out firmware/ # firmware that matches the board + +pinside probe fixture.json # talk to a flashed fixture and check it ``` `pinside board.kicad_pcb` with no subcommand still means `check`. -Exit status is `0` clean, `1` warnings under `--strict`, `2` errors, `3` bad usage — so it drops +`probe` is the only command that needs anything installed beyond the standard library: +`pip install 'pinside[client]'` for pyserial. + +Exit status is `0` clean, `1` warnings under `--strict`, `2` errors, `3` bad usage, so it drops into CI as a gate on the board, not just as a report. ### Coordinates @@ -62,8 +81,8 @@ want when the fixture is drawn as its own board. `--mirror x` additionally flips transform for a DUT laid face-down onto upward-pointing probes. Getting it wrong yields a perfect mirror image of the fixture you need, which is not obvious until the pins miss. -Every row carries both frames — `dut_x/dut_y` as the board stores them, `fix_x/fix_y` after the -transform — so you can always check one against the other. +Every row carries both frames: `dut_x/dut_y` as the board stores them, and `fix_x/fix_y` after +the transform, so you can always check one against the other. ### Tuning the limits @@ -71,33 +90,66 @@ The checks are measured against real hardware, and the defaults describe a Mill- receptacle. Change them when your probes differ: ```bash -pinside board.kicad_pcb --probe-pitch 1.9 --edge-clearance 1.5 --min-pad 0.7 +pinside board.kicad_pcb --probe-pitch 1.9 --probe-body 1.27 --edge-clearance 1.5 --min-pad 0.7 ``` `--ignore PS041,PS042` silences findings you have already decided about. +### Baselines + +`--ignore` is per-invocation and global: it silences a code on every board, forever. That is the +wrong shape for the usual situation, which is a board with two findings somebody has looked at +and accepted and one that has not happened yet. + +```bash +pinside check board.kicad_pcb --write-baseline pinside-baseline.json +pinside check board.kicad_pcb --baseline pinside-baseline.json # exit 0 for the accepted ones +``` + +A baseline records each accepted finding by code **and by reference**. Accepting `PS041 on TP5` +says nothing about `PS041 on TP9`, so a new occurrence still fails while the old one stays quiet. +That is what makes the file safe to commit: it cannot absorb a finding nobody has seen. Every +entry is written with an empty `note`; fill them in before committing, because a suppression +whose reason nobody wrote down is indistinguishable from a mistake six months later. + +`generate` and `project` take `--baseline` too, and both accept `--json` to put their findings on +stdout as a machine-readable object rather than prose on stderr: + +```bash +pinside generate fixture.json --out firmware/ --json | jq '.errors' +``` + ## What it checks | Code | Severity | What it means | |---|---|---| | PS001 | error | No `Edge.Cuts` outline: the board size is unknown | -| PS002 | error | The outline does not close into one ring — unfillable, unmillable | +| PS002 | error | Edge.Cuts segments that close into nothing: unfillable, unmillable | +| PS003 | info | The outline has internal cutouts; they are holes, not a broken edge | +| PS004 | warning | Closed Edge.Cuts shapes outside the board: a panel, or a leftover | | PS010 | error | Test points sit outside the outline, so they were never placed | | PS011 | error | Mounting holes sit outside the outline | -| PS012 | warning | The probes lie on a uniform lattice — KiCad's import spread, not a layout | +| PS012 | warning | The probes lie on a uniform lattice: KiCad's import spread, not a layout | +| PS013 | error | A test point sits over a cutout, where there is no board | +| PS014 | error | A mounting hole sits over a cutout, so there is nothing to bolt to | | PS020 | error | Two test points share a position | | PS021 | error | Two probes are closer than the receptacle pitch; the bodies collide | | PS022 | warning | A probe is up against the board edge, where the fixture wall lives | | PS023 | warning | A probe crowds a mounting hole, where the standoff lives | -| PS024 | error | A probe lands inside another footprint — it would strike the component | +| PS024 | error | A probe lands inside another footprint, so it would strike the component | | PS025 | warning | A test pad is too small for a spring tip plus placement tolerance | | PS026 | warning | Test points on both sides; one plate cannot reach them all | -| PS030 | error | No ground test point — there is no return path to measure against | +| PS027 | warning | The tip clears a component but the receptacle body does not | +| PS030 | error | No ground test point: there is no return path to measure against | | PS031 | warning | Only one ground probe | | PS032 | info | Plated mounting holes carry no net; grounding them is a free return path | -| PS040 | warning | A test point has no net — it probes nothing | +| PS033 | warning | A supply rail on the board has no test point, so nothing proves it came up | +| PS034 | info | A reset or strap line has no test point: readable, but not resettable | +| PS040 | warning | A test point has no net, so it probes nothing | | PS041 | info | A test point is on a KiCad auto-named net; label it in the schematic | -| PS042 | info | Two probes on one signal net — a wasted fixture channel | +| PS042 | info | Two probes on one signal net: a wasted fixture channel | +| PS043 | error | One net number with several names: KiCad will merge them and drop the rest | +| PS044 | error | A named net on number 0, KiCad's no-connection net, so it probes nothing | | PS050 | warning | Fewer mounting holes than are needed to locate the board | | PS051 | info | Mounting holes have differing drill sizes | | PS052 | info | Probes lie outside the mounting-hole span, where the plate cantilevers | @@ -105,6 +157,13 @@ pinside board.kicad_pcb --probe-pitch 1.9 --edge-clearance 1.5 --min-pad 0.7 PS010 and PS012 are the two that matter most in practice: both mean *the layout is not finished*, and any fixture cut from those coordinates is scrap. +`PS043` and `PS044` are about net *numbering* rather than geometry. Through KiCad 9 a net is +identified by its ordinal and the name beside it is only a label, so two names on one number are +one net as far as KiCad is concerned: it keeps the first and drops the rest to no-net on the next +save. The file stays well-formed and nothing warns anyone. pinside's own example board had +exactly this, and pinside called it clean for two releases. Both checks are verified against +`kicad-cli pcb export ipcd356`, which is KiCad's own answer to what a board's netlist is. + ## The fixture board `pinside project` writes a KiCad project for the fixture itself. The part it generates is the @@ -114,15 +173,21 @@ part that has to be exact: - **The outline and mounting holes**, taken from the DUT, so the two boards bolt together. - **A pogo receptacle footprint**, generated to the chosen probe's dimensions, with the project's `fp-lib-table` already pointing at it. +- **A GND pour** on both copper layers, held back from the board edge. On a fixture that pour is + most of the return path, and unlike a trace it is one fixed shape rather than a guess. Routing is not generated. A ratsnest and an accurate drill plan are the useful part; guessing trace paths is not, and an autorouter or a person does it better. +The generated `README.md` turns the plate force into hardware: how many newtons the probes add +up to, whether that needs a clamp or a thumbscrew, what each standoff carries, and how long the +standoffs have to be to leave the probes their travel. + ```bash pinside project fixture.json --out fixture-board/ ``` -It refuses to lay out a fixture against a DUT that has not been laid out itself — unplaced test +It refuses to lay out a fixture against a DUT that has not been laid out itself. Unplaced test points would put every hole in the wrong place, and nothing about the output would look wrong until the boards came back: @@ -134,8 +199,8 @@ pinside: error: PS010 30 of 30 test points sit outside the board outline ... ### The default board The fixture is built around a **Raspberry Pi Pico 2** unless told otherwise. Soldering a module -onto a carrier costs one part and no support circuitry — no crystal, no flash, no USB connector, -no regulator — and it unplugs when a probe shorts something. +onto a carrier costs one part and no support circuitry (no crystal, no flash, no USB connector, +no regulator), and it unplugs when a probe shorts something. A module exposes only some of its chip's pins, and pinside checks against the board rather than the chip: @@ -152,13 +217,34 @@ error: PF024 1 pins are not brought out on the pico2 [GPIO25 (led)] -- GPIO23/24 | `pico2w` | RP2350A + wireless | 26 | | `bare` | whatever `target.mcu` says | all of them | -Naming a board is enough — the chip follows from it. A fixture needing more than 26 channels has +Naming a board is enough; the chip follows from it. A fixture needing more than 26 channels has to carry the chip itself, which is what `bare` is for. +### The targets + +`target.mcu` is what `bare` needs, and what a carrier board resolves to. `targets.py` holds pin +capability rather than a datasheet: how many GPIO the part has, which of them reach an ADC, and +which peripheral function each one can carry. + +| `target.mcu` | Package | GPIO | ADC inputs | +|---|---|---|---| +| `rp2350b` (the `init` default) | QFN-80 | 48 | 8, on GPIO40-47 | +| `rp2350a` | QFN-60 | 30 | 4, on GPIO26-29 | +| `rp2354b` | QFN-80, 2 MB stacked flash | 48 | 8, on GPIO40-47 | +| `rp2354a` | QFN-60, 2 MB stacked flash | 30 | 4, on GPIO26-29 | + +The stacked-flash parts are the plain ones with flash in the package, so they share a pin map; +KiCad's own library says the same thing, and the tests check pinside's map against it. + +All four are the RP2350 family, so one HAL covers them. Adding a part from another vendor means +a second HAL beside `fixture_hal_rp2350.c`. That contract is 17 functions in 56 lines and +[CONTRIBUTING.md](CONTRIBUTING.md#a-target-from-another-vendor) scopes what implementing it +involves. + ### The default probe `millmax_0985`: a Mill-Max 0985 receptacle with an 0900 spring pin, on a 2.54 mm pitch. The -receptacle is what makes a fixture maintainable — a worn pin pulls out and a new one goes in. +receptacle is what makes a fixture maintainable: a worn pin pulls out and a new one goes in. | Probe | Hole | Pad | Minimum pitch | |---|---|---|---| @@ -169,8 +255,10 @@ receptacle is what makes a fixture maintainable — a worn pin pulls out and a n The probe sets the spacing limit, so choosing a finer one relaxes the `PS021` check without a second edit. -Dimensions are what pinside builds to — check them against your supplier's drawing before -ordering. +Dimensions are what pinside builds to, so check them against your supplier's drawing before +ordering. Each entry records where its numbers came from and whether anyone has done that check; +none has yet, and the generated fixture README says so where somebody about to order a board +will see it. ### Mirroring @@ -182,12 +270,29 @@ Before ordering, print `pinside check -f svg` at 1:1 and lay the real boar Open the project and run **Update PCB from Schematic**. The resistors and the controller arrive from KiCad's own libraries with correct pads and nets, and the probes stay exactly where pinside -put them — KiCad matches footprints by UUID, and pinside derives those from the config rather +put them: KiCad matches footprints by UUID, and pinside derives those from the config rather than randomising them, so regenerating an unchanged config produces a byte-identical project. `pinside project` needs KiCad installed, because a schematic embeds a copy of every symbol it places and the only honest source for those is a KiCad library. `check` and `generate` do not. +### KiCad versions + +| | Format | +|---|---| +| Board files `check` and `generate` read | tested against `20241229` (KiCad 8/9) and the KiCad 10 net record | +| Files `project` writes | `.kicad_pcb` `20260206`, `.kicad_sch` `20260306` (KiCad 10) | +| `project` output opened and checked in | KiCad 10.0.3 | + +The reader is deliberately tolerant: it walks the S-expressions looking for the tags it cares +about and ignores everything else, so a file from a version it has never seen still reads, and a +newer version that adds tags does not break it. The one format change it handles explicitly is +the net record, which was `(net "NAME")` through KiCad 9 and is `(net "NAME")` in 10. +Both forms are covered by tests. + +The writer emits one version and does not negotiate. Opening a generated project in an older +KiCad will not work. + ## Firmware A fixture is only half a tool: something has to drive those probes and hand what it sees to @@ -237,7 +342,7 @@ Three things in there are worth knowing about. fixture's **rx**. Wire it to `tx` because both are called TXD and nothing works, with no error to explain why. `pinside init` gets this right when it drafts a config. -**Guards.** A bus the DUT also masters — an Ethernet SPI, a shared panel bus — is declared with +**Guards.** A bus the DUT also masters (an Ethernet SPI, a shared panel bus) is declared with `"guard": "esp_en"`. The firmware then refuses to drive it until that channel is asserted: ``` @@ -245,7 +350,7 @@ explain why. `pinside init` gets this right when it drafts a config. <- {"jsonrpc":"2.0","id":3,"error":{"code":-32003,"message":"guard not asserted","data":"esp_en"}} ``` -It is an interlock, not a substitute for series resistors — but it stops the ordinary mistake, +It is an interlock, not a substitute for series resistors, but it stops the ordinary mistake, which is probing a bus while the DUT's own controller is mid-transaction. **Open drain means released, not driven high.** An `open_drain` channel only ever pulls towards @@ -297,33 +402,85 @@ Methods: `fixture.info`, `fixture.channels`, `gpio.read`, `gpio.write`, `gpio.sn `adc.read`, `adc.snapshot`, `uart.write`, `uart.read`, `uart.configure`, `i2c.scan`, `i2c.write`, `i2c.read`, `spi.transfer`. Full schemas in the generated `openrpc.json`. +That contract is checked against the firmware rather than merely emitted: the test suite reads +the dispatch table out of `fixture_core.c` and fails if the two disagree in either direction, so +a method the document promises and the firmware does not answer cannot ship. + ### The config hash `fixture.info` reports a digest of the channel map. Compare it against the config before trusting -a run — a test rig one revision behind is the most common way a bench lies to you. Rewording a +a run: a test rig one revision behind is the most common way a bench lies to you. Rewording a description does not change it; moving a pin does. +You do not have to compare it by eye. `pinside.client` does it on connect, and refuses. + +## Talking to a fixture from the host + +```bash +pip install 'pinside[client]' # pyserial; the core stays dependency-free +pinside probe fixture.json # is this the right fixture, and is it wired up +pinside probe fixture.json --port /dev/ttyACM0 --json +``` + +`pinside probe` opens the port, checks the firmware's config hash against the config you handed +it, prints every channel with the DUT signal it lands on, and exits non-zero if any monitored +rail is out of range. It is the answer to "is the thing on my bench the thing I think it is". + +The same client is a library: + +```python +from pinside.client import connect +from pinside.config import load + +config = load("fixture.json") +with connect("/dev/ttyACM0", config) as fixture: # raises if the hash disagrees + fixture.gpio_write("dut_reset", True) + print(fixture.adc_snapshot()) + for message in fixture.poll(seconds=2): + print(message.channel, message.data) +``` + +Two things it does that a hand-rolled client usually does not: + +**It refuses a fixture that does not match.** `connect()` compares `fixture.info`'s hash against +the config before returning, and raises `ConfigMismatchError` rather than warning. Pass +`check_hash=False` if you mean to, and say why. + +**It keeps notifications out of the response stream.** A streaming UART pushes `uart.data` +between replies. Reading one line per request means eventually returning a log line as the answer +to an ADC read; every line is classified before anything waits on it, and pushed messages go to +`fixture.poll()`. + ### Config findings | Code | What it means | |---|---| | PF001 | Unknown target microcontroller | | PF002 | The DUT board was not read, so probe names went unchecked | +| PF003 | The output directory is not empty and pinside did not write it | +| PF004 | A template placeholder survived into a generated file | +| PF005, PF006, PF007 | An unknown board or probe, or a chip the board does not carry | +| PF008 | An unrecognised `fixture.mirror` | | PF010, PF011 | A channel name C cannot use, or two channels with one name | | PF020 | A pin the target does not have | | PF021 | A pin that cannot carry the role asked of it, with the pins that can | | PF022 | An ADC channel on a pin with no converter | | PF023 | One pin claimed by two channels | -| PF030-PF038 | An unrecognised role, guard, parity, direction, pull, or SPI mode | -| PF005-PF007 | An unknown board or probe, or a chip the board does not carry | -| PF008 | An unrecognised `fixture.mirror` | | PF024 | A pin the carrier board does not bring out | | PF025 | The board has almost no GPIO left | +| PF030-PF038 | An unrecognised role, guard, parity, direction, pull, or SPI mode | | PF040 | A probe naming a signal the board does not have | | PF041 | A test point with no channel | | PF042 | A divider that would present more than the ADC reference | -`pinside project` additionally reports the board findings (`PS...`) and refuses on any error. +`pinside project` additionally reports the board findings (`PS...`) and refuses on any error. It +has three of its own, for the things that stop a project being written at all: + +| Code | What it means | +|---|---| +| PK001 | No DUT board, so there are no coordinates to lay a fixture out from | +| PK002 | The output directory is not empty and pinside did not write it | +| PK003 | KiCad's symbol libraries were not found, so a schematic cannot be built | ## As a library @@ -352,7 +509,7 @@ There is no setup step: `scripts/lint.sh` installs the ruff version pinned in `p into `.venv-tools/`, so CI and your machine run the same one. The test suite builds its own synthetic boards, so it depends on no real project and no KiCad -install. One test goes further and compiles the generated firmware, then runs *its* tests — the +install. One test goes further and compiles the generated firmware, then runs *its* tests, the only check that the C templates and the generated tables actually agree. It skips if there is no compiler. Another generates a KiCad project and runs KiCad's own ERC and DRC over it; it skips without KiCad, so **that path is verified locally rather than in CI**. @@ -369,17 +526,19 @@ src/pinside/ scaffold.py drafting a config from a board modules.py carrier boards, and which pins they bring out pogo.py spring-pin probes and the holes they need - cli.py check | init | generate | project + client.py talking to a flashed fixture over USB CDC + cli.py check | init | generate | project | probe kicad/ the KiCad project emitter firmware/ the emitter, and the C templates it emits ``` -[CONTRIBUTING.md](CONTRIBUTING.md) has the rules that are not obvious from the code, and -[CHANGELOG.md](CHANGELOG.md) records what changed. +[CONTRIBUTING.md](CONTRIBUTING.md) has the rules that are not obvious from the code, +[CHANGELOG.md](CHANGELOG.md) records what changed, and [TASKLIST.md](TASKLIST.md) is what is +planned next. ## Why not the KiCad Python API It only exists inside a KiCad installation, which rules out CI and any machine that just wants to look at a board file. KiCad's S-expression format is stable enough to read directly, so pinside -parses it and stays a dependency-free script. Writing is a different matter — text edits break -the UUID cross-references KiCad relies on — which is why pinside only ever reads. +parses it and stays a dependency-free script. Writing is a different matter: text edits break +the UUID cross-references KiCad relies on, which is why pinside only ever reads. diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..4ec48ac --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,54 @@ +# Security + +## What pinside does + +pinside is a command-line tool and a Python library. It reads local files, and +it writes files where you tell it to. It has: + +- **No runtime dependencies.** `pyproject.toml` lists none, and CI installs the + package into a bare environment and runs the CLI from it. +- **No network access.** Nothing in `src/` opens a socket. It does not fetch + footprints, check for updates, or report usage. +- **No execution of what it reads.** A `.kicad_pcb` is parsed as S-expressions + by `sexpr.py` into lists and strings. There is no `eval`, no pickle, and no + code path that runs anything out of a board file or a config. + +## What it writes + +`pinside generate` and `pinside project` create directories and files, and +`--force` lets them write into a non-empty directory. Both refuse by default to +write into a directory pinside did not produce (`PF003`). Neither ever writes +to the KiCad sources it reads. + +The firmware pinside generates is C for a microcontroller. It is compiled by +you, from a source tree you can read: only `src/fixture_config.c` and a block of +test assertions are generated, and the rest is copied verbatim from +`src/pinside/firmware/templates/`. + +## Reporting a vulnerability + +Report anything security-relevant privately, not as a public issue: + +Open a [private security advisory][advisory] on this repository. That reaches +the maintainers without the report being public, and it is the only channel +worth using: an issue is visible the moment it is filed. + +Please include the input that triggers it. A board file or a config is usually +the whole reproduction. If you cannot share the file, the shape of it (which +primitive, how deeply nested, how large) is usually enough. + +Expect an acknowledgement within a week. Because pinside is a local, offline +tool, most findings will be handled as ordinary bugs in the open, with credit. +Anything that is not gets a fix and an advisory before it is described. + +## Scope + +In scope: anything that makes pinside write outside the directory it was +pointed at, crash in a way that could be exploited by a crafted board file, or +produce firmware that does not match its own config hash. + +Out of scope: the safety of a fixture built from pinside's output. That is what +the checks are for, and they are advisory. **Nothing pinside prints substitutes +for checking the drill plan against the real board before you order it.** + +[advisory]: https://github.com/bitcrushtesting/pinside/security/advisories/new diff --git a/TASKLIST.md b/TASKLIST.md new file mode 100644 index 0000000..3256b2a --- /dev/null +++ b/TASKLIST.md @@ -0,0 +1,125 @@ +# Tasklist + +What is planned next for pinside, and what has just been done. Each open item +says why it matters and what "done" looks like, so it can be picked up without +re-deriving the reasoning. + +Codes in brackets are the finding namespaces: `PS...` for board checks +(`checks.py`), `PF...` for config checks (`config.py`). + +## Done in 0.2.0 + +The previous version of this file listed seven sections of work. Most of it +landed; the entries below record what came of it, because two items turned into +bug fixes rather than features and that is worth saying out loud. + +- **Cut 0.2.0 and publish to PyPI.** `release.yml` now builds, publishes through + a PyPI trusted publisher, and only then creates the GitHub release. **One + manual step remains**, and the first tag will fail without it: configure the + trusted publisher at `pypi.org/manage/project/pinside/settings/publishing` + with owner `bitcrushtesting`, repository `pinside`, workflow `release.yml`, + environment `pypi`, and create a `pypi` environment in the repository + settings. +- **A host client and `pinside probe`.** `pinside.client` speaks the generated + protocol, refuses a fixture whose config hash disagrees, and keeps pushed + notifications out of the response stream. Optional extra: `pinside[client]`. +- **Real contract validation.** `tests/test_contract.py` checks `openrpc.json` + as OpenRPC and against `fixture_core.c`'s own dispatch table, in both + directions. +- **The CI-invisible paths.** The KiCad tests now run in the `kicad/kicad:10.0` + container with `PINSIDE_REQUIRE_KICAD=1`, which turns a skip into a failure; + the generated firmware is cross-compiled against a pinned Pico SDK. +- **Coverage**, at 87% via `scripts/test.sh --coverage`. +- **Baselines and `--json`**, on `check`, `generate` and `project`. +- **New checks**: `PS003`, `PS004`, `PS013`, `PS014`, `PS027`, `PS033`, `PS034`. +- **A ground pour and standoff guidance** on the generated fixture board. +- **Probe provenance**, `rp2354a`, and the HAL boundary scoped in + CONTRIBUTING.md. +- **The finding-code policy**, written down in CONTRIBUTING.md. + +Three bugs surfaced while doing the above, all of which shipped in 0.1.0: + +- **`pinside init` did not work at all.** The positional `board` and the carrier + option `--board` shared an argparse destination. The option is now + `--carrier`, and `main()` derives its command list from the parser rather than + a hand-kept set, which is what let a new subcommand be swallowed too. +- **`pinside project --board pico2w` emitted a schematic KiCad would not open.** + Derived symbols are now flattened against their parent. +- **A board with a cutout was reported as having an unclosed outline.** + Edge.Cuts holds every edge; the largest ring is the perimeter now. + +## 1. What the new checks still cannot see + +`PS033` and `PS034` read net names off component pads, which is the only +evidence a `.kicad_pcb` carries. That has limits worth closing. + +- [ ] **Read net classes, not just net names.** A board that puts its rails in a + `Power` net class states the fact that `PS033` currently infers from a + leading `+`. The class is in the `.kicad_pcb`; nothing reads it yet. +- [ ] **Courtyards, not pad bounding boxes.** `PS024` and `PS027` measure + against the envelope of a footprint's pads, so a tall part with small pads + (an electrolytic, a shielded module) reads as smaller than it is. The + `F.CrtYd` polygon is the real answer and `geometry.py` can already flatten + one. +- [ ] **Component height.** The fixture-side collision `PS027` checks is planar. + A probe's clearance actually depends on how far the part stands off the + board, which is in the 3D model reference and nowhere else useful. + Probably needs a per-footprint height override in the config. + +## 2. Breadth of hardware + +- [ ] **A second microcontroller family.** Everything is an RP2350, so one HAL + covers it. `CONTRIBUTING.md` now scopes what a second one costs: 17 + functions, of which `fx_hal_gpio_release` (must be genuine high-Z) and + `fx_hal_uart_read` (must not block) are the two that go quietly wrong. + An ESP32-S3 or an STM32 would say whether `Target` is the right shape. + Also needs the emitted `CMakeLists.txt` to stop assuming the Pico SDK. +- [ ] **A third carrier board.** Deliberately not done: the only RP2350-family + module symbols KiCad ships are the Picos, and a header map with nothing to + check it against is the "validates and does not work" failure + `CONTRIBUTING.md` warns about. `TestModules` now checks *every* module + against KiCad's symbol, so adding one is safe as soon as there is a symbol + to add it against. +- [ ] **Verify the probe catalogue against supplier drawings.** Each entry now + records its `source` and an empty `verified`, and the generated fixture + README says in as many words that nobody has checked. Someone with the + Mill-Max drawings should fill those in. This is the one open item that + needs paper, not code. + +## 3. The fixture board + +- [ ] **Thermal relief on the ground pour is guessed.** 0.5 mm gap and bridge + are reasonable defaults, not measured ones. They should follow the probe: + a receptacle pressed into a full pour is what makes a worn pin + unreplaceable, and that is the whole argument for receptacles. +- [ ] **Place the controller somewhere sensible.** It is dropped off the board + edge for a person to move. Putting it inside the outline, clear of the + probe field, is a small placement problem with an obvious answer. +- [ ] **Panelisation or a frame outline** for fixtures mounted into an + off-the-shelf enclosure. Still only worth doing once someone names the + enclosure. + +## 4. The host side + +- [ ] **`pinside probe --watch`.** Streaming notifications to stdout as they + arrive is most of a bench log, and `Fixture.poll` already returns them. +- [ ] **Auto-detect the fixture.** `_sole_port` uses the only port when there is + exactly one and refuses to guess otherwise. Opening each candidate and + asking `fixture.info` would do better, at the cost of poking things that + are not fixtures. +- [ ] **A recorded transcript for the tests.** `FakePort` is a hand-written + stand-in. Capturing one real session against a flashed board and replaying + it would pin the client to the firmware's actual bytes rather than to a + second implementation of what they should be. + +## 5. Repository + +- [ ] **A `py.typed` that means something.** It ships, and nothing type-checks + the package. Adding mypy or pyright to `scripts/lint.sh` would make the + annotations a claim rather than decoration. +- [ ] **`cli.py` is at 70% coverage**, the lowest of anything that matters. The + command bodies are mostly untested; the logic inside them is not trivial + any more now that baselines and `--json` run through them. +- [ ] **`scaffold.py` is at 74%.** `pinside init` drafts the config everything + else is built on, and it went a whole release completely broken. The + grouping heuristics deserve tests of their own. diff --git a/examples/README.md b/examples/README.md index 36e8e59..18ab98c 100644 --- a/examples/README.md +++ b/examples/README.md @@ -15,7 +15,7 @@ pinside check examples/demo-board.kicad_pcb -f svg > plan.svg `demo-fixture.json` is the config `pinside init` drafts from that board, with one edit: the SPI bus is given `"guard": "dut_reset"`, because on a real board that bus belongs to the DUT's own controller and the fixture may only master it while -the DUT is held in reset. That edit is the part a person has to make — the +the DUT is held in reset. That edit is the part a person has to make; the grouping and the pin assignment come out of `init` already correct. ```bash @@ -30,7 +30,7 @@ mirrored coordinates, plus the outline and the four mounting holes. Open it and from Schematic** to bring in the resistors and the Pico 2; the probes stay where they are. The config targets a **Raspberry Pi Pico 2**, which is the default. Its 26 header GPIO are ample -for these 14 channels — the Cuarto 500's 34 need a bare RP2350B, which is what `"board": "bare"` +for these 14 channels. The Cuarto 500's 34 need a bare RP2350B, which is what `"board": "bare"` is for. The generated `README.md` carries the channel map as a table, and `openrpc.json` diff --git a/examples/demo-board.kicad_pcb b/examples/demo-board.kicad_pcb index 8e0f251..2f611fb 100644 --- a/examples/demo-board.kicad_pcb +++ b/examples/demo-board.kicad_pcb @@ -1,4 +1,20 @@ (kicad_pcb (version 20241229) (generator "pinside-tests") + (net 0 "") + (net 1 "/DUT_TXD") + (net 2 "/DUT_RXD") + (net 3 "/DUT_RTS") + (net 4 "/DUT_CTS") + (net 5 "/EXT_I2C_SDA") + (net 6 "/EXT_I2C_SCL") + (net 7 "/EXT_SPI_MISO") + (net 8 "/EXT_SPI_MOSI") + (net 9 "/EXT_SPI_CLK") + (net 10 "/EXT_SPI_CS") + (net 11 "/DUT_RESET") + (net 12 "/DUT_BOOT") + (net 13 "/PWR_FLT") + (net 14 "/+3.3V") + (net 15 "GND") (gr_rect (start 0 0) (end 50 40) (radius 2) (layer "Edge.Cuts")) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") @@ -12,105 +28,105 @@ (at 13 8) (property "Reference" "TP2") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/DUT_RXD")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 2 "/DUT_RXD")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 18 8) (property "Reference" "TP3") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/DUT_RTS")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 3 "/DUT_RTS")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 23 8) (property "Reference" "TP4") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/DUT_CTS")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 4 "/DUT_CTS")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 8 14) (property "Reference" "TP5") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/EXT_I2C_SDA")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 5 "/EXT_I2C_SDA")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 13 14) (property "Reference" "TP6") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/EXT_I2C_SCL")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 6 "/EXT_I2C_SCL")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 18 14) (property "Reference" "TP7") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/EXT_SPI_MISO")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 7 "/EXT_SPI_MISO")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 23 14) (property "Reference" "TP8") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/EXT_SPI_MOSI")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 8 "/EXT_SPI_MOSI")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 28 14) (property "Reference" "TP9") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/EXT_SPI_CLK")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 9 "/EXT_SPI_CLK")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 33 14) (property "Reference" "TP10") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/EXT_SPI_CS")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 10 "/EXT_SPI_CS")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 8 20) (property "Reference" "TP11") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/DUT_RESET")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 11 "/DUT_RESET")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 13 20) (property "Reference" "TP12") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/DUT_BOOT")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 12 "/DUT_BOOT")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 18 20) (property "Reference" "TP13") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/PWR_FLT")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 13 "/PWR_FLT")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 23 20) (property "Reference" "TP14") (property "Value" "TestPoint") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "/+3.3V")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 14 "/+3.3V")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 8 30) (property "Reference" "TP90") (property "Value" "GND") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "GND")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 15 "GND")) ) (footprint "TestPoint:TestPoint_Pad_D1.0mm" (layer "F.Cu") (at 33 30) (property "Reference" "TP91") (property "Value" "GND") - (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 1 "GND")) + (pad "1" smd circle (at 0 0) (size 1.0 1.0) (layers "F.Cu" "F.Mask") (net 15 "GND")) ) (footprint "MountingHole:MountingHole_3.5mm_Pad" (layer "F.Cu") @@ -118,7 +134,7 @@ (property "Reference" "H1") (property "Value" "3.5mm") (pad "1" thru_hole circle (at 0 0) (size 7 7) (drill 3.5) - (layers "*.Cu" "*.Mask") (net 2 "GND")) + (layers "*.Cu" "*.Mask") (net 15 "GND")) ) (footprint "MountingHole:MountingHole_3.5mm_Pad" (layer "F.Cu") @@ -126,7 +142,7 @@ (property "Reference" "H2") (property "Value" "3.5mm") (pad "1" thru_hole circle (at 0 0) (size 7 7) (drill 3.5) - (layers "*.Cu" "*.Mask") (net 2 "GND")) + (layers "*.Cu" "*.Mask") (net 15 "GND")) ) (footprint "MountingHole:MountingHole_3.5mm_Pad" (layer "F.Cu") @@ -134,7 +150,7 @@ (property "Reference" "H3") (property "Value" "3.5mm") (pad "1" thru_hole circle (at 0 0) (size 7 7) (drill 3.5) - (layers "*.Cu" "*.Mask") (net 2 "GND")) + (layers "*.Cu" "*.Mask") (net 15 "GND")) ) (footprint "MountingHole:MountingHole_3.5mm_Pad" (layer "F.Cu") @@ -142,6 +158,6 @@ (property "Reference" "H4") (property "Value" "3.5mm") (pad "1" thru_hole circle (at 0 0) (size 7 7) (drill 3.5) - (layers "*.Cu" "*.Mask") (net 2 "GND")) + (layers "*.Cu" "*.Mask") (net 15 "GND")) ) ) diff --git a/pyproject.toml b/pyproject.toml index b83a568..906e86b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "pinside" -version = "0.1.0" +version = "0.2.0" description = "Read a KiCad board and check whether a bed-of-nails test fixture can be built against it" readme = "README.md" requires-python = ">=3.10" @@ -21,7 +21,12 @@ dependencies = [] [project.optional-dependencies] # Everything the scripts under scripts/ need. They install it themselves into # .venv-tools/ when it is missing, so a fresh checkout needs no setup step. -dev = ["ruff==0.16.5"] +dev = ["ruff==0.16.5", "coverage==7.13.0"] + +# Talking to a fixture over USB CDC. Only pinside.client and `pinside probe` +# need this; reading boards, checking them and generating firmware stay +# dependency-free, which is what lets pinside run anywhere a board file does. +client = ["pyserial>=3.5"] [project.urls] Repository = "https://github.com/bitcrushtesting/pinside" @@ -40,6 +45,22 @@ where = ["src"] # tells a consumer the annotations are real. pinside = ["py.typed", "firmware/templates/*"] +[tool.coverage.run] +# Measure the package as installed from src/, not the tests that drive it. +source = ["pinside"] +branch = true + +[tool.coverage.report] +# The generator's own output is not this package. Templates are C, and the +# firmware emitter's f-strings are covered by compiling what they produce. +show_missing = true +skip_covered = false +exclude_also = [ + "if TYPE_CHECKING:", + "raise NotImplementedError", + "if __name__ == .__main__.:", +] + [tool.ruff] # 100 columns, matching .clang-format, so the Python and the C templates that # sit beside each other wrap at the same place. diff --git a/scripts/lib.sh b/scripts/lib.sh index 27609ff..bd4dd01 100644 --- a/scripts/lib.sh +++ b/scripts/lib.sh @@ -39,6 +39,29 @@ ensure_ruff() { echo "$venv/bin/ruff" } +# coverage.py, for scripts/test.sh --coverage. Pinned in pyproject's dev extra +# alongside ruff and installed into the same .venv-tools/, so a coverage number +# from CI and one from your machine came off the same tool. +coverage_spec() { + sed -n 's/.*"\(coverage==[0-9.]*\)".*/\1/p' "$root/pyproject.toml" | head -1 +} + +ensure_coverage() { + local spec want have + spec="$(coverage_spec)" + want="${spec#coverage==}" + + if [ -x "$venv/bin/coverage" ]; then + have="$("$venv/bin/coverage" --version | head -1 | awk '{print $2}')" + [ "$have" = "$want" ] && { echo "$venv/bin/coverage"; return 0; } + fi + + echo "scripts: installing $spec into .venv-tools/" >&2 + [ -d "$venv" ] || python3 -m venv "$venv" >&2 + "$venv/bin/pip" install --quiet --disable-pip-version-check "$spec" >&2 + echo "$venv/bin/coverage" +} + # clang-format formats the C templates. It is not installed automatically: # it comes from a toolchain (LLVM, Xcode, apt) rather than from pip, and # guessing which one a machine wants is worse than saying what is missing. diff --git a/scripts/test.sh b/scripts/test.sh index 6c6408c..fe27591 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -3,13 +3,44 @@ # # scripts/test.sh # run everything # scripts/test.sh -v # ... verbosely; arguments go to unittest +# scripts/test.sh --coverage # ... under coverage.py, then print the report +# scripts/test.sh --no-kicad # ... as a machine without KiCad sees it # # One test compiles the firmware the generator emits and runs its own suite # against a mock HAL. It is the only check that the C templates and the # generated tables agree, and it skips itself when there is no compiler. +# +# Coverage is a map of what the suite does not reach, not a target to hit. +# The interesting number is per-file: a check in checks.py or config.py with no +# covered branch is a finding nobody has ever seen fire. . "$(dirname "${BASH_SOURCE[0]}")/lib.sh" cd "$root" -PYTHONPATH="$root/src${PYTHONPATH:+:$PYTHONPATH}" \ - exec python3 -m unittest discover -s tests "$@" + +coverage=0 +args=() +for arg in "$@"; do + case "$arg" in + --coverage) coverage=1 ;; + # Anyone likely to change the KiCad emitter has KiCad installed, so the configuration + # CI actually runs -- every KiCad test skipped -- is the one nobody exercises. That is + # how a skip guard goes missing and the whole suite errors on the pull request instead. + --no-kicad) export PINSIDE_NO_KICAD=1 ;; + *) args+=("$arg") ;; + esac +done + +export PYTHONPATH="$root/src${PYTHONPATH:+:$PYTHONPATH}" + +if [ "$coverage" -eq 0 ]; then + exec python3 -m unittest discover -s tests "${args[@]}" +fi + +cov="$(ensure_coverage)" +"$cov" erase +# --branch, not just statements: most of what these checks do is decide, and a +# check whose "no finding" arm is never taken is half tested. +"$cov" run -m unittest discover -s tests "${args[@]}" +echo +"$cov" report diff --git a/src/pinside/__init__.py b/src/pinside/__init__.py index 18dbda9..c5ded53 100644 --- a/src/pinside/__init__.py +++ b/src/pinside/__init__.py @@ -9,7 +9,7 @@ from .board import Board, MountingHole, TestPoint, read_board, transform from .checks import ERROR, INFO, WARNING, Finding, Limits, run -__version__ = "0.1.0" +__version__ = "0.2.0" __all__ = [ "ERROR", "INFO", diff --git a/src/pinside/baseline.py b/src/pinside/baseline.py new file mode 100644 index 0000000..155d958 --- /dev/null +++ b/src/pinside/baseline.py @@ -0,0 +1,129 @@ +"""A checked-in record of the findings a board has already been judged on. + +`--ignore PS041,PS042` is per-invocation and global: it silences a code everywhere, on every +board, forever. That is the wrong shape for the usual situation, which is a board with two +findings somebody has looked at and accepted and one that has not happened yet. + +A baseline records the accepted ones by code *and by reference*. Suppressing "PS041 on TP5" says +nothing about PS041 on TP9, so a new occurrence still fails CI while the old one stays quiet. +That is the property that makes a baseline safe to check in: it cannot silently absorb a finding +nobody has seen. + +The file is JSON, meant to be committed and reviewed in a diff. Each entry carries an empty +`note` for the reason, because a suppression whose reason nobody wrote down is indistinguishable +from a mistake six months later. +""" + +from __future__ import annotations + +import json +from dataclasses import dataclass, field +from pathlib import Path + +from .checks import Finding + +VERSION = 1 + + +class BaselineError(Exception): + """A baseline file that cannot be used, as opposed to one that suppresses nothing.""" + + +@dataclass +class Entry: + code: str + refs: set[str] = field(default_factory=set) + note: str = "" + + def matches(self, finding: Finding) -> bool: + """Does this entry cover the whole of that finding? + + An entry with no refs covers the code outright, which is `--ignore` written down. An + entry with refs covers a finding only when every reference in it was already accepted: + one new reference and the finding comes back, which is the point. + """ + if self.code != finding.code: + return False + if not self.refs: + return True + return set(finding.refs) <= self.refs + + +@dataclass +class Baseline: + entries: list[Entry] = field(default_factory=list) + source: str = "" + + def split(self, findings: list[Finding]) -> tuple[list[Finding], list[Finding]]: + """Partition findings into (still reported, suppressed by this baseline).""" + kept, suppressed = [], [] + for finding in findings: + if any(entry.matches(finding) for entry in self.entries): + suppressed.append(finding) + else: + kept.append(finding) + return kept, suppressed + + def as_dict(self, board: str = "") -> dict: + return { + "version": VERSION, + "board": board, + "accepted": [ + {"code": e.code, "refs": sorted(e.refs), "note": e.note} + for e in sorted(self.entries, key=lambda e: (e.code, sorted(e.refs))) + ], + } + + +def from_findings(findings: list[Finding]) -> Baseline: + """A baseline accepting exactly these findings, and nothing else.""" + return Baseline( + entries=[Entry(code=f.code, refs=set(f.refs)) for f in findings], + ) + + +def load(path: str | Path) -> Baseline: + try: + raw = json.loads(Path(path).read_text(encoding="utf-8")) + except OSError as err: + raise BaselineError(f"cannot read baseline {path}: {err}") from None + except json.JSONDecodeError as err: + raise BaselineError(f"{path} is not valid JSON: {err}") from None + + if not isinstance(raw, dict): + raise BaselineError(f"{path}: expected an object at the top level") + version = raw.get("version") + # Refusing an unknown version rather than reading what it recognises: a baseline read + # half-correctly suppresses the wrong findings, and does it quietly. + if version != VERSION: + raise BaselineError( + f"{path}: baseline version {version!r}, but this pinside writes version {VERSION}" + ) + + accepted = raw.get("accepted", []) + if not isinstance(accepted, list): + raise BaselineError(f"{path}: 'accepted' must be a list") + + entries = [] + for i, item in enumerate(accepted): + if not isinstance(item, dict) or not isinstance(item.get("code"), str): + raise BaselineError(f"{path}: accepted[{i}] has no code") + refs = item.get("refs") or [] + if not isinstance(refs, list): + raise BaselineError(f"{path}: accepted[{i}].refs must be a list") + entries.append( + Entry( + code=item["code"].strip().upper(), + refs={str(r) for r in refs}, + note=str(item.get("note", "")), + ) + ) + return Baseline(entries=entries, source=str(path)) + + +def write(path: str | Path, findings: list[Finding], board: str = "") -> int: + """Write a baseline accepting these findings. Returns how many were accepted.""" + baseline = from_findings(findings) + text = json.dumps(baseline.as_dict(board), indent=2) + "\n" + Path(path).write_text(text, encoding="utf-8") + return len(baseline.entries) diff --git a/src/pinside/board.py b/src/pinside/board.py index fa589ab..6eae2a7 100644 --- a/src/pinside/board.py +++ b/src/pinside/board.py @@ -15,6 +15,17 @@ GROUND_NET = re.compile(r"^(/)?(GND|GNDA|GNDD|AGND|DGND|VSS|0)$", re.I) AUTO_NET = re.compile(r"^(Net-\(|unconnected-)") +# A supply rail. KiCad's own power symbols produce most of these names, and the leading + is +# the convention for a numbered rail (+3V3, +5V, +1V8). +POWER_NET = re.compile(r"^\+|^(VCC|VDD|VBUS|VBAT|VIN|VSYS|AVDD|DVDD)([_A-Z0-9]*)$", re.I) + +# The lines that decide whether a fixture can put the DUT into a known state. A fixture that +# reaches every data bus and none of these can read a board it cannot reset, which is the +# difference between a test rig and a monitor. +CONTROL_NET = re.compile( + r"(^|/|_)(N?RE?SET|NRST|RST|BOOT\d*|BOOTSEL|PROG|EN|ENABLE|PWR_?EN|SHDN|WAKE)$", re.I +) + # Which bus a probed signal belongs to, decided by its name. First match wins. # # Grouping matters beyond tidy reporting: a fixture wants each bus on contiguous, @@ -59,6 +70,7 @@ class Pad: net: str x: float # absolute, footprint rotation applied y: float + net_ordinal: int | None = None # None in the KiCad 10 form, which has no ordinals @property def max_dimension(self) -> float: @@ -144,15 +156,53 @@ class Board: obstacles: list[Footprint] # every other placed footprint, for collision checks frame: dict = field(default_factory=dict) + # ordinal -> every name the file gives it, from the net table and from every pad. Empty for + # a KiCad 10 board, which has no ordinals to disagree about. + net_ordinals: dict[int, set[str]] = field(default_factory=dict) + + @property + def nets(self) -> set[str]: + """Every named net the board file mentions, from any pad on any footprint. + + A .kicad_pcb has no net list of its own worth reading -- the names live on the pads -- + so this is assembled rather than parsed. Auto-named nets are left out: they carry no + intent, so their absence from the probe list says nothing. + """ + found = {pad.net for fp in self.obstacles for pad in fp.pads} + found |= {t.net for t in self.test_points} + found |= {h.net for h in self.mounting_holes} + return {n for n in found if n and not AUTO_NET.match(n.rsplit("/", 1)[-1])} + + @property + def probed_nets(self) -> set[str]: + return {t.net for t in self.test_points if t.net} + + +def _net_ref(node) -> tuple[int | None, str]: + """The (ordinal, name) a `(net ...)` expression carries. + + KiCad <= 9 writes `(net "NAME")` and keys the net by the *ordinal*: the name is a + label. KiCad 10 writes `(net "NAME")` and the name is the identity. A zone writes `(net 3)` + with no name at all. All three shapes turn up, so the ordinal is read rather than skipped: + two names sharing one ordinal is a board that loses nets the moment KiCad opens it, and + nothing else in the file gives that away. + """ + if node is None: + return None, "" + if len(node) > 2 and isinstance(node[2], str): + try: + return int(node[1]), node[2] + except (TypeError, ValueError): + return None, node[2] + token = atom(node, 1) + try: + return int(token), "" # a zone's bare (net 3) + except ValueError: + return None, token # KiCad 10's (net "NAME") + def _net_of(pad_node) -> str: - """KiCad <= 9 writes (net "NAME"); KiCad 10 writes (net "NAME").""" - net = child(pad_node, "net") - if net is None: - return "" - if len(net) > 2 and isinstance(net[2], str): - return net[2] - return atom(net, 1) + return _net_ref(child(pad_node, "net"))[1] def _property(footprint, name: str) -> str: @@ -175,6 +225,7 @@ def _read_pad(node, fx: float, fy: float, rot: float) -> Pad: drill_node = child(node, "drill") drill_vals = floats(drill_node) if drill_node is not None else [] layers = child(node, "layers") + ordinal, net_name = _net_ref(child(node, "net")) return Pad( number=atom(node, 1, "?"), type=atom(node, 2, "?"), @@ -182,7 +233,8 @@ def _read_pad(node, fx: float, fy: float, rot: float) -> Pad: size=(size[0], size[1]) if len(size) >= 2 else None, drill=max(drill_vals) if drill_vals else None, layers=[t for t in (layers[1:] if layers else []) if isinstance(t, str)], - net=_net_of(node), + net=net_name, + net_ordinal=ordinal, x=fx + dx, y=fy + dy, ) @@ -268,10 +320,25 @@ def read_outline(tree) -> g.Outline: outline.shapes.append(shape) outline.segments.extend(g.polyline_segments(points)) - outline.ring = g.chain_ring(outline.segments) + rings, outline.open_segments = g.chain_rings(outline.segments) + outline.ring, outline.cutouts, outline.islands = g.resolve_rings(rings) return outline +def read_net_ordinals(tree) -> dict[int, set[str]]: + """Every net ordinal in the file, with every name attached to it. + + Both the board's own `(net N "NAME")` table and the copy on each pad, because they can + disagree with each other and that disagreement is itself the defect. + """ + ordinals: dict[int, set[str]] = {} + for node in find_all(tree, "net"): + ordinal, name = _net_ref(node) + if ordinal is not None and name: + ordinals.setdefault(ordinal, set()).add(name) + return ordinals + + def read_board(path: str) -> Board: tree = load(path) outline = read_outline(tree) @@ -326,6 +393,7 @@ def order(ref: str) -> tuple[str, int]: test_points=test_points, mounting_holes=holes, obstacles=obstacles, + net_ordinals=read_net_ordinals(tree), ) diff --git a/src/pinside/checks.py b/src/pinside/checks.py index e032600..a436d92 100644 --- a/src/pinside/checks.py +++ b/src/pinside/checks.py @@ -12,8 +12,8 @@ from dataclasses import dataclass, field from itertools import pairwise -from .board import Board -from .geometry import BBox +from .board import CONTROL_NET, POWER_NET, Board +from .geometry import BBox, ring_area ERROR, WARNING, INFO = "error", "warning", "info" @@ -23,6 +23,7 @@ class Limits: """The physical facts a fixture is built from. Defaults suit a Mill-Max 0985 receptacle.""" probe_pitch: float = 2.54 # centre-to-centre minimum between two receptacles + probe_body: float = 1.70 # outside diameter of the receptacle body, at the DUT face edge_clearance: float = 2.0 # probe centre to board edge hole_clearance: float = 1.0 # probe pad edge to mounting-hole pad edge min_pad_diameter: float = 0.9 # DUT pad the spring tip has to land on @@ -98,25 +99,54 @@ def check_outline(board: Board, limits: Limits) -> list[Finding]: detail="every geometric check below is disabled without one", ) ] - if not board.outline.closed: + outline = board.outline + if not outline.closed: return [ Finding( "PS002", ERROR, - "the Edge.Cuts outline does not close into one ring", + f"{len(outline.open_segments)} Edge.Cuts segments do not close into a ring", detail="KiCad cannot fill zones and the fab cannot mill it; " "checks fall back to the bounding box", ) ] - return [] + + out = [] + if outline.cutouts: + out.append( + Finding( + "PS003", + INFO, + f"the outline has {len(outline.cutouts)} internal cutouts", + [f"{a:.1f}mm2" for a in sorted(ring_area(c) for c in outline.cutouts)], + "there is no board over these, so a probe landing on one reaches nothing; " + "they are treated as holes, not as a broken outline", + ) + ) + if outline.islands: + out.append( + Finding( + "PS004", + WARNING, + f"Edge.Cuts carries {len(outline.islands)} closed shapes outside the board", + [f"{a:.1f}mm2" for a in sorted(ring_area(i) for i in outline.islands)], + "the largest shape was taken as the board; the rest are a panel, or an outline " + "somebody left behind, and either way the fixture is cut to the wrong extent", + ) + ) + return out def check_placement(board: Board, limits: Limits) -> list[Finding]: """Anything sitting outside the board was never placed -- its coordinates are not a location.""" if not board.outline.segments: return [] - stray = [t.ref for t in board.test_points if not board.outline.contains(t.x, t.y)] - stray_holes = [h.ref for h in board.mounting_holes if not board.outline.contains(h.x, h.y)] + # within_perimeter, not contains: a probe in the middle of a cutout is inside the board's + # extent and is PS013's finding, not this one. Reporting it here too would say it was never + # placed, which is both wrong and the opposite of what to do about it. + inside = board.outline.within_perimeter + stray = [t.ref for t in board.test_points if not inside(t.x, t.y)] + stray_holes = [h.ref for h in board.mounting_holes if not inside(h.x, h.y)] out = [] if stray: out.append( @@ -142,6 +172,42 @@ def check_placement(board: Board, limits: Limits) -> list[Finding]: return out +def check_cutouts(board: Board, limits: Limits) -> list[Finding]: + """A probe over a slot or a window descends through the board and touches nothing. + + This is separate from the outside-the-outline check because it fails the other way round: + the coordinates are a real placement, inside the board's extent, and still wrong. Nothing + about the drill plan looks odd, and the fault shows up as a channel that reads open on a + fixture everybody has already paid for. + """ + if not board.outline.cutouts: + return [] + over = [t.ref for t in board.test_points if board.outline.in_cutout(t.x, t.y)] + holes = [h.ref for h in board.mounting_holes if board.outline.in_cutout(h.x, h.y)] + out = [] + if over: + out.append( + Finding( + "PS013", + ERROR, + f"{len(over)} test points sit over a cutout in the board", + over, + "there is no copper there and no board to hold it; the probe would pass through", + ) + ) + if holes: + out.append( + Finding( + "PS014", + ERROR, + f"{len(holes)} mounting holes sit over a cutout", + holes, + "nothing to bolt the fixture to", + ) + ) + return out + + def check_import_grid(board: Board, limits: Limits) -> list[Finding]: """A regular lattice is what KiCad makes when it drops a netlist onto a fresh board. @@ -278,6 +344,41 @@ def check_obstructions(board: Board, limits: Limits) -> list[Finding]: return [] +def check_probe_body(board: Board, limits: Limits) -> list[Finding]: + """The tip lands clear of the component and the receptacle around it does not. + + PS024 asks where the spring tip comes down. This asks how much room the thing holding it + needs: a 0985 receptacle is 1.70 mm across its body, so a probe whose tip clears a QFN by + half a millimetre still has that body pressing on the package. The tip lands where it should + and the plate never closes far enough for it to make contact. + """ + radius = limits.probe_body / 2 + close = [] + for t in board.test_points: + for fp in board.obstacles: + if fp.side != t.side: + continue + box: BBox | None = fp.bbox + if not box or box.contains(t.x, t.y): + continue # inside is PS024's finding, and a worse one + gap = box.distance_to(t.x, t.y) + if gap < radius: + close.append(f"{t.ref}-{fp.ref} {gap:.2f}mm") + if close: + return [ + Finding( + "PS027", + WARNING, + f"{len(close)} probes have less than the {radius:.2f} mm body radius " + "to a neighbouring component", + close, + "the tip clears it but the receptacle body does not, so the plate cannot close; " + "use a finer probe or move the pad", + ) + ] + return [] + + def check_pad_size(board: Board, limits: Limits) -> list[Finding]: small = [ f"{t.ref} {t.pad.min_dimension:g}mm" @@ -397,6 +498,108 @@ def check_nets(board: Board, limits: Limits) -> list[Finding]: return out +def check_net_identity(board: Board, limits: Limits) -> list[Finding]: + """Whether the board's netlist survives being opened. + + Through KiCad 9 a net is identified by its *ordinal*; the name beside it is a label. Give two + different names the same ordinal and KiCad reads them as one net, keeps whichever name it saw + first, and drops the rest to no-net the moment the file is saved. Nothing warns anyone: the + file is well-formed, it opens, and fifteen test points quietly stop being connected to + anything. + + That is not hypothetical. It is what pinside's own example board did, and pinside called it + clean for two releases, because the reader takes the name off each pad and never compares the + ordinals. Confirmed against `kicad-cli pcb export ipcd356`, which is KiCad's own answer to + "what is this board's netlist". + + Silent on a KiCad 10 board: that format dropped the ordinal, so there is nothing to collide. + """ + out = [] + + collisions = { + ordinal: sorted(names) + for ordinal, names in board.net_ordinals.items() + if ordinal != 0 and len(names) > 1 + } + if collisions: + # In board order, which read_board already sorted naturally: sorting again here gives + # TP1, TP10, TP11, TP2, which reads as though the refs were picked at random. + affected = [t.ref for t in board.test_points if t.pad and t.pad.net_ordinal in collisions] + out.append( + Finding( + "PS043", + ERROR, + f"{len(collisions)} net numbers carry more than one net name", + [f"net {n}: {', '.join(names)}" for n, names in sorted(collisions.items())], + "KiCad identifies a net by its number, so it reads these as one net and keeps " + "only the first name; the rest lose their connection on the next save" + + (f". Affects {', '.join(affected)}" if affected else ""), + ) + ) + + named_zero = sorted(board.net_ordinals.get(0, ())) + if named_zero: + refs = [t.ref for t in board.test_points if t.pad and t.pad.net_ordinal == 0 and t.net] + out.append( + Finding( + "PS044", + ERROR, + f"{len(named_zero)} named nets are on net number 0", + refs or named_zero, + "net 0 is KiCad's no-connection net, so the name is ignored and these pads probe " + "nothing; pinside reads the name and would build a fixture channel for each", + ) + ) + return out + + +def _bare(net: str) -> str: + return net.rsplit("/", 1)[-1] + + +def check_signal_coverage(board: Board, limits: Limits) -> list[Finding]: + """Which nets the board has that the fixture will not be able to reach. + + Every other check here asks whether the probes that exist are placed correctly. This one + asks what is missing, which is the failure nothing else can see: a fixture that reaches + every data bus and no reset line can watch a board it cannot put into a known state, and + that is discovered on the bench, after the plate is built. + """ + if not board.test_points: + return [] + + probed = {_bare(n) for n in board.probed_nets} + unprobed = sorted(_bare(n) for n in board.nets if _bare(n) not in probed) + + out = [] + rails = [n for n in unprobed if POWER_NET.match(n)] + if rails: + out.append( + Finding( + "PS033", + WARNING, + f"{len(rails)} supply rails have no test point", + rails, + "a rail nobody probes is a rail the fixture cannot prove came up; one pad each " + "turns a dead board into a measurement", + ) + ) + + control = [n for n in unprobed if CONTROL_NET.search(n) and not POWER_NET.match(n)] + if control: + out.append( + Finding( + "PS034", + INFO, + f"{len(control)} reset or strap lines have no test point", + control, + "without one the fixture can read the DUT but not put it into a known state, " + "so a test that fails cannot be retried from a clean start", + ) + ) + return out + + def check_mounting(board: Board, limits: Limits) -> list[Finding]: out = [] holes = board.mounting_holes @@ -430,7 +633,7 @@ def check_mounting(board: Board, limits: Limits) -> list[Finding]: outside = [t.ref for t in board.test_points if not span.contains(t.x, t.y)] # Only meaningful once the probes are actually placed. placed = board.outline.segments and all( - board.outline.contains(t.x, t.y) for t in board.test_points + board.outline.within_perimeter(t.x, t.y) for t in board.test_points ) if outside and placed: out.append( @@ -448,16 +651,20 @@ def check_mounting(board: Board, limits: Limits) -> list[Finding]: CHECKS = [ check_outline, check_placement, + check_cutouts, check_import_grid, check_stacked, check_pitch, check_edge_clearance, check_hole_clearance, check_obstructions, + check_probe_body, check_pad_size, check_sides, check_ground, check_nets, + check_net_identity, + check_signal_coverage, check_mounting, ] diff --git a/src/pinside/cli.py b/src/pinside/cli.py index 5e39c68..327f324 100644 --- a/src/pinside/cli.py +++ b/src/pinside/cli.py @@ -7,9 +7,9 @@ import sys from pathlib import Path -from . import __version__, modules, pogo +from . import __version__, baseline, modules, pogo from .board import read_board, transform -from .checks import ERROR, WARNING, Limits, run +from .checks import ERROR, WARNING, Finding, Limits, run from .config import ConfigError, load, resolve_board, validate from .firmware import GenerationError, generate from .kicad.project import ProjectError, generate_project @@ -18,15 +18,62 @@ EXIT_OK, EXIT_WARN, EXIT_ERROR, EXIT_USAGE = 0, 1, 2, 3 +_BASELINE_HELP = ( + "JSON file of findings already accepted for this board. Suppression is by code and by " + "reference, so a new occurrence of an accepted code still fails." +) -def _print_findings(findings, stream=sys.stderr) -> None: + +def _print_findings(findings, stream=None) -> None: + # Resolved on the call, not in the signature: a default of sys.stderr binds whatever + # sys.stderr was when this module was imported, which is the wrong stream for anything + # that redirects it afterwards. + stream = sys.stderr if stream is None else stream for finding in findings: print(f"pinside: {finding}", file=stream) +def _apply_baseline(findings: list[Finding], path: str | None) -> list[Finding] | None: + """Drop the findings a baseline already accepts. None means the baseline was unusable.""" + if not path: + return findings + try: + accepted = baseline.load(path) + except baseline.BaselineError as err: + print(f"pinside: {err}", file=sys.stderr) + return None + kept, suppressed = accepted.split(findings) + if suppressed: + # Said out loud, every time. A baseline that silently swallows half the report is how a + # board ships with a finding nobody remembers accepting. + print( + f"pinside: {len(suppressed)} finding(s) accepted by {path}: " + f"{', '.join(sorted({f.code for f in suppressed}))}", + file=sys.stderr, + ) + return kept + + +def _emit_json_findings(findings: list[Finding], **extra) -> None: + """The machine-readable half of what generate and project report. + + Both of them write their real output to a directory and their findings to stderr, which + leaves nothing for a caller to parse. This goes to stdout so the two do not mix. + """ + payload = { + "findings": [f.as_dict() for f in findings], + "errors": sum(1 for f in findings if f.severity == ERROR), + "warnings": sum(1 for f in findings if f.severity == WARNING), + **extra, + } + json.dump(payload, sys.stdout, indent=2) + print() + + def _limits_from(args) -> Limits: return Limits( probe_pitch=args.probe_pitch, + probe_body=args.probe_body, edge_clearance=args.edge_clearance, hole_clearance=args.hole_clearance, min_pad_diameter=args.min_pad, @@ -50,6 +97,20 @@ def cmd_check(args) -> int: ignored = {c.strip().upper() for c in args.ignore.split(",") if c.strip()} findings = [f for f in findings if f.code not in ignored] + # Written before the baseline is applied, or the second run would record an empty file. + if args.write_baseline: + count = baseline.write(args.write_baseline, findings, args.board) + print( + f"pinside: wrote {args.write_baseline} accepting {count} finding(s); " + "add a note to each before committing it", + file=sys.stderr, + ) + return EXIT_OK + + findings = _apply_baseline(findings, args.baseline) + if findings is None: + return EXIT_USAGE + if args.output: with Path(args.output).open("w", encoding="utf-8") as handle: FORMATS[args.format](board, findings, handle) @@ -99,7 +160,7 @@ def cmd_init(args) -> int: name=name, mcu=args.mcu, board_path=board_path, - module_name=args.board, + module_name=args.carrier, probe=args.probe, ) except ValueError as err: @@ -142,11 +203,26 @@ def cmd_project(args) -> int: result = generate_project(cfg, board, args.out, force=args.force) except ProjectError as err: print(f"pinside: {err}", file=sys.stderr) - _print_findings(err.findings) + if args.json: + _emit_json_findings(err.findings, written=False) + else: + _print_findings(err.findings) return EXIT_ERROR - if result.findings: - _print_findings(result.findings) + findings = _apply_baseline(result.findings, args.baseline) + if findings is None: + return EXIT_USAGE + + if args.json: + _emit_json_findings( + findings, + out_dir=str(result.out_dir), + files=[str(f) for f in result.files], + probes_placed=result.probes_placed, + unplaced=list(result.unplaced), + ) + elif findings: + _print_findings(findings) probe = cfg.probe_part print(f"pinside: wrote {len(result.files)} files to {result.out_dir}", file=sys.stderr) print( @@ -161,15 +237,109 @@ def cmd_project(args) -> int: file=sys.stderr, ) print( - "pinside: routing, the controller's placement and the ground pour are left to you", + "pinside: a GND pour is drawn on both layers; routing and the controller's placement " + "are left to you", file=sys.stderr, ) - if args.strict and any(f.severity == WARNING for f in result.findings): + if args.strict and any(f.severity == WARNING for f in findings): return EXIT_WARN return EXIT_OK +def cmd_probe(args) -> int: + """Connect to a fixture and prove it is the right one, wired the way the config says.""" + from . import client # imported here: it needs pyserial, and nothing else does + + try: + cfg = load(args.config) + except ConfigError as err: + print(f"pinside: {err}", file=sys.stderr) + return EXIT_USAGE + + try: + device = args.port or _sole_port(client) + except client.FixtureError as err: + print(f"pinside: {err}", file=sys.stderr) + return EXIT_USAGE + + try: + fixture = client.connect( + device, cfg, baud=args.baud, timeout=args.timeout, check_hash=not args.no_hash_check + ) + except client.ConfigMismatchError as err: + # Its own exit code would be nicer, but this is what --strict-style tooling already + # keys on, and a mismatched rig is an error by any reading. + print(f"pinside: {err}", file=sys.stderr) + return EXIT_ERROR + except client.FixtureError as err: + print(f"pinside: {err}", file=sys.stderr) + return EXIT_USAGE + + with fixture: + try: + channels = fixture.channels() + rails = fixture.adc_snapshot() + gpio = fixture.gpio_snapshot() if not args.no_gpio else [] + except client.FixtureError as err: + print(f"pinside: {err}", file=sys.stderr) + return EXIT_ERROR + + out_of_range = [r for r in rails if not r.get("in_range", True)] + + if args.json: + json.dump( + { + "port": device, + "info": fixture.info, + "channels": channels, + "adc": rails, + "gpio": gpio, + "out_of_range": [r.get("channel") for r in out_of_range], + }, + sys.stdout, + indent=2, + ) + print() + else: + info = fixture.info or {} + print(f"{info.get('fixture', '?')} on {device}") + print(f" firmware {info.get('version', '?')} config {info.get('config_hash', '?')}") + print(f" {len(channels)} channels") + for channel in channels: + probe = channel.get("probe") or "-" + print(f" {channel.get('kind', '?'):5} {channel.get('name', '?'):20} {probe}") + if rails: + print(" rails") + for rail in rails: + mark = " " if rail.get("in_range", True) else "!" + print( + f" {mark} {rail.get('channel', '?'):20} " + f"{rail.get('millivolts', 0) / 1000:.3f} V ({rail.get('probe', '-')})" + ) + + if out_of_range: + names = ", ".join(r.get("channel", "?") for r in out_of_range) + print(f"pinside: {len(out_of_range)} rail(s) out of range: {names}", file=sys.stderr) + return EXIT_ERROR + return EXIT_OK + + +def _sole_port(client) -> str: + """The one serial port on this machine, or a message naming the ones there are. + + Guessing between several is worse than asking: the wrong guess talks to whatever else is + plugged in, and the hash check only catches it if that thing happens to answer. + """ + found = client.ports() + if len(found) == 1: + return found[0][0] + if not found: + raise client.FixtureError("no serial ports found; is the fixture plugged in?") + listing = "\n".join(f" {device} {description}" for device, description in found) + raise client.FixtureError(f"several serial ports; name one with --port:\n{listing}") + + def cmd_generate(args) -> int: try: cfg = load(args.config) @@ -182,33 +352,58 @@ def cmd_generate(args) -> int: return EXIT_USAGE if args.dry_run: - findings = validate(cfg, board) - _print_findings(findings, sys.stdout) + findings = _apply_baseline(validate(cfg, board), args.baseline) + if findings is None: + return EXIT_USAGE + if args.json: + _emit_json_findings(findings, config=cfg.name, mcu=cfg.mcu, written=False) + else: + _print_findings(findings, sys.stdout) if any(f.severity == ERROR for f in findings): return EXIT_ERROR - print(f"pinside: {cfg.name} validates against {cfg.mcu} and {cfg.dut_board or 'no board'}") + if not args.json: + print( + f"pinside: {cfg.name} validates against {cfg.mcu} and {cfg.dut_board or 'no board'}" + ) return EXIT_OK try: result = generate(cfg, board, args.out, force=args.force) except GenerationError as err: print(f"pinside: {err} -- nothing was written", file=sys.stderr) - _print_findings(err.findings) + if args.json: + _emit_json_findings(err.findings, config=cfg.name, written=False) + else: + _print_findings(err.findings) return EXIT_ERROR - if result.findings: - _print_findings(result.findings) - print( - f"pinside: wrote {len(result.files)} files to {result.out_dir} " - f"(config {result.config_hash})", - file=sys.stderr, - ) - print( - f"pinside: build with cmake, or run {result.out_dir}/test/run.sh for the host tests", - file=sys.stderr, - ) + findings = _apply_baseline(result.findings, args.baseline) + if findings is None: + return EXIT_USAGE - if args.strict and any(f.severity == WARNING for f in result.findings): + if args.json: + _emit_json_findings( + findings, + config=cfg.name, + config_hash=result.config_hash, + out_dir=str(result.out_dir), + files=[str(f) for f in result.files], + written=True, + ) + else: + if findings: + _print_findings(findings) + print( + f"pinside: wrote {len(result.files)} files to {result.out_dir} " + f"(config {result.config_hash})", + file=sys.stderr, + ) + print( + f"pinside: build with cmake, or run {result.out_dir}/test/run.sh for the host tests", + file=sys.stderr, + ) + + if args.strict and any(f.severity == WARNING for f in findings): return EXIT_WARN return EXIT_OK @@ -259,6 +454,12 @@ def build_parser() -> argparse.ArgumentParser: default=d.probe_pitch, help="minimum centre-to-centre spacing of two receptacles, mm", ) + limits.add_argument( + "--probe-body", + type=float, + default=d.probe_body, + help="outside diameter of the receptacle body, mm; what has to clear a neighbouring part", + ) limits.add_argument( "--edge-clearance", type=float, @@ -289,6 +490,17 @@ def build_parser() -> argparse.ArgumentParser: default="", help="comma-separated finding codes to suppress, e.g. PS041,PS042", ) + check.add_argument( + "--baseline", + metavar="FILE", + help=_BASELINE_HELP, + ) + check.add_argument( + "--write-baseline", + metavar="FILE", + help="write a baseline accepting every finding this board currently has, and stop. " + "Review it, write the reason into each note, and commit it.", + ) check.add_argument("--no-checks", action="store_true", help="extract only, run no checks") check.add_argument("--strict", action="store_true", help=strict_help) check.set_defaults(func=cmd_check) @@ -302,13 +514,16 @@ def build_parser() -> argparse.ArgumentParser: ) init.add_argument("board", help="path to the DUT's .kicad_pcb") init.add_argument("-o", "--output", help="write here instead of stdout") + # --carrier, not --board: the positional is already a board, argparse gives both the same + # dest, and the option silently won. Every `pinside init` then looked up the .kicad_pcb path + # in the module catalogue and refused. init.add_argument( - "--board", + "--carrier", default=modules.DEFAULT, choices=[*sorted(modules.MODULES), modules.BARE], help="carrier board the fixture is built around", ) - init.add_argument("--mcu", default="rp2350b", help="microcontroller, when --board is 'bare'") + init.add_argument("--mcu", default="rp2350b", help="microcontroller, when --carrier is 'bare'") init.add_argument( "--probe", default=pogo.DEFAULT, @@ -336,6 +551,16 @@ def build_parser() -> argparse.ArgumentParser: ) gen.add_argument("--dry-run", action="store_true", help="validate and report, writing nothing") gen.add_argument("--strict", action="store_true", help=strict_help) + gen.add_argument( + "--json", + action="store_true", + help="report findings as JSON on stdout instead of prose on stderr", + ) + gen.add_argument( + "--baseline", + metavar="FILE", + help=_BASELINE_HELP, + ) gen.set_defaults(func=cmd_generate) proj = sub.add_parser( @@ -357,18 +582,66 @@ def build_parser() -> argparse.ArgumentParser: help="write into a non-empty directory pinside did not create", ) proj.add_argument("--strict", action="store_true", help=strict_help) + proj.add_argument( + "--json", + action="store_true", + help="report findings as JSON on stdout instead of prose on stderr", + ) + proj.add_argument( + "--baseline", + metavar="FILE", + help=_BASELINE_HELP, + ) proj.set_defaults(func=cmd_project) + probe_cmd = sub.add_parser( + "probe", + help="connect to a fixture and check it against its config", + description="Open the fixture's serial port, confirm the firmware on it was generated " + "from this config, and report every channel and rail. This is the bench smoke test: " + "it answers 'is the fixture the one I think it is, and is it wired up'.", + formatter_class=argparse.ArgumentDefaultsHelpFormatter, + ) + probe_cmd.add_argument("config", help="path to the fixture config JSON") + probe_cmd.add_argument( + "--port", help="serial device; the only one present is used when there is exactly one" + ) + probe_cmd.add_argument("--baud", type=int, default=115200, help="USB CDC ignores this") + probe_cmd.add_argument("--timeout", type=float, default=5.0, help="seconds to wait per call") + probe_cmd.add_argument( + "--no-hash-check", + action="store_true", + help="talk to the fixture even when its config hash disagrees with this config. " + "The mismatch is exactly what this command exists to catch, so say why in the commit.", + ) + probe_cmd.add_argument( + "--no-gpio", action="store_true", help="skip the GPIO snapshot; ADC rails only" + ) + probe_cmd.add_argument("--json", action="store_true", help="report as JSON on stdout") + probe_cmd.set_defaults(func=cmd_probe, strict=False) + return p +def commands(parser: argparse.ArgumentParser) -> set[str]: + """Every subcommand the parser accepts, asked of the parser itself.""" + # argparse exposes no public accessor for this, and the alternative -- a list of names + # kept beside the parser -- is the thing that broke. + for action in parser._actions: + if isinstance(action, argparse._SubParsersAction): + return set(action.choices) + return set() + + def main(argv: list[str] | None = None) -> int: argv = list(sys.argv[1:] if argv is None else argv) parser = build_parser() - # `pinside board.kicad_pcb` keeps working as a shorthand for `pinside check`. - known = {"check", "init", "generate", "project"} - if argv and not argv[0].startswith("-") and argv[0] not in known: + # `pinside board.kicad_pcb` keeps working as a shorthand for `pinside check`. The command + # names come from the parser rather than a list kept alongside it: a hand-maintained set + # silently swallows any subcommand added later, turning `pinside probe cfg.json` into + # `pinside check probe`, which fails while complaining about the wrong thing. + if argv and not argv[0].startswith("-") and argv[0] not in commands(parser): argv.insert(0, "check") args = parser.parse_args(argv) diff --git a/src/pinside/client.py b/src/pinside/client.py new file mode 100644 index 0000000..d1203c1 --- /dev/null +++ b/src/pinside/client.py @@ -0,0 +1,316 @@ +"""Talking to a fixture from the host. + +`pinside generate` emits firmware that speaks JSON-RPC 2.0 over USB CDC and ships an +`openrpc.json` describing it. Until this module existed nothing on the host spoke that protocol, +so everyone who used a fixture wrote the same serial client by hand, and the config-hash check +the README recommends was a comparison somebody did by eye. + +Two things here are not conveniences: + +**The hash check is on by default.** `connect()` reads `fixture.info` and refuses to hand back a +client whose `config_hash` disagrees with the config it was given. A rig one revision behind is +the most common way a bench lies to you, and it lies quietly: every call succeeds, against the +wrong pin. + +**Notifications are not responses.** A streaming UART pushes `uart.data` between replies, so a +client that reads one line per request eventually returns a log line as the answer to an ADC +read. Every line is classified before anything waits on it, and notifications go to a queue. + +pyserial is an optional dependency (`pip install pinside[client]`). The core stays +dependency-free; only this module needs it, and it says so when it is missing. +""" + +from __future__ import annotations + +import json +import threading +import time +from collections import deque +from collections.abc import Callable +from dataclasses import dataclass, field +from typing import Any + +from .config import FixtureConfig +from .firmware.generate import config_hash + +DEFAULT_BAUD = 115200 +DEFAULT_TIMEOUT = 5.0 + +# How long to wait for `fixture.ready` before assuming the board was already running. A fixture +# announces itself once at start-up, and opening the port does not always reset it. +READY_GRACE = 2.0 + + +class FixtureError(Exception): + """The fixture could not be reached, or answered something that is not a valid response.""" + + +class RpcError(FixtureError): + """The fixture understood the call and refused it.""" + + def __init__(self, code: int, message: str, data: Any = None): + self.code = code + self.message = message + self.data = data + detail = f" ({data})" if data is not None else "" + super().__init__(f"{message}{detail} [{code}]") + + +class ConfigMismatchError(FixtureError): + """The fixture is running firmware generated from a different config. + + This is the failure the config hash exists to catch. Every subsequent call would succeed and + reach the wrong pin, so it is raised rather than warned about. + """ + + def __init__(self, expected: str, found: str, name: str = ""): + self.expected = expected + self.found = found + super().__init__( + f"the fixture on this port reports config {found!r}, but the config given here " + f"hashes to {expected!r}" + + (f" ({name})" if name else "") + + ". Regenerate and reflash, or pass check_hash=False if you meant to." + ) + + +@dataclass +class Notification: + """Something the fixture said without being asked.""" + + method: str + params: dict + received: float = field(default_factory=time.monotonic) + + @property + def channel(self) -> str: + return self.params.get("channel", "") + + @property + def data(self) -> bytes: + """The payload of a `uart.data` notification, decoded from its hex.""" + return bytes.fromhex(self.params.get("hex", "")) + + +def _require_serial(): + try: + import serial + except ImportError: + raise FixtureError( + "talking to a fixture needs pyserial, which pinside does not install by default. " + "Install it with: pip install 'pinside[client]'" + ) from None + return serial + + +def ports() -> list[tuple[str, str]]: + """Every serial port the machine can see, as (device, description). + + Not filtered to fixtures: a fixture identifies itself by answering `fixture.info`, not by its + USB descriptor, and guessing from vendor IDs would hide the board somebody actually plugged in. + """ + serial = _require_serial() + from serial.tools import list_ports + + del serial + return [(p.device, p.description or "") for p in list_ports.comports()] + + +class Fixture: + """One open connection to a fixture. Use `connect()` rather than constructing this.""" + + def __init__(self, port, timeout: float = DEFAULT_TIMEOUT): + self._port = port + self._timeout = timeout + self._next_id = 1 + self._lock = threading.Lock() + self.notifications: deque[Notification] = deque(maxlen=1000) + self.info: dict = {} + + # ---------------------------------------------------------------- plumbing + + def _read_line(self, deadline: float) -> dict: + """One JSON object off the wire, or a timeout.""" + while True: + remaining = deadline - time.monotonic() + if remaining <= 0: + raise FixtureError(f"the fixture did not answer within {self._timeout:g}s") + self._port.timeout = remaining + raw = self._port.readline() + if not raw: + continue + text = raw.decode("utf-8", "replace").strip() + if not text: + continue + try: + message = json.loads(text) + except json.JSONDecodeError: + # Not fatal. A board that was mid-line when the port opened, or one printing + # boot chatter before the protocol starts, produces exactly this. + continue + if isinstance(message, dict): + return message + + def _pump(self, deadline: float, want_id: int | None) -> dict: + """Read until the response with this id arrives, filing notifications on the way. + + A response is classified the way JSON-RPC 2.0 defines one: it carries `result` or + `error` and never `method`. Matching on the id alone is not enough. Anything that echoes + what it was sent -- a loopback, a terminal in local-echo, a half-configured USB gadget -- + sends back a message with the right id and no result, and a client keyed on the id calls + that the answer and reports a successful read of nothing. + """ + while True: + message = self._read_line(deadline) + if "method" in message: + if "id" not in message: + self.notifications.append( + Notification(message["method"], message.get("params") or {}) + ) + # A request coming the other way, or our own echo. Neither is a response. + continue + if "result" not in message and "error" not in message: + continue # neither a response nor a notification: not part of the protocol + if want_id is None or message.get("id") == want_id: + return message + # A response to a call that already timed out. Dropping it keeps the stream in + # step; keeping it would answer the next call with the previous one's result. + + def call(self, method: str, timeout: float | None = None, **params) -> Any: + """Make one JSON-RPC call and return its result, raising RpcError on refusal.""" + with self._lock: + request = {"jsonrpc": "2.0", "id": self._next_id, "method": method} + if params: + request["params"] = params + self._next_id += 1 + + self._port.write((json.dumps(request) + "\n").encode("utf-8")) + self._port.flush() + + deadline = time.monotonic() + (timeout or self._timeout) + response = self._pump(deadline, request["id"]) + + if "error" in response: + err = response["error"] + raise RpcError(err.get("code", 0), err.get("message", "?"), err.get("data")) + return response.get("result") + + def notify(self, method: str, **params) -> None: + """Send a request with no id, expecting no reply.""" + with self._lock: + request = {"jsonrpc": "2.0", "method": method} + if params: + request["params"] = params + self._port.write((json.dumps(request) + "\n").encode("utf-8")) + self._port.flush() + + def poll(self, seconds: float = 0.0) -> list[Notification]: + """Collect notifications the fixture pushed, waiting up to `seconds` for more.""" + deadline = time.monotonic() + seconds + while seconds and time.monotonic() < deadline: + try: + message = self._read_line(deadline) + except FixtureError: + break + if "method" in message and "id" not in message: + self.notifications.append( + Notification(message["method"], message.get("params") or {}) + ) + drained = list(self.notifications) + self.notifications.clear() + return drained + + def close(self) -> None: + with self._lock: + self._port.close() + + def __enter__(self) -> Fixture: + return self + + def __exit__(self, *exc) -> None: + self.close() + + # ---------------------------------------------------------------- the protocol + + def channels(self) -> list[dict]: + """Every channel, with the DUT signal it lands on. The map an agent navigates by.""" + return self.call("fixture.channels") + + def gpio_read(self, channel: str) -> dict: + return self.call("gpio.read", channel=channel) + + def gpio_write(self, channel: str, value: bool) -> dict: + return self.call("gpio.write", channel=channel, value=bool(value)) + + def gpio_snapshot(self) -> list[dict]: + return self.call("gpio.snapshot") + + def adc_read(self, channel: str) -> dict: + return self.call("adc.read", channel=channel) + + def adc_snapshot(self) -> list[dict]: + return self.call("adc.snapshot") + + def uart_write(self, channel: str, data: bytes) -> dict: + return self.call("uart.write", channel=channel, hex=bytes(data).hex()) + + def uart_read(self, channel: str) -> bytes: + result = self.call("uart.read", channel=channel) + return bytes.fromhex(result.get("hex", "") if isinstance(result, dict) else "") + + def i2c_scan(self, channel: str) -> list[int]: + result = self.call("i2c.scan", channel=channel) + return result.get("addresses", []) if isinstance(result, dict) else result + + def i2c_write(self, channel: str, address: int, data: bytes) -> dict: + return self.call("i2c.write", channel=channel, address=address, hex=bytes(data).hex()) + + def i2c_read(self, channel: str, address: int, length: int) -> bytes: + result = self.call("i2c.read", channel=channel, address=address, length=length) + return bytes.fromhex(result.get("hex", "") if isinstance(result, dict) else "") + + def spi_transfer(self, channel: str, data: bytes) -> bytes: + result = self.call("spi.transfer", channel=channel, hex=bytes(data).hex()) + return bytes.fromhex(result.get("hex", "") if isinstance(result, dict) else "") + + +def connect( + device: str, + config: FixtureConfig | None = None, + *, + baud: int = DEFAULT_BAUD, + timeout: float = DEFAULT_TIMEOUT, + check_hash: bool = True, + opener: Callable[..., Any] | None = None, +) -> Fixture: + """Open a port, identify what is on it, and refuse a fixture that is not the one expected. + + `opener` exists so the tests can drive a fake port; leave it alone in real use. + """ + if opener is None: + serial = _require_serial() + opener = serial.serial_for_url + + try: + port = opener(device, baudrate=baud, timeout=timeout) + except Exception as err: # pyserial raises several unrelated types here + raise FixtureError(f"cannot open {device}: {err}") from None + + fixture = Fixture(port, timeout=timeout) + try: + fixture.info = fixture.call("fixture.info") + except FixtureError: + fixture.close() + raise + + if check_hash: + if config is None: + raise FixtureError( + "check_hash needs a config to check against; pass one, or check_hash=False" + ) + expected = config_hash(config) + found = (fixture.info or {}).get("config_hash", "") + if found != expected: + fixture.close() + raise ConfigMismatchError(expected, found, config.name) + return fixture diff --git a/src/pinside/firmware/generate.py b/src/pinside/firmware/generate.py index eaf767c..268fc14 100644 --- a/src/pinside/firmware/generate.py +++ b/src/pinside/firmware/generate.py @@ -493,13 +493,20 @@ def emit_cmakelists(cfg: FixtureConfig) -> str: target_link_libraries(${{PROJECT_NAME}} pico_stdlib hardware_adc + hardware_clocks hardware_gpio hardware_i2c hardware_spi hardware_uart ) -target_compile_options(${{PROJECT_NAME}} PRIVATE -Wall -Wextra) +# -Werror on implicit declarations, specifically. C11 makes calling an undeclared function a +# warning, so a missing header compiles cleanly and fails at link with nothing but a symbol +# name. That is how this project shipped without `hardware/clocks.h`: every translation unit +# built, and only the linker objected. +target_compile_options(${{PROJECT_NAME}} PRIVATE + -Wall -Wextra -Werror=implicit-function-declaration +) # The protocol owns the CDC line; nothing else may write to it. pico_enable_stdio_usb(${{PROJECT_NAME}} 1) diff --git a/src/pinside/firmware/templates/main.c b/src/pinside/firmware/templates/main.c index 9451071..c109876 100644 --- a/src/pinside/firmware/templates/main.c +++ b/src/pinside/firmware/templates/main.c @@ -11,6 +11,10 @@ #include "fixture_config.h" #include "fixture_core.h" +/* set_sys_clock_khz is a static inline here, and pico/stdlib.h does not pull this in. Without + * it the call is an implicit declaration -- a warning under C11, not an error -- so the file + * compiles and the link fails with "undefined reference to set_sys_clock_khz". */ +#include "hardware/clocks.h" #include "pico/stdlib.h" int main(void) { diff --git a/src/pinside/geometry.py b/src/pinside/geometry.py index 4c8aa66..9a74d1d 100644 --- a/src/pinside/geometry.py +++ b/src/pinside/geometry.py @@ -39,6 +39,12 @@ def height(self) -> float: def contains(self, x: float, y: float) -> bool: return self.min_x <= x <= self.max_x and self.min_y <= y <= self.max_y + def distance_to(self, x: float, y: float) -> float: + """Distance from a point to the nearest edge of the box, or 0 inside it.""" + dx = max(self.min_x - x, 0.0, x - self.max_x) + dy = max(self.min_y - y, 0.0, y - self.max_y) + return math.hypot(dx, dy) + def as_dict(self) -> dict: return { "min_x": round(self.min_x, 4), @@ -52,15 +58,25 @@ def as_dict(self) -> dict: @dataclass class Outline: - """The board edge, as drawn and as flattened.""" + """The board edge, as drawn and as flattened. + + Edge.Cuts is one layer holding every edge the board has, and a board with a slot, a + connector relief or a mouse-bite window has more than one closed shape on it. The largest + is the perimeter; anything closed and inside it is a hole in the board, and a probe over + one is a probe over nothing. + """ shapes: list[dict] = field(default_factory=list) segments: list[Segment] = field(default_factory=list) - ring: list[Point] = field(default_factory=list) # empty when the outline is not closed + ring: list[Point] = field(default_factory=list) # the perimeter; empty when nothing closed + cutouts: list[list[Point]] = field(default_factory=list) # closed rings inside the perimeter + islands: list[list[Point]] = field(default_factory=list) # closed rings outside it + open_segments: list[Segment] = field(default_factory=list) # never joined into any ring @property def closed(self) -> bool: - return bool(self.ring) + """One perimeter, and no edge left dangling. Cutouts do not make an outline open.""" + return bool(self.ring) and not self.open_segments @property def bbox(self) -> BBox | None: @@ -72,14 +88,40 @@ def bbox(self) -> BBox | None: return BBox(min(xs), min(ys), max(xs), max(ys)) def contains(self, x: float, y: float) -> bool: - """Inside the board? Uses the true ring when the outline closes, else its bounding box.""" - if self.ring: - return point_in_ring((x, y), self.ring) - box = self.bbox - return bool(box and box.contains(x, y)) + """Is there board here? Inside the perimeter and not inside a cutout. + + Falls back to the bounding box when nothing closed, so the geometric checks still say + something useful about a board whose edge is broken. + """ + if not self.ring: + box = self.bbox + return bool(box and box.contains(x, y)) + if not point_in_ring((x, y), self.ring): + return False + return not self.in_cutout(x, y) + + def within_perimeter(self, x: float, y: float) -> bool: + """Inside the board's extent, cutouts ignored. + + This is the question "was this ever placed", which a cutout does not change: a probe in + the middle of a slot is somewhere deliberate and wrong, not still sitting where KiCad + dropped it. ``contains`` answers the other question, whether there is board underneath. + """ + if not self.ring: + box = self.bbox + return bool(box and box.contains(x, y)) + return point_in_ring((x, y), self.ring) + + def in_cutout(self, x: float, y: float) -> bool: + """Inside one of the holes in the board.""" + return any(point_in_ring((x, y), c) for c in self.cutouts) def distance_to_edge(self, x: float, y: float) -> float | None: - """Shortest distance from a point to the board edge. None when there is no outline.""" + """Shortest distance to any board edge, cutout edges included. + + A cutout edge is as much of a wall as the perimeter is, so the clearance a probe needs + from one is the clearance it needs from the other. + """ if not self.segments: return None return min(point_segment_distance((x, y), a, b) for a, b in self.segments) @@ -199,36 +241,82 @@ def point_in_ring(p: Point, ring: list[Point]) -> bool: return inside -def chain_ring(segments: list[Segment], tolerance: float = JOIN_TOLERANCE) -> list[Point]: - """Walk the segments into one closed ring, or return [] if they do not form exactly one. +def ring_area(ring: list[Point]) -> float: + """Enclosed area by the shoelace formula, unsigned so winding direction does not matter.""" + total = 0.0 + for (x1, y1), (x2, y2) in polyline_segments(ring): + total += x1 * y2 - x2 * y1 + return abs(total) / 2 + + +def chain_rings( + segments: list[Segment], tolerance: float = JOIN_TOLERANCE +) -> tuple[list[list[Point]], list[Segment]]: + """Walk the segments into as many closed rings as they form. + + Returns (rings, leftovers). A leftover is a segment that never joined anything into a closed + shape, which is the real defect: KiCad refuses to fill zones against an open edge and the fab + cannot mill it. - A board edge that does not close is a real defect -- KiCad refuses to fill zones and the fab - cannot mill it -- so failing here is a finding, not an inconvenience to route around. + More than one ring is not a defect. A board with a slot, a connector relief or an antenna + keepout window has several closed shapes on Edge.Cuts, and treating that as an open outline + -- which is what a single-ring walk does -- calls a perfectly good board unbuildable. """ - if not segments: - return [] remaining = list(segments) - start, current = remaining[0][0], remaining[0][1] - ring = [start, current] - remaining.pop(0) + rings: list[list[Point]] = [] + orphans: list[Segment] = [] while remaining: - for i, (a, b) in enumerate(remaining): - if math.dist(current, a) <= tolerance: - current = b - elif math.dist(current, b) <= tolerance: - current = a - else: - continue - ring.append(current) - remaining.pop(i) - break + start, current = remaining.pop(0) + path = [start, current] + advanced = True + while advanced and math.dist(current, start) > tolerance: + advanced = False + for i, (a, b) in enumerate(remaining): + if math.dist(current, a) <= tolerance: + current = b + elif math.dist(current, b) <= tolerance: + current = a + else: + continue + path.append(current) + remaining.pop(i) + advanced = True + break + + # Three points is the least that can enclose anything; two is a line drawn back on + # itself, which closes arithmetically and encloses nothing. + if math.dist(current, start) <= tolerance and len(path) > 3: + path[-1] = start + rings.append(path) else: - return [] # a gap: the edge is open, or there is more than one closed shape - if math.dist(current, start) <= tolerance: - break - - if remaining or math.dist(current, start) > tolerance: - return [] - ring[-1] = start - return ring + orphans.extend(polyline_segments(path)) + + return rings, orphans + + +def chain_ring(segments: list[Segment], tolerance: float = JOIN_TOLERANCE) -> list[Point]: + """The single closed ring these segments form, or [] if they do not form exactly one.""" + rings, orphans = chain_rings(segments, tolerance) + return rings[0] if len(rings) == 1 and not orphans else [] + + +def resolve_rings( + rings: list[list[Point]], +) -> tuple[list[Point], list[list[Point]], list[list[Point]]]: + """Sort closed rings into (perimeter, cutouts, islands). + + The perimeter is the one enclosing the most area. A ring inside it is a hole in the board. + A ring outside it is a second board: a panel, or an Edge.Cuts shape somebody left behind. + """ + if not rings: + return [], [], [] + ordered = sorted(rings, key=ring_area, reverse=True) + perimeter, rest = ordered[0], ordered[1:] + cutouts, islands = [], [] + for ring in rest: + # Any vertex will do to place a ring: rings on this layer do not cross each other, so + # one point inside the perimeter means the whole ring is. + inside = any(point_in_ring(p, perimeter) for p in ring[:-1]) + (cutouts if inside else islands).append(ring) + return perimeter, cutouts, islands diff --git a/src/pinside/kicad/library.py b/src/pinside/kicad/library.py index 548ab73..cb7c7c1 100644 --- a/src/pinside/kicad/library.py +++ b/src/pinside/kicad/library.py @@ -12,8 +12,13 @@ from __future__ import annotations import os +import re from pathlib import Path +# A symbol that inherits its pins and graphics from another: KiCad writes the Pico W, and every +# stacked-flash RP2354, this way. +_EXTENDS = re.compile(r'\(extends\s+"([^"]+)"\)') + # Where KiCad keeps its symbols, newest first. $KICAD_SYMBOL_DIR overrides the lot. _CANDIDATES = [ "/Applications/KiCad/KiCad.app/Contents/SharedSupport/symbols", @@ -63,11 +68,72 @@ def _matching(text: str, start: int) -> int: raise LibraryError("unbalanced parentheses in a symbol library") +def _find_block(text: str, name: str, library: str) -> str: + """The top-level `(symbol "name" ...)` definition in a library's text.""" + needle = f'(symbol "{name}"' + index = text.find(needle) + while index != -1: + # Only a definition at the top level of the library, not a unit nested inside one. + if text.count("(", 0, index) - text.count(")", 0, index) == 1: + return text[index : _matching(text, index)] + index = text.find(needle, index + 1) + raise LibraryError(f"symbol {name!r} not found in {library}") + + +def _children(block: str, opener: str) -> list[tuple[int, int]]: + """Spans of the direct children of `block` that start with `opener`, e.g. `(property `.""" + spans = [] + index = block.find(opener, 1) + while index != -1: + if block.count("(", 0, index) - block.count(")", 0, index) == 1: + end = _matching(block, index) + spans.append((index, end)) + index = block.find(opener, end) + continue + index = block.find(opener, index + 1) + return spans + + +def _flatten(derived: str, parent: str, derived_name: str, parent_name: str) -> str: + """Fold a derived symbol onto its parent. + + The result is built from the *parent*, not from the derived symbol, and that direction + matters. A derived symbol in KiCad's format carries only what it overrides: its properties, + and nothing else. Splicing the parent's units into it produces a symbol with pins and + without `pin_names`, `in_bom`, `on_board` and the rest, which KiCad will not load at all. + + So: take the parent whole, drop the properties it defines, and put the derived symbol's + properties in their place. Everything is spliced as text, so bare tokens stay bare. + """ + props = [derived[a:b] for a, b in _children(derived, "(property ")] + spans = _children(parent, "(property ") + if not spans: + raise LibraryError("a parent symbol carries no properties to override") + + # Rebuild the parent with its property block replaced by the derived one, in place, so the + # rest of the definition keeps its original order and indentation. + first, last = spans[0][0], spans[-1][1] + merged = parent[:first] + "\n\t\t".join(props) + parent[last:] + + # The units carry the parent's name -- `(symbol "RaspberryPi_Pico_1_1" ...)` -- and KiCad + # matches them to their enclosing symbol by that prefix. Leave them and it loads a symbol + # with no units at all: not an error message, just "Failed to load schematic". + merged = merged.replace(f'(symbol "{parent_name}_', f'(symbol "{derived_name}_') + return merged.replace(f'(symbol "{parent_name}"', f'(symbol "{derived_name}"', 1) + + def load_symbol(lib_id: str, search: Path | None = None) -> str: """The raw text of `Library:Symbol`, renamed and ready to splice into a lib_symbols block. Text rather than a parse tree on purpose: KiCad's format distinguishes bare tokens from quoted strings, and a parse loses that. Copying the library's own bytes cannot. + + A derived symbol -- `(symbol "RaspberryPi_Pico_W" (extends "RaspberryPi_Pico"))` -- is + flattened against its parent. A schematic's `lib_symbols` has to stand on its own, because + that block is what lets the file open on a machine without the library, and `extends` + pointing at a symbol that is not in it resolves to nothing. Copied through unflattened, the + Pico W arrives with zero pins: the schematic opens, the controller has nothing to wire to, + and every net in the design is isolated. """ if ":" not in lib_id: raise LibraryError(f"{lib_id!r} is not a Library:Symbol identifier") @@ -77,16 +143,23 @@ def load_symbol(lib_id: str, search: Path | None = None) -> str: raise LibraryError(f"symbol library {library!r} not found at {path}") text = path.read_text(encoding="utf-8") - needle = f'(symbol "{name}"' - index = text.find(needle) - while index != -1: - # Only a definition at the top level of the library, not a unit nested inside one. - if text.count("(", 0, index) - text.count(")", 0, index) == 1: - block = text[index : _matching(text, index)] - # The embedded copy is keyed by its full Library:Symbol name. - return block.replace(f'(symbol "{name}"', f'(symbol "{lib_id}"', 1) - index = text.find(needle, index + 1) - raise LibraryError(f"symbol {name!r} not found in {library}") + block = _find_block(text, name, library) + + match = _EXTENDS.search(block) + if match: + parent = match.group(1) + parent_block = _find_block(text, parent, library) + if _EXTENDS.search(parent_block): + # KiCad's own libraries derive from root symbols only, never in a chain. Refusing + # is better than following one badly and emitting a symbol short of half its pins. + raise LibraryError( + f"symbol {name!r} extends {parent!r}, which is itself derived; " + "pinside flattens one level only" + ) + block = _flatten(block, parent_block, name, parent) + + # The embedded copy is keyed by its full Library:Symbol name. + return block.replace(f'(symbol "{name}"', f'(symbol "{lib_id}"', 1) def symbol_pins(definition: str) -> dict[str, str]: diff --git a/src/pinside/kicad/pcb.py b/src/pinside/kicad/pcb.py index 9bd2953..531c4d6 100644 --- a/src/pinside/kicad/pcb.py +++ b/src/pinside/kicad/pcb.py @@ -17,10 +17,14 @@ from ..pogo import Probe from .footprint import mounting_hole_shape, pogo_shape from .schematic import channel_slots -from .write import Node, Raw, at, document, effects, num, uid, uid_node +from .write import NO, YES, Node, Raw, at, document, effects, num, uid, uid_node, xy PCB_VERSION = "20260206" +# How far the ground pour is held back from the board edge. 0.5 mm clears the copper-to-edge +# minimum of every fab worth using, and a fixture is not tight for space. +GROUND_POUR_INSET_MM = 0.5 + _LAYERS = [ (0, "F.Cu", "signal", None), (2, "B.Cu", "signal", None), @@ -70,6 +74,57 @@ def outline(self, segments) -> None: ) ) + def pour(self, polygon, layers: list[str], net_name: str = "GND") -> None: + """A filled zone on the given layers, tied to a net. + + Routing is not generated here and deliberately so, but a ground pour is not routing. It + is one fixed shape -- the board outline, pulled in by the fab's clearance -- and on a + fixture it is most of the return path: the thing every probed signal is measured + against. Leaving it out means the first thing anyone does after opening the project is + draw the same rectangle by hand. + + The zone is emitted unfilled. KiCad fills it on the first `Edit > Fill All Zones`, and + writing a fill polygon here would mean recomputing the fab's clearance rules, badly. + """ + pts = Node("pts") + for x, y in polygon: + pts.add(xy(x, y)) + + zone = Node( + "zone", + Node("net", Raw(str(self.net(net_name)))), + Node("net_name", net_name), + ) + if len(layers) == 1: + zone.add(Node("layer", layers[0])) + else: + layer_node = Node("layers") + for name in layers: + layer_node.add(name) + zone.add(layer_node) + zone.add( + uid_node(self.project, f"zone.{net_name}.{'.'.join(layers)}"), + Node("name", f"{net_name} pour"), + Node("hatch", Raw("edge"), num(0.5)), + Node( + "connect_pads", + # Thermal reliefs, not solid: a fixture gets hand-reworked when a pin wears out, + # and a ground pad soldered straight into a full pour cannot be desoldered + # without lifting it. + Node("clearance", num(0.5)), + ), + Node("min_thickness", num(0.25)), + Node("filled_areas_thickness", NO), + Node( + "fill", + YES, + Node("thermal_gap", num(0.5)), + Node("thermal_bridge_width", num(0.5)), + ), + Node("polygon", pts), + ) + self.items.append(zone) + def text( self, message: str, x: float, y: float, size: float = 1.5, layer: str = "F.SilkS" ) -> None: @@ -198,6 +253,11 @@ def build(config: FixtureConfig, board: Board | None, probe: Probe) -> str: box = board.outline.bbox if box: + # A pour on both copper layers, pulled in from the edge. The inset is the fab's + # usual copper-to-edge minimum with room to spare: a zone drawn to the outline gets + # clipped anyway, and one drawn past it gets flagged by DRC. + layout.pour(_pour_polygon(board, GROUND_POUR_INSET_MM), ["F.Cu", "B.Cu"]) + layout.text(f"{config.name}", 2, -6, 2.5) layout.text( f"probes {placed} | {probe.receptacle} | DUT {config.dut_board}", 2, -2, 1.2 @@ -205,6 +265,30 @@ def build(config: FixtureConfig, board: Board | None, probe: Probe) -> str: return layout.render() +def _pour_polygon(board: Board, inset: float) -> list[tuple[float, float]]: + """The rectangle the ground pour fills: the board's extent, pulled in on every side. + + The perimeter's true shape is not used. A pour follows the outline once KiCad fills it -- + copper stops at the board edge whatever the zone says -- so the honest thing to draw is the + simplest boundary that cannot poke outside, which is the bounding box less the inset. Trying + to offset an arbitrary polygon inwards is a real geometry problem and this does not need it. + """ + box = board.outline.bbox + if box is None: + return [] + frame = board.frame or {} + ox, oy = frame.get("offset", [0.0, 0.0]) + # The fixture frame puts the outline's corner at the origin, so the pour is measured from + # there rather than from the DUT's page coordinates. + left = box.min_x - ox + inset + top = box.min_y - oy + inset + right = box.min_x - ox + box.width - inset + bottom = box.min_y - oy + box.height - inset + if right <= left or bottom <= top: + return [] # a board smaller than twice the inset; there is nothing to pour + return [(left, top), (right, top), (right, bottom), (left, bottom)] + + def _probe_map(config: FixtureConfig) -> dict[str, str]: """channel-or-bus-role name -> the DUT signal it probes.""" mapping: dict[str, str] = {} diff --git a/src/pinside/kicad/project.py b/src/pinside/kicad/project.py index 1feea12..e8246c3 100644 --- a/src/pinside/kicad/project.py +++ b/src/pinside/kicad/project.py @@ -22,6 +22,7 @@ from . import schematic as sch_builder from .footprint import mounting_hole_name, mounting_hole_shape, pogo_shape from .library import LibraryError +from .pcb import GROUND_POUR_INSET_MM class ProjectError(Exception): @@ -109,6 +110,60 @@ def _fp_lib_table() -> str: ) +def _hardware_note(probes: int, probe, holes: int) -> str: + """What the plate force means for the hardware that has to carry it. + + The number itself was already in the generated README and nobody could act on it. Newtons + are not what anyone buys; what they need to know is whether a hand clamp will do, how many + standoffs are carrying the load, and how much each one takes. + """ + newtons = probes * probe.force_n + kgf = newtons / 9.81 + per_hole = newtons / holes if holes else 0.0 + + if kgf < 2: + verdict = ( + "A hand clamp or a couple of thumbscrews will close this. The load is small enough " + "that the plate's own stiffness is what decides whether contact is even." + ) + elif kgf < 10: + verdict = ( + "This is past comfortable finger pressure. Use a toggle clamp or screws; a plate " + "held down by hand will make intermittent contact on whichever probe is furthest " + "from where you are pressing." + ) + else: + verdict = ( + "This needs a lever or pneumatic clamp, and the plate needs to be stiff enough not " + "to bow between its supports. At this load a 1.6 mm FR4 plate deflects visibly and " + "the probes in the middle stop reaching." + ) + + lines = [ + f"{probes} probes at {probe.force_n} N each is **{newtons:.0f} N** " + f"({kgf:.1f} kgf) at full travel.", + "", + verdict, + "", + ] + if holes: + lines += [ + f"That load is carried by the DUT's {holes} mounting holes, so roughly " + f"**{per_hole:.0f} N ({per_hole / 9.81:.1f} kgf) per standoff**. Use a standoff " + f"rated well above that: they are specified for axial load, and the number that " + f"matters here is the compressive one, not the thread's tensile rating.", + "", + f"Allow at least {probe.travel_mm} mm of probe travel plus the DUT's thickness in " + "the standoff length, or the plate bottoms out before the pins compress.", + ] + else: + lines += [ + "The DUT has no mounting holes, so there is nowhere to put a standoff. The plate " + "has to be located some other way, and whatever locates it carries all of this.", + ] + return "\n".join(lines) + + def _readme( config: FixtureConfig, board: Board | None, result_probes: int, unplaced: list[str] ) -> str: @@ -125,6 +180,17 @@ def _readme( f"add the MCU, its supply, crystal and USB, then wire the `FIX_*` nets.\n" ) + provenance = ( + "These dimensions were transcribed from a catalogue and nobody has checked them against " + "the supplier's own drawing. **Do that before you order**: series get revised, and a " + "receptacle 0.1 mm fatter than this expects is a board that has to be redrilled." + if not probe.verified + else f"Checked against the supplier drawing: {probe.verified}." + ) + inset = GROUND_POUR_INSET_MM + hole_count = len(board.mounting_holes) if board else 0 + hardware = _hardware_note(result_probes, probe, hole_count) + unplaced_note = "" if unplaced: unplaced_note = ( @@ -138,13 +204,13 @@ def _readme( Generated by pinside {__version__} on {datetime.now(timezone.utc).strftime("%Y-%m-%d")} from `{Path(config.source).name if config.source else "the fixture config"}`. -**Regenerating overwrites this project.** Change the config, not the board — except for routing, +**Regenerating overwrites this project.** Change the config, not the board, except for routing, which pinside does not generate and will not overwrite once you have done it. If you intend to route it, copy the project somewhere else first. ## What is already right -The part that has to be exact is the placement, and it is not laid out here at all — it is read +The part that has to be exact is the placement, and it is not laid out here at all: it is read out of the DUT and carried through the fixture transform: - **{result_probes} probes**, each at its DUT test point's own coordinates. @@ -157,7 +223,8 @@ def _readme( - **Routing.** A ratsnest and an accurate drill plan are the useful part; guessing trace paths is not. Autoroute it or draw it. - **Placing the controller and any connectors.** -- **A ground pour**, which is most of the return path on a fixture. +- **Filling the zones.** A GND pour is already drawn on both copper layers, held + {inset:g} mm back from the edge. KiCad fills it on `Edit > Fill All Zones`. ## The probe @@ -173,20 +240,24 @@ def _readme( | Travel | {probe.travel_mm} mm | | Force | {probe.force_n} N per pin at mid-travel | | Mounting | {probe.mounting} | +| Source | {probe.source or "not recorded"} | +| Checked against the drawing | {probe.verified or "**no**"} | -With {result_probes} probes the plate needs roughly -**{result_probes * probe.force_n:.0f} N** ({result_probes * probe.force_n / 9.81:.1f} kgf) to -close. That is what the standoffs and the clamp have to carry. +{provenance} The footprint is generated into `pinside.pretty/`, and `fp-lib-table` already points at it. +## Closing force + +{hardware} + ## The controller {controller} ## Mirroring `mirror = {frame.get("mirror", "none")}`. A fixture whose probes point up takes the DUT face-down, -which mirrors X. Get this wrong and the board is a perfect mirror image of the one you need — so +which mirrors X. Get this wrong and the board is a perfect mirror image of the one you need, so before ordering, print `pinside check -f svg` at 1:1 and lay the real board on it. {unplaced_note}""" @@ -202,7 +273,13 @@ def generate_project( # until it comes back from the fab. The probe sets the spacing limit, so choosing a finer # pin relaxes it without a second edit. if board is not None: - findings = findings + check_board(board, Limits(probe_pitch=config.probe_part.min_pitch_mm)) + # The probe the config chose is what the board is being checked against, so its own + # dimensions set the limits rather than the defaults. + probe = config.probe_part + findings = findings + check_board( + board, + Limits(probe_pitch=probe.min_pitch_mm, probe_body=probe.body_dia_mm), + ) if any(f.severity == ERROR for f in findings): raise ProjectError(findings) diff --git a/src/pinside/pogo.py b/src/pinside/pogo.py index d2dc645..fcfa4af 100644 --- a/src/pinside/pogo.py +++ b/src/pinside/pogo.py @@ -36,6 +36,12 @@ class Probe: tip: str mounting: str = "press-fit" + # Where the numbers above came from, and whether anyone has checked them against it. These + # end up as drill sizes on a board somebody orders, and a receptacle 0.1 mm fatter than + # expected is a board that has to be redrilled, so the provenance travels with the values. + source: str = "" + verified: str = "" # who checked it against the drawing, and when; empty means nobody has + @property def footprint_name(self) -> str: return f"PogoPin_{self.name}" @@ -62,6 +68,7 @@ def summary(self) -> str: travel_mm=2.5, force_n=0.75, tip="crown -- bites through the light oxide on a bare copper or HASL pad", + source="Mill-Max catalogue, 0985 receptacle and 0900 spring pin series", ), # For a board whose test pads were not laid out with a fixture in mind, and which therefore # sit closer together than 2.54 mm. @@ -77,6 +84,7 @@ def summary(self) -> str: travel_mm=1.8, force_n=0.55, tip="crown", + source="Mill-Max catalogue, 0906 receptacle and 0850 spring pin series", ), # No receptacle: the pin is soldered straight into the board. Cheaper and lower profile, at # the cost of a soldering iron every time a pin wears out. @@ -93,12 +101,25 @@ def summary(self) -> str: force_n=0.70, tip="crown", mounting="soldered", + source="generic P75-series spring pin; no single supplier drawing, so these are " + "the values common to the series rather than any one part", ), } DEFAULT = "millmax_0985" +def unverified() -> list[Probe]: + """Probes whose dimensions nobody has checked against the supplier's own drawing. + + Every entry here was transcribed from a catalogue. That is enough to lay a board out and not + enough to order one on: series get revised, and the failure is silent until the boards + arrive. Filling in `verified` is a deliberate act by someone who put a drawing next to the + numbers, which is why it starts empty rather than defaulting to true. + """ + return [p for p in PROBES.values() if not p.verified] + + def get(name: str | None = None) -> Probe: key = (name or DEFAULT).lower() try: diff --git a/src/pinside/targets.py b/src/pinside/targets.py index 3aec48a..e6d3209 100644 --- a/src/pinside/targets.py +++ b/src/pinside/targets.py @@ -97,6 +97,21 @@ def _rp2350_adc(first_gpio: int, channels: int) -> dict[int, int]: gpio_count=48, adc_pins=_rp2350_adc(40, 8), ), + "rp2354a": Target( + name="rp2354a", + description="Raspberry Pi RP2354A, QFN-60 with 2 MB stacked flash; RP2350A pinout", + gpio_count=30, + adc_pins=_rp2350_adc(26, 4), + ), +} + +# The stacked-flash parts are the plain ones with flash in the package: same die, same pinout, +# same peripherals. This records that rather than leaving two copies of a pin map to drift, and +# tests/test_kicad.py checks it against KiCad's own library, which spells the same relationship +# as `(symbol "RP2354A" (extends "RP2350A"))`. +SAME_PINOUT = { + "rp2354a": "rp2350a", + "rp2354b": "rp2350b", } diff --git a/tests/boards.py b/tests/boards.py index fea64db..00b9640 100644 --- a/tests/boards.py +++ b/tests/boards.py @@ -4,6 +4,7 @@ import atexit import itertools +import re import shutil import tempfile from pathlib import Path @@ -66,8 +67,103 @@ def _part(ref: str, x: float, y: float, w: float = 4.0, h: float = 2.0) -> str: )''' +def _part_with_nets(ref: str, x: float, y: float, nets: list[str]) -> str: + """A component whose pads carry named nets, for the coverage checks.""" + pads = "\n".join( + f' (pad "{i}" smd rect (at {i * 1.5 - 3} 0) (size 0.6 1.2) ' + f'(layers "F.Cu") (net {i + 10} "{net}"))' + for i, net in enumerate(nets, start=1) + ) + return f''' + (footprint "Package_SO:SOIC-8" + (layer "F.Cu") + (at {x} {y}) + (property "Reference" "{ref}") + (property "Value" "U") +{pads} + )''' + + def _wrap(body: str) -> str: - return f'(kicad_pcb (version 20241229) (generator "pinside-tests"){body}\n)\n' + """Close a board, giving every net its own ordinal and declaring them all. + + The helpers above each write `(net 1 "NAME")`, because a helper cannot know what the rest of + the board is using. Left that way every net in the board shares ordinal 1, which is a file + KiCad reads as a single net and re-saves with fifteen test points connected to nothing -- + the defect PS043 exists to catch, and the one the shipped example board had. Numbering + happens here because here is the only place that sees the whole board. + """ + names = [] + for _, name in _NET_REF.findall(body): + if name and name not in names: + names.append(name) + ordinals = {name: i for i, name in enumerate(names, start=1)} + body = _NET_REF.sub( + lambda m: f'(net {ordinals[m.group(2)]} "{m.group(2)}")' if m.group(2) else m.group(0), + body, + ) + table = "".join(f'\n (net {ordinals[n]} "{n}")' for n in names) + return f'(kicad_pcb (version 20241229) (generator "pinside-tests"){table}{body}\n)\n' + + +def without(text: str, *refs: str) -> str: + """The same board with these footprints removed, by reference. + + Tests used to do this by rebuilding a helper's output and string-replacing it away, which + only worked while the helper's text was byte-identical to what landed in the board. It is + not: `_wrap` renumbers the nets, so the reconstructed footprint carries a different ordinal + and the replace silently matches nothing, leaving the test asserting against a board it did + not build. + """ + wanted = {f'(property "Reference" "{ref}")' for ref in refs} + out = [] + i = 0 + while True: + start = text.find("\n (footprint ", i) + if start == -1: + out.append(text[i:]) + return "".join(out) + depth, j = 0, text.index("(", start) + while True: + if text[j] == "(": + depth += 1 + elif text[j] == ")": + depth -= 1 + if depth == 0: + break + j += 1 + block = text[start : j + 1] + out.append(text[i:start]) + if not any(marker in block for marker in wanted): + out.append(block) + i = j + 1 + + +def sharing_one_net_ordinal(text: str) -> str: + """Put the whole board back on net 1, the way a hand-written .kicad_pcb tends to be.""" + return _NET_REF.sub(lambda m: f'(net 1 "{m.group(2)}")' if m.group(2) else m.group(0), text) + + +def on_net_zero(text: str, name: str) -> str: + """Move one named net onto ordinal 0, KiCad's no-connection net.""" + return re.sub(rf'\(net \d+ "{re.escape(name)}"\)', f'(net 0 "{name}")', text) + + +_NET_ORDINAL = re.compile(r'\(net \d+ ("[^"]*")\)') + +# `(net "NAME")`, with both halves captured. +_NET_REF = re.compile(r'\(net (\d+) "([^"]*)"\)') + + +def as_kicad10(text: str) -> str: + """Rewrite a board the way KiCad 10 writes it. + + KiCad 9 and earlier number the nets in the file, `(net 3 "GND")`; KiCad 10 dropped the + ordinal. Any board helper here can be run through this to get the other form, so both + branches of ``board._net_of`` are exercised by the same expectations. + """ + text = text.replace("(version 20241229)", "(version 20260206)") + return _NET_ORDINAL.sub(r"(net \1)", text) def rect_outline(x1=0.0, y1=0.0, x2=50.0, y2=40.0, radius=0.0) -> str: @@ -86,6 +182,55 @@ def segment_outline(x1=0.0, y1=0.0, x2=50.0, y2=40.0, gap: bool = False) -> str: (gr_line (start {x1} {y2}) (end {last_x} {y1}) (layer "Edge.Cuts"))""" +def slotted() -> str: + """A perfectly good board with a slot milled through it. + + Two closed shapes on Edge.Cuts: the perimeter and the window. A single-ring walk calls this + an unclosed outline, which is a hard error on a board a fab would cut without complaint. + """ + body = rect_outline(0, 0, 50, 40) + body += rect_outline(20, 15, 30, 25) # the window + body += _testpoint("TP1", 10, 10, "/SCL") + body += _testpoint("TP2", 14, 10, "/SDA") + body += _testpoint("TP90", 10, 20, "GND", value="GND") + body += _testpoint("TP91", 14, 20, "GND", value="GND") + for i, (x, y) in enumerate([(5, 5), (45, 5), (5, 35), (45, 35)], start=1): + body += _hole(f"H{i}", x, y, net="GND") + return _wrap(body) + + +def probe_over_a_slot() -> str: + """The slotted board with TP2 moved into the middle of the window.""" + return slotted().replace("(at 14 10)", "(at 25 20)", 1) + + +def panelised() -> str: + """Two separate board outlines on one Edge.Cuts layer.""" + body = rect_outline(0, 0, 50, 40) + body += rect_outline(60, 0, 110, 40) + body += _testpoint("TP1", 10, 10, "/SCL") + body += _testpoint("TP90", 10, 20, "GND", value="GND") + body += _testpoint("TP91", 14, 20, "GND", value="GND") + for i, (x, y) in enumerate([(5, 5), (45, 5), (5, 35), (45, 35)], start=1): + body += _hole(f"H{i}", x, y, net="GND") + return _wrap(body) + + +def unreachable_rails() -> str: + """Probes on the data lines and on nothing that powers or resets the board.""" + body = rect_outline() + body += _testpoint("TP1", 10, 10, "/SCL") + body += _testpoint("TP2", 14, 10, "/SDA") + body += _testpoint("TP90", 10, 20, "GND", value="GND") + body += _testpoint("TP91", 14, 20, "GND", value="GND") + # The rails and the reset line exist on the board, on a component's pads, and nothing + # probes them. + body += _part_with_nets("U1", 30, 20, ["+3V3", "+1V8", "/MCU_NRST", "/SCL"]) + for i, (x, y) in enumerate([(5, 5), (45, 5), (5, 35), (45, 35)], start=1): + body += _hole(f"H{i}", x, y, net="GND") + return _wrap(body) + + def healthy() -> str: """A board a fixture can actually be built against.""" body = rect_outline() diff --git a/tests/test_baseline.py b/tests/test_baseline.py new file mode 100644 index 0000000..674d20e --- /dev/null +++ b/tests/test_baseline.py @@ -0,0 +1,186 @@ +"""The baseline file: findings a board has already been judged on. + +The property worth testing is the one that makes a baseline safe to check in. `--ignore PS041` +silences a code forever; a baseline silences one occurrence of it. If the second thing quietly +behaved like the first, a board could pick up a new defect under an existing suppression and CI +would stay green. +""" + +from __future__ import annotations + +import contextlib +import io +import json +import sys +import tempfile +import unittest +from pathlib import Path + +_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(_ROOT / "src")) +sys.path.insert(0, str(_ROOT / "tests")) + +import boards +from boards import write as write_board +from pinside import baseline, read_board, run, transform +from pinside.checks import Finding +from pinside.cli import main + + +def cli(args: list[str]) -> int: + with contextlib.redirect_stderr(io.StringIO()): + return main(args) + + +def scratch(name: str) -> str: + return str(Path(tempfile.mkdtemp(prefix="pinside-baseline-")) / name) + + +class Matching(unittest.TestCase): + def test_an_entry_with_no_refs_covers_the_whole_code(self): + entry = baseline.Entry(code="PS041") + self.assertTrue(entry.matches(Finding("PS041", "info", "x", ["TP1"]))) + self.assertTrue(entry.matches(Finding("PS041", "info", "x", []))) + self.assertFalse(entry.matches(Finding("PS042", "info", "x", []))) + + def test_an_entry_with_refs_covers_only_those(self): + entry = baseline.Entry(code="PS041", refs={"TP5"}) + self.assertTrue(entry.matches(Finding("PS041", "info", "x", ["TP5"]))) + self.assertFalse(entry.matches(Finding("PS041", "info", "x", ["TP9"]))) + + def test_a_new_reference_brings_the_finding_back(self): + # The whole point. TP5 was accepted; TP5 and TP9 together were not. + entry = baseline.Entry(code="PS041", refs={"TP5"}) + self.assertFalse(entry.matches(Finding("PS041", "info", "x", ["TP5", "TP9"]))) + + def test_a_finding_that_lost_a_reference_stays_accepted(self): + # Fixing half of an accepted finding should not make the other half fail: the refs that + # remain are a subset of what was already judged. + entry = baseline.Entry(code="PS041", refs={"TP5", "TP9"}) + self.assertTrue(entry.matches(Finding("PS041", "info", "x", ["TP5"]))) + + +class RoundTrip(unittest.TestCase): + def setUp(self): + self.board = write_board(boards.troubled()) + self.path = scratch("baseline.json") + + def test_a_written_baseline_accepts_the_board_it_came_from(self): + self.assertEqual( + cli([self.board, "--write-baseline", self.path, "-f", "json", "-o", "/dev/null"]), 0 + ) + # troubled() has errors, so without the baseline this is exit 2. + self.assertEqual(cli([self.board, "-f", "json", "-o", "/dev/null"]), 2) + self.assertEqual( + cli([self.board, "--baseline", self.path, "-f", "json", "-o", "/dev/null"]), 0 + ) + + def test_every_entry_has_an_empty_note_to_fill_in(self): + cli([self.board, "--write-baseline", self.path, "-f", "json", "-o", "/dev/null"]) + data = json.loads(Path(self.path).read_text()) + self.assertTrue(data["accepted"]) + self.assertTrue(all(e["note"] == "" for e in data["accepted"])) + self.assertEqual(data["version"], baseline.VERSION) + + def test_the_baseline_records_which_board_it_was_taken_from(self): + cli([self.board, "--write-baseline", self.path, "-f", "json", "-o", "/dev/null"]) + self.assertEqual(json.loads(Path(self.path).read_text())["board"], self.board) + + def test_a_baseline_does_not_hide_a_new_finding(self): + cli([self.board, "--write-baseline", self.path, "-f", "json", "-o", "/dev/null"]) + # The same board with one more probe stacked on an existing one: PS020's refs change, + # so the accepted entry no longer covers it. + worse = boards.troubled().replace( + "\n)\n", boards._testpoint("TP11", 10, 10, "/ALSO_DUP") + "\n)\n" + ) + self.assertEqual( + cli([write_board(worse), "--baseline", self.path, "-f", "json", "-o", "/dev/null"]), + 2, + ) + + +class BadFiles(unittest.TestCase): + def test_a_missing_baseline_is_a_usage_error_not_a_silent_pass(self): + board = write_board(boards.healthy()) + self.assertEqual( + cli([board, "--baseline", "/nonexistent/b.json", "-f", "json", "-o", "/dev/null"]), 3 + ) + + def test_a_future_version_is_refused(self): + path = scratch("future.json") + Path(path).write_text(json.dumps({"version": 99, "accepted": []})) + with self.assertRaises(baseline.BaselineError): + baseline.load(path) + + def test_malformed_json_is_refused(self): + path = scratch("bad.json") + Path(path).write_text("{not json") + with self.assertRaises(baseline.BaselineError): + baseline.load(path) + + def test_an_entry_without_a_code_is_refused(self): + path = scratch("nocode.json") + Path(path).write_text(json.dumps({"version": 1, "accepted": [{"refs": ["TP1"]}]})) + with self.assertRaises(baseline.BaselineError): + baseline.load(path) + + +class JsonFindings(unittest.TestCase): + """generate and project write their output to a directory and their findings to stderr, + which leaves a caller nothing to parse. --json puts the findings on stdout.""" + + def _run(self, args: list[str]) -> tuple[int, dict]: + out, err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err): + status = main(args) + return status, json.loads(out.getvalue()) + + def test_generate_dry_run_reports_json(self): + board = write_board(boards.uart_board()) + cfg = scratch("fixture.json") + self.assertEqual(cli(["init", board, "-o", cfg]), 0) + status, payload = self._run(["generate", cfg, "--dry-run", "--json"]) + self.assertEqual(status, 0) + self.assertIn("findings", payload) + self.assertEqual(payload["errors"], 0) + self.assertFalse(payload["written"]) + + def test_a_refusal_still_reports_json(self): + # A config that cannot generate must still be machine-readable: a caller that only + # parses the success path has to fall back to scraping stderr exactly when it matters. + board = write_board(boards.uart_board()) + cfg = scratch("fixture.json") + self.assertEqual(cli(["init", board, "-o", cfg]), 0) + data = json.loads(Path(cfg).read_text()) + data["gpio"] = [{"name": "nope", "pin": 999, "probe": "PWR_FLT"}] + Path(cfg).write_text(json.dumps(data)) + + status, payload = self._run(["generate", cfg, "--dry-run", "--json"]) + self.assertEqual(status, 2) + self.assertGreater(payload["errors"], 0) + self.assertTrue(any(f["severity"] == "error" for f in payload["findings"])) + + def test_generate_writes_and_reports_what_it_wrote(self): + board = write_board(boards.uart_board()) + cfg = scratch("fixture.json") + self.assertEqual(cli(["init", board, "-o", cfg]), 0) + out = scratch("firmware") + status, payload = self._run(["generate", cfg, "--out", out, "--json"]) + self.assertEqual(status, 0) + self.assertTrue(payload["written"]) + self.assertTrue(payload["config_hash"]) + self.assertIn("CMakeLists.txt", " ".join(payload["files"])) + + +class Splitting(unittest.TestCase): + def test_split_partitions_every_finding(self): + board = transform(read_board(write_board(boards.troubled()))) + findings = run(board) + accepted = baseline.from_findings(findings[:3]) + kept, suppressed = accepted.split(findings) + self.assertEqual(len(kept) + len(suppressed), len(findings)) + self.assertEqual(len(suppressed), 3) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_client.py b/tests/test_client.py new file mode 100644 index 0000000..fa2ad04 --- /dev/null +++ b/tests/test_client.py @@ -0,0 +1,370 @@ +"""The host client, driven against a fake fixture. + +No hardware and no pyserial: `connect()` takes an `opener`, so these tests hand it a port that +behaves the way the generated firmware does, including the parts that make a naive client wrong. +The firmware's own behaviour is verified by tests/test_firmware.py compiling and running it; what +is checked here is that the host half copes with it. +""" + +from __future__ import annotations + +import contextlib +import io +import json +import sys +import unittest +from pathlib import Path + +_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(_ROOT / "src")) +sys.path.insert(0, str(_ROOT / "tests")) + +from pinside import client +from pinside.cli import main +from pinside.config import load +from pinside.firmware.generate import config_hash + +_DEMO = _ROOT / "examples" / "demo-fixture.json" + + +class FakePort: + """A serial port that answers like the generated firmware. + + `lines` is a list of extra raw lines to emit before the next response, which is how the + awkward cases get reproduced: boot chatter, a notification arriving mid-exchange, a partial + line left over from before the port was opened. + """ + + def __init__(self, hash_="", *, interleave=None, fail_methods=(), name="demo-fixture"): + self.hash = hash_ + self.name = name + self.sent: list[dict] = [] + self.pending: list[bytes] = [] + self.interleave = list(interleave or []) + self.fail_methods = set(fail_methods) + self.closed = False + self.timeout = 5.0 + + # -- the pyserial surface the client uses -------------------------------- + + def write(self, data: bytes) -> int: + request = json.loads(data.decode()) + self.sent.append(request) + for extra in self.interleave: + self.pending.append(extra if isinstance(extra, bytes) else extra.encode()) + self.interleave = [] + self.pending.append(json.dumps(self._respond(request)).encode() + b"\n") + return len(data) + + def flush(self) -> None: + pass + + def readline(self) -> bytes: + return self.pending.pop(0) if self.pending else b"" + + def close(self) -> None: + self.closed = True + + # -- the fixture's side -------------------------------------------------- + + def _respond(self, request: dict) -> dict: + method = request.get("method") + rid = request.get("id") + if method in self.fail_methods: + return { + "jsonrpc": "2.0", + "id": rid, + "error": {"code": -32003, "message": "guard not asserted", "data": "esp_en"}, + } + results = { + "fixture.info": { + "fixture": self.name, + "version": "0.1.0", + "config_hash": self.hash, + }, + "fixture.channels": [ + {"name": "dut_uart", "kind": "uart", "probe": "DUT_TXD"}, + {"name": "dut_3v3", "kind": "adc", "probe": "+3.3V"}, + ], + "adc.snapshot": [ + {"channel": "dut_3v3", "probe": "+3.3V", "millivolts": 3298, "in_range": True} + ], + "gpio.snapshot": [{"channel": "dut_reset", "value": False}], + "spi.transfer": {"hex": "aabb"}, + "i2c.scan": {"addresses": [0x50]}, + "uart.read": {"hex": "6f6b"}, + } + return {"jsonrpc": "2.0", "id": rid, "result": results.get(method, {})} + + +def opener_for(port: FakePort): + def opener(device, **kwargs): + return port + + return opener + + +def cli(args: list[str]) -> tuple[int, str, str]: + out, err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err): + status = main(args) + return status, out.getvalue(), err.getvalue() + + +class Handshake(unittest.TestCase): + def setUp(self): + self.config = load(str(_DEMO)) + self.hash = config_hash(self.config) + + def test_a_matching_fixture_connects(self): + port = FakePort(self.hash) + with client.connect("fake", self.config, opener=opener_for(port)) as fixture: + self.assertEqual(fixture.info["config_hash"], self.hash) + + def test_a_mismatched_fixture_is_refused(self): + # The whole reason the hash exists. Every later call would succeed against the wrong pin. + port = FakePort("deadbeefdead") + with self.assertRaises(client.ConfigMismatchError) as caught: + client.connect("fake", self.config, opener=opener_for(port)) + self.assertIn(self.hash, str(caught.exception)) + self.assertIn("deadbeefdead", str(caught.exception)) + + def test_a_refused_connection_closes_the_port(self): + port = FakePort("deadbeefdead") + with contextlib.suppress(client.ConfigMismatchError): + client.connect("fake", self.config, opener=opener_for(port)) + self.assertTrue(port.closed, "a refused fixture left its port open") + + def test_the_check_can_be_waived_deliberately(self): + port = FakePort("deadbeefdead") + fixture = client.connect("fake", self.config, check_hash=False, opener=opener_for(port)) + self.assertEqual(fixture.info["config_hash"], "deadbeefdead") + + def test_checking_without_a_config_is_a_usage_error(self): + port = FakePort(self.hash) + with self.assertRaises(client.FixtureError): + client.connect("fake", None, opener=opener_for(port)) + + +class Framing(unittest.TestCase): + """The parts of the wire that make a one-line-per-request client wrong.""" + + def setUp(self): + self.config = load(str(_DEMO)) + self.hash = config_hash(self.config) + + def test_a_notification_is_not_mistaken_for_a_response(self): + # A streaming UART pushes uart.data between replies. A client that reads one line and + # calls it the answer returns a log line as the result of an ADC read. + stream = json.dumps( + { + "jsonrpc": "2.0", + "method": "uart.data", + "params": {"channel": "dut_uart", "hex": "626f6f74", "t_ms": 12}, + } + ) + port = FakePort(self.hash, interleave=[stream + "\n"]) + fixture = client.connect("fake", self.config, opener=opener_for(port)) + rails = fixture.adc_snapshot() + self.assertEqual(rails[0]["channel"], "dut_3v3") + pushed = fixture.poll() + self.assertEqual(len(pushed), 1) + self.assertEqual(pushed[0].method, "uart.data") + self.assertEqual(pushed[0].data, b"boot") + self.assertEqual(pushed[0].channel, "dut_uart") + + def test_boot_chatter_is_skipped_rather_than_fatal(self): + # A board that was already running prints whatever it prints. None of it is JSON. + port = FakePort(self.hash, interleave=[b"MicroPython v1.2\r\n", b"\n", b"{partial"]) + fixture = client.connect("fake", self.config, opener=opener_for(port)) + self.assertEqual(fixture.channels()[0]["name"], "dut_uart") + + def test_an_rpc_error_is_raised_with_its_code_and_data(self): + port = FakePort(self.hash, fail_methods={"spi.transfer"}) + fixture = client.connect("fake", self.config, opener=opener_for(port)) + with self.assertRaises(client.RpcError) as caught: + fixture.spi_transfer("eth_spi", b"\x0f\x00") + self.assertEqual(caught.exception.code, -32003) + self.assertEqual(caught.exception.data, "esp_en") + self.assertIn("guard not asserted", str(caught.exception)) + + def test_a_silent_fixture_times_out_rather_than_hanging(self): + port = FakePort(self.hash) + fixture = client.connect("fake", self.config, opener=opener_for(port)) + port.pending.clear() + port.write = lambda data: len(data) # accepted, never answered + with self.assertRaises(client.FixtureError): + fixture.call("adc.snapshot", timeout=0.05) + + def test_an_echo_is_not_a_response(self): + """A device that echoes what it was sent must not look like it answered. + + The echoed request carries the very id the client is waiting for, so matching on the id + alone accepts it, unwraps a `result` that is not there, and reports a successful read of + None. Found by pointing the client at pyserial's own `loop://` port. + """ + port = FakePort(self.hash) + fixture = client.connect("fake", self.config, opener=opener_for(port)) + + echoing = json.dumps({"jsonrpc": "2.0", "id": 99, "method": "adc.snapshot"}) + port.interleave = [echoing + "\n"] + rails = fixture.adc_snapshot() + self.assertEqual(rails[0]["channel"], "dut_3v3") + + def test_a_message_that_is_neither_response_nor_notification_is_skipped(self): + port = FakePort(self.hash) + fixture = client.connect("fake", self.config, opener=opener_for(port)) + port.interleave = ['{"jsonrpc":"2.0","id":4}\n', '{"hello":"world"}\n'] + self.assertTrue(fixture.channels()) + + def test_request_ids_increase(self): + port = FakePort(self.hash) + fixture = client.connect("fake", self.config, opener=opener_for(port)) + fixture.channels() + fixture.adc_snapshot() + ids = [r["id"] for r in port.sent] + self.assertEqual(ids, sorted(ids)) + self.assertEqual(len(ids), len(set(ids))) + + +class TypedCalls(unittest.TestCase): + """The convenience wrappers, which exist so callers do not hand-roll hex every time.""" + + def setUp(self): + self.config = load(str(_DEMO)) + port = FakePort(config_hash(self.config)) + self.port = port + self.fixture = client.connect("fake", self.config, opener=opener_for(port)) + + def test_bytes_go_out_as_hex(self): + self.fixture.uart_write("dut_uart", b"ok\n") + self.assertEqual(self.port.sent[-1]["params"]["hex"], "6f6b0a") + + def test_hex_comes_back_as_bytes(self): + self.assertEqual(self.fixture.uart_read("dut_uart"), b"ok") + self.assertEqual(self.fixture.spi_transfer("eth_spi", b"\x00"), b"\xaa\xbb") + + def test_i2c_scan_returns_addresses(self): + self.assertEqual(self.fixture.i2c_scan("ext_i2c"), [0x50]) + + def test_gpio_write_sends_a_real_boolean(self): + self.fixture.gpio_write("dut_reset", 1) + self.assertIs(self.port.sent[-1]["params"]["value"], True) + + +class ProbeCommand(unittest.TestCase): + """`pinside probe`, the bench smoke test.""" + + def setUp(self): + self.config = load(str(_DEMO)) + self.hash = config_hash(self.config) + + def run_probe(self, port: FakePort, args=()): + real = client.connect + + def fake_connect(device, config=None, **kwargs): + kwargs.pop("opener", None) + return real(device, config, opener=opener_for(port), **kwargs) + + client.connect = fake_connect + try: + return cli(["probe", str(_DEMO), "--port", "fake", *args]) + finally: + client.connect = real + + def test_a_matching_fixture_passes(self): + status, out, _ = self.run_probe(FakePort(self.hash)) + self.assertEqual(status, 0) + self.assertIn("demo-fixture", out) + self.assertIn("dut_3v3", out) + + def test_a_mismatched_fixture_fails(self): + status, _, err = self.run_probe(FakePort("deadbeefdead")) + self.assertEqual(status, 2) + self.assertIn("config", err) + + def test_a_rail_out_of_range_fails(self): + port = FakePort(self.hash) + original = port._respond + + def low_rail(request): + reply = original(request) + if request.get("method") == "adc.snapshot": + reply["result"][0].update(millivolts=1100, in_range=False) + return reply + + port._respond = low_rail + status, _, err = self.run_probe(port) + self.assertEqual(status, 2) + self.assertIn("out of range", err) + + def test_json_output_is_parseable(self): + status, out, _ = self.run_probe(FakePort(self.hash), ["--json"]) + self.assertEqual(status, 0) + payload = json.loads(out) + self.assertEqual(payload["port"], "fake") + self.assertEqual(payload["out_of_range"], []) + self.assertTrue(payload["channels"]) + + +try: + import serial # noqa: F401 + + HAVE_PYSERIAL = True +except ImportError: + HAVE_PYSERIAL = False + + +@unittest.skipUnless(HAVE_PYSERIAL, "pyserial not installed (pip install 'pinside[client]')") +class AgainstRealPyserial(unittest.TestCase): + """The fake above stands in for a pyserial port. This checks the stand-in is honest. + + `loop://` is pyserial's own in-memory port: real Serial semantics, and it echoes. That makes + it the exact adversary for the id-matching bug, and it is where that bug was found. + """ + + def test_the_timeout_is_real_and_bounded(self): + import time + + import serial + + port = serial.serial_for_url("loop://", baudrate=115200, timeout=1) + self.addCleanup(port.close) + fixture = client.Fixture(port, timeout=0.3) + start = time.monotonic() + with self.assertRaises(client.FixtureError): + fixture.call("adc.snapshot") + elapsed = time.monotonic() - start + self.assertLess(elapsed, 2.0, "the timeout did not bound the wait") + + def test_an_echoing_port_never_looks_like_an_answer(self): + import serial + + port = serial.serial_for_url("loop://", baudrate=115200, timeout=1) + self.addCleanup(port.close) + fixture = client.Fixture(port, timeout=0.3) + with self.assertRaises(client.FixtureError): + fixture.call("fixture.info") + + +class WithoutPyserial(unittest.TestCase): + def test_the_missing_dependency_says_how_to_install_it(self): + real = client._require_serial + + def missing(): + raise client.FixtureError( + "talking to a fixture needs pyserial, which pinside does not install by " + "default. Install it with: pip install 'pinside[client]'" + ) + + client._require_serial = missing + try: + with self.assertRaises(client.FixtureError) as caught: + client.ports() + finally: + client._require_serial = real + self.assertIn("pinside[client]", str(caught.exception)) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_contract.py b/tests/test_contract.py new file mode 100644 index 0000000..34105e4 --- /dev/null +++ b/tests/test_contract.py @@ -0,0 +1,164 @@ +"""The generated openrpc.json, checked against the firmware it describes. + +The contract is the whole agent-facing story: `fixture.info` and `fixture.channels` are supposed +to tell a caller everything it needs, with no other map. That only holds if the document is +actually valid OpenRPC and actually describes the firmware sitting next to it. + +The existing CI check asserted the file parses as JSON and has a non-empty `methods` list. Both +would stay true of a contract that promises three methods the firmware does not implement. +""" + +from __future__ import annotations + +import json +import re +import sys +import tempfile +import unittest +from pathlib import Path + +_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(_ROOT / "src")) +sys.path.insert(0, str(_ROOT / "tests")) + +from pinside.config import load, resolve_board +from pinside.firmware import generate + +_TEMPLATES = _ROOT / "src" / "pinside" / "firmware" / "templates" +_CORE_C = (_TEMPLATES / "fixture_core.c").read_text(encoding="utf-8") + +# `if (strcmp(method, "gpio.read") == 0) return ...` -- the firmware's real method list. +_DISPATCHED = re.compile(r'strcmp\(method,\s*"([a-z0-9_.]+)"\)') +# A notification the firmware sends unasked. Every one goes out through notify_begin, whose +# second argument is the method name: `notify_begin(&o, "uart.data")`. +_NOTIFIED = re.compile(r'notify_begin\([^,]+,\s*"([a-z0-9_.]+)"') + + +def _generate() -> tuple[dict, Path]: + config = load(str(_ROOT / "examples" / "demo-fixture.json")) + board = resolve_board(config, str(_ROOT / "examples" / "demo-board.kicad_pcb")) + out = Path(tempfile.mkdtemp(prefix="pinside-contract-")) + result = generate(config, board, out, force=True) + contract = json.loads((out / "openrpc.json").read_text(encoding="utf-8")) + return contract, result + + +class Structure(unittest.TestCase): + """OpenRPC 1.x, as the specification defines it rather than as JSON.""" + + @classmethod + def setUpClass(cls): + cls.contract, cls.result = _generate() + + def test_it_declares_an_openrpc_version(self): + self.assertRegex(self.contract["openrpc"], r"^1\.\d+\.\d+$") + + def test_the_info_object_is_complete(self): + info = self.contract["info"] + # title and version are the two required fields of the Info object. + self.assertTrue(info["title"]) + self.assertTrue(info["version"]) + + def test_every_method_is_a_valid_method_object(self): + for method in self.contract["methods"]: + with self.subTest(method=method.get("name")): + self.assertTrue(method["name"], "a method with no name") + self.assertIsInstance(method["params"], list) + for param in method["params"]: + self.assertTrue(param["name"]) + self.assertIsInstance(param["schema"], dict) + self.assertTrue(param["schema"], "an empty schema describes nothing") + self.assertIn("result", method) + self.assertTrue(method["result"]["name"]) + self.assertIsInstance(method["result"]["schema"], dict) + + def test_method_names_are_unique(self): + names = [m["name"] for m in self.contract["methods"]] + self.assertEqual(len(names), len(set(names)), f"duplicate method names in {names}") + + def test_every_ref_resolves(self): + """A dangling $ref is a schema that says nothing, and JSON validity will not catch it.""" + components = self.contract.get("components", {}) + + def walk(node, path="$"): + if isinstance(node, dict): + ref = node.get("$ref") + if isinstance(ref, str): + self.assertTrue( + ref.startswith("#/components/"), + f"{path}: external $ref {ref!r}; the contract must stand alone", + ) + target = components + for part in ref.split("/")[2:]: + self.assertIn(part, target, f"{path}: $ref {ref!r} does not resolve") + target = target[part] + for key, value in node.items(): + walk(value, f"{path}.{key}") + elif isinstance(node, list): + for i, value in enumerate(node): + walk(value, f"{path}[{i}]") + + walk(self.contract) + + +class AgainstTheFirmware(unittest.TestCase): + """The drift that matters: a contract describing firmware that is not there.""" + + @classmethod + def setUpClass(cls): + cls.contract, cls.result = _generate() + cls.described = {m["name"] for m in cls.contract["methods"]} + cls.dispatched = set(_DISPATCHED.findall(_CORE_C)) + + def test_the_scan_found_a_dispatch_table_at_all(self): + # Without this, both directions below pass vacuously the moment the C is reformatted. + self.assertGreater(len(self.dispatched), 10) + self.assertIn("fixture.info", self.dispatched) + + def test_the_contract_promises_nothing_the_firmware_lacks(self): + missing = sorted(self.described - self.dispatched) + self.assertFalse(missing, f"described but not dispatched by fixture_core.c: {missing}") + + def test_the_firmware_implements_nothing_the_contract_hides(self): + undocumented = sorted(self.dispatched - self.described) + self.assertFalse(undocumented, f"dispatched but absent from the contract: {undocumented}") + + def test_every_notification_is_actually_sent(self): + promised = {n["name"] for n in self.contract["x-notifications"]} + sent = set(_NOTIFIED.findall(_CORE_C)) + self.assertTrue(sent, "no notification names found in fixture_core.c") + self.assertFalse( + sorted(promised - sent), + f"promised but never sent: {sorted(promised - sent)}", + ) + + +class AgainstTheConfig(unittest.TestCase): + """The channel map, which is the part an agent navigates by.""" + + @classmethod + def setUpClass(cls): + cls.contract, cls.result = _generate() + cls.config = load(str(_ROOT / "examples" / "demo-fixture.json")) + + def test_the_hash_matches_what_generation_reported(self): + # If these two disagree, fixture.info reports a hash nobody can compare against. + self.assertEqual(self.contract["x-pinside"]["config_hash"], self.result.config_hash) + + def test_every_configured_channel_is_in_the_contract(self): + described = {c["name"] for c in self.contract["x-pinside"]["channels"]} + configured = {c.name for c in [*self.config.gpio, *self.config.adc]} + configured |= {b.name for b in self.config.buses} + self.assertEqual(described, configured) + + def test_every_channel_declares_its_kind(self): + for channel in self.contract["x-pinside"]["channels"]: + with self.subTest(channel=channel["name"]): + self.assertIn(channel["kind"], {"gpio", "adc", "uart", "i2c", "spi"}) + + def test_the_target_is_named(self): + self.assertEqual(self.contract["x-pinside"]["mcu"], self.config.mcu) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_docs.py b/tests/test_docs.py new file mode 100644 index 0000000..02ce9a5 --- /dev/null +++ b/tests/test_docs.py @@ -0,0 +1,138 @@ +"""Tests that keep the documentation honest about the code. + +Finding codes are a public interface: people put them in `--ignore` lists and in baselines, and +the README's two tables are the only place their meaning is written down. Both the tables and +the codes are maintained by hand, so they drift, and they have: PF003 and PF004 existed for a +release without appearing in any table. These tests make that a failure rather than a surprise. +""" + +from __future__ import annotations + +import re +import sys +import unittest +from pathlib import Path + +_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(_ROOT / "src")) +sys.path.insert(0, str(_ROOT / "tests")) + +_README = (_ROOT / "README.md").read_text(encoding="utf-8") +_SOURCE = _ROOT / "src" / "pinside" + +# A code as it appears where a Finding is constructed: a bare quoted literal. +# +# PS board checks, PF config checks, PK project generation. The PK family was missed for a +# release because this pattern only looked for the first two, which is the same drift the test +# exists to catch, one level up. +_EMITTED = re.compile(r'"(P[SFK]\d{3})"') +# A code as the README refers to it, either alone or as an endpoint of a PF030-PF038 range. +_DOCUMENTED = re.compile(r"\b(P[SFK]\d{3})\b") + + +def _emitted_codes() -> dict[str, set[str]]: + """Every finding code constructed under src/, and which files construct it.""" + where: dict[str, set[str]] = {} + for path in sorted(_SOURCE.rglob("*.py")): + for code in _EMITTED.findall(path.read_text(encoding="utf-8")): + where.setdefault(code, set()).add(str(path.relative_to(_ROOT))) + return where + + +def _documented_codes() -> set[str]: + """Every code the README names, expanding `PFxxx-PFyyy` ranges.""" + named = set(_DOCUMENTED.findall(_README)) + for family, low, high in re.findall(r"\b(P[SFK])(\d{3})\s*-\s*(?:P[SFK])?(\d{3})\b", _README): + named.update(f"{family}{n:03d}" for n in range(int(low), int(high) + 1)) + return named + + +class DocumentedCodes(unittest.TestCase): + def test_the_source_emits_codes_at_all(self): + # A guard on the guard: if the regex stops matching, every other assertion here passes + # vacuously and the drift it exists to catch goes back to being invisible. + codes = _emitted_codes() + self.assertGreater(len(codes), 30, "the emitted-code scan found almost nothing") + self.assertIn("PS001", codes) + self.assertIn("PF001", codes) + self.assertIn("PK001", codes) + + def test_every_emitted_code_is_in_the_readme(self): + documented = _documented_codes() + missing = { + code: sorted(files) + for code, files in _emitted_codes().items() + if code not in documented + } + self.assertFalse( + missing, + "these finding codes are emitted but appear in no README table: " + + ", ".join(f"{c} ({', '.join(f)})" for c, f in sorted(missing.items())), + ) + + def test_the_readme_documents_no_code_that_does_not_exist(self): + # The other direction: a code withdrawn from the source but left in the table sends + # people to `--ignore` entries that will never match anything. + emitted = set(_emitted_codes()) + stale = sorted(c for c in _documented_codes() if c not in emitted) + self.assertFalse(stale, f"the README documents codes nothing emits: {stale}") + + +class Versions(unittest.TestCase): + """The version lives in two files and the changelog. The release workflow fails on a tag + that disagrees with pyproject, which is the right place to catch it and the latest.""" + + def _pyproject_version(self) -> str: + # Not tomllib: that is 3.11 and later, and pinside supports 3.10. The whole point of + # this file is to notice when something claims a version it does not have, so it would + # be a poor place to depend on one. + text = (_ROOT / "pyproject.toml").read_text(encoding="utf-8") + project = text.split("\n[project]\n", 1)[1].split("\n[", 1)[0] + found = re.search(r'^version\s*=\s*"([^"]+)"', project, re.M) + self.assertIsNotNone(found, "pyproject.toml has no version in [project]") + return found.group(1) + + def test_the_package_and_the_metadata_agree(self): + import pinside + + self.assertEqual(pinside.__version__, self._pyproject_version()) + + def test_the_changelog_has_a_section_for_this_version(self): + changelog = (_ROOT / "CHANGELOG.md").read_text(encoding="utf-8") + version = self._pyproject_version() + self.assertIn(f"## [{version}]", changelog) + + def test_the_changelog_links_every_version_it_names(self): + changelog = (_ROOT / "CHANGELOG.md").read_text(encoding="utf-8") + named = set(re.findall(r"^## \[([^\]]+)\]", changelog, re.M)) + linked = set(re.findall(r"^\[([^\]]+)\]: https://", changelog, re.M)) + self.assertFalse(named - linked, f"unlinked versions: {sorted(named - linked)}") + + +class DocumentedCatalogues(unittest.TestCase): + """The probe and board tables in the README against the catalogues they describe.""" + + def test_every_probe_is_in_the_readme(self): + from pinside import pogo + + for name in pogo.PROBES: + self.assertIn(f"`{name}`", _README, f"probe {name} is in no README table") + + def test_every_carrier_board_is_in_the_readme(self): + from pinside import modules + + for name in modules.MODULES: + self.assertIn(f"`{name}`", _README, f"board {name} is in no README table") + + def test_every_target_is_named(self): + from pinside import targets + + for name in targets.TARGETS: + self.assertTrue( + name.upper() in _README.upper(), + f"target {name} is in targets.TARGETS but the README never names it", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_kicad.py b/tests/test_kicad.py index f4e99fe..7fd32a9 100644 --- a/tests/test_kicad.py +++ b/tests/test_kicad.py @@ -14,6 +14,7 @@ import tempfile import unittest from pathlib import Path +from typing import ClassVar _ROOT = Path(__file__).resolve().parent.parent sys.path.insert(0, str(_ROOT / "src")) @@ -21,8 +22,8 @@ import boards from boards import write -from pinside import modules, pogo, read_board, transform -from pinside.config import from_dict, resolve_board, validate +from pinside import modules, pogo, read_board, run, targets, transform +from pinside.config import from_dict, load, resolve_board, validate from pinside.kicad import library from pinside.kicad.footprint import mounting_hole_shape, pogo_shape from pinside.kicad.project import ProjectError, generate_project @@ -44,6 +45,24 @@ except library.LibraryError: HAVE_SYMBOLS = False +# Set in the CI job that runs inside the KiCad container. A skip is the right +# behaviour on a machine without KiCad and exactly the wrong one there: a job +# whose whole purpose is these tests goes green having run none of them. +REQUIRE_KICAD = os.environ.get("PINSIDE_REQUIRE_KICAD") == "1" + +# The other direction, for `scripts/test.sh --no-kicad`. Anyone likely to touch the KiCad +# emitter has KiCad installed, so the path CI actually runs -- everything here skipped -- is the +# one they never exercise. A guard that goes missing then fails only on the pull request. +if os.environ.get("PINSIDE_NO_KICAD") == "1": + KICAD_CLI, HAVE_SYMBOLS, REQUIRE_KICAD = None, False, False + + +def need_kicad(case: unittest.TestCase, what: str) -> None: + """Skip, unless we were promised KiCad is here, in which case fail.""" + if REQUIRE_KICAD: + case.fail(f"PINSIDE_REQUIRE_KICAD=1 but {what}") + case.skipTest(what) + class TestModules(unittest.TestCase): def test_the_default_is_a_pico(self): @@ -51,19 +70,25 @@ def test_the_default_is_a_pico(self): self.assertIsNotNone(module) self.assertIn("Pico", module.description) - def test_the_pico_header_matches_kicads_own_symbol(self): - """The generated schematic uses this symbol, so the two must agree on pin numbers.""" + def test_every_module_header_matches_kicads_own_symbol(self): + """The generated schematic uses these symbols, so the two must agree on pin numbers. + + Every module, not just the pico2: this is the check that makes adding a carrier board + safe, because a header map transcribed by hand is otherwise unverifiable until a probe + lands on the wrong pin. + """ if not HAVE_SYMBOLS: - self.skipTest("KiCad symbol libraries not installed") - module = modules.get("pico2") - pins = library.symbol_pins(library.load_symbol(module.symbol)) - for gpio, header in module.header.items(): - # KiCad spells the analogue pins GPIO26_ADC0 and so on. - self.assertRegex( - pins[str(header)], - rf"^GPIO{gpio}(_ADC\d)?$", - f"GPIO{gpio} is not on header pin {header}", - ) + need_kicad(self, "KiCad symbol libraries not installed") + for name, module in modules.MODULES.items(): + with self.subTest(module=name): + pins = library.symbol_pins(library.load_symbol(module.symbol)) + for gpio, header in module.header.items(): + # KiCad spells the analogue pins GPIO26_ADC0 and so on. + self.assertRegex( + pins[str(header)], + rf"^GPIO{gpio}(_ADC\d)?$", + f"{name}: GPIO{gpio} is not on header pin {header}", + ) def test_the_pins_the_module_swallows(self): module = modules.get("pico2") @@ -84,6 +109,217 @@ def test_an_unknown_board_lists_the_known_ones(self): self.assertIn("pico2", str(caught.exception)) +@unittest.skipIf(not KICAD_CLI and not REQUIRE_KICAD, "kicad-cli not installed") +class TestNetIdentityAgainstKiCad(unittest.TestCase): + """PS043 and PS044 claim KiCad loses nets. This asks KiCad. + + `pcb export ipcd356` is KiCad's own answer to "what is this board's netlist", so a pad it + reports as N/C is one KiCad thinks connects to nothing. Without this the two checks would + rest on a description of KiCad's behaviour rather than on its behaviour. + """ + + def netlist(self, text: str) -> dict[str, str]: + """ref -> the net name KiCad says that pad is on.""" + out = Path(tempfile.mkdtemp(prefix="pinside-net-")) + self.addCleanup(shutil.rmtree, out, True) + (out / "b.kicad_pcb").write_text(text, encoding="utf-8") + proc = subprocess.run( + [KICAD_CLI, "pcb", "export", "ipcd356", "-o", str(out / "b.d356"), "b.kicad_pcb"], + capture_output=True, + text=True, + cwd=out, + timeout=180, + ) + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + found = {} + for line in (out / "b.d356").read_text(encoding="utf-8").splitlines(): + if line.startswith(("317", "327")): + # 3x7, then the net padded to 17 columns, then the reference. + found[line[20:26].strip()] = line[3:20].strip() + return found + + def codes_for(self, text: str) -> set[str]: + return {f.code for f in run(transform(read_board(boards.write(text))))} + + def test_kicad_loses_the_nets_pinside_flags_with_ps043(self): + broken = boards.sharing_one_net_ordinal(boards.healthy()) + self.assertIn("PS043", self.codes_for(broken)) + + seen = self.netlist(broken) + lost = sorted(ref for ref, net in seen.items() if net == "N/C") + self.assertTrue(lost, f"KiCad kept every net; PS043 would be a false positive: {seen}") + + def test_kicad_keeps_the_nets_when_pinside_is_quiet(self): + good = boards.healthy() + self.assertNotIn("PS043", self.codes_for(good)) + + seen = self.netlist(good) + self.assertFalse( + [ref for ref, net in seen.items() if net == "N/C"], + f"KiCad dropped a net pinside called fine: {seen}", + ) + self.assertEqual(seen.get("TP1"), "/SCL") + + def test_kicad_ignores_the_name_on_net_zero_as_ps044_says(self): + broken = boards.on_net_zero(boards.healthy(), "/SCL") + self.assertIn("PS044", self.codes_for(broken)) + self.assertEqual(self.netlist(broken).get("TP1"), "N/C") + + def test_several_ordinals_sharing_a_name_really_are_merged(self): + """The case pinside deliberately does not flag, confirmed rather than assumed.""" + merged = boards.healthy().replace('(net 2 "/SDA")', '(net 99 "/SCL")') + self.assertNotIn("PS043", self.codes_for(merged)) + seen = self.netlist(merged) + self.assertEqual(seen.get("TP1"), "/SCL") + self.assertEqual(seen.get("TP2"), "/SCL") + + def test_the_example_board_keeps_every_net(self): + """The one that matters: the board pinside ships has to survive being opened.""" + seen = self.netlist((_ROOT / "examples" / "demo-board.kicad_pcb").read_text()) + self.assertFalse([r for r, n in seen.items() if n == "N/C"], seen) + self.assertEqual(seen.get("TP1"), "/DUT_TXD") + self.assertEqual(seen.get("TP14"), "/+3.3V") + + +@unittest.skipIf(not HAVE_SYMBOLS and not REQUIRE_KICAD, "KiCad symbol libraries not installed") +class TestDerivedSymbols(unittest.TestCase): + """Symbols that inherit from another, which is how KiCad writes half the parts pinside uses. + + `(symbol "RaspberryPi_Pico_W" (extends "RaspberryPi_Pico"))` carries properties and nothing + else. A schematic's lib_symbols block has to stand alone, so a copy of that reaches KiCad + with no pins: the file opens, the controller has nothing to wire to, and every net in the + design is isolated. `pinside project --board pico2w` produced exactly that. + """ + + DERIVED = "MCU_Module:RaspberryPi_Pico_W" + ROOT = "MCU_Module:RaspberryPi_Pico" + + def test_a_derived_symbol_arrives_with_its_parents_pins(self): + derived = library.symbol_pins(library.load_symbol(self.DERIVED)) + root = library.symbol_pins(library.load_symbol(self.ROOT)) + self.assertEqual(derived, root) + self.assertEqual(len(derived), 40) + + def test_the_extends_does_not_survive(self): + # A dangling extends resolves to nothing, because the parent is not in lib_symbols. + self.assertNotIn("extends", library.load_symbol(self.DERIVED)) + + def test_it_keeps_its_own_footprint_and_datasheet(self): + # The reason it is a separate symbol at all. Taking the parent's would put the wrong + # footprint on the board. + definition = library.load_symbol(self.DERIVED) + self.assertIn("RaspberryPi_Pico_W_SMD_HandSolder", definition) + self.assertNotIn("RaspberryPi_Pico_Common_Unspecified", definition) + + def test_the_units_are_renamed_to_match_the_symbol(self): + """KiCad matches a unit to its symbol by name prefix, and says nothing when it cannot. + + Leaving the units called RaspberryPi_Pico_1_1 inside a symbol called + RaspberryPi_Pico_W yields "Failed to load schematic" and no further explanation. + """ + definition = library.load_symbol(self.DERIVED) + self.assertIn('(symbol "RaspberryPi_Pico_W_1_1"', definition) + self.assertNotIn('(symbol "RaspberryPi_Pico_1_1"', definition) + + def test_the_flattened_symbol_keeps_the_attributes_kicad_requires(self): + # A derived symbol has none of these; they come from the parent, and without them + # KiCad refuses the file. + definition = library.load_symbol(self.DERIVED) + for token in ("(pin_names", "(in_bom", "(on_board", "(exclude_from_sim"): + self.assertIn(token, definition, f"{token} was lost in flattening") + + def test_a_root_symbol_is_untouched(self): + raw = Path(library.symbol_dir(), "MCU_Module.kicad_sym").read_text(encoding="utf-8") + definition = library.load_symbol(self.ROOT) + # Everything but the one renamed line should be verbatim library text. + body = definition.split("\n", 1)[1] + self.assertIn(body[:200], raw) + + +@unittest.skipIf(not HAVE_SYMBOLS and not REQUIRE_KICAD, "KiCad symbol libraries not installed") +class TestTargetsAgainstKiCad(unittest.TestCase): + """The function map is a datasheet transcribed by hand, which is how it goes wrong. + + KiCad's own symbol library is a second, independent transcription of the same pinout, so + disagreement between the two means one of them is wrong and it is worth knowing which. + """ + + SYMBOLS: ClassVar[dict[str, str]] = { + "rp2350a": "MCU_RaspberryPi:RP2350A", + "rp2350b": "MCU_RaspberryPi:RP2350B", + "rp2354a": "MCU_RaspberryPi:RP2354A", + "rp2354b": "MCU_RaspberryPi:RP2354B", + } + + def test_every_target_has_a_kicad_symbol(self): + for name in targets.TARGETS: + self.assertIn(name, self.SYMBOLS, f"{name} has no KiCad symbol to check against") + + def test_the_gpio_count_matches_kicads_symbol(self): + for name, symbol in self.SYMBOLS.items(): + if name in targets.SAME_PINOUT: + continue # checked through the part it extends, below + with self.subTest(target=name): + pins = library.symbol_pins(library.load_symbol(symbol)) + gpio = {v.split("/")[0] for v in pins.values() if v.startswith("GPIO")} + self.assertEqual(len(gpio), targets.get(name).gpio_count) + + def test_the_adc_pins_match_kicads_symbol(self): + for name, symbol in self.SYMBOLS.items(): + if name in targets.SAME_PINOUT: + continue + with self.subTest(target=name): + pins = library.symbol_pins(library.load_symbol(symbol)) + # KiCad spells them GPIO26/ADC0; the target holds {gpio: adc channel}. + from_symbol = {} + for label in pins.values(): + if "/ADC" in label and label.startswith("GPIO"): + gpio, adc = label.split("/") + from_symbol[int(gpio[4:])] = int(adc[3:]) + self.assertEqual(from_symbol, targets.get(name).adc_pins) + + def test_the_stacked_flash_parts_really_do_share_a_pinout(self): + """Not an assumption: KiCad writes it as `(symbol "RP2354A" (extends "RP2350A"))`.""" + raw = Path(library.symbol_dir(), "MCU_RaspberryPi.kicad_sym").read_text(encoding="utf-8") + for stacked, plain in targets.SAME_PINOUT.items(): + with self.subTest(target=stacked): + block = raw[raw.index(f'(symbol "{stacked.upper()}"') :][:200] + self.assertIn(f'(extends "{plain.upper()}")', block) + # And pinside's own two entries agree with each other. + self.assertEqual(targets.get(stacked).gpio_count, targets.get(plain).gpio_count) + self.assertEqual(targets.get(stacked).adc_pins, targets.get(plain).adc_pins) + + +class TestProbeProvenance(unittest.TestCase): + """Where the probe dimensions came from, which is not a detail: they become drill sizes.""" + + def test_every_probe_records_its_source(self): + for name, probe in pogo.PROBES.items(): + with self.subTest(probe=name): + self.assertTrue(probe.source, f"{name} does not say where its numbers came from") + + def test_verification_is_opt_in_rather_than_assumed(self): + # An empty `verified` is the honest state for a value read off a catalogue page. This + # asserts the field exists and defaults to unverified, not that it stays that way: when + # somebody does check one against a drawing they fill it in and this still passes. + for probe in pogo.PROBES.values(): + self.assertIsInstance(probe.verified, str) + self.assertEqual( + len(pogo.unverified()), len([p for p in pogo.PROBES.values() if not p.verified]) + ) + + def test_the_generated_readme_says_so(self): + from pinside.kicad.project import _readme + + config = load(str(_ROOT / "examples" / "demo-fixture.json")) + text = _readme(config, None, 14, []) + if config.probe_part.verified: + self.assertIn(config.probe_part.verified, text) + else: + # Somebody ordering a board off this needs to see it, not find it in a docstring. + self.assertIn("before you order", text) + + class TestProbes(unittest.TestCase): def test_the_default_probe_is_a_receptacle_and_a_pin(self): probe = pogo.get() @@ -211,7 +447,7 @@ def test_verbatim_text_is_reindented_not_reparsed(self): self.assertTrue(rendered.startswith("\t(")) -@unittest.skipUnless(HAVE_SYMBOLS, "KiCad symbol libraries not installed") +@unittest.skipIf(not HAVE_SYMBOLS and not REQUIRE_KICAD, "KiCad symbol libraries not installed") class TestLibrary(unittest.TestCase): def test_a_symbol_keeps_its_bare_tokens(self): """Re-serialising a parse would quote these, and KiCad would refuse the file.""" @@ -281,7 +517,7 @@ def test_analogue_channels_take_no_series_resistor(self): self.assertFalse(any(s.series for s in adc)) -@unittest.skipUnless(HAVE_SYMBOLS, "KiCad symbol libraries not installed") +@unittest.skipIf(not HAVE_SYMBOLS and not REQUIRE_KICAD, "KiCad symbol libraries not installed") class TestProjectGeneration(unittest.TestCase): def setUp(self): self.dut_path = write(boards.healthy()) @@ -379,7 +615,42 @@ def test_an_invalid_config_writes_nothing(self): self.assertEqual(list(self.out.iterdir()), []) -@unittest.skipUnless(KICAD_CLI and HAVE_SYMBOLS, "kicad-cli not installed") +class TestHardwareNote(unittest.TestCase): + """The plate-force arithmetic, which needs no KiCad.""" + + def setUp(self): + self.probe = pogo.get("millmax_0985") + + def note(self, probes: int, holes: int = 4) -> str: + from pinside.kicad.project import _hardware_note + + return _hardware_note(probes, self.probe, holes) + + def test_a_small_fixture_says_a_thumbscrew_will_do(self): + self.assertIn("thumbscrew", self.note(4)) + + def test_a_medium_fixture_asks_for_a_clamp(self): + self.assertIn("toggle clamp", self.note(60)) + + def test_a_large_fixture_warns_about_plate_deflection(self): + text = self.note(200) + self.assertIn("pneumatic", text) + self.assertIn("deflects", text) + + def test_the_per_standoff_load_divides_by_the_holes(self): + # 60 probes at 0.75 N is 45 N over four holes: 11 N each. + self.assertIn("11 N", self.note(60, holes=4)) + + def test_no_mounting_holes_says_so_rather_than_dividing_by_zero(self): + text = self.note(14, holes=0) + self.assertIn("no mounting holes", text) + self.assertNotIn("per standoff", text) + + def test_the_travel_the_standoff_has_to_allow_is_named(self): + self.assertIn(f"{self.probe.travel_mm} mm of probe travel", self.note(14)) + + +@unittest.skipIf(not (KICAD_CLI and HAVE_SYMBOLS) and not REQUIRE_KICAD, "kicad-cli not installed") class TestKiCadAcceptsIt(unittest.TestCase): """The only test that proves the output is a project KiCad will actually open.""" @@ -387,8 +658,6 @@ class TestKiCadAcceptsIt(unittest.TestCase): def setUpClass(cls): cls.out = Path(tempfile.mkdtemp()) dut = _ROOT / "examples" / "demo-board.kicad_pcb" - from pinside.config import load - config = load(str(_ROOT / "examples" / "demo-fixture.json")) board = resolve_board(config, str(dut)) cls.result = generate_project(config, board, cls.out, force=True) @@ -403,15 +672,103 @@ def _run(self, *args) -> subprocess.CompletedProcess: [KICAD_CLI, *args], capture_output=True, text=True, cwd=self.out, timeout=180 ) + def test_the_board_carries_a_ground_pour(self): + """Not routing, and not optional: on a fixture the pour is most of the return path. + + DRC passing says the zone is legal. This says it is there at all, on both copper + layers and on GND, which DRC would be just as happy without. + """ + text = (self.out / f"{self.name}.kicad_pcb").read_text() + self.assertIn("(zone", text) + self.assertIn('(name "GND pour")', text) + zone = text[text.index("(zone") :] + self.assertIn('"F.Cu"', zone[:400]) + self.assertIn('"B.Cu"', zone[:400]) + + def test_the_pour_stays_inside_the_board(self): + from pinside.kicad.pcb import GROUND_POUR_INSET_MM + + board = resolve_board( + load(str(_ROOT / "examples" / "demo-fixture.json")), + str(_ROOT / "examples" / "demo-board.kicad_pcb"), + ) + box = board.outline.bbox + text = (self.out / f"{self.name}.kicad_pcb").read_text() + zone = text[text.index('(name "GND pour")') :] + pts = zone[zone.index("(polygon") : zone.index("(polygon") + 400] + coords = [ + tuple(float(v) for v in line.strip()[4:-1].split()) + for line in pts.splitlines() + if line.strip().startswith("(xy ") + ] + self.assertEqual(len(coords), 4) + for x, y in coords: + # A pour drawn past the outline is copper KiCad clips and DRC complains about. + self.assertGreaterEqual(x, GROUND_POUR_INSET_MM - 1e-6) + self.assertGreaterEqual(y, GROUND_POUR_INSET_MM - 1e-6) + self.assertLessEqual(x, box.width - GROUND_POUR_INSET_MM + 1e-6) + self.assertLessEqual(y, box.height - GROUND_POUR_INSET_MM + 1e-6) + + def test_the_readme_turns_the_force_into_hardware(self): + text = (self.out / "README.md").read_text() + self.assertIn("Closing force", text) + # Newtons alone are not actionable. The generated note has to reach the thing somebody + # actually has to buy or decide: the clamp, and the standoffs carrying the load. + self.assertIn("per standoff", text) + self.assertIn("kgf", text) + self.assertIn("travel", text) + + def test_a_pico2w_project_is_equally_clean(self): + """The carrier whose symbol is derived, run through KiCad end to end. + + Every other test here uses the pico2, whose symbol is a root symbol. The pico2w was the + one that produced a schematic KiCad could not load at all, and no unit test of the + emitter noticed: the file was well-formed S-expressions the whole time. + """ + config = load(str(_ROOT / "examples" / "demo-fixture.json")) + config.board = "pico2w" + config.mcu = "rp2350a" + board = resolve_board(config, str(_ROOT / "examples" / "demo-board.kicad_pcb")) + out = Path(tempfile.mkdtemp(prefix="pinside-picow-")) + self.addCleanup(shutil.rmtree, out, True) + generate_project(config, board, out, force=True) + + proc = subprocess.run( + [KICAD_CLI, "sch", "erc", "-o", str(out / "erc.rpt"), f"{config.name}.kicad_sch"], + capture_output=True, + text=True, + cwd=out, + timeout=180, + ) + self.assert_erc_clean(proc, out / "erc.rpt") + + def assert_erc_clean(self, proc, report: Path) -> None: + """ERC found nothing, and says which kind of nothing it did not find. + + A machine with KiCad's libraries on disk but no *profile* -- a fresh container, which + is exactly what CI is -- cannot resolve `MCU_Module:` or `power:` to anything, and ERC + reports one warning per reference. Those say the environment is unconfigured, not that + the schematic is wrong, and reading forty of them to work that out is a waste of + somebody's afternoon. So name it. + """ + text = report.read_text(encoding="utf-8") if report.exists() else "" + self.assertNotIn("Failed to load", proc.stdout + proc.stderr) + if "Found 0 violations" in proc.stdout: + return + if "lib_symbol_issues" in text or "footprint_link_issues" in text: + self.fail( + "ERC cannot resolve the stock library nicknames, so every symbol is a " + "warning. That is a missing KiCad profile, not a defect in the generated " + "schematic: seed sym-lib-table and fp-lib-table from KiCad's own templates " + "into its config directory, which is what a first GUI launch does.\n" + f"{proc.stdout}" + ) + self.fail(f"{proc.stdout}\n{text}") + def test_erc_is_clean(self): report = self.out / "erc.rpt" proc = self._run("sch", "erc", "-o", str(report), f"{self.name}.kicad_sch") - self.assertNotIn("Failed to load", proc.stdout + proc.stderr) - self.assertIn( - "Found 0 violations", - proc.stdout, - f"{proc.stdout}\n{report.read_text() if report.exists() else ''}", - ) + self.assert_erc_clean(proc, report) def test_the_board_loads_and_has_no_clearance_errors(self): report = self.out / "drc.rpt" diff --git a/tests/test_pinside.py b/tests/test_pinside.py index 9b4c0ce..c8afa92 100644 --- a/tests/test_pinside.py +++ b/tests/test_pinside.py @@ -7,6 +7,7 @@ import json import os import sys +import tempfile import unittest from pathlib import Path @@ -149,8 +150,7 @@ def test_limits_are_honoured(self): self.assertNotIn("PS022", codes(boards.troubled(), loose)) def test_one_ground_warns_and_two_do_not(self): - one = boards.healthy().replace(boards._testpoint("TP91", 22, 20, "GND", value="GND"), "") - found = codes(one) + found = codes(boards.without(boards.healthy(), "TP91")) self.assertIn("PS031", found) # only one ground probe self.assertNotIn("PS030", found) # but not "no ground at all" self.assertNotIn("PS031", codes(boards.healthy())) @@ -171,10 +171,7 @@ def test_exit_codes(self): def test_strict_promotes_warnings(self): # Two mounting holes let the board pivot: a warning, not an error. - text = boards.healthy() - for ref, x, y in [("H3", 5, 35), ("H4", 45, 35)]: - text = text.replace(boards._hole(ref, x, y, net="GND"), "") - path = write(text) + path = write(boards.without(boards.healthy(), "H3", "H4")) self.assertEqual(cli([path, "-f", "json", "-o", os.devnull]), 0) self.assertEqual(cli([path, "-f", "json", "-o", os.devnull, "--strict"]), 1) @@ -204,5 +201,216 @@ def test_missing_file_is_usage_error(self): self.assertEqual(cli(["/nonexistent/board.kicad_pcb"]), 3) +class InitCommand(unittest.TestCase): + """`pinside init` is the entry point for everything downstream, so it has to actually run. + + It did not: the positional `board` and the carrier option `--board` shared an argparse dest, + the option won, and every invocation looked the .kicad_pcb path up in the module catalogue + and refused. The end-to-end CI job ran this exact command and it failed there too. + """ + + def _draft(self, args: list[str]) -> dict: + out = Path(tempfile.mkdtemp(prefix="pinside-init-")) / "fixture.json" + self.assertEqual(cli(["init", write(boards.uart_board()), "-o", str(out), *args]), 0) + return json.loads(out.read_text()) + + def test_init_drafts_a_config_from_a_board(self): + draft = self._draft([]) + self.assertTrue(draft["name"]) + self.assertIn("target", draft) + + def test_the_carrier_defaults_to_the_pico(self): + self.assertEqual(self._draft([])["target"]["board"], "pico2") + + def test_the_carrier_can_be_chosen(self): + draft = self._draft(["--carrier", "bare", "--mcu", "rp2350b"]) + self.assertEqual(draft["target"]["board"], "bare") + self.assertEqual(draft["target"]["mcu"], "rp2350b") + + def test_the_positional_is_still_the_board_path(self): + # The collision this guards against is silent: argparse accepts both spellings and one + # of them quietly wins, so only reading a value back out catches it. + draft = self._draft([]) + self.assertIn(".kicad_pcb", draft["dut"]["board"]) + + def test_a_drafted_config_generates(self): + board = write(boards.uart_board()) + cfg = Path(tempfile.mkdtemp(prefix="pinside-init-")) / "fixture.json" + self.assertEqual(cli(["init", board, "-o", str(cfg)]), 0) + self.assertEqual(cli(["generate", str(cfg), "--dry-run"]), 0) + + +class Cutouts(unittest.TestCase): + """Edge.Cuts holds every edge the board has, not one ring.""" + + def test_a_slot_is_not_a_broken_outline(self): + # The regression this guards: a board with a milled window used to chain into no ring + # at all and report PS002, an error, on geometry a fab would cut without comment. + board = transform(read_board(write(boards.slotted()))) + self.assertTrue(board.outline.closed) + self.assertEqual(len(board.outline.cutouts), 1) + self.assertNotIn("PS002", codes(boards.slotted())) + self.assertIn("PS003", codes(boards.slotted())) + + def test_there_is_no_board_inside_a_cutout(self): + board = transform(read_board(write(boards.slotted()))) + self.assertTrue(board.outline.within_perimeter(25, 20)) + self.assertTrue(board.outline.in_cutout(25, 20)) + self.assertFalse(board.outline.contains(25, 20)) + self.assertTrue(board.outline.contains(10, 10)) + + def test_a_probe_over_a_cutout_is_an_error(self): + found = codes(boards.probe_over_a_slot()) + self.assertIn("PS013", found) + # Not also PS010: the probe was placed, it was placed over a hole. Saying "never + # placed" would send somebody to re-run the netlist import instead of moving the pad. + self.assertNotIn("PS010", found) + + def test_a_second_outline_is_reported_not_swallowed(self): + found = codes(boards.panelised()) + self.assertIn("PS004", found) + self.assertNotIn("PS002", found) + + def test_an_open_edge_is_still_an_error(self): + text = boards._wrap(boards.segment_outline(gap=True)) + self.assertIn("PS002", codes(text)) + + +class NetIdentity(unittest.TestCase): + """Whether the board's netlist survives being opened in KiCad. + + Through KiCad 9 a net is identified by its ordinal and the name is a label. pinside reads the + name, which is why it called its own example board clean for two releases while KiCad, asked + for the same board's netlist, reported fifteen of sixteen test points as N/C. + + Every expectation here was checked against `kicad-cli pcb export ipcd356`. + """ + + def test_one_ordinal_with_several_names_is_an_error(self): + found = codes(boards.sharing_one_net_ordinal(boards.healthy())) + self.assertIn("PS043", found) + + def test_the_finding_names_the_nets_and_the_probes(self): + board = transform(read_board(write(boards.sharing_one_net_ordinal(boards.healthy())))) + finding = next(f for f in run(board) if f.code == "PS043") + self.assertIn("/SCL", finding.refs[0]) + self.assertIn("/SDA", finding.refs[0]) + # And which test points go dark, in board order rather than lexicographic. + self.assertIn("TP1, TP2", finding.detail) + + def test_a_properly_numbered_board_says_nothing(self): + self.assertNotIn("PS043", codes(boards.healthy())) + self.assertNotIn("PS044", codes(boards.healthy())) + + def test_a_named_net_on_ordinal_zero_is_an_error(self): + # Net 0 is KiCad's no-connection net: it ignores the name, pinside would not. + found = codes(boards.on_net_zero(boards.healthy(), "/SCL")) + self.assertIn("PS044", found) + + def test_the_kicad_10_form_has_no_ordinals_to_collide(self): + # That format dropped the ordinal, so neither check can apply and neither should fire. + board = transform(read_board(write(boards.as_kicad10(boards.healthy())))) + found = {f.code for f in run(board)} + self.assertNotIn("PS043", found) + self.assertNotIn("PS044", found) + self.assertEqual(board.net_ordinals, {}) + + def test_several_ordinals_sharing_a_name_is_not_a_finding(self): + """KiCad merges those into one net, which is what pinside already reads. No defect.""" + text = boards.healthy().replace('(net 2 "/SDA")', '(net 99 "/SCL")') + self.assertNotIn("PS043", codes(text)) + + def test_the_board_that_shipped_in_0_1_0_would_have_been_caught(self): + """The regression this check exists for. + + The example board gave all sixteen test points net ordinal 1 with sixteen different + names. `pinside check` passed it. Opening it in KiCad and saving destroyed the netlist. + """ + shipped = boards.sharing_one_net_ordinal( + (_ROOT / "examples" / "demo-board.kicad_pcb").read_text(encoding="utf-8") + ) + board = transform(read_board(write(shipped))) + finding = next(f for f in run(board) if f.code == "PS043") + self.assertEqual(finding.severity, ERROR) + self.assertIn("/DUT_TXD", finding.refs[0]) + self.assertIn("TP90", finding.detail) + + def test_the_example_board_as_it_stands_is_clean(self): + board = transform(read_board(str(_ROOT / "examples" / "demo-board.kicad_pcb"))) + self.assertEqual([f for f in run(board) if f.code in ("PS043", "PS044")], []) + # Sixteen test points, fifteen distinct nets (the two grounds share one). + self.assertEqual(len(board.net_ordinals), 15) + + +class SignalCoverage(unittest.TestCase): + """What the board has that the fixture will not reach.""" + + def test_unprobed_rails_and_reset_lines_are_reported(self): + found = codes(boards.unreachable_rails()) + self.assertIn("PS033", found) + self.assertIn("PS034", found) + + def test_a_board_whose_rails_are_probed_says_nothing(self): + self.assertNotIn("PS033", codes(boards.healthy())) + self.assertNotIn("PS034", codes(boards.healthy())) + + def test_the_finding_names_the_nets(self): + board = transform(read_board(write(boards.unreachable_rails()))) + rails = next(f for f in run(board) if f.code == "PS033") + self.assertCountEqual(rails.refs, ["+3V3", "+1V8"]) + reset = next(f for f in run(board) if f.code == "PS034") + self.assertIn("MCU_NRST", reset.refs) + + +class ProbeBody(unittest.TestCase): + def test_a_receptacle_body_overlapping_a_part_is_reported(self): + # U1's pad envelope starts at x=22. TP1 at x=21.5 clears it by 0.5 mm, so the tip + # lands on copper, and the 0.85 mm body radius of a 0985 receptacle does not fit. + body = boards.rect_outline() + body += boards._testpoint("TP1", 21.5, 20.0, "/SIG") + body += boards._testpoint("TP90", 10, 30, "GND", value="GND") + body += boards._testpoint("TP91", 14, 30, "GND", value="GND") + body += boards._part("U1", 24.0, 20.0, w=4.0, h=2.0) + found = codes(boards._wrap(body)) + self.assertIn("PS027", found) + self.assertNotIn("PS024", found) # the tip itself is clear + + def test_a_finer_probe_relaxes_it(self): + # A soldered_1mm body is 1.00 mm across, so 0.5 mm of clearance is exactly enough. + body = boards.rect_outline() + body += boards._testpoint("TP1", 21.5, 20.0, "/SIG") + body += boards._testpoint("TP90", 10, 30, "GND", value="GND") + body += boards._testpoint("TP91", 14, 30, "GND", value="GND") + body += boards._part("U1", 24.0, 20.0, w=4.0, h=2.0) + self.assertNotIn("PS027", codes(boards._wrap(body), Limits(probe_body=1.0))) + + +class NetRecordFormats(unittest.TestCase): + """KiCad 9 writes (net "NAME"); KiCad 10 dropped the ordinal. + + Reading only one of the two forms does not fail loudly. It yields test points whose net is + the string "3", which classifies as power, probes nothing, and produces a fixture wired to + signals that do not exist. + """ + + def test_both_forms_read_the_same_nets(self): + old = transform(read_board(write(boards.healthy()))) + new = transform(read_board(write(boards.as_kicad10(boards.healthy())))) + self.assertEqual( + [(t.ref, t.net, t.signal) for t in old.test_points], + [(t.ref, t.net, t.signal) for t in new.test_points], + ) + self.assertIn("/SCL", [t.net for t in new.test_points]) + + def test_both_forms_produce_the_same_findings(self): + for board in (boards.healthy(), boards.troubled(), boards.unplaced()): + with self.subTest(board=board[:40]): + self.assertEqual(codes(board), codes(boards.as_kicad10(board))) + + def test_the_kicad_10_form_still_grounds_mounting_holes(self): + board = transform(read_board(write(boards.as_kicad10(boards.healthy())))) + self.assertTrue(all(h.net == "GND" for h in board.mounting_holes)) + + if __name__ == "__main__": unittest.main()