-
-
Notifications
You must be signed in to change notification settings - Fork 891
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
Modify MyMainESP8266 for core 2.6 and 3.0 (#1496) #1524
Modify MyMainESP8266 for core 2.6 and 3.0 (#1496) #1524
Conversation
and replace ICACHE_RAM_ATTR by IRAM_ATTR
These changes are obfuscating and unstable regarding the future, like they were in the past. |
Thanks @d-a-v Unfortunately, I don't have enough knowledge to determine pros and cons myself, so I will need to rely on input from others. @virtual-maker do you have any input on #1524 vs #1513? |
I could also have been citing @Yveaux
... and this is exactly what #1513 answers to.
@virtual-maker can you please elaborate on this comment ?
|
Hi all, However, in the ESP8266 version 3.x this core file has changed considerably, so that the copy in MySensors no longer fits and works. My first suggestion was to copy the new core code from core_esp8266_main.cpp into MySensors MyMainESP8266.cpp and adjust the two calls accordingly. To this end, @Yveaux commented as quoted above and notes that the same problem exists with the other hardware architectures (e.g. ESP32 and SAMD) and with copies of outdated main.cpp files from the respective Arduino core inside the MySensors library. Both the solution in PR #1513 and this #1524 avoid duplicating the core code in MySensors for ESP8266. However, PR #1513 does not provide a solution (compared to #1524) for calling to
This means that at least the MySensors examples to the ESP8266 have to be adapted. Furthermore, it has to be explained to the user that other MySensors node sketches and examples have to be extended accordingly. Solution #1524 avoids this and is compatible to the existing approach and examples. Furthermore, I think that the solution here can also be adopted for other hardware architectures to avoid the problems with outdated code copies from the respective Arduino cores in MySensors library. |
Thank for explanation @virtual-maker I think we all agree that rewriting a core file in an application or a library is not a preferred solution. The aforementioned function #1513's pull request has been living for months and left without bringing to my attention the missing call to Anyway, I am not a user, and you are a maintainer. |
@d-a-v Since you wrote that you are not a user of MySensors, I had tried to clarify the setup problem with @kasparsd in this comment. Unfortunately I didn't get an answer. Here, too, the communication went badly. What a pity! I think your ESP8266 feature I think the MySensors core team should clarify internally how such a communication failure can be avoided in the future. Many thanks again for your offer to help! |
and replace ICACHE_RAM_ATTR by IRAM_ATTR
Simplify MyMainESP8266.cpp for core 2.6, 2.7, & 3.0.x and replace ICACHE_RAM_ATTR by IRAM_ATTR.
This solution works with ESP8266 cores 2.6x, 2.7x and 3.0x according the description in:
1496#issuecomment-898485845
This solution doesn't need any changes in existing MySensors example sketches for the setup() code.
Additionally added is a check for the ESP8266 core major version 3 and added some #if #else
to keep the compatibility with the older version 2 core.
Also are updated the code locations for use of IRAM_ATTR in two example sketches.