tinygo-org / tinygo

Go compiler for small places. Microcontrollers, WebAssembly (WASM/WASI), and command-line tools. Based on LLVM.
https://tinygo.org
Other
15.31k stars 905 forks source link

proposal: interrupt API #515

Closed aykevl closed 4 years ago

aykevl commented 5 years ago

We need to define an API to interact with interrupts in TinyGo in an idiomatic way. At the moment, there is some support but they rely on magic comments and are hard to use correctly. For example:

//go:export RTC1_IRQHandler
func handleRTC1() {
    // ...
}

Here are a few requirements I could come up with, for such an API:

This is the design I came up with uses two different kinds of interrupt handlers, as I recognize there are certain interrupts that are easier to use in one style or in the other:

Here is my proposed API in pseudo-Go:

// Low level interrupt style
type runtime.Interrupt struct { ... }
func runtime.NewInterrupt(interruptNumber uintptr, handler func()) runtime.Interrupt {}
func (i runtime.Interrupt) Enable() {}
func (i runtime.Interrupt) Disable() {}
func (i runtime.Interrupt) IsEnabled() {}
func (i runtime.Interrupt) SetPriority(priority uint8) {} // TODO: how should priority numbers be specified?

// High level interrupt style
func runtime.BlockOnInterrupt(interruptNumber uintptr) {}

This system will have some limitations:

The high level implementation could be implemented by a compiler transformation that inserts a new interrupt handler and suspends/resumes the goroutine at the right time. Multiple suspend points should be allowed (unlike runtime.NewInterrupt).

An example of the low-level API:

var rtc1Interrupt = runtime.NewInterrupt(nrf.IRQ_RTC1, func() {
    // handler code
})
func init() {
    rtc1Interrupt.SetPriority(prio) // TODO: how should priorities be numbered?
    rtc1Interrupt.Enable()
}

An example of the high-level API:

func (spi SPI) Transfer(w byte) (byte, error) {
    // send byte
    for /* has byte been sent? */ {
        runtime.BlockOnInterrupt(nrf.IRQ_SPI0)
    }
    // read response byte
}

The compiler may eventually learn to rewrite high-level blocking operations to low-level interrupt handlers, if it can determine this would result in equivalent behavior. An example would be a single-function goroutine that is started once at startup and only ever blocks on a single interrupt, with no external calls.

Problems I see now with this proposal:

niaow commented 5 years ago

Another consideration is how to deal with things that should not be interrupted. We may need to add EnableInterrupts() and DisableInterrupts() somewhere. When a goroutine enters the scheduler, interrupts should be disabled, and the goroutine's interrupt state should be saved. This would allow low-level code to safely avoid TOCTOU.

After resuming, a goroutine should return to its previous interrupt enabled state.

On ARM and AVR at least, a temporary interrupt masking operation is one instruction to start/stop.

kylelemons commented 5 years ago

Not-super-well-formulated thoughts below:

I don't see anything immediately problematic about the API, but I'm a bit unsure about the implementation that you're envisioning. Will each runtime package need to implement the same API? Is the compiler going to have to be taught about each individual architecture? Is there going to be a naming convention for assembly implementations of the intrics that the compiler and common runtime code will depend on to make these work?

Writing code for the GBA, it was super refreshing to be able to register an interrupt handler with a function that closed around local variables. I am not sure I would be able to do this with the envisioned API, and I think that's a shame.

I'm also not sure if two levels of interrupt handling are sufficient to cover the spectrum of handling someone might want even within a GBA game. Specifically for the GBA, you can choose a number of different levels of interrupt handling: you can have anything from a simple interrupt handler that runs in IRQ mode (and its 160-byte stack!) under the ARM instruction set to a full-blown reentrant interrupt handler that executes in system mode (with user stacks) under the THUMB instruction set. What I have currently implemented in #494 is somewhere in between: it runs a user-specified THUMB interrupt handler in system mode, but interrupts are disabled while it is running. It would be possible to implement user-specified interrupt priorities, but I don't immediately know how to do that in a way that doesn't waste cycles, though I'm sure it can be done.

In line with this design, the GBA has a few BIOS functions (which I have not implemented yet at all) that can interact with the interrupt mechanisms in the way you describe for the high-level interrupts. In particular, VBlankIntrWait is used to halt the CPU in a low-power state until the next V-Blank. For arbitrary interrupts there is IntrWait, and for the general "wait for anyone's interrupt to fire" case there is Halt. Support for these will require both adding bios-interrupt acks to the interrupt handler itself, but would also presumably require some way to ask the runtime to unblock any Gs that are blocked waiting on them, which isn't something I've yet dug into. It certainly seems like these could be the basis for a number of scheduler primitives, including time.Sleep.

With respect to Enable/Disable interrupt functions: it might be interesting to have a general runtime.CriticalSection(func() { ... }) mechanism that suspends interrupts while it is executing, but I have bitten myself enough by interrupt enable/disablement that I would be worried about things like injected yields breaking these up and causing problems because of cooperative scheduling. They exist in the GBA implementation for the moment, though.

For saving/restoring interrupts on a per-goroutine basis, I'm not sure I understand how this would work. If my goroutine is waiting for a V-Blank interrupt, that can't be saved away while another goroutine that doesn't care about a V-Blank is running, or my goroutine will never wake up.

aykevl commented 5 years ago

Another consideration is how to deal with things that should not be interrupted. We may need to add EnableInterrupts() and DisableInterrupts() somewhere.

Good point. Such functions have already been implemented for Cortex-M in "device/arm" but should perhaps be moved to the machine package with a standard interface.

When a goroutine enters the scheduler, interrupts should be disabled, and the goroutine's interrupt state should be saved. This would allow low-level code to safely avoid TOCTOU.

I think disabling/enabling interrupts is a very low level operation (unsafe, even) that should only be done in limited circumstances, and never be interrupted by the scheduler. I think it's a bug in the program to perform a blocking operation (invoking the scheduler) while in a critical section. With good design, most uses of it should not be necessary.

One case where it is necessary is in the driver for the WS2812. Sending the bits out is currently done with bitbanging because these chips have very tight timing constraints and interrupts during this operation will usually result in white flickering. Certainly no scheduler operation should be done during that time.

EDIT: the primitives that are usually used in an RTOS look like the following:

mask := DisableInterrupts()
// critical section
EnableInterrupts(mask)

In Go, it might even be used as follows:

mask := DisableInterrupts()
defer EnableInterrupts(mask)
// critical section

Or even shorter, but perhaps less readable:

defer EnableInterrupts(DisableInterrupts())
// critical section

doing it this way allows nested critical sections. It enables calling a function that also has a critical section, without that critical section enabling interrupts again (silently removing the protection offered by the critical section).

With respect to Enable/Disable interrupt functions: it might be interesting to have a general runtime.CriticalSection(func() { ... }) mechanism that suspends interrupts while it is executing [...]

I've considered that one too. It is possible, but I'm not sure whether that is easier to read than the examples above. The effect should be the same.

aykevl commented 5 years ago

Will each runtime package need to implement the same API?

No, when the underlying system is different that should be reflected in the API. But where it is similar, the API should be the same. For example, the Enable and SetPriority methods will be implemented for Cortex-M, but I believe that AVR does not support enabling/disabling specific interrupts let alone setting a priority. This means that such methods would not be implemented.

Is the compiler going to have to be taught about each individual architecture?

Possibly. There is architecture-specific code in the compiler already and in general it's almost impossible to write a useful compiler without some knowledge of the architecture baked in the compiler (even when using LLVM). In the case of the GBA, it may be possible to avoid compiler changes entirely.

Is there going to be a naming convention for assembly implementations of the intrics that the compiler and common runtime code will depend on to make these work?

This is an implementation decision. However, I think assembly is not needed in most cases so that sidesteps that issue entirely.

Writing code for the GBA, it was super refreshing to be able to register an interrupt handler with a function that closed around local variables. I am not sure I would be able to do this with the envisioned API, and I think that's a shame.

Can you give an example where that is necessary? I'm not saying that closures will never be possible. They probably will, but not initially to avoid complexity.

I'm also not sure if two levels of interrupt handling are sufficient to cover the spectrum of handling someone might want even within a GBA game.

Please realize that the GBA is, to be fair, a niche platform. It would be great to have good support in TinyGo and it is very useful to make sure the API is flexible enough, but at the end of the day what is important is supporting current generation hardware.

Regarding IntrWait and the like: I think they may be useful but maybe not. It might be easier to implement it manually using the standard interrupt system of the GBA.

niaow commented 5 years ago

@aykevl

I think it's a bug in the program to perform a blocking operation (invoking the scheduler) while in a critical section. With good design, most uses of it should not be necessary.

Not being able to safely yield prevents interrupt blocking from using any normal blocking systems, and forces all goroutine-interrupt interactions to use a specialized mechanism inside the scheduler, because we cannot guarantee that the interrupt will not fire before the current goroutine is yielded. This prevents doing things like non-blocking sends to buffered channels from an interrupt, and forces us too use two separate resume mechanisms - one for things blocked on interrupts, and one for everything else.

This might be a reasonable tradeoff to make. It is just something important to think about.

kylelemons commented 5 years ago

Will each runtime package need to implement the same API?

No, when the underlying system is different that should be reflected in the API. But where it is similar, the API should be the same. For example, the Enable and SetPriority methods will be implemented for Cortex-M, but I believe that AVR does not support enabling/disabling specific interrupts let alone setting a priority. This means that such methods would not be implemented.

Ah, ok so I think that would help a lot. I didn't get this from the overall description. Is the idea that the interrupts APIs all have the same shape or would there be e.g. scheduler primitives that require them (or some of them)?

Is the compiler going to have to be taught about each individual architecture?

Possibly. There is architecture-specific code in the compiler already and in general it's almost impossible to write a useful compiler without some knowledge of the architecture baked in the compiler (even when using LLVM). In the case of the GBA, it may be possible to avoid compiler changes entirely.

Is the compiler change only required for recognizing the exports declarations?

Is there going to be a naming convention for assembly implementations of the intrics that the compiler and common runtime code will depend on to make these work?

This is an implementation decision. However, I think assembly is not needed in most cases so that sidesteps that issue entirely.

Writing code for the GBA, it was super refreshing to be able to register an interrupt handler with a function that closed around local variables. I am not sure I would be able to do this with the envisioned API, and I think that's a shame.

Can you give an example where that is necessary? I'm not saying that closures will never be possible. They probably will, but not initially to avoid complexity.

I think my favorite example would be DMA/timers, and a motivating use-case would be sound. For sound, as long as you are playing a contiguous waveform it's typically sufficient to let a DMA controller fill it for you, particularly if it's repeating, but if you need to change waveforms (to stitch together different ones or to change them or generate them) you need your interrupt handler to execute corresponding to the DMA complete or timer overflow. Over the course of a program or game's life, it feels easier to be able to register methods or closures that maintain references to the necessary memory locations or waveform snippets rather than arrange for them to be set in global variables at the right time.

I'm also not sure if two levels of interrupt handling are sufficient to cover the spectrum of handling someone might want even within a GBA game.

Please realize that the GBA is, to be fair, a niche platform. It would be great to have good support in TinyGo and it is very useful to make sure the API is flexible enough, but at the end of the day what is important is supporting current generation hardware.

Haha, yeah I definitely recognize that it's a niche architecture, and an old one at that. My concern comes from the opposite side, really: the API that you propose doesn't give me, as the implementer, a lot of guidance on what will be expected of my interrupt handlers. I don't think the GBA is unique here, either, though perhaps the ARM vs THUMB distinction is less common. For example, it seems reasonable to request that all implementations implement switching into the correct mode for user stacks. I'm less sure whether implementations should allow nested interrupts or if they should buffer interrupts that are received while another is executing. This is certainly going to be something that is easy (free?) if you have an NVIC, but will require extra work on less powerful architectures like the GBA or MSP430.

Regarding IntrWait and the like: I think they may be useful but maybe not. It might be easier to implement it manually using the standard interrupt system of the GBA.

Hmm, interesting; to my eye, it looks like they are precisely the kind of high-level interrupt API that you are suggesting above. The user code is asking to have its execution resumed when an interrupt is received, no? That seems like precisely how the BIOS functions work, though I think you may be saying that in a cooperatively scheduled environment it's unfair to use a BIOS suspend call directly from user code because you're also making a decision for all other coroutines.

LarryPfeffer commented 5 years ago

For completeness, the interrupt API ought to support non-maskable interrupt(s) for CPUs that have them, plus the ability to write the non-maskable interrupt-service routine in tinyGo. Since the core purpose of an NMI is urgency (it's the input that the CPU, by design cannot ignore), the NMI's service routine might need to be special-cased for a few things where one might otherwise tolerate short delays.

kylelemons commented 5 years ago

OK, so I played around with implementing the low-level version of this API for GBA, and then trying to port my dumb interrupt test cart over to it.

Some thoughts from this exercise:

  1. Init Cycles

    The global / init context requirement can rapidly leads to init cycles

    I want to write something like

    timer3 = runtime.NewInterrupt(machine.INT_TIMER3, func() {
        mark[machine.INT_TIMER3].Set(1)
        timer3.Disable()
    })

    but you can't do this:

    interrupts.go:56:2: initialization cycle for timer3
    interrupts.go:56:2:     timer3 refers to
    interrupts.go:56:2:     timer3

    This will come up any time you want an interrupt to disable itself or if you have interrupts that circularly refer to one another (e.g. a V-Blank interrupt handler that refers to a DMA interrupt whose handler refers to a timer handler that refers to the V-Blank interrupt).

    You can't resolve this by referring to a global function instead of passing in a closure either, you're effectively stuck. For my purposes I had to bypass the API and write directly to the control register:

    timer3 = runtime.NewInterrupt(machine.INT_TIMER3, func() {
        mark[machine.INT_TIMER3].Set(1)
        machine.Interrupts.Request.ClearBits(1 << machine.INT_TIMER3)
    })

    Proposal: Add a boolean return value so that functions can unregister themselves by returning false.

    func NewInterrupt(uintptr, func() bool) Interrupt

    This won't solve all cycles, but it will cover the most-obvious one.

  2. Repetitiveness

    My interrupt handlers got super boilerplatey, particularly for timers. This is because I have to write a separate function for each handler, I can't write one handler that handles all timer interrupts.

    This is likely to come up when handling interrupts from different (particularly hierarchical) timers, encoders, DMA, etc. It is not uncommon that different interrupts will share similar logic, and it can be very convenient to have switch statements but otherwise share code without having to introduce your own level of abstraction.

    Proposal: Pass the interrupt uintptr that's being handled to the interrupt handler.

    func NewInterrupt(uintptr, func(uintptr)) Interrupt

  3. Enable/DisableInterrupts

    From an implementer standpoint, I became unsure about how to handle these. Do these disable interrupts globally, but not touch the interrupt request flags or the peripheral interrupt source flags? I was initially going to encode the global interrupt enable and interrupt request flags into the uintptr, but then I realized that this introduces the question of how the restore works. Does it OR itself into the request flags, allowing new interrupts to be enabled during the critical section? This is what I was leaning towards, so that runtime.Interrupt.Enable() could use it, but then Disable would still have to do it itself.

    In the end, I only had these save/restore the global interrupt enable bit, which seems fine.

I also ran into some weird things that I can't really put my finger on and are probably unrelated to the API (it looks like my loop was unrolled but in a very strange way, it is very easy to get corrupted binaries with innocent code changes, and for reasons I can't explain the jump return to the interrupt service return throws a warning at runtime after the refactor), but which all seem to have to do with the increased confidence that the optimizer has given the fact that so much is known at compile time.

Edit: Added API Examples for proposals.

iansmith commented 5 years ago

you guys are all more familiar than me on this topic, so if you think is trash you won't hurt my feelings by saying so.

The interrupts on RPI3 (cortex-m 53) come in four flavors, four sets of the four flavors (one for each execution level), and two priorities. When on the "fast priority" you generally don't need to spill the registers to service the interrupt--there is a special set of registers for this. Further, there are some interrupts that are "system" and can be handled by any core, and "local" which can only be handled by a particular core. All of this can be configured by software, naturally....The docs list something like 100 different interrupt sources.

Seems to me that the right thing to do is to create a really small, simple way to do obvious things with interrupt handlers, and not try to cover all the cases and be general. I hope the above convinces you the the rpi3 alone has a lot of weirdnesses. What I did for the simple case was basically allowed the user to pass a function that got called when the interrupt was handled and if that function returned some non-zero value then it would be called again, otherwise it was cancelled. I didn't try to allow the user to turn interrupts back on during their handler and if they did a blocking op it would be "bad". Giving the user a way to get notified on timers and when input is available seems like the two simple cases we should be worried about....

deadprogram commented 4 years ago

I mostly like the syntax that @aykevl proposed:

func SomeHandler() {
    mask := DisableInterrupts()
    defer EnableInterrupts(mask)
    // do some critical thing here...
}

Probably better to name the function BlockOnInterrupt() as something more obvious like WaitForInterrupt().

Still reading thru the rest of the comments here, great research!

deadprogram commented 4 years ago

Regarding the questions raised by @kylelemons I think those can be resolved in similar fashion as what you suggest, but I propose a slightly different syntax:

type InterruptHandler func(Interrupt)

func runtime.NewInterrupt(interruptNumber uintptr, handler runtime.InterruptHandler) runtime.Interrupt {}

This should allow having a single handler defined for multiple interrupts:

func disableHandler(irq Interrupt) {
     mark[irq.String()].Set(1)
     irq.Disable()
}

timer2 := runtime.NewInterrupt(machine.INT_TIMER2, disableHandler)
timer3 := runtime.NewInterrupt(machine.INT_TIMER3, disableHandler)
aykevl commented 4 years ago

(sorry for getting back to this so late...)

@jaddr2line

Not being able to safely yield prevents interrupt blocking from using any normal blocking systems, and forces all goroutine-interrupt interactions to use a specialized mechanism inside the scheduler, because we cannot guarantee that the interrupt will not fire before the current goroutine is yielded. This prevents doing things like non-blocking sends to buffered channels from an interrupt, and forces us too use two separate resume mechanisms - one for things blocked on interrupts, and one for everything else.

I don't really understand the concern. Critical sections should always be short chunks of code, with few or no function calls. Critical sections also shouldn't be used normally, only for very special-purpose operations. The only things I can think of now is bitbanged drivers such as the ws2812 or the DHT22 (etc.) line of sensors. So in practice, almost no code will use critical sections. Invoking blocking operations while in the scheduler subverts the whole purpose of the critical section itself, because all time guarantees are lost.

aykevl commented 4 years ago

Is the idea that the interrupts APIs all have the same shape or would there be e.g. scheduler primitives that require them (or some of them)?

Yes, I think so. This will become clearer while implementing this system.

Is the compiler change only required for recognizing the exports declarations?

Depends on the interrupt system. Cortex-M will probably require more compiler changes.

Regarding IntrWait and the like: I think they may be useful but maybe not. It might be easier to implement it manually using the standard interrupt system of the GBA.

Hmm, interesting; to my eye, it looks like they are precisely the kind of high-level interrupt API that you are suggesting above. The user code is asking to have its execution resumed when an interrupt is received, no?

No, because while one goroutine may be waiting for the interrupt, another may be waiting for another interrupt. The scheduler should wait until the first interrupt fires and awake the appropriate goroutine. Using these BIOS functions may miss an important interrupt for one goroutine while waiting on another interrupt.

aykevl commented 4 years ago

For completeness, the interrupt API ought to support non-maskable interrupt(s) for CPUs that have them, plus the ability to write the non-maskable interrupt-service routine in tinyGo. Since the core purpose of an NMI is urgency (it's the input that the CPU, by design cannot ignore), the NMI's service routine might need to be special-cased for a few things where one might otherwise tolerate short delays.

I think that should fit in well with the rest of this API. Many interrupts would like to have the least possible delay before invoking the relevant handler.

aykevl commented 4 years ago

Responding to https://github.com/tinygo-org/tinygo/issues/515#issuecomment-524133094 (@kylelemons) who brings up some very good points.

Init cycles and repetitiveness are indeed real issues. I have seen init cycles as well when I tried experimenting with an API. I think a way to solve this is by passing runtime.Interrupt as a parameter to the handler. This means they don't map directly to Cortex-M interrupt handlers but that's not a big problem, the compiler could insert wrapper functions. Also, inlining and dead-code elimination should take care of the extra cycles that would otherwise be introduced. And when the compiler introduces wrapper functions anyway, it can also handle closures.

So you'd have something like the following:

func runtime.NewInterrupt(interruptNumber uintptr, handler func(intr runtime.Interrupt)) runtime.Interrupt {}

Adding a return to disable this interrupt would not be necessary, as the handler can directly disable the interrupt with the Interrupt object.

EDIT: reading further comments, looks like @deadprogram came up with the same idea. However, the disadvantage of type InterruptHandler func(Interrupt) is that you would need to cast the function type to that type before passing it to NewInterrupt. But the general idea is the same :)

Regarding disabling/enabling interrupts:

In the end, I only had these save/restore the global interrupt enable bit, which seems fine.

Yes, the idea is that they only disable/enable the taking of interrupts. AFAIK even in Cortex-M, interrupts are still triggered when they are disabled but the NVIC will simply mark them as pending, until interrupts are enabled again. No configuration is changed anywhere.

aykevl commented 4 years ago

See https://github.com/tinygo-org/tinygo/pull/782 for a working proposal, implementing only the Cortex-M bits. The API is as follows:

// Configure the UART.
func (uart UART) Configure(config UARTConfig) {
    // ...

    // Enable RX IRQ.
    intr := interrupt.New(nrf.IRQ_UART0, func(intr interrupt.Interrupt) {
        UART0.handleInterrupt()
    })
    intr.SetPriority(0xc0) // low priority
    intr.Enable()
}

This API should avoid most of the init cycle issues. The interrupt.New call is evaluated at compile time. That may seem counter-intuitive, but it should work fine: the program wouldn't receive an interrupt before this "call" anyway because the interrupt is still disabled. This also means you can't use variables from the outer scope inside the interrupt handler (closures).

aykevl commented 4 years ago

I'm still working on this. While the system I implemented works for many use cases, it breaks down in more complex situations where there is more than one implementation of a given peripheral (and thus more than one interrupt handler).

I'm also trying to find a way that allows the compiler to optimize away unused interrupt handlers in a safe way. This is done in the example in my previous comment (if UART.Configure is unused and thus optimized away, the entire interrupt handler is not defined) but I'm still struggling to make such an optimization work in more complex use cases where you separate the interrupt ID and the interrupt configuration. Optimizing away unused interrupt handlers would allow us to more freely use them throughout the machine API without worrying about code size bloat.

aykevl commented 4 years ago

I think I have a solution, with basically the same API:

var UART2 = UART{
    Buffer: NewRingBuffer(),
    Bus:    sam.SERCOM5_USART,
    SERCOM: 5,
}

func init() {
    // Work around circular definitions.
    UART2.interrupt = interrupt.New(sam.IRQ_SERCOM5, UART2.handleInterrupt)
}

And then in the Configure call, it's as simple as calling uart.interrupt.Enable() to enable the given interrupt. I found a way to optimize away all UART code when none of the UARTs are in use, and optimize away the other UART when only one is in use. Interrupts are optimized away when they're unreferenced (no Enable call mainly). Looking at the IRQ handler function after optimization, I doubt it can be made much more efficient.

The code is still a mess, I'll push to the interrupts branch (#782) when ready.

Thoughts? Comments?

bgould commented 4 years ago

This latest iteration of the proposal looks very clean from an API standpoint. Also I think the level of expressiveness it provides to the compiler could prove to be useful if you ever want to enforce restrictions at compile time on what can be done in an interrupt handler. I can imagine that could be a way to deal with interrupt-related edge cases in the scheduler or garbage collector if they come up.

Also the runtime/interrupt package might be a good place to provide a hardware-agnostic way to disable and enable interrupts globally

aykevl commented 4 years ago

Also I think the level of expressiveness it provides to the compiler could prove to be useful if you ever want to enforce restrictions at compile time on what can be done in an interrupt handler.

I didn't even think about that, but that's a very good idea. With the handlers statically known the compiler could check for code that does things that are not allowed in an interrupt handler (either in a best-effort manner or in a strict manner failing when it can't be sure the code is safe).

Also the runtime/interrupt package might be a good place to provide a hardware-agnostic way to disable and enable interrupts globally.

Good idea, although that can technically be done separate from this proposal.

aykevl commented 4 years ago

I have pushed my changes to #782. Not all code has been converted to the new API yet (in particular, atsamd51 targets) but so far I'm seeing small but significant reductions when I do a make smoketest. In particular, for atsamd21 builds RAM consumption goes down 144 bytes in many cases. I think the compiler manages to optimize out the UART receive buffer now that the interrupt is not needed anymore.

deadprogram commented 4 years ago

A few last questions, before we declare victory on this API design:.

aykevl commented 4 years ago

Definitely something I want to try before merging. This API should make it easy to expose pin interrupts in a clean way.

How will we handle interrupts such as those on the GBA InterruptRegs

I have a branch (interrupts-gba) to demonstrate it is possible. This commit adds support for interrupts to the GBA using that API: https://github.com/tinygo-org/tinygo/commit/ebde8b58751c120ad6358e753d86928034af8e07. It can be used for VSync interrupts in the following way: https://gist.github.com/aykevl/5a9f48082a74579fd9e10efa4f20257a. Even though there is maybe a slight mismatch it actually works surprisingly well.

@kylelemons also for you, what do you think of it? I intentionally made the interrupt support as simple as possible so it could be extended in the future if needed. In particular, it doesn't do stack switching (it should have a 1kB stack as set in the startup code), no nested interrupts, and no auto-enabling of specific interrupt sources in peripherals (to match other platforms such as Cortex-M). The code in the gist above compiles down to this:

   code    data     bss |   flash     ram
    652       4    3188 |     656    3192

Somehow the compiler even managed to inline the update function into the interrupt handler, which I find interesting.

What about interoperating with CGo code that does its own IRQ handling such as https://github.com/hathach/tinyusb

That should work well (untested). The only issue I see is that the code uses the _Handler suffix (such as USB_0_Handler) instead of _IRQHandler as used in TinyGo, but that should be trivial to adjust.

Will this help us with implementation of RISC-V interrupts both CLINT and PLIC types?

That's what I want to verify next with a bare bones implementation (just like I did for the GBA).

aykevl commented 4 years ago

One gotcha with the implementation in #782 is that when you use bound methods, the method must have a pointer receiver. For example, for this interrupt:

var UART2 = UART{...}
func init() {
    UART2.interrupt = interrupt.New(sam.IRQ_SERCOM5, UART2.handleInterrupt)
}

handleInterrupt can look like this:

func (uart *UART) handleInterrupt(interrupt.Interrupt) { ... }

but not like this:

func (uart UART) handleInterrupt(interrupt.Interrupt) { ... }

The reason is that the former directly passes &UART1 around as a pointer and the latter makes a copy of UART1 and stores that in a heap-allocated object. That's just the way that Go works, and I'm not sure whether it's possible to make that work with the current implementation (while still providing optimal performance).

deadprogram commented 4 years ago

As long as we document that, it is really no big deal.

aykevl commented 4 years ago

I have implemented this API for the RISC-V PLIC, see the last 2 commits in this branch: https://github.com/tinygo-org/tinygo/compare/dev...interrupts-riscv

The implementation is very different but the (external) API is identical. Basically, it generates code like this at compile time:

func callInterruptHandler(id uintptr) {
    switch id {
    case sifive.IRQ_UART0:
        UART0.interruptHandler(3)
    case ...:
        // ...etc
    default:
        // do nothing
}

But it only generates cases for interrupts that are registered somewhere in the program.

I think the GBA could use the same mechanism as it also uses software vectoring.

The proposed RISC-V CLIC (Core Local Interrupt Controller) looks very close to the Cortex-M interrupt system with the major difference being that the interrupt handler must manually push and pop used registers (for which there is __attribute__((interrupt)) in C and something similar in LLVM). I see no problems with supporting a system like that.

aykevl commented 4 years ago

Two updates.

First, I made a new branch for GBA interrupt support (interrupts-gba-2) that bases itself on the RISC-V software interrupt vector. Code size is reduced by 20 bytes and (more importantly) RAM usage is reduced by 112 bytes with the moving block gist I posted above. The code in that branch will need some cleaning up, though.

Second, I implemented pin change interrupts based on #782, you can see it in the interrupts-pins branch. It only works for nrf so far, I'm investigating samd21 support. The API looks like this:

func main() {
    machine.BUTTONA.SetInterrupt(machine.PinRising, func(p machine.Pin) {
        println("pin was changed:", p)
    })
    // ...
}
aykevl commented 4 years ago

I've managed to implement external interrupts on the samd21 too. The EIC is just another peripheral interrupt really, just like UART/RTC/etc. Unfortunately I've been unable to optimize away the mapping from EXTINT or GPIOTE channel to function handler, nor do I see a way to do it reliably. I guess there's just no way around it (a different interrupt API won't help here).


However, that means the API has been proven to work. So now it's maybe time for some bikeshedding. Which do you like more:

    // Enable RX IRQ.
    intr := interrupt.New(nrf.IRQ_UART0, func(intr interrupt.Interrupt) {
        UART0.handleInterrupt()
    })
    intr.SetPriority(0xc0) // low priority
    intr.Enable()

Or:

    // Enable RX IRQ.
    intr := nrf.IRQ_UART0.Register(func(intr interrupt.Interrupt) {
        UART0.handleInterrupt()
    })
    intr.SetPriority(0xc0) // low priority
    intr.Enable()

The former is what is implemented in #782 but the latter seems slightly more readable to me. Any preferences?

Also, I think we need to do something about interrupt priorities. Perhaps just by defining some constants in the machine package. At the moment it just uses the priority numbers of the underlying system, which don't even count the same way (number 0 means highest on Cortex-M but disabled entirely on RISC-V). I think this can be fixed in a separate proposal/PR.

aykevl commented 4 years ago

For completeness, the interrupt API ought to support non-maskable interrupt(s) for CPUs that have them, plus the ability to write the non-maskable interrupt-service routine in tinyGo. Since the core purpose of an NMI is urgency (it's the input that the CPU, by design cannot ignore), the NMI's service routine might need to be special-cased for a few things where one might otherwise tolerate short delays.

I've been thinking NMI should be handled separately after all. My idea would be to handle NMI in a similar way as a HardFault or uncaught Go panic: problems you cannot reasonably recover from without a system reset.

bgould commented 4 years ago

However, that means the API has been proven to work. So now it's maybe time for some bikeshedding. Which do you like more:

Overall this is my preference I think:

  // Enable RX IRQ.
  intr := interrupt.New(nrf.IRQ_UART0, func(intr interrupt.Interrupt) {
      UART0.handleInterrupt()
  })
  intr.SetPriority(0xc0) // low priority
  intr.Enable()

I find this slightly more readable personally. Also it seems closer to the signature of signal.Notify() from the standard library os/signal package, and so might be kind of familiar to Go programmers.

That said, the other version could allow for defining an interface based on the Register() function; however I can't think off hand if that would even be useful and I wouldn't be surprised if the special handling of interrupts by the compiler would obviate that possibility anyhow.

I don't have strong feelings about this, just a slight preference for the way you already have it; ultimately I think either way would make for a good clean API.

aykevl commented 4 years ago

@bgould Thanks for the feedback!

That said, the other version could allow for defining an interface based on the Register() function; however I can't think off hand if that would even be useful and I wouldn't be surprised if the special handling of interrupts by the compiler would obviate that possibility anyhow.

Unfortunately that's not possible in the current design as the Register/New calls are treated specially by the compiler.

deadprogram commented 4 years ago

Now that https://tinygo.org/compiler-internals/interrupts/ is implemented, I am going to close this issue. Thanks everyone!