digitalPinToInterrupt: fix double pin remapping #10373
Conversation
The digitalPinToInterrupt() macro currently remaps the pin number to the GPIO number. This is not necessary, as most users will then use the returned value in attachInterrupt() or other similar API functions, which already perform the same remapping. The first half of the macro (the condition) does indeed require the remapping to ensure the check operates on GPIO numbers. Fixes espressif#10367.
👋 Hello pillo79, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 56 files - 83 56 suites - 83 4m 32s ⏱️ - 32m 33s Results for commit ccc07d9. ± Comparison against base commit ae052f4. This pull request removes 13 tests.♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
this code cannot be omitted this may be non-trivial actions and the return value does not have to be equal to the input |
| @@ -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) | |||
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 🙂
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I know, I am the original author of that code... 🤭 Please take a look at the linked issue, you will understand why I'm all for removing that single occurrence. @per1234 did a great job explaining the issue and workaround 👍 |
I just mean that the changed code is not corresponding in return value from the original |
|
I don't see any problem with doing all this as functions instead of defines |
|
why not just remove the define declaration then? arduino-esp32/cores/esp32/io_pin_remap.h Line 44 in 7018cd1 |
Description of Change
The
digitalPinToInterrupt()macro currently remaps the pin number to the GPIO number. This is not necessary, as most users will then use the returned value inattachInterrupt()or other similar API functions, which already perform the same remapping.The first half of the macro (the condition) does indeed require the remapping to ensure the check operates on GPIO numbers.
Related links
Fixes #10367 on 3.x (current) cores.
See also #10372 for the same fix on 2.x (legacy) cores.