Skip to content
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

Incompatible with Arduino core for ESP8266 version 3 #1496

Open
Yveaux opened this issue Aug 12, 2021 · 14 comments
Open

Incompatible with Arduino core for ESP8266 version 3 #1496

Yveaux opened this issue Aug 12, 2021 · 14 comments
Labels
enhancement ESP8266 release-notes Issues that have information that should be included in the release notes

Comments

@Yveaux
Copy link
Member

Yveaux commented Aug 12, 2021

When compiling against arduino core 3.x, MySensors startup code seems to hang during startup, without producing any output.

What needs to be fixed in MySensors:

  • MySensors takes over the startup code of a platform, which has changed significantly with core 3.
    • Just updating the MySensors startup code to the core 3 version doesn't seem to work out of the box
  • Interrupt handlers need to be defined using IRAM_ATTR instead of ICACHE_RAM_ATTR
  • Implementation of MY_CRITICAL_SECTION needs reviewing
  • Part of ESP8266 code is shared with ESP32 code; make sure this doesn't break

Quick hack to get things working again:

  • MyHwHal.h
    • Replace #define IRQ_HANDLER_ATTR ICACHE_RAM_ATTR by #define IRQ_HANDLER_ATTR IRAM_ATTR
  • MySensors.h
    • Comment the line #include "hal/architecture/ESP8266/MyMainESP8266.cpp"
  • Delete the file MySensors/hal/architecture/ESP8266/MyMainESP8266.cpp

Change your sketch as follows:

void setup()
{
    static bool runonce = true;
    if (runonce)
    {
        runonce = false;
        Serial.begin(MY_BAUD_RATE, SERIAL_8N1, MY_ESP8266_SERIAL_MODE, 1);
        _begin();
    }
    // ...your regular setup() code...
}

void loop()
{
    // ...your regular loop() code...
    wait(0);        // At start/end of loop, Important!
}

@Yveaux Yveaux added enhancement ESP8266 release-notes Issues that have information that should be included in the release notes labels Aug 12, 2021
virtual-maker added a commit to virtual-maker/MySensors that referenced this issue Aug 12, 2021
and replace ICACHE_RAM_ATTR by IRAM_ATTR
@virtual-maker
Copy link
Contributor

@Yveaux Good finding and nice workaround!

Yes since core 3.0.2 the MySensors dev lib is buildable again (no PRIu8 and PSTR errors anymore),
but unfortunately it hangs at startup now. I'm using a lot of ESP8266 in my sensors network
so I'm very interested to be able to use the core 3 version.

The code of MyMainESP8266 HAL is basically a copy from Arduino ESP8266 core -
Arduino/cores/esp8266/core_esp8266_main.cpp with two additional lines in loop_wrapper()
to call the "magic" MySensors background tasks.
I have updated it with the new code from core ver. 3.0.2. Then MySensors gateway works for me.

Additionally I added a check for the ESP8266 core major version 3 and added some #if #else
to keep the compatibility with the older version 2 core. I have checked it with versions 2.6.2, 2.7.4 and 3.0.2.
So I think this could be a solution for point 1. from your todo list above.

Then I have also replaced the code locations for your point 2. to use IRAM_ATTR with core 3.

So only your last both points 3. and 4. are still open. May be you can explain more what to do here.

It would also be great if you could take a look into my commit above and maybe give it a try.

@Yveaux
Copy link
Member Author

Yveaux commented Aug 12, 2021

@virtual-maker hey man, you beat me to it! 👍
I took a quick look and this is basically identical to what I did.

On the other hand, I'm not happy with including files from the Arduino core (also for other architectures) as part of the MySensors stack. The main esp8266 startup code is being modified on a regular basis (see https://github.com/esp8266/Arduino/commits/master/cores/esp8266/core_esp8266_main.cpp) so we will always struggle to keep up.

I did some experiments using the core's preinit()-hook, which we might be able to use as startup entrypoint, or to register a one-time task that runs just before setup() is called. So far I couldn't get it running, but maybe my esp knowledge is just lacking...

However, I did manage to implement the loop()-hook, which is required to update the stack, by overriding the loop_end() function; i.e.

extern "C" void loop_end (void)
{
    // Handle the ESP ARduino core
    run_scheduled_functions();
    run_scheduled_recurrent_functions();
    // Handle the MySensors stack
    _process();
}

Still some esp-core code in there, but 2 lines are better than a complete file.

We could pursue this path to become, to some degree, less dependent on Arduino core changes, or even remove all the magic (for all platforms) and just call some MySensors function in setup() and another one in loop() as MySensors did in the past (and all other libraries do...)

@virtual-maker
Copy link
Contributor

@Yveaux I completely agree with this copy/paste maintenance nightmare.
I assume that preinit() is way to early for a call of _begin().

Unfortunately I have no experience with ESP tasks but what's about something like this?

MySensors\hal\architecture\ESP8266\MyMainESP8266.cpp

inline void _my_sensors_loop()
{
	// Process incoming data
	_process();
	// Call of loop() in the Arduino sketch
	loop(); 
}

/*
 * Use preprocessor defines for code injection of the MySensors calls to _begin() and _process(). 
 * This is the "magic" how the MySensors stack is setup and executed in background without
 * need for explicit calls from the Arduino sketch.
*/

// Start up MySensors library including call of setup() in the Arduino sketch
#define setup _begin
// Helper function to _process() and call of loop() in the Arduino sketch
#define loop _my_sensors_loop 

#include <core_esp8266_main.cpp>

// Tidy up code injection defines
#undef loop
#undef setup

This way only the included filename core_esp8266_main.cpp would be core specific.

@virtual-maker
Copy link
Contributor

virtual-maker commented Aug 13, 2021

@Yveaux Remove of the MySensors magic would be a clean solution, especially for newcomers.
But this would break all sketches floating around in the web.
So I would keep the magic as it is.

@dungdao191299
Copy link

dungdao191299 commented Oct 7, 2021

When compiling against arduino core 3.x, MySensors startup code seems to hang during startup, without producing any output.

What needs to be fixed in MySensors:

  • MySensors takes over the startup code of a platform, which has changed significantly with core 3.

    • Just updating the MySensors startup code to the core 3 version doesn't seem to work out of the box
  • Interrupt handlers need to be defined using IRAM_ATTR instead of ICACHE_RAM_ATTR

  • Implementation of MY_CRITICAL_SECTION needs reviewing

  • Part of ESP8266 code is shared with ESP32 code; make sure this doesn't break

Quick hack to get things working again:

  • MyHwHal.h

    • Replace #define IRQ_HANDLER_ATTR ICACHE_RAM_ATTR by #define IRQ_HANDLER_ATTR IRAM_ATTR
  • MySensors.h

    • Comment the line #include "hal/architecture/ESP8266/MyMainESP8266.cpp"
  • Delete the file MySensors/hal/architecture/ESP8266/MyMainESP8266.cpp

Change your sketch as follows:

void setup()
{
    static bool runonce = true;
    if (runonce)
    {
        runonce = false;
        Serial.begin(MY_BAUD_RATE, SERIAL_8N1, MY_ESP8266_SERIAL_MODE, 1);
        _begin();
    }
    // ...your regular setup() code...
}

void loop()
{
    // ...your regular loop() code...
    wait(0);        // At start/end of loop, Important!
}

I come from my issues #1509 (comment)
but when i upload my code to my ESP8266 MQTT gateway not working. So i follow your tip and my gateway stuck after setup.
image
When i replace wait(0); by wait(100); . It working.

@d-a-v
Copy link

d-a-v commented Nov 1, 2021

Hi,
I'm not a user of MySensors but I was brought to this issue by @konstruktors.
Reading the above, I'd like to draw your attention to esp8266 arduino core recurrent scheduled functions designed for background tasks in a non-multitasking environment, that would allow to avoid this:

extern "C" void loop_end (void)
{
    // Handle the ESP ARduino core
    run_scheduled_functions();
    run_scheduled_recurrent_functions();
    // Handle the MySensors stack
    _process();
}

They allow to automatically call a function (here _process()) on a regular basis and from the user context.
In this use case, this->handlePackets() is called not more often than every 100us at every loop() and yield().

On the other hand, I'm not happy with including files from the Arduino core (also for other architectures) as part of the MySensors stack.

Could the above respond to this consideration ?

It is not clear to me whether other changes in 3.0.2 (regarding 2.7.4) are preventing compatibility with this library.

@kasparsd
Copy link

I've tested the #1513 fix and it appears to fix the issue, see #1513 (comment)

@d-a-v
Copy link

d-a-v commented Dec 1, 2021

CI is failing #1513 for an unknown reason to me, can someone help me fix it ?
Also, hal/architecture/ESP8266/MyMainESP8266.cpp needs removal (I simply inserted #if 0).
Should I remove it in the proposed PR ?
Or is it preferred - after testing - that a maintainer applies the changes, which I don't mind ?

@virtual-maker
Copy link
Contributor

Hi Kaspars,

I've tested the #1513 fix and it appears to fix the issue, see #1513 (comment)

When I follow the link to your test project from the issue comment I get a 404.
I'm interested how you manage the setup() part.
Can you please check the link, may be your repo is private?

Thank you

@kasparsd
Copy link

kasparsd commented Dec 6, 2021

@virtual-maker Sorry for the confusion! It was indeed private. Could you please test it again?

@virtual-maker
Copy link
Contributor

@kasparsd Thanks, now the link works. I see, you use the ESP as a node which only sends messages.
This seems to work. But I think you need to extend your setup() function with the code example from @Yveaux.

Something like this:

void setup()
{
    static bool runonce = true;
    if (runonce)
    {
        runonce = false;
        Serial.begin(MY_BAUD_RATE, SERIAL_8N1, MY_ESP8266_SERIAL_MODE, 1);
        _begin();
    }
    // ...your regular setup() code...
}

I have seen you have defined MY_DEBUG.
Your errors (!MCO) in your debug output show that the _begin() code from MySensors was not executed.

5097 !MCO:SND:NODE NOT REG
10125 !MCO:SND:NODE NOT REG
15154 !MCO:SND:NODE NOT REG

Can you check this? Thank you

@dukicn
Copy link

dukicn commented Jan 30, 2022

Is there any progress on this issue? I think I am hitting the same issue. I am building an ESP8266 gateway (ESP12) and I am not getting anything on serial monitor. I can't even print messages. However, if I comment out #include <MySensors.h> statement, serial messages work.

I am using MySensors 2.3.2.

@mvdw
Copy link
Contributor

mvdw commented May 3, 2022

I had the same issue in one of my projects today.
For now, I could solve it by adapting my platformio.ini file:

[env]
platform = [email protected]
board = nodemcuv2
framework = arduino

This means: use the old espressif8266 platform - version 3 does not work.

@kasparsd
Copy link

kasparsd commented May 3, 2022

There are attempts to resolve this in #1513. Pinning the Arduino core to 2.6.3 as suggested above is the fix until that gets merged.

virtual-maker added a commit to virtual-maker/MySensors that referenced this issue Jun 13, 2022
and replace ICACHE_RAM_ATTR by IRAM_ATTR
mfalkvidd pushed a commit that referenced this issue Jul 8, 2022
and replace ICACHE_RAM_ATTR by IRAM_ATTR
tekka007 pushed a commit to tekka007/MySensors that referenced this issue Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ESP8266 release-notes Issues that have information that should be included in the release notes
Projects
None yet
Development

No branches or pull requests

7 participants