Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds noInterrupt() and interrupt() functionality #7226

Merged
merged 5 commits into from
Sep 15, 2022

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Sep 5, 2022

Description of Change

Adds noInterrupt() and interrupt() to Arduino.
This must be used carefully because ESP32 Arduino runs under FreeRTOS and it is easy to get Watchdog Timeout error if it is no correctly used.

Tests scenarios

Tested with this sketch (as suggested in the issue)

//  To run the example sketch to demonstrate the issue, 
//  two digital pins need to be connected together somehow. 
//  This was done on a breadboard using a jumper wire.

#define OUT_PIN 14
#define IN_PIN 32

int count = 0;

void counter() {
  count++;
}

void setup() {
  Serial.begin(115200);

  pinMode(OUT_PIN, OUTPUT);
  digitalWrite(OUT_PIN, LOW);
  pinMode(IN_PIN, INPUT);

  attachInterrupt(digitalPinToInterrupt(IN_PIN), counter, RISING);

  for (int i=0; i<50; i++) {
    digitalWrite(OUT_PIN, HIGH);
    digitalWrite(OUT_PIN, LOW);
  }

  noInterrupts();
  
  for (int i=0; i<50; i++) {
    digitalWrite(OUT_PIN, HIGH);
    digitalWrite(OUT_PIN, LOW);
  }
  
  interrupts();

  Serial.print("count = "); Serial.println(count);
}

void loop() {
}

Expected output:

ets Jun  8 2016 00:22:57

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff0030,len:1344
load:0x40078000,len:13864
load:0x40080400,len:3608
entry 0x400805f0
count = 50

wrong output (before this PR) says count = 100

Related links

Fixes #7211

@SuGlider SuGlider added this to the 2.0.5 milestone Sep 5, 2022
@SuGlider SuGlider self-assigned this Sep 5, 2022
@PilnyTomas
Copy link
Contributor

Tested on S3 and I'm getting 51 instead of 50. Tried various pins:
OUT | IN
14 | 13
14 | 47
40 | 47

@caternuson
Copy link
Contributor

Thanks. This seems to resolve the issue with the library here:
adafruit/Adafruit_VS1053_Library#45

However, I'm also seeing the extra count show up in the simple test sketch. Revised test sketch:

#define OUT_PIN 14
#define IN_PIN 32

int count = 0;

void counter() {
  count++;
}

void setup() {
  Serial.begin(9600);

  pinMode(OUT_PIN, OUTPUT);
  digitalWrite(OUT_PIN, LOW);
  pinMode(IN_PIN, INPUT);

  attachInterrupt(digitalPinToInterrupt(IN_PIN), counter, RISING);

  for (int i=0; i<50; i++) {
    digitalWrite(OUT_PIN, HIGH);
    digitalWrite(OUT_PIN, LOW);
  }

  Serial.print("count = "); Serial.println(count);

  noInterrupts();

  for (int i=0; i<50; i++) {
    digitalWrite(OUT_PIN, HIGH);
    digitalWrite(OUT_PIN, LOW);
  }

  interrupts();

  Serial.print("count = "); Serial.println(count);
}

void loop() {
}

Outputs:

count = 50
count = 51

@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Sep 6, 2022

I am seeing just same result as guys mentioned above, tested on ESP32.
I think it may be due to interrupt flag is still set in registers after noInterrupts();, but its triggered just after interrupts(); is called.

@P-R-O-C-H-Y
Copy link
Member

I have edited the sketch @caternuson posted to demonstrate, that my previous comment really happening.
I have added next print of count right before interrupts() is called.

#define OUT_PIN 14
#define IN_PIN 32

int count = 0;

void counter() {
  count++;
}

void setup() {
  Serial.begin(115200);

  pinMode(OUT_PIN, OUTPUT);
  digitalWrite(OUT_PIN, LOW);
  pinMode(IN_PIN, INPUT);

  attachInterrupt(digitalPinToInterrupt(IN_PIN), counter, RISING);

  for (int i=0; i<50; i++) {
    digitalWrite(OUT_PIN, HIGH);
    digitalWrite(OUT_PIN, LOW);
  }

  Serial.print("count = "); Serial.println(count);

  noInterrupts();

  for (int i=0; i<50; i++) {
    digitalWrite(OUT_PIN, HIGH);
    digitalWrite(OUT_PIN, LOW);
  }
  
  Serial.print("count = "); Serial.println(count);
  
  interrupts();

  Serial.print("count = "); Serial.println(count);
}

void loop() {
}

Output is now:

count = 50
count = 50
count = 51

Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view it works as expected, there is no easy way to clear all coming interrupts before Interrupts() is called. As suggested you must be careful when using noInterrupts(), and its better to use detachInterrupt().

@@ -79,10 +79,9 @@
#define degrees(rad) ((rad)*RAD_TO_DEG)
#define sq(x) ((x)*(x))

#define sei()
#define cli()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have define sei and cli instead for backwards compatibility with older code. Any reason you didn't @SuGlider?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, SEI and CLI are ASM instructions used in ATMEL chips.
Arduino official API is interrupts() and noInterrupts()
I see no reason for defining those ATMEL specific functions.

//interrupts is defined as:

#define interrupts() sei()
//and sei is defined as for the UNO/328p:

# define sei()  __asm__ __volatile__ ("sei" ::)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking ESP8266 Arduino.h I see that it also doesn't define sei() nor cli().
My vote is for keeing it as defined in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interrupts() and noInterrupts() are described in Arduino Language Reference so I vote for this too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, added back.

Copy link
Contributor

@PilnyTomas PilnyTomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@me-no-dev
Copy link
Member

sei() and cli() are AVR functions present in lots of old Arduino code. I see no fault to have them there

@PilnyTomas
Copy link
Contributor

sei() and cli() are AVR functions present in lots of old Arduino code. I see no fault to have them there

Having both variants should not hurt...

@me-no-dev
Copy link
Member

@SuGlider will merge as soon as you re-add sei and cli :)

@me-no-dev me-no-dev merged commit 55d608e into espressif:master Sep 15, 2022
@SuGlider SuGlider deleted the noInterrupt branch September 15, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

noInterrupts() not implemented - can not turn off interrupts
5 participants