SpenceKonde / megaTinyCore

Arduino core for the tinyAVR 0/1/2-series - Ones's digit 2,4,5,7 (pincount, 8,14,20,24), tens digit 0, 1, or 2 (featureset), preceded by flash in kb. Library maintainers: porting help available!
Other
544 stars 141 forks source link

Interrupt does not trigger on all pins #988

Closed ilFuria closed 11 months ago

ilFuria commented 11 months ago

Hi, I'm using and attiny 804, and I need to wake up from sleep to power a couple of LED strips (yes, you may have seen a similar code elsewhere). The code is this:

#include <Adafruit_NeoPixel.h>
#include <avr/delay.h>
#include <avr/sleep.h>
// multiple LED strips
#define MULTI

#define PIN_F PIN_PB0
#if defined(MULTI)
#define PIN_D PIN_PA2
#endif

// test LED pin
#define T PIN_PA3

#define NUM_F 12
#define NUM_O 5

#define DELAY 40
#define C 1000
#define TYPE NEO_GRB + NEO_KHZ800

#define R PIN_PA4
#define G PIN_PA5
#define B PIN_PA6
#define S PIN_PA7

// Color structure
struct Colore {
  uint8_t r;
  uint8_t g;
  uint8_t b;
  Colore(uint8_t rosso, uint8_t verde, uint8_t blu) {
    r = rosso;
    g = verde;
    b = blu;
  }
  Colore() {}
};

Adafruit_NeoPixel faccia = Adafruit_NeoPixel(NUM_F, PIN_F, TYPE);
#if defined(MULTI)
Adafruit_NeoPixel Od = Adafruit_NeoPixel(NUM_O, PIN_D, TYPE);
#endif

const Colore blu_1 = Colore(0, 100, 255);
const Colore blu_2 = Colore(0, 0, 255);
const Colore blu_3 = Colore(0, 0, 200);
const Colore rosso_1 = Colore(255, 0, 0);
const Colore rosso_2 = Colore(200, 0, 0);
const Colore rosso_3 = Colore(200, 0, 100);
const Colore a_1 = Colore(255, 150, 0);
const Colore a_2 = Colore(255, 100, 0);
const Colore a_3 = Colore(255, 50, 0);
const Colore nero = Colore(0, 0, 0);

struct Effetto {
  Colore colori[2];
   void set_colori( Colore f, Colore o) {
    colori[0] = f;
    colori[1] = o;
  }
};

volatile Effetto spento;
volatile Effetto blu;
volatile Effetto rosso;
volatile Effetto giallo;
volatile Effetto* Effetti[] = { &blu, &rosso, &giallo,&spento };

volatile Effetto* effetto;

bool chg;

void setup() {
pinMode(T,OUTPUT);

// setup colors
  spento.set_colori(nero, nero);
  blu.set_colori(blu_1, blu_3);
  rosso.set_colori(rosso_1, rosso_3);
  giallo.set_colori(a_1, a_2);
   effetto = &spento;
  chg=false;

// setup leds
 faccia.begin();
  _delay_ms(C);
  faccia.show();
#if defined(MULTI)
  Od.begin();
  Od.show();
#endif

    beginPins();
 }
void loop(){
 // sleep
  set_sleep_mode(SLEEP_MODE_PWR_DOWN);
  sleep_enable();
  sleep_cpu();
if(chg)
 { 
  change();
  attachInterrupts();
 }
 chg=false;
 _delay_ms(500);
  }

void change(){
  if(chg)
  {
    effetta2(0, NUM_F, &faccia);
#if defined(MULTI)
  effetta2(1, NUM_O, &Od);
#endif
}
  }

void effetta2(uint8_t ix, uint8_t num, Adafruit_NeoPixel* strip){
  uint8_t r= effetto->colori[ix].r;
  uint8_t g= effetto->colori[ix].g;
  uint8_t b= effetto->colori[ix].b;
  for(uint8_t i=0; i<num; i++)
    strip->setPixelColor(i,strip->Color(r,g,b));
  strip->show();
  chg=false;
}

void beginPins(){
  pinMode(R,INPUT_PULLUP);
  pinMode(G,INPUT_PULLUP);
  pinMode(B,INPUT_PULLUP);
  pinMode(S,INPUT_PULLUP);
  attachInterrupts();
  }
void attachInterrupts(){
// tried using the attachInterrupt with the same result
  PORTA.PIN4CTRL&=~PORT_ISC_gm;
  PORTA.PIN4CTRL|=PORT_ISC_FALLING_gc;
  PORTA.PIN5CTRL&=~PORT_ISC_gm;
  PORTA.PIN5CTRL|=PORT_ISC_FALLING_gc;
  PORTA.PIN6CTRL&=~PORT_ISC_gm;
  PORTA.PIN6CTRL|=PORT_ISC_FALLING_gc;
PORTA.PIN7CTRL&=~PORT_ISC_gm;
  PORTA.PIN7CTRL|=PORT_ISC_FALLING_gc;
  }
ISR(PORTA_PORT_vect){
  // 4 r 5 g 6 b 7 s
  byte flags=VPORTA.INTFLAGS;
  PORTA.PIN4CTRL&=~PORT_ISC_gm;
  PORTA.PIN5CTRL&=~PORT_ISC_gm;
  PORTA.PIN6CTRL&=~PORT_ISC_gm;
  PORTA.PIN7CTRL&=~PORT_ISC_gm;
  VPORTA.INTFLAGS=0xFF;
   digitalWrite(T,HIGH);
  _delay_ms(300);
  digitalWrite(T,LOW);
  if(flags&(1<<4))
    effetto=&rosso;
   else if (flags&(1<<5))
    effetto=&giallo;
   else if (flags&(1<<6))
    effetto=&blu;
   else
    effetto=&spento;
   chg=true;
  }

the result is that only PA6 triggers the interrupt (thus flickering the test LED and the LED strip). This is odd. I tried with the "attachInterrupt" method too (but I prefer manually setting the pins, I know I'm strange) with the same results mutatis mutandis.

What I have confirmed:

Honestly I don't know whether there's a bug in the library or something I'm doing wrong. I'm using version 2.6.8 with Arduino IDE 1.8.13

Can you please help? (Also note that from Mon, 24th Jul I won't be responsive for 3 weeks or so, so this is not dead if I don't answer any longer)

Cheers!

EDIT: upon further investigation is the waking up from sleep that only works with pa6. Removing the sleep routine I get woonderful results.

SpenceKonde commented 11 months ago

Is adafruit neopixel compatible with these parts? It wasn't last I checked because their ASM no longer works because instruction timing has gotten faster for the ST instruction (this is actually awesome, and makes wonderful things possible for this library), I rewrote all the asm for modern AVRs, and extended it all the way up to 48 MHz, the highest speed that I am aware of a AVR Dx-series being overclocked to. That's included with the core in tinyNeoPixel.h, which is a dropin replacement for adafruit_neopixel (and heavily based on their code) - and tinyNeoPixel_Static.h - this is not quite dropin, but the required changes are small (see https://github.com/SpenceKonde/megaTinyCore/blob/master/megaavr/extras/tinyNeoPixel.md ), but it saves over 1k of flash, and it causes the size output at the end of the compile process to include the size of the buffer when seeing if you have enough ram That is responsible for any flickering with the neopixels, switching to one of the tinyneopixels should work, both are included with the core (because they are architecture specific)

attachInterrupt is an abomination, nobody should ever use it, so that much sounds correct., and my attempt to make it less abominable fell just short of the goal before encountering insurrmountable obstacles. It's smaller and faster, but it still has the same problem as the stock implementation that it borgs every pin interrupt and you can't manually define any if you've used attachInterrupt everywhere). So your treatment of that is correct - attachInterrupt should be avoided like COVID back in early days - wear a mask when you must deal with it, and stay 10 feet away from any code using it until it's been quarantined for 10 days of continuous operation without interrupt-induced failures. attachInterrupt's API is impossible to implement in any manner which doesn't absolutely suck on the new parts - it was a great match for the INTn pins on classic AVR (I mean, no, really it wasn't, it was crap there too, but it was only around 50-60 clocks deep - that is fundamentally unavoidable by any method by which you define a function in one place, and then pass a pointer to it to some sort of attach function - that will always impose the maximum length prologue and epilogue (not considering cases where code in the prologue and epilogue would be required if the function was called from outside an ISR context too) , and it only covered one to four pins. It's now it's a thick layer of bull manure (over 100 clocks deep stock, and I wasn't able to do much better, though I was able to prevent it from having to keep the code in triplicate) spread over all the I/O pins. ). Those numbers are ballpark figures for the time overhead of an interrupt firing that way. As opposed to defining it with ISR(). and doing it manually

Edit: Yes, you are, and the following paragraph describes why and how to fix it. Are you waking from a sleep mode that stops the CPU clock (basically anything that saves much power) on a rising or falling trigger? If so, that is the problem. Powerdown, and standby unless instructed specifically not to, stop the CPU clock. the CLK_PER/CLK_CPU - this is responsible for much of the reduced power consumption). However, we describe in Ref_Interrupts.md how on tinyAVR parts (as well as AVR DA and DB, but not DD or EA) and as clearly documented in the datasheet, only the "Fully Asynchronous" pins, of which there are two per port (2 and 6), can recognize arising or falling edge without the peripheral clock, These pins respond to shorter pulses (also more sensitive to noise) in interrupt mode). The other pins can only wake the chip from a sleep mode without the CPU clock running on a "low level" or "change" (apparently it's easier to tell that a pin changes states without a clock than it is to tell whether it rose or fell?). Assuming they're pulled down by the button, use low level interrupts, and make sure that there's no harm in the interrupt running an arbitrary number of times as the person holds down the button. This and similarly crude methods are very widely used in commercial products, because the whole low-level-interrupt-or-change-interrupt-only-way-to-wake-from-sleep is very common constraint on interrupts. You'll notice that very few consumer electronics will respond to a button press, until you release it, especially battery operated ones. It's a change interrupt and a while loop that waits for the pin to go high before proceeding so that the second interrupt doesn't confuse it , or a low level interrupt that locks the system as long as any pin is pressed because the interrupt fires continually.

Everyone is loathe to disable an interrupt with plans to reenable it when the interrupt condition was no longer true out of fear that they would miss a place where they needed to be reenabled.

SpenceKonde commented 11 months ago

Closing, this is not a defect in the core, it is a user code issue. The hardware cannot do what you ask of it while sleeping, except on PA2, PA6 and PB2, to wake without clock from other pins you must use low level or change interrupts.

ilFuria commented 11 months ago

thanks for your time, sorry to have wasted it. I Confirm it works.

SpenceKonde commented 11 months ago

This gets people all the time.

Microchip realized that what they had been doing was not worth the pain it inflicted upon users for the silicon it saved, and since the DD-series, they have not had partially async pins. So DD, DU, EA, EB, any other future Dx or Ex parts - those have or will have when released, only fully async pins that all behave like 2 and 6 on each port do on tinyAVR (I do not expect to see another tinyAVR release in the forseeable future - the writing was on the wall regarding that when they released the DA, which made clear that ATmega was a dead brand (and indeed, no more have come out). Why would Microchip have at ATmega product line? They're not Atmel anymore! I think they were just using the tinyAVRs as test balloons to use us as their QA before putting the fixed peripherals into their future products) and 1-series paved the way for the DA/DB series, 2-series paved the way for EA - but now there's no system left that' seems large enough to warrant that yet is present on low pincount parts, so what would they do another tinyAVR for? Personally, I think they should make a 3-series - but only with the one pincount that they stiffed on the 2-series and the 0/1-series: 8-pin. Like, i'm proposing a 432, 832 and 1632, all 8-pin parts... maybe give em a TCA and two TCBs. 2 CCL LUTs, and none of the bugs that the 0/1-series has. The tiny85 is still so popular for the sole reason that it's only 8 pins)

So on DD and later, any pin can wake on rising edges only or falling edges only in clockless sleep, all tinyAVR+DA+DB and megaAVR0 has the pin2/pin6 fully async deal