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

Make LwIP Core Locking available #186

Closed
wants to merge 2 commits into from

Conversation

HamzaHajeir
Copy link

Make the implementation of LwIP core locking available:
https://github.com/espressif/esp-lwip/blob/a45be9e438f6cf9c54ec150581819c3b95d5af6b/src/include/lwip/tcpip.h#L52-L59

Important when dealing with Raw APIs

@HamzaHajeir HamzaHajeir marked this pull request as ready for review June 25, 2024 15:17
@me-no-dev
Copy link
Member

please provide more info. Why, how, etc.

@HamzaHajeir
Copy link
Author

For OS Mode, the threading of LwIP Raw APIs should be taken care of, APIs should never be called in parallel to lwip task operations (which happens by using an RTOS, in calling APIs by both main and tcpip tasks), can be solved by two possible methods:

  1. Using of tcpip_callback().
  2. Using LwIP Core Locking.

The nongnu website is down right now, so here's a reference to the raw file of the documentation regarding it:
https://github.com/espressif/esp-lwip/blob/5378fd84df498759d4ba231f6ca3a42d6aa0fb93/doc/doxygen/main_page.h#L162-L182

This method is used and tested in H4AsyncTCP the library I maintain after the passage of its author, where it shows high reliability under heavy testing in ESP32 environment (The 8266 too but no OS).

And here's the relevant Kconfig description in esp-idf component lwip:
https://github.com/espressif/esp-idf/blob/0479494e7abe5aef71393fba2e184b3a78ea488f/components/lwip/Kconfig#L37-L46

@Jason2866
Copy link
Contributor

Jason2866 commented Jul 13, 2024

What's the need/benefit for the Arduino code? In other words real life use case?
This forked lib from me-no-dev does not need.

https://github.com/mathieucarbou/AsyncTCP
https://github.com/mathieucarbou/ESPAsyncWebServer

@HamzaHajeir
Copy link
Author

HamzaHajeir commented Jul 13, 2024 via email

@Jason2866
Copy link
Contributor

One! use case. Why should a setup be changed that may introduce side effects hitting all espressif32 Arduino users? My experience Lwip setup changes should be only done to fix a bug hitting most use cases. For special use cases there is the Arduino Lib Builder ;-)

@HamzaHajeir
Copy link
Author

One! use case. Why should a setup be changed that may introduce side effects hitting all espressif32 Arduino users? My experience Lwip setup changes should be only done to fix a bug hitting most use cases. For special use cases there is the Arduino Lib Builder ;-)

I know that, and for that matter I've been using the builder for at least more than a year.

But per the lock feature, it has no side effects, as it's well ported to esp-idf:
https://github.com/espressif/esp-lwip/blob/a45be9e438f6cf9c54ec150581819c3b95d5af6b/src/include/lwip/tcpip.h#L52-L64
https://github.com/espressif/esp-idf/tree/master/components/lwip/port/freertos

BTW, the H4 Stack is completely Async, with complete reliable stack starting from TCP to the application (MQTT/HTTPS WebServer/WSS/HTTPS Client). I've a running MQTT that publishes 13kB payload every 3 seconds over TLS, alongwith running HTTPS WebServer/WSS/HTTP Client, with lots of other stress tests performed.

The Lock is needed to allow it for public use with the official Arduino Core for at least the plain TCP part now.

@Jason2866
Copy link
Contributor

Jason2866 commented Jul 13, 2024

But per the lock feature, it has no side effects, as it's well ported to esp-idf

Famous last words :-)

So it has NO impact of Flash Usage, RAM usage. Every piece of code does run as before?

@HamzaHajeir
Copy link
Author

But per the lock feature, it has no side effects, as it's well ported to esp-idf

Famous last words :-)

So it has NO impact of Flash Usage, RAM usage. Every piece of code does run as before?

Too little overhead due to the usage of the mutex, check sys_arch.c. Count the bytes :-)

Verified

This commit was created on github.com and signed with GitHub’s verified signature.
@me-no-dev
Copy link
Member

as an unforeseen consequence of adding matter to Arduino, core locking is now enabled (for the new 3.1.x versions). So I can close this now and you can give it a show on the 3.1.0-RC2 release

@me-no-dev me-no-dev closed this Nov 1, 2024
@HamzaHajeir
Copy link
Author

as an unforeseen consequence of adding matter to Arduino, core locking is now enabled (for the new 3.1.x versions). So I can close this now and you can give it a show on the 3.1.0-RC2 release

Thanks for accepting the Core Locking config.

As it's not merged, I think there should be the esp_matter component (or CHIP) enables it?

Where can I see the built libs of 3.1.0-RC2? Note that the release does not provide the built sdk libs.

@me-no-dev
Copy link
Member

you could install from git and work that way. Just make sure you use the release/v3.1.x branch of Arduino

@HamzaHajeir
Copy link
Author

Great, just found the config being checked.

@Jason2866
Copy link
Contributor

Btw. enabling has side effects and broke code ;-) So your statement

But per the lock feature, it has no side effects, as it's well ported to esp-idf

is wrong

@HamzaHajeir
Copy link
Author

Btw. enabling has side effects and broke code ;-) So your statement

But per the lock feature, it has no side effects, as it's well ported to esp-idf

is wrong

It has no side effects. The breaking code was due to core safety CONFIG_LWIP_CHECK_THREAD_SAFETY being checked, which BTW exposes a risk source of bugs.

Check:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants