noInterrupts(), interrupts()

[pgScorpio]

Hardware

Hardware: n.a.
Core Version: 2.2.0

Description

Is there any good reason why we are using xt_rsil(15)/xt_rsil(0) for noInterrupts()/interrupts() ???

Doesn't ets_intr_lock()/ets_intr_unlock() do the same job without the need for storing the result of xt_rsil(...) and calling xt_wsr_ps(..) ???

i use the following class to disable interrupts for some time now without any problems:

    class disableInterrupts       //Locks all maskable interrupts while any instance exists...
    {
    protected:
        static uint32_t _count;

    public:
        disableInterrupts()
        {
            ets_intr_lock();
            _count++;
            if (_count == 0) //Overrun ??
            {
                //Over 4 billion locks ??
                //Something is going terribly wrong !!
                //Lock up... (and trigger the watchdog ?)
                ets_intr_unlock();
                while(1);
            }
        }

        ~disableInterrupts()
        {
            if (--_count == 0)
            {
                ets_intr_unlock();
            }
            else if (_count == 0xFFFFFFFF) //Overrun ??
            {
                //Fatal error !!
                //destructor called more often than constructor ????,  _count must be corrupted !
                //Lock up... (and trigger the watchdog ?)
                ets_intr_unlock();
                while(1);
            }
        }
   };

[igrr]

ets_intr_lock increments a counter and sets PS.INTLEVEL to 2. ets_intr_unlock decrements the counter and sets PS.INTLEVEL to 0 if the counter is 0. We have actually used ets_intr_lock and ets_intr_unlock for a while in noInterrupts/interrupts, but this got changed because level 3 interrupts were not disabled that way.

Current implementation of noInterrupts/interrupts doesn't allow nested noInterrupts/interrupts calls. I have looked through a bunch of libraries and didn't find any use of such nested calls, so overall i think this is okay. But in general, this should be improved.

For the ESP8266, the only values of PS.INTLEVEL which make sense are 1, 2, and 3. Every value above 3 works the same way as 3. So in practice it is sufficient to store the value of PS.INTLEVEL in the first call to noInterrupts and then increment lock count in every successive call to noInterrupts. This would allow correct unwinding of PS.INTLEVEL values.

Now for the ESP32 things get slightly more interesting because we have more interrupt levels and also we have two SMP cores. I'm still deliberating on the best model to use in that case. I'd like to use the same code for both chips, if possible.

[igrr]

Also /cc @Makuna who participated in noInterrupts/interrupts refactoring last time.

[Makuna]

Something we should also research is the use of the AVR asm wrappers.

cli() and sei() which map directly to or they directly map to them.

Further, there is a technique used for Nesting, using the SREG sandwich where you store the SREG (where the global interrupt flag resides), then cli(), then do work, and end by reapplying the saved SREG. You may find this use a little more common.

On the subject of why did we use xt_rsil(15) in the first place. There is one interrupt in the core for WiFi that was causing problems with bitbang timing. This was the only means I found to disable it at the time. BUT, in the end, the Esp8266 really can't have that interrupt disabled for too long. I believe I commented to igrr at some point last fall that we should probably undo that as it could cause worse problems; but we needed investigation into what level was best and it looks like it is happening now.

[pgScorpio]

I checked Bootrom code:

00000f74 <ets_intr_lock>:
     f74:   006320          rsil    a2, 3
     f77:   fffe31          l32r    a3, f70
     f7a:   0329        s32i.n  a2, a3, 0
     f7c:   f00d        ret.

00000f80 <ets_intr_unlock>:
     f80:   006020          rsil    a2, 0
     f83:   f00d        ret.n

Interrupt level 3, and no counter.... ?

[igrr]

You are right, I was looking at a different revision of the code.

[Makuna]

I also want to raise an important issue that a lot of esp8266 developers forget. Igrr does a great job keeping to this; but I want to restate this as lately when dealing with my own Arduino libraries I have to keep reminding users.

We need to make sure we are "logically" compatible with Arduino AVR targeted sketches/libraries. Extensions that are specific to esp8266 are fine; but can't be a requirement to use.

[eshkrab]

Hello!
I'm working on a port of an led library for esp32 and stumbled here looking for info on ets_intr_lock/unlock. @igrr have you made any decisions on how to deal with interrupts on esp32? I'm just trying to disable interrupts from wifi and such during some led operations. Found something about FreeRTOS and not using ets_intr_lock if in that, but I assume Arduino isn't running on top of FreeRTOS

[igrr]

1. you should be able to disable interrupts on the esp32 without issues such as those seen on the 8266
2. use noInterrups/interrupts functions defined in Arduino.h, ets_intr functions are not supported under FreeRTOS
3. for further esp32 related issues, please visit espressif/arduino-esp32 issue tracker.

[eshkrab]

Thank you so much! Yes, sorry, I'll post to esp32 issue tracker next time

[devyte]

@igrr you expressed interest in refactoring the interrupt code here, to unify esp8266/esp32. However, the issue is labeled only as Question.
I've added a code cleanup label. Do you have a milestone for this?

[devyte]

Closing due to lack of feedback in a long time.