-
Notifications
You must be signed in to change notification settings - Fork 18
feat: disco tool #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: disco tool #35
Conversation
|
Nice to know that it also work for humans, lol 😆 |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add disco-tool-tests job to CI (pytest on Python 3.11) - Move tests to tests/unit/ for CI, tests/integration/ for hardware - Add scripts/requirements.txt and pytest.ini - Update .gitignore (build, .direnv, .pytest_cache) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- create ocd_provider.py for centralized OCD instance w/ test injection - create OCDMock that tracks halt/resume state - add 29 tests for halt/resume balance - fix 13 commands missing resume after halt: flash: identify/read/verify/erase mem: read/vectors/dump/save cpu: regs/pc/stack check/cables: add try/finally for error handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Move fingerprint create/update/test to flash_fingerprint.py - Add detailed help text for fingerprint subcommand - Rename ocd connect -> ocd start, ocd disconnect -> ocd stop - Update quickstart with fingerprint tree, docs, and ocd renames - Add session completion workflow to AGENTS.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace specter-diy references with f469-disco - Note disco tool makes manual debugging rarely needed - Add Known Issues section (QSPI shadow dirs, SPI flash hang) - Update command examples to use ocd start 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Test 6 firmwares in fwbox using --static-only flag - Run in CI without hardware - Remove specter_releases (outdated fingerprints) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Explain flake.nix provides all build tools (arm-gcc, openocd, gdb, SDL2). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a comprehensive Python CLI tool (disco) for interacting with the STM32F469-Discovery board. The tool provides a unified interface for JTAG debugging, flash programming, serial communication, and MicroPython REPL interaction. It includes automated diagnostics, firmware fingerprinting for CI/regression testing, and extensive test coverage.
Key changes:
- New
scripts/discoCLI tool with hierarchical command structure (ocd, cpu, mem, flash, serial, repl subcommands) - Comprehensive test suite with unit tests and integration tests for fingerprint validation
- Automated diagnostics system (
disco doctor) with markdown logging - Firmware fingerprinting for regression testing and CI validation
Reviewed changes
Copilot reviewed 57 out of 67 changed files in this pull request and generated 37 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/disco |
Main CLI entry point with venv auto-activation |
scripts/disco_lib/ |
Core library modules (openocd, serial, repl, flash, diagnostics, cpu, memory) |
scripts/disco_lib/commands/ |
Command implementations for all CLI subcommands |
scripts/disco_lib/tests/unit/ |
Unit tests for core modules with mocking |
scripts/disco_lib/tests/integration/ |
Integration tests for fingerprint validation |
scripts/disco_lib/tests/mocks/ |
Mock objects for OpenOCD testing |
tests/fwbox/ |
Firmware test box with fingerprint files for various builds |
docs/debugging.md |
Comprehensive 1000+ line debugging guide |
docs/architecture/firmware_layouts.md |
Documentation of firmware memory layouts |
.github/workflows/ci.yml |
CI workflow for disco tool tests |
| Documentation updates | Various doc files with URL updates (github.io domain change) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove 10 unused imports across 8 files - Remove unused variable (code_regions) and result assignments - cables.py: enrich return dict with error/platform, catch specific exceptions - diagnostics.py: _parse_mdw returns (words, skipped) tuple - memory.py: read_vectors adds skipped key for unreadable values - openocd.py: use non-blocking recv, add OPENOCD_LOG constant - Add tests for skipped/unreadable memory values 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I've fixed all of copilot's issue with bc11187 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This review has been created with the help of Claude cli (/review)
Code Review: PR #35 - feat: disco tool
Overview
This PR adds a Python CLI tool (scripts/disco) for interacting with the STM32F469-Discovery board. It provides commands for:
- OpenOCD server management
- CPU control (halt, resume, reset, step)
- Memory inspection
- Flash programming with smart verification
- MicroPython REPL interaction
- Automated board diagnostics
Size: +7088/-16 lines across 50 files
Code Quality: ✅ Good
Architecture
- Well-structured: Clean separation between CLI (
commands/), backend logic (flash.py,openocd.py, etc.), and tests - Click framework: Good use of Click with
\bfor help formatting - Context managers: Proper resource handling (
with cpu_backend.halted()) - Type hints: Used consistently throughout
Testing
- Unit tests for flash analysis logic (
test_flash.py) - Integration tests for fingerprint validation
- Good test documentation explaining what each test validates
Documentation
- Extensive
docs/debugging.md(~1000 lines) - Firmware layouts documentation
- Well-documented CLI commands with examples
Suggestions for Improvement
1. Bare exception handling
Some except Exception clauses could mask issues:
diagnostics.py:151, 317-318
except Exception:
report.add("TARGET_NOT_RESPONDING", ...)Consider catching specific exceptions or at minimum logging the actual error.
2. Platform portability concerns
| Issue | Location | Impact |
|---|---|---|
pkill -x openocd |
openocd.py:133 |
Linux/macOS only |
/tmp/disco_log |
diagnostics.py:408 |
Linux/macOS only |
usbmodem* pattern |
diagnostics.py:298 |
macOS-specific |
The CI runs on ubuntu-latest but serial detection seems macOS-focused. Consider:
- Using
tempfile.gettempdir()for logs - Adding platform detection for serial patterns
3. Test cleanup safety
test_flash.py: Manual os.unlink() in try/finally - consider pytest tmp_path fixture:
def test_all_code(self, tmp_path):
path = tmp_path / "test.bin"
path.write_bytes(b"\xff" * 8192)
regions = analyze_firmware(str(path))
# No cleanup needed - pytest handles it4. Minor: _is_blank_chunk only checks 0x00
Erased flash is typically 0xFF. The code intentionally treats 0x00 as "blank" for filesystem preservation detection - consider adding a comment explaining this design choice in the function docstring.
Potential Issues/Risks
Low Risk
- Timeout behavior: 10 consecutive 2s timeouts (20s total) before giving up in
openocd.py:70- slow failures could be very slow
Security
pkill -x openocdcould affect other users' processes on shared systems (unlikely to be a concern for embedded dev boards)
Edge Cases
- What happens if OpenOCD start fails but a stale process exists?
- Serial device detection may miss non-standard USB CDC implementations
Summary
| Aspect | Rating | Notes |
|---|---|---|
| Code quality | ⭐⭐⭐⭐ | Clean, well-organized |
| Documentation | ⭐⭐⭐⭐⭐ | Excellent, thorough |
| Test coverage | ⭐⭐⭐ | Good for core logic, could expand |
| Portability | ⭐⭐⭐ | macOS-focused serial detection |
| Error handling | ⭐⭐⭐ | Some bare except clauses |
Recommendation: ✅ Approve with minor suggestions
The PR is well-implemented and provides valuable tooling. The suggestions above are improvements, not blockers. The AI agent optimization focus is clear and the documentation is excellent.
- Replace bare except with specific exceptions (OSError, ClickException, SerialException) - Use tempfile.gettempdir() for platform-portable log paths - Add platform detection for USB CDC (macOS: usbmodem*, Linux: ttyACM*) - Use pytest tmp_path fixture in test_flash.py - Document _is_blank_chunk 0x00 design choice 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I've fixed all claude recommendations in 5e27501 |
- test_cpu.py: 32 tests for halted() ctx mgr, read_pc/sp parsing - test_memory.py: 27 tests for read_vectors, unreadable memory - test_ocd_provider.py: 19 tests for singleton/injection - test_flash.py: +27 error path tests (version tags, flash addr) - test_serial.py: +12 edge case tests - test_diagnostics.py: +13 tests (check_target, cpu_stuck) - test_halt_resume.py: remove stale "Currently FAILS" comments Total: 169 → 267 tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
7c19ea2 added a lot more tests and also reviewed and improved existing tests. |
Add commands to manage STM32F469 Read-out Protection (RDP): - disco flash rdp: Show current RDP level (0/1/2) - disco flash unlock: Remove RDP1 protection (causes mass erase) - disco flash lock: Enable RDP1 protection Backend functions: read_option_bytes, parse_rdp_level, unlock_rdp, lock_rdp Unit tests: 19 new tests for RDP parsing and commands Safety: Refuses RDP2 operations (permanent), confirms destructive actions Closes: fw-0ol, fw-p27 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
5e57039 added a lock and unlock command and a rdp command to get the current status of the read-protection. Details(.venv) ➜ scripts git:(kn/disco_tool) ✗ ./disco flash --help
Usage: disco flash [OPTIONS] COMMAND [ARGS]...
Flash programming commands.
Program and erase the STM32F469 internal flash via JTAG/OpenOCD. The flash
is 2MB organized as dual-bank with mixed sector sizes.
Flash Layout (STM32F469, 2MB dual-bank):
Bank 1 (0x08000000 - 0x080FFFFF):
Sectors 0-3: 16KB each (0x08000000 - 0x0800FFFF)
Sector 4: 64KB (0x08010000 - 0x0801FFFF)
Sectors 5-11: 128KB each (0x08020000 - 0x080FFFFF)
Bank 2 (0x08100000 - 0x081FFFFF): Same layout as Bank 1
Filesystem Preservation:
MicroPython firmware files typically have zeros between code regions
to preserve the internal filesystem during updates. Zero regions
won't overwrite existing flash data when programmed.
Use 'disco flash analyze <file>' to see the firmware layout.
Programming Addresses:
0x08000000 Full image (bootloader + firmware, preserves filesystem)
0x08020000 Main firmware only (default for 'program' command)
Examples:
disco flash analyze firmware.bin # Show firmware layout
disco flash program firmware.bin # Flash to 0x08020000
disco flash program full.bin --addr 0x08000000
disco flash erase # Requires confirmation
Safety:
- 'program' verifies after writing by default (--no-verify to skip)
- 'program' resets the board after flashing (--no-reset to skip)
- 'erase' requires explicit confirmation
Options:
--help Show this message and exit.
Commands:
analyze Analyze firmware file layout.
erase Mass erase flash (DANGEROUS).
fingerprint Firmware fingerprint commands.
identify Identify firmware build type (debug vs production).
info Show flash bank info.
lock Enable flash read protection (RDP Level 0 -> 1).
program Program firmware to flash.
rdp Show current read protection (RDP) level.
read Read flash contents to file.
unlock Remove flash read protection (RDP Level 1 -> 0).
verify Verify flash contents agai |
|
Hey @k9ert I was trying to test the PR and followed instructions of I've also had to add I tried to execute Then I've unplugged and plugged again, and tried few times, got two diff errors: The board is connected with the USB mini and micro on the PC (with power jumper on USB), I've flashed it with the |
|
I got this msg (tried to change power jumper to st-link and connecting mini and micro USB cables, but same result): |
|
Hey @k9ert, I noticed that |
|
I fixed some Linux-specific issues in this PR: k9ert#1 However, driver behavior differs between macOS and Linux. On Linux, once OpenOCD starts, it holds the USB endpoint long enough to disrupt the CDC interface, causing |
The
scripts/discotool is a Python CLI for interacting with the STM32F469-Discovery board. Provides commands for flashing, serial console, OpenOCD control, memory inspection, fingerprint testing, and diagnostics. Includes test suite.Optimized for AI agent usage. But works for humans alike.
Quick Start
Full command tree
Also included