feat: Add machine/usb package with USB device class support#29
Conversation
Summary of ChangesHello @luoliwoshang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive USB device support by porting the machine/usb package from TinyGo. The changes mainly consist of adapting import paths to the project's structure. The port is well-structured and follows the original TinyGo implementation closely.
I've reviewed the newly added code and found two potential issues that should be addressed:
- A potential data corruption bug in the CDC
txRingBufferwhen handling large writes. - An incorrect calculation in the MSC driver for the SCSI UNMAP command, which could lead to not all requested blocks being unmapped.
Details and suggestions for fixes are in the comments below.
Adapt all import paths in machine/_usb directory to use full module paths for LLGO compatibility. Changes: - Update "machine" imports to "github.com/goplus/emb/machine" - Update "machine/usb" imports to "github.com/goplus/emb/machine/_usb" - Update "runtime/interrupt" imports to "github.com/goplus/emb/runtime/interrupt" This is step 1 of 2: Import path adaptation with _usb intermediate naming. Affected files: - machine/_usb/* - All USB subpackages (adc/midi, cdc, descriptor, hid, msc) Total: 23 files modified 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| "runtime/interrupt" | ||
| "github.com/goplus/emb/device/sam" | ||
| "github.com/goplus/emb/machine/usb" | ||
| "github.com/goplus/emb/runtime/interrupt" |
There was a problem hiding this comment.
IMPORTANT: Unbounded Busy-Wait During USB Reset
This loop has no timeout and will hang indefinitely if hardware fails to acknowledge the reset.
Issue: The nRF52840 implementation includes a 300,000 iteration timeout for similar operations (machine_nrf52840_usb.go:86-92), but ATSAMD implementations lack this safety mechanism.
Recommendation:
Add a timeout counter similar to the nRF implementation:
timeout := 300000
for (sam.USB_DEVICE.SYNCBUSY.HasBits(sam.USB_DEVICE_SYNCBUSY_SWRST) ||
sam.USB_DEVICE.SYNCBUSY.HasBits(sam.USB_DEVICE_SYNCBUSY_ENABLE)) && timeout > 0 {
timeout--
}
if timeout == 0 {
return errors.New("USB reset timeout")
}
| "machine" | ||
| "machine/usb" | ||
| "runtime/interrupt" | ||
| "github.com/goplus/emb/machine" |
There was a problem hiding this comment.
IMPORTANT: Silent Error Suppression
Errors from ReadByte() are silently ignored, potentially reading zero values instead of actual data.
Recommendation:
Either propagate the error or document why it can be safely ignored:
for i := 0; i < size; i++ {
v, err := usbcdc.ReadByte()
if err != nil {
return i, err
}
data[i] = v
}
| "errors" | ||
| "internal/binary" | ||
| "internal/bytealg" | ||
| "github.com/goplus/emb/internal/binary" |
There was a problem hiding this comment.
Performance: Redundant Volatile Register Reads
Each call to rb.head.Get() is an expensive volatile register read. The value is fetched twice here (lines 31-32 in buffer implementations).
Optimization for embedded systems:
newHead := rb.head.Get() + 1
rb.head.Set(newHead)
rb.buffer[newHead%rxRingBufferSize].Set(val)Additionally, since buffer sizes are powers of 2, use bitwise AND instead of modulo:
const rxRingBufferMask = rxRingBufferSize - 1
rb.buffer[newHead&rxRingBufferMask].Set(val) // Much faster on ARM Cortex-MComplete the USB package migration by: 1. Renaming machine/_usb directory to machine/usb 2. Updating all import paths from "github.com/goplus/emb/machine/_usb" to "github.com/goplus/emb/machine/usb" This is step 2 of 2: Directory rename and final import path updates. This restores the standard package naming convention while maintaining LLGO-compatible import paths. Affected: - 44 files renamed from machine/_usb/* to machine/usb/* - 14 files with updated import paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| "machine" | ||
| "machine/usb" | ||
| "machine/usb/descriptor" | ||
| "github.com/goplus/emb/machine" |
There was a problem hiding this comment.
Code Quality: Global Singleton Without Thread Safety
The global Midi variable is initialized in init() without synchronization. While this may work in single-threaded embedded contexts, it's not future-proof for concurrent access.
Recommendation:
Either:
- Use
sync.Oncefor lazy initialization if concurrent access is possible - Document clearly that this package assumes single-threaded execution
- Add build constraints if this is architecture-specific
Code Review SummaryComprehensive review completed using specialized agents for code quality, performance, security, and documentation. Critical Issues (Must Address)Race Conditions in Ring Buffers: CDC/HID/MIDI buffer operations lack atomic guarantees and proper synchronization, creating data corruption risks in interrupt contexts. See inline comments on Acknowledged Data Corruption: TODO comment in TX buffer indicates known issue when buffer is full ( Unbounded Busy-Wait: ATSAMD21/51 USB reset loops lack timeouts, risking system hangs if hardware malfunctions. Important Issues
Positive Aspects
RecommendationAddress critical race conditions and data corruption risks before merging. Other issues can be tracked as follow-up technical debt. Review completed with code-quality, performance, security, and documentation analysis agents. |
|
|
||
| import ( | ||
| "runtime/volatile" | ||
| "github.com/goplus/emb/runtime/volatile" |
There was a problem hiding this comment.
CRITICAL: Race Condition in Ring Buffer
The Put() operation is not atomic and lacks proper synchronization. Between checking Used() and updating the buffer, an interrupt could modify the buffer state, leading to data corruption.
Issues:
Used()result can become stale between check at line 30 and buffer update at line 32- No interrupt masking around the critical section
- Similar pattern exists in HID and MIDI ring buffers
Recommendation:
Use interrupt.Disable()/Restore() around critical sections (CDC code already uses this pattern elsewhere in usbcdc.go):
func (rb *rxRingBuffer) Put(val byte) bool {
state := interrupt.Disable()
defer interrupt.Restore(state)
if rb.Used() != rxRingBufferSize {
rb.head.Set(rb.head.Get() + 1)
rb.buffer[rb.head.Get()%rxRingBufferSize].Set(val)
return true
}
return false
}
|
|
||
| import ( | ||
| "runtime/volatile" | ||
| "github.com/goplus/emb/runtime/volatile" |
There was a problem hiding this comment.
Performance: Redundant Volatile Register Reads
Each rb.head.Get() is an expensive volatile register read. Line 31-32 performs this twice.
Optimization for embedded systems:
newHead := rb.head.Get() + 1
rb.head.Set(newHead)
rb.buffer[newHead%rxRingBufferSize].Set(val)Further optimization: Since rxRingBufferSize = 128 is a power of 2, use bitwise AND instead of modulo (much faster on ARM Cortex-M without hardware divider):
const rxRingBufferMask = rxRingBufferSize - 1
rb.buffer[newHead&rxRingBufferMask].Set(val)
luoliwoshang
force-pushed
the
feat/machine-usb
branch
from
a704f49 to
36b0a1a
Compare
| "machine/usb/msc/csw" | ||
| "machine/usb/msc/scsi" | ||
| "github.com/goplus/emb/machine" | ||
| "github.com/goplus/emb/machine/usb" |
There was a problem hiding this comment.
Performance: Inefficient Busy-Wait Goroutine
This goroutine wakes 10,000 times per second to check a single boolean flag.
Issues:
- Continuous wakeup prevents deep sleep states (critical for battery-powered devices)
- Adds scheduler pressure in resource-constrained embedded systems
- Goroutine stack (~2KB minimum) permanently allocated
Recommendation:
Use channel-based signaling for event notification:
type msc struct {
// ... existing fields ...
taskChan chan struct{}
}
func (m *msc) processTasks() {
for range m.taskChan {
cmd := m.cbw.SCSICmd()
// ... process task ...
m.queuedBytes = 0
m.taskQueued = false
machine.AckUsbOutTransfer(usb.MSC_ENDPOINT_OUT)
}
}
// In rxHandler, send signal: select { case m.taskChan <- struct{}{}: default: }
2 participants
Summary
Add machine/usb package synced from TinyGo v0.39.0 to provide USB device support for LLGO embedded applications.
This PR extracts the USB package from #26 as an independent component for easier review and merging.
Synced Content
machine/usb/machine/usb/adc/midi/machine/usb/cdc/machine/usb/descriptor/machine/usb/hid/machine/usb/hid/joystick/machine/usb/hid/keyboard/machine/usb/hid/mouse/machine/usb/msc/Plus platform-specific USB implementations:
machine/machine_atsamd21_usb.go- Atmel SAMD21 USBmachine/machine_atsamd51_usb.go- Atmel SAMD51 USBmachine/machine_nrf52840_usb.go- Nordic nRF52840 USBmachine/machine_rp2040_usb.go- Raspberry Pi RP2040 USBmachine/machine_rp2350_usb.go- Raspberry Pi RP2350 USBmachine/machine_rp2_usb.go- Common RP2xxx USB codemachine/machine_rp2040_usb_fix_usb_device_enumeration.go- RP2040 USB enumeration fixPackage Details
USB Device Classes
1. CDC (Communications Device Class)
2. HID (Human Interface Device)
3. MIDI (Musical Instrument Digital Interface)
4. MSC (Mass Storage Class)
USB Descriptors
Comprehensive descriptor builders for:
Implementation Approach
This PR uses a two-step migration approach to ensure clarity in the git history:
Step 1: Import Path Adaptation (Commit 1)
First, we adapt all import paths for the
machine/_usbpackage:"machine"→"github.com/goplus/emb/machine""machine/usb"→"github.com/goplus/emb/machine/_usb""runtime/interrupt"→"github.com/goplus/emb/runtime/interrupt"This creates a temporary intermediate state with
_usbnaming.Rationale: Using
_usbas an intermediate step makes the migration traceable and allows reviewers to clearly see the import path changes separate from the directory rename.Step 2: Directory Rename (Commit 2)
Then, we rename the directory and update imports:
machine/_usb/→machine/usb/"github.com/goplus/emb/machine/_usb"→"github.com/goplus/emb/machine/usb"This restores the standard package naming convention while maintaining LLGO-compatible full import paths.
Result: Clean separation of concerns:
LLGO Adaptations
Import Path Adaptation
All imports updated to use full module paths:
"machine""github.com/goplus/emb/machine""machine/usb""github.com/goplus/emb/machine/usb""machine/usb/cdc""github.com/goplus/emb/machine/usb/cdc""machine/usb/hid""github.com/goplus/emb/machine/usb/hid""machine/usb/msc""github.com/goplus/emb/machine/usb/msc""runtime/interrupt""github.com/goplus/emb/runtime/interrupt"No Other Changes Required
The USB packages are pure Go implementations and work directly with LLGO without additional modifications. They do not use:
Files Changed
Two commits with clear separation:
Commit 1: Import path adaptation
_usbintermediate stateCommit 2: Directory rename and final updates
_usb→usband update all referencesTotal files in USB package:
Platform Support
The USB package supports multiple microcontroller platforms:
Each platform has dedicated USB controller integration with:
Dependencies
This PR is extracted from #26 to allow independent review.
Dependency chain:
Test Plan
Migration Strategy
The two-commit approach provides:
This follows the principle of "separate concerns" in version control history.
🤖 Generated with Claude Code