-
Notifications
You must be signed in to change notification settings - Fork 94
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.
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
Fixes buildability with Arduino Core 2.5.1 #24
Conversation
Added ICACHE_RAM_ATTR to the isrPin otherwise Arduino core complains "ISR not in IRAM" in extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFunc, void *arg, int mode) in cores/esp8266/core_esp8266_wiring_digital.cpp
@@ -46,7 +46,7 @@ bool XPT2046_Touchscreen::begin() | |||
#ifdef ESP32 | |||
void IRAM_ATTR isrPin( void ) | |||
#else | |||
void isrPin( void ) | |||
void ICACHE_RAM_ATTR isrPin( void ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this isn't breaking non-ESP8266 devices? Wouldn't a fix like this be a tad safer: https://github.com/sandeepmistry/arduino-LoRa/pull/257/files
Side note: this is a companion PR to #17 from @CelliesProjects - this time for ESP8266 rather than ESP32. |
I am pretty sure. I am testing / using the same lib on both ESP8266/32
Odesláno z iPhonu
24. 6. 2019 v 22:16, Marcel Stör <notifications@github.com>:
… @marcelstoer commented on this pull request.
In XPT2046_Touchscreen.cpp:
> @@ -46,7 +46,7 @@ bool XPT2046_Touchscreen::begin()
#ifdef ESP32
void IRAM_ATTR isrPin( void )
#else
-void isrPin( void )
+void ICACHE_RAM_ATTR isrPin( void )
Are you sure this isn't breaking non-ESP8266 devices? Wouldn't a fix like this be a tad safer: https://github.com/sandeepmistry/arduino-LoRa/pull/257/files
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
That's not what I meant. I see in the code that ESP32 is properly handled. This library, however, can also be used on non-ESP chips. The photo in the README shows a Teensy I believe. |
{sigh} - why do people using ESP attempt to submit fixes that break libraries for all other non-ESP boards? |
I've committed a fix. Please confirm if this works for you on ESP8266. Please do NOT submit any pull request which you have not tested at least one 1 of the official Arduino boards. If you have only tested on ESP, do not send the pull request. |
Did anyone try the fix? |
I just tested with ESP8266 Arduino-core 2.5.2 and that works. I don't have an official Arduino board but I guess that's what you tested. |
@PaulStoffregen can I suggest you close this PR and instead release your fix as 1.3.1? |
@PaulStoffregen could please give us a rough estimate by when you plan to lease that fix? |
As far as I know, this issue is fixed by 26b691b |
Correct, thanks for that. I was wondering when we could expect a new release in the Arduino/PlatformIO library manager. |
I would like to add a +1 to @marcelstoer's request. I just encountered some problems when I upgraded to the latest revision of this board and could not build some ESP code. Would you please consider releasing this to the Arduino library manager? |
@PaulStoffregen Can we please get a new release to the Library Manager? Using the TouchTestIRQ example on a Wemos D1 mini Pro ESP8266, I have just now verified that 26b691b fixes the the "ISR not in IRAM!" crash. @marcelstoer had also verified it back on Jun 25. Thank you very much for the fix. Is there anything further that needs testing or is blocking? Yes, I know this PR (which will probably be closed unmerged) isn't the place to discuss 26b691b and I'm happy to open an issue if that's better, but all the context is here. |
I did that a while back in #25 but I closed it when I found this PR. |
This can now be closed as it was fixed with 26b691b and released with https://github.com/PaulStoffregen/XPT2046_Touchscreen/releases/tag/v1.4. |
Thank you @PaulStoffregen for releasing a new version! |
Added ICACHE_RAM_ATTR to the isrPin otherwise Arduino core complains "ISR not in IRAM" in extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFunc, void *arg, int mode) in cores/esp8266/core_esp8266_wiring_digital.cpp