noInterrupts() and interrupts() potential problem

[bbx10]

I think there is a problem with disabling/restoring interrupts in some cases. Interrupts may be disabled permanently.

noInterrupts/interrupts saves and restores interrupt state to a global.

#define xt_disable_interrupts(state, level) __asm__ __volatile__("rsil %%0," __STRINGIFY(level) : "=a" (state))
#define xt_enable_interrupts(state)  __asm__ __volatile__("wsr %%0,ps; isync" :: "a" (state) : "memory")

extern uint32_t interruptsState;
#define interrupts() xt_enable_interrupts(interruptsState)
#define noInterrupts() __asm__ __volatile__("rsil %%0,15" : "=a" (interruptsState))

In the following, assuming interrupts are enabled before calling f2(), interrupts will be disabled after returning from f2().

void f1() {
  noInterrupts(); // (2) Save interruptsState (interrupts disabled on entry)
  ...
  interrupts();   // (3) Restore interrupts from interruptsState (disabled)
}

void f2() {
  noInterrupts(); // (1) Save interruptsState and disable interrupts
  ...
  f1();
  ...
  interrupts();   // (4) Restore interrupts from interrupstState (disabled)
}

I am not familiar with ESP8266 ASM so perhaps I am missing something. Or is there a requirement for functions that call noInterrupts to never call another function that calls noInterrupts?

AVR code saves SREG (status register), disables interrupts, then restores SREG. Each function has its own copy of SREG to ensure correct save/restore of the interrupt state.

Assuming interrupts are enabled when f2() is called, interrupts are enabled at the end of f2(), not f1(). This looks correct. ARM code does something similar.

void f1() {
  uint8_t sreg = SREG;
  noInterrupts();     // (2) Save SREG, interrupts disabled before entry
  ...
  SREG = sreg;        // (3) Restore SREG, interrupts still disabled
}

void f2() {
  uint8_t sreg = SREG;
  noInterrupts();       // (1) Save SREG, interrupts were enabled but
                        // now disabled.
  ...
  f1();
  ...
  SREG = sreg;          // (4) Restore SREG, enables interrupts.
}

[me-no-dev]

maybe a state = saveInterrupt(); restoreInterrupt(state); can be added to mimic what the AVR does.
good catch though! I do not think we thought of that situation

[igrr]

Yes, current implementation is flawed in this regard. Unfortunately unlike ARM and AVR here we don't seem to have a "disable all interrupts" kind of register.

Some comments in the original xtos stack are quite helpful:
https://github.com/qca/open-ath9k-htc-firmware/blob/master/sboot/magpie_1_1/sboot/athos/src/xtos/ints-off.S
https://github.com/qca/open-ath9k-htc-firmware/blob/master/sboot/magpie_1_1/sboot/athos/src/xtos/ints-on.S

[bbx10]

The xt_* functions take a state parameter which I assume save/restore the interrupt state so interrupts/noInterrupts could be modified to pass a stack variable instead of a global. @me-no-dev takes it one step further in the right direction by renaming the functions so they make more sense.

#define xt_disable_interrupts(state, level) __asm__ __volatile__("rsil %0," __STRINGIFY(level) : "=a" (state))
#define xt_enable_interrupts(state)  __asm__ __volatile__("wsr %0,ps; isync" :: "a" (state) : "memory")

#define restoreInterrupt(interruptsState) xt_enable_interrupts(interruptsState)
#define saveInterrupt(interruptsState_addr) __asm__ __volatile__("rsil %0,15" : "=a" (interruptsState_addr))

void f1() {
  uint32_t interruptSave;
  saveInterrupt(&interruptSave);
  ...
  restoreInterrupt(interruptSave);
}

The xtos_ints* functions appear to be doing something completely different compared to the xt* functions above. I do not know enough about the CPU to know why.

[bbx10]

I took a closer look at _xtos_ints* and they look good too. Both return the old interrupts enabled value. It appears these functions were intended to be used versus directly accessing CPU PS register. But this is only a guess.

uint32_t _xtos_ints_on(uint32_t);
uint32_t _xtos_ints_off(uint32_t);
// Disable all interrupts and return the interrupts enabled before disabling them.
#define saveInterrupts() _xtos_ints_off(0xFFFFFFFFUL)
// Enable interrupts specified by intSave
#define restoreInterrupts(intSave) _xtos_ints_on(intSave)

[Makuna]

Sorry, computer not handy, but what does the _xtos_ints* actually do? By its signature it is just changing the intenable reg, intenable has no effect on some interrupts. The original sdk functions didn't adjust the interrupt level which is the only way to disable all interrupts in one call (rsil).

The esp intrrupts are a multi priority system while the AVR is not. This makes the noInterrupt() support hard to get right. Further, without going to level 15, the ISR are being interrupted by higher level interrupts causing timing issues. The was the key to getting NeoPixel working reliably.

Originally I was going to have the noInterrupt() macro define the state variable, but there are samples that do it within one scope and the interrupts() in another scope, so this doesn't work.

Looks like there is a need for a stack of intlevel state needed.

[igrr]

I was also thinking about having a stack of intlevels, but unfortunately it's not obvious for someone coming with AVR background that the following code is not correct:

noInterrupts();
// some code
noInterrupts(); // again
// more code
interrupts();

My suggestion is to do the best we can inside core and libraries though. I think we should replace individual noInterrupts/interrupts calls with locking objects and rely on C++ scopes to manage (un)locking. Here's what I did in the FS wrapper:

https://github.com/esp8266/Arduino/blob/esp8266/hardware/esp8266com/esp8266/cores/esp8266/spiffs_hal.cpp#L34

https://github.com/esp8266/Arduino/blob/esp8266/hardware/esp8266com/esp8266/cores/esp8266/interrupts.h#L15

This will create a 'stack' of intlevels if we happen to lock interrupts multiple times.

[Makuna]

I like that @igrr , but maybe expose the int level as param to constructor with a default parameter of 15?

So, we are thinking just leave NoInterrupts () and Interupts () alone then?

If so, we should really have note to their limitations and suggest esp specific code use your auto intr class.

[igrr]

I did make it a constructor argument first, but that broke stringify macro (it expected a literal value). As much as I love assembly, I can never remember the right syntax for gcc inline assembly :)

[Makuna]

No, your right, stringy only works with literals. I remember trying to make the xt_disable better and running into that issue.

I would like a larger discussion on int level and how or if we will support levels at all.

So today, gpio ISR can be interrupted by a timer, this makes sense from a point of view that timing is critical. But if you don't know this could happen, you might make assumptions that could cause problems.

If we go this way, then a more int level flexible standard solution will be needed.

[Makuna]

Here is one solution after talking with @igrr , the following will make interrupts() /nointerrupts() no different than AVR.

To provide a replacement to the specific AVR SREG/cli() model, we will have the xt_rsil/xt_wsr_ps macros. Where the code is in a cpp where a class can be used, there is an auto class InterruptLock.

// these low level routines provide a replacement for SREG interrupt save that AVR uses
// but are esp8266 specific. A normal use pattern is like
//
//{
//    uint32_t savedPS = xt_rsil(1); // this routine will allow level 1 and above
//    // do work here
//    xt_wsr_ps(savedPS); // restore the state
//}
#define xt_rsil(level) (__extension__({uint32_t state; __asm__ __volatile__("rsil %0," __STRINGIFY(level) : "=a" (state)); state;}))
#define xt_wsr_ps(state)  __asm__ __volatile__("wsr %0,ps; isync" :: "a" (state) : "memory")

// Set Interrupt Level
// level (0-15),
// level 15 will disable ALL interrupts,
// level 0 will enable ALL interrupts
// 

#define interrupts() xt_rsil(0)
#define noInterrupts() xt_rsil(15)

// this auto class wraps up xt_rsil so your code can be simplier, but can only be
// used in an ino or cpp files. A normal use pattern is like
//
//{
//    {
//      InterruptLock(1); // this routine will allow level 1 and above
//      // do work within interrupt lock here
//    }
//    do work outside of interrupt lock here outside its scope
//}
//
#define InterruptLock(intrLevel) \
class _AutoDisableIntr { \
public: \
    _AutoDisableIntr() { _savedPS = xt_rsil(intrLevel);  } \
    ~_AutoDisableIntr() { xt_wsr_ps(_savedPS); } \
private: \
    uint32_t _savedPS; \
    }; \
_AutoDisableIntr _autoDisableInter \