PaulStoffregen / Encoder

Quadrature Encoder Library for Arduino
http://www.pjrc.com/teensy/td_libs_Encoder.html
540 stars 239 forks source link

digitalPinToInterrupt #67

Open gagarcr opened 3 years ago

gagarcr commented 3 years ago

According to: https://www.arduino.cc/reference/en/language/functions/external-interrupts/attachinterrupt/

"attachInterrupt(pin, ISR, mode) (Not recommended. Additionally, this syntax only works on Arduino SAMD Boards, Uno WiFi Rev2, Due, and 101.)"

Instead, it proposes to use: "attachInterrupt(digitalPinToInterrupt(pin), ISR, mode) (recommended)"

PaulStoffregen commented 3 years ago

Did you test this on any boards? If so, which ones? And which pins did you use during those tests?

gagarcr commented 3 years ago

I have only tested it in the Arduino Nano Every

PaulStoffregen commented 3 years ago

Does that board define CORE_INT0_PIN, CORE_INT1_PIN, CORE_INT2_PIN, etc? My understanding is Arduino doesn't define this abstraction for any of their boards.

If you only test this change on boards which don't define those symbols, well, that means your change is essentially untested. It must be actually tested on boards which define those names.

kescholm commented 2 years ago

It might be helpful to know the pin can be checked to ensure it can be used as an interrupt:

    uint8_t pin_interrupt = digitalPinToInterrupt(PIN);
    if (pin_interrupt == NOT_AN_INTERRUPT) { /* fail here */  }

However, utility/interrupt_pins.h pin interrupt values are already hard-coded:

// Arduino Uno, Duemilanove, Diecimila, LilyPad, Mini, Fio, etc...
#elif defined(__AVR_ATmega328P__) || defined(__AVR_ATmega328PB__) ||defined(__AVR_ATmega168__) || defined(__AVR_ATmega8__)
  #define CORE_NUM_INTERRUPT    2
  #define CORE_INT0_PIN     2
  #define CORE_INT1_PIN     3

So the switch statement already identifies the proper pin value:

case CORE_INT0_PIN:
    interruptArgs[0] = state;
    attachInterrupt(0, isr0, CHANGE);
    break;

It is equivalent to

    attachInterrupt(digitalPinToInterrupt(CORE_INT0_PIN), isr0, CHANGE);

or

    attachInterrupt(digitalPinToInterrupt(pin), isr0, CHANGE);

So I'd either: 1) leave it 2) modify interrupt_pins.h if it misses some cases or boards Also perhaps a fallback can be done with digitalPinToInterrupt instead of preprocessor errors:

#if !defined(CORE_NUM_INTERRUPT)
#error "Interrupts are unknown for this board, please add to this code"
#endif
#if CORE_NUM_INTERRUPT <= 0
#error "Encoder requires interrupt pins, but this board does not have any :("
#error "You could try defining ENCODER_DO_NOT_USE_INTERRUPTS as a kludge."
#endif
ezhik1 commented 2 years ago

any progress on this issue? I've actually stumbled on this PR after I made local changes to attachInterrupt( digitalPinToInterrupt( but still no success on a Seeeduino Xiao. created an issue #77 for that.