fix: keep CoreS3 USB available in normal builds - #652
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHost ChangesRelease-mode firmware commands
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant firmware_mjs as firmware.mjs
participant BuildConfig as run-mcconfig.mjs
participant CoreS3USB as CoreS3 USB Serial/JTAG
Developer->>firmware_mjs: run build, flash, or deploy
firmware_mjs->>BuildConfig: select release mode by default
BuildConfig-->>firmware_mjs: return build configuration
firmware_mjs-->>CoreS3USB: warn when CoreS3 uses non-release mode
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Cloudflare PR previewThis pull request is closed. Its preview has been replaced with a closed page at https://pr-652.stack-chan-pr-preview.pages.dev. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
firmware/scripts/lib/firmware-command.test.mjs (1)
39-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExpand coverage to the complete command and mode matrix.
The implementation applies explicit modes to
build,flash, anddeploy, and the warning applies to debug and instrument modes. These tests cover explicit modes only forbuildand cover only debug mode for the warning. Add coverage forflash,deploy,--mode=instrument, and the legacy-dand-iselectors.As per path instructions: “Because this PR changes user-visible firmware command behavior, retain the changeset/release-impact documentation and include target-specific CoreS3 verification where applicable.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@firmware/scripts/lib/firmware-command.test.mjs` around lines 39 - 48, Expand the firmware command tests around dryRunCommand to cover explicit debug and instrument modes for build, flash, and deploy, including both --mode=debug/instrument and legacy -d/-i selectors. Extend the CoreS3 Codex Voice USB warning assertions to instrument mode while preserving the no-warning checks for release builds and non-CoreS3 targets; retain the required changeset/release-impact documentation and target-specific CoreS3 verification.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@firmware/scripts/lib/firmware-command.test.mjs`:
- Around line 27-48: Update the command tests around dryRunCommand to make
device selection deterministic by explicitly setting or clearing
STACKCHAN_DEVICE for every case, especially the CoreS3 warning assertions.
Ensure the tests that expect CoreS3 behavior invoke the command with an explicit
CoreS3 device, while preserving coverage of the default-device behavior
separately if needed.
---
Nitpick comments:
In `@firmware/scripts/lib/firmware-command.test.mjs`:
- Around line 39-48: Expand the firmware command tests around dryRunCommand to
cover explicit debug and instrument modes for build, flash, and deploy,
including both --mode=debug/instrument and legacy -d/-i selectors. Extend the
CoreS3 Codex Voice USB warning assertions to instrument mode while preserving
the no-warning checks for release builds and non-CoreS3 targets; retain the
required changeset/release-impact documentation and target-specific CoreS3
verification.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 00264384-592a-474a-8d70-de25668b9068
📒 Files selected for processing (6)
.changeset/clean-usb-release-builds.mdfirmware/docs/flashing-firmware.mdfirmware/docs/flashing-firmware_ja.mdfirmware/package.jsonfirmware/scripts/firmware.mjsfirmware/scripts/lib/firmware-command.test.mjs
1 participant
Summary
What Changed
build,flash, anddeployto release mode unless a mode is explicitly selected.debugand MOD commands in debug mode by default.Root Cause
Moddable SDK 9.0.0 changed ESP32-S3 debug builds with
USE_USB=2to use its CDC debugger implementation without claiming UART pins. That implementation still installs the USB Serial/JTAG driver even when xsbug is not launched, so Stack-chan's Codex Voice USB transport cannot install the same driver.Verification
cd firmware && npm run formatcd firmware && npm run lintcd firmware && npm run test:unit— 391 tests passedcd firmware && npm run check:legacy-names/dev/ttyACM1; image hashes verifiedmax_payload=4096 capabilities=0x00000f7fAffected Areas
Breaking Changes
Release Impact
patch— fixes CoreS3 Codex Voice USB initialization for normal build and flash workflows.Related Issues
Summary by CodeRabbit
New Features
Documentation