feat: Add runtime/interrupt package and update volatile package#27
Conversation
Add complete runtime/interrupt package with platform-specific implementations: - interrupt.go: Core interrupt handling interface - checkpoint.go: Checkpoint mechanism for interrupt state management - Platform-specific implementations for AVR, Cortex-M, ESP32-C3, GBA, K210, SiFive, RISC-V, Xtensa Update runtime/volatile package: - Change LLGoPackage from "decl" to "link" for proper linking This change provides a cross-platform interrupt handling abstraction that supports multiple embedded architectures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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 significantly enhances the embedded systems support by introducing 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 introduces a comprehensive runtime/interrupt package, providing a crucial cross-platform abstraction for interrupt handling in embedded systems, along with a minor but important update to the runtime/volatile package. The architecture-specific implementations are extensive and well-structured. My review has identified a critical logic error in the ESP32-C3 implementation's boundary check, as well as some significant issues in the Game Boy Advance implementation regarding type correctness and reliance on a confusing and likely incorrect device peripheral definition. These issues should be addressed to ensure the reliability and maintainability of the new package.
| // ... | ||
| // }).Enable() | ||
| func (i Interrupt) Enable() error { | ||
| if i.num < 1 && i.num > 31 { |
There was a problem hiding this comment.
The condition i.num < 1 && i.num > 31 for checking the interrupt range is incorrect and will always evaluate to false, as a number cannot be simultaneously less than 1 and greater than 31. This effectively disables the bounds check for the interrupt ID. The logical operator should be || (OR) to correctly validate that the ID is outside the valid range of [1, 31].
| if i.num < 1 && i.num > 31 { | |
| if i.num < 1 || i.num > 31 { |
| state = State(gba.INTERRUPT.PAUSE.Get()) | ||
| // Disable all interrupts. | ||
| gba.INTERRUPT.PAUSE.Set(0) |
There was a problem hiding this comment.
This code uses gba.INTERRUPT.PAUSE to disable interrupts. This is very confusing, as the GBA's PAUSE register is for halt control, while the IME (Interrupt Master Enable) register at 0x04000208 should be used for controlling interrupts.
The gba.INTERRUPT_Type struct in device/gba/gba.go appears to have incorrect field offsets and names, causing the PAUSE field to point to the IME register's address. This implementation is error-prone and relies on an underlying bug in the device definition. The device definition in device/gba/gba.go should be corrected to accurately represent the hardware registers, and this code should be updated to use gba.INTERRUPT.IME.
| if jumpPC == 0 { | ||
| panic("jumping to 0") | ||
| } |
There was a problem hiding this comment.
This check for jumpPC == 0 is redundant. The c.Saved() check at the beginning of the function on line 53 already ensures that c.jumpPC is not zero, and jumpPC is assigned this value. This block of code is therefore unreachable and can be removed.
| } | ||
|
|
||
| // State represents the previous global interrupt state. | ||
| type State uint8 |
There was a problem hiding this comment.
The State type is defined as uint8, but it's used to store the value from a 16-bit register (IME, which is accessed via the misnamed PAUSE field). The Get() method on volatile.Register16 returns a uint16. Using uint8 here truncates the value, which could lead to unexpected behavior. To ensure type safety and correctness, State should be defined as uint16.
| type State uint8 | |
| type State uint16 |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| // ... | ||
| // }).Enable() | ||
| func (i Interrupt) Enable() error { | ||
| if i.num < 1 && i.num > 31 { |
There was a problem hiding this comment.
Critical: Logic error in range validation
This condition uses && (AND) when it should use || (OR). A number cannot simultaneously be less than 1 AND greater than 31, so this validation will never trigger.
| if i.num < 1 && i.num > 31 { | |
| if i.num < 1 || i.num > 31 { |
This bug allows invalid interrupt numbers to proceed to line 40 where unsafe pointer arithmetic is performed, potentially causing memory corruption.
|
|
||
| // SetPriority sets the interrupt priority for this interrupt. A lower number | ||
| // means a higher priority. Additionally, most hardware doesn't implement all | ||
| // priority bits (only the uppoer bits). |
There was a problem hiding this comment.
Typo in documentation
| // priority bits (only the uppoer bits). | |
| // priority bits (only the upper bits). |
| return errors.New("interrupt for ESP32-C3 must be in range of 1 through 31") | ||
| } | ||
|
|
||
| // TODO(zzy):https://github.com/goplus/llgo/issues/1290 |
There was a problem hiding this comment.
Security: Missing critical section protection - race condition
The commented-out critical section protection creates a race condition. Multiple read-modify-write operations (lines 36, 39, 43, 44) can be interrupted, potentially corrupting register state.
Impact: If an interrupt fires between any of these operations, the hardware state may become inconsistent.
Recommendation: Until LLGO issue #1290 is resolved, this should be documented as a known limitation in the function documentation, or consider using platform-specific atomic operations if available.
|
|
||
| // Pseudo function call that is replaced by the compiler with the actual | ||
| // functions registered through interrupt.New. If there are none, calls will be | ||
| // replaced with 'unreachablecalls will be replaced with 'unreachable'. |
There was a problem hiding this comment.
Documentation: Duplicate text
| // replaced with 'unreachablecalls will be replaced with 'unreachable'. | |
| // replaced with 'unreachable'. |
| // _ = Interrupt.New(5, func(interrupt.Interrupt) { | ||
| // ... | ||
| // }).Enable() | ||
| func (i Interrupt) Enable() error { |
There was a problem hiding this comment.
API Inconsistency: This is the only platform where Enable() returns an error. All other platforms have func (irq Interrupt) Enable() with no return value.
Recommendations:
- Standardize the API across all platforms (either all return errors or none do)
- Document this platform-specific behavior in the package-level documentation
- Consider using panics for invalid inputs (consistent with embedded runtime patterns) rather than error returns
| // A checkpoint is a setjmp like buffer, that can be used as a flag for | ||
| // interrupts. | ||
| // | ||
| // It can be used as follows: |
There was a problem hiding this comment.
Documentation: Example code references undefined functions
The example calls setupInterrupt() and waitForInterrupt() which are not defined anywhere, making the example non-compilable.
Suggestion: Replace with a concrete example using actual interrupt package functions:
// It can be used as follows:
//
// // global var
// var c Checkpoint
//
// // Set up the checkpoint and configure your interrupt handler.
// // The handler should call c.Jump() when the checkpoint is saved.
// if c.Save() {
// // Returns true on first execution (checkpoint saved)
// myInterrupt := interrupt.New(5, func(interrupt.Interrupt) {
// if c.Saved() {
// c.Jump()
// }
// })
// myInterrupt.Enable()
// for {
// // Wait for interrupt (platform-specific)
// }
// }
// // Returns false after c.Jump() is called from interrupt handler
Code Review SummaryThis PR introduces a well-structured interrupt handling package with good platform separation. However, there are critical issues that must be fixed before merging. Critical Issues (Must Fix)
High Priority
Additional Fixes
Positive Aspects
Please address the critical issues before merging. |
2 participants
Summary
Add
runtime/interruptpackage and updateruntime/volatilepackage, synced from TinyGo v0.39.0 to provide interrupt handling support for LLGO embedded applications.Synced Content
runtime/interrupt/runtime/volatile/Platform Support
The
runtime/interruptpackage provides platform-specific implementations for:LLGO Adaptations
1. Import Path Adaptation
TinyGo uses relative paths, which need to be changed to full module paths.
"device/*""github.com/goplus/emb/device/*""runtime/volatile""github.com/goplus/emb/runtime/volatile"2. volatile Package Adaptation
runtime/volatile/volatile.goneeds to declareLLGoPackage = "link":This change is required for proper linking with LLGO's C interop mechanism.
3. Compiler Directive Placeholders
The following functions in
runtime/interruptrequire LLGO compiler or runtime support, currently usingpanicas placeholders:runtime/interrupt/interrupt.go:24New()runtime/interrupt/checkpoint.go:39Checkpoint.Save()These will be implemented when LLGO compiler adds support for:
Files Changed
11 new files in
runtime/interrupt/:checkpoint.go- Checkpoint mechanism for interrupt stateinterrupt.go- Core interrupt interfaceinterrupt_avr.go- AVR implementationinterrupt_cortexm.go- Cortex-M implementationinterrupt_esp32c3.go- ESP32-C3 implementationinterrupt_gameboyadvance.go- Game Boy Advance implementationinterrupt_k210.go- K210 implementationinterrupt_none.go- Generic/non-baremetal implementationinterrupt_sifive.go- SiFive implementationinterrupt_tinygoriscv.go- Generic RISC-V implementationinterrupt_xtensa.go- Xtensa implementation1 modified file:
runtime/volatile/volatile.go- Updated LLGoPackage constantTotal: +713 lines, -1 line
Dependencies
This PR is extracted from #26 to allow independent review and merging of the runtime interrupt functionality, separate from the machine package updates.
The machine package (PR #26) will depend on this PR being merged first.
🤖 Generated with Claude Code