tinygo icon indicating copy to clipboard operation
tinygo copied to clipboard

machine: remove level triggered pin interrupts

Open aykevl opened this issue 2 years ago • 4 comments

This removes level-triggered interrupts.

While working on https://github.com/tinygo-org/tinygo/pull/3170, I found these level triggered interrupt constants. Apart from them being inconsistent with each other (PinLowLevel vs PinLevelLow) I don't think they are actually used anywhere. In addition, I removed the PinNoInterrupt constant on the esp32c3 as there is already a generic way to disable interrupts. This makes the esp32c3 pass the tests in #3170.

I looked into level-triggered interrupts and I really couldn't find a good justification for them:

  • They were added to the esp32c3 and the rp2040 together with other pin interrupt types, meaning they were probably just added because the chip supports the feature and not because they were actually needed.
  • Level interrupts aren't supported in TinyGo for any other chip, and I haven't seen anybody ask for this feature.
  • They aren't supported in the nrf series chips at all, and with a quick search I found only very little demand for them in general.
  • I tried to see whether there is any good use case for them, but I couldn't really find one (where an edge triggered interrupt wouldn't work just as well). If there is one where level triggered interrupts are a real advantage over edge triggered interrupts, please let me know.

Of course, we shouldn't remove a feature lightly. But in this case, I can't think of an advantage of having this feature. However I can think of downsides: more maintenance and having to specify their behavior in the machine package documentation. In general, I would like to keep the machine package clean and only support things that have a proven use case.

I have tested PinRising and PinFalling and they both still work on the esp32-c3 and the rp2040.

@soypat @ardnew @zdima

aykevl avatar Sep 19 '22 17:09 aykevl

...and now I've discovered another chip that implements level-triggered interrupts: the mimxrt1062 (teensy40, teensy41). Again using different constants: PinLow and PinHigh. This once again shows the need for automatic checking against the documentation.

aykevl avatar Sep 19 '22 18:09 aykevl

The removal sounds good to me- I had not investigated the matter and added them only because I thought they were a desirable and novel feature. Had no idea that other boards had similar implementations.

I approve of the changes to the rp2040 implementation. LGTM!

soypat avatar Sep 20 '22 18:09 soypat

LGTM, I don't even remember adding them 🙃

ardnew avatar Sep 20 '22 20:09 ardnew

The only scenario where level interrupt might help is when many sources using the same interrupt line. In such case edge level may not work. Not sure how practical this is.

zdima avatar Sep 21 '22 01:09 zdima