arduino / ArduinoCore-mbed

330 stars 195 forks source link

attachInterrupt unexpectedly resets the pin mode #427

Open nneonneo opened 2 years ago

nneonneo commented 2 years ago

Platform: Nano 33 BLE Sense Version: Arduino IDE 1.8.19, Arduino Mbed OS Nano Boards 2.8.0

attachInterrupt incorrectly resets the mode of an input pin back to the default pull mode (INPUT_PULLUP). This silently overrides an existing pinMode declaration.

This occurs because attachInterrupt calls attachInterruptParam, which constructs an InterruptIn object. The mbed::InterruptIn::InterruptIn(PinName) constructor calls gpio_init_in at the end, which reinitializes the target pin to PullDefault.

Steps to reproduce

Attach an input to pin 2 (e.g. button) which is high when active and high-z (disconnected) when inactive. Run the following sketch:

void setup() {
  pinMode(2, INPUT_PULLDOWN);
  pinMode(LED_BUILTIN, OUTPUT);
  attachInterrupt(digitalPinToInterrupt(2), buttonISR, CHANGE);
}

void buttonISR() {
  static int state = 0;
  state = !state;
  digitalWrite(LED_BUILTIN, state);
}

void loop() {
}

Expected behaviour: clicking or releasing the button toggles the builtin LED state Observed behaviour: clicking the button does nothing (because the input is now INPUT_PULLUP, so the active-high input of the button does nothing).

Possible fix

One possible fix would be to simply reset the pin to the configured pin mode, if set, in attachInterrupt after using the InterruptIn object. (If unset, attachInterrupt already configures the pin with a default configuration).

A workaround is to simply call pinMode after attachInterrupt; however, this goes against the usual way that attachInterrupt is used (even the attachInterrupt example on the official documentation puts pinMode before attachInterrupt).

facchinm commented 2 years ago

Hi @nneonneo , some time ago I prepared a hacky PR to target this issue https://github.com/arduino/ArduinoCore-mbed/pull/256 . Would you mind testing it and reporting if it could work for you?

nneonneo commented 2 years ago

@facchinm Tested - looks like it works for me. It'd be nice to have that merged!