Skip to content

digitalPinToInterrupt: fix double pin remapping #10373

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 25, 2024
Merged

digitalPinToInterrupt: fix double pin remapping #10373

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cores/esp32/Arduino.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
#endif
#define EXTERNAL_NUM_INTERRUPTS NUM_DIGITAL_PINS // All GPIOs
#define analogInputToDigitalPin(p) (((p) < NUM_ANALOG_INPUTS) ? (analogChannelToDigitalPin(p)) : -1)
#define digitalPinToInterrupt(p) ((((uint8_t)digitalPinToGPIONumber(p)) < NUM_DIGITAL_PINS) ? digitalPinToGPIONumber(p) : NOT_AN_INTERRUPT)
#define digitalPinToInterrupt(p) ((((uint8_t)digitalPinToGPIONumber(p)) < NUM_DIGITAL_PINS) ? (p) : NOT_AN_INTERRUPT)
Copy link
Contributor

Choose a reason for hiding this comment

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

did you want to do something like this?

int8_t digitalPinToInterrupt(int8_t p) { 
    const int8_t pin = digitalPinToGPIONumber(p);
    return pin < NUM_DIGITAL_PINS ? pin : NOT_AN_INTERRUPT;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What you've written is the old behavior.
The current behavior has just become a simple check to see if it is a valid pin???
I don't see yet how this is better as it doesn't convert the value.

Copy link
Contributor Author

@pillo79 pillo79 Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@TD-er true, it does not. However the vast majority of uses of digitalPinToInterrupt are as an input to attachInterrupt, which does already remap the pin. So this patch allows to use either attachInterrupt(digitalPinToInterrupt(Dx)) or attachInterrupt(Dx), since remapping is applied once, only in the outer function.
See #10367 for more context 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Which begs the question, do attachInterrupt(digitalPinToInterrupt(Dx)) and attachInterrupt(Dx) yield the same result? (well they do now, but probably didn't with the older code)
So is this just an optimization or can it also break existing code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either has always worked on all ESP32 targets; this was reported in the above issue as broken on the Arduino Nano ESP32, the only board that uses pin remapping; this PR fixed that issue restoring proper functionality everywhere.

#define digitalPinHasPWM(p) (((uint8_t)digitalPinToGPIONumber(p)) < NUM_DIGITAL_PINS)

typedef bool boolean;
Expand Down
Loading