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

Many changes regarding Json support and memory improvement #195

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

mathieucarbou
Copy link
Contributor

@mathieucarbou mathieucarbou commented Jan 11, 2024

Also

  • Changed card and stat names to const char* to improve memory usage. This makes them immutable and require to use constants but avoids a memory copy on heap of all the string values

  • Help mitigate concurrency issue with isAsyncAccessInProgress() to avoid updating cards while the layout is being generated from the async_http task

  • Support both ESPAsyncWebServer Ws buffer API and yubox-node-org/ESPAsyncWebServer buffer API (which as a better api for sending websocket buffers and dealing with concurrency)

  • Supports WebSocket batching to avoid crash with the initial layout sending. Batch sizes can be controlled with DASH_JSON_SIZE and DASH_JSON_DOCUMENT_ALLOCATION (Arduino Json 6)

  • refreshLayout() refactoring in order to avoid too many layout refresh requests when updating components dynamically: let the caller trigger a layout refresh once.

  • Removed refreshStatistics() because it it not refreshing the stats only but all the updated cards also

  • Removed update calls when adding / removing cards and stats in order to avoid trigger a sequence of full layout updates: this is u to the user to call refreshLayout() when he has finished

  • DASH_MAX_WS_CLIENTS allows to configure the max WS clients: the default value being set by the ESPAsyncWebServer lib. I recommend setting this value to 1 or 2.

RECOMMANDED DEPENDENCIES

IMPORTANT NOTES

  • Do not use the original ESPAsyncWebServer or ESPHome fork for WebSockets: there have several concurrency issues leading to crashes on dual core systems which are not fixed which are fixed in the above mentioned fork

  • Do not update card values while another task (like the async_http task running the WS callback) is reading the cards to build the layout!

TESTS

I am using ESP-DASH Pro with a lot of tabs, over 200 cards and stats, dashboard updated each 1 sec.

Everything runs file with these dependencies above for a few days without crash.

- Support for ArduinoJson 7
- Fixed missing `step` for slider when using short ranges or big ranges
- Supports WebSocket batching to avoid crash with the initial layout sending. Batch sizes can be controlled with `DASH_JSON_SIZE`.
- Mitigate concurrency issue with `isAsyncAccessInProgress()` to avoid updating cards while the layout is being generated
- Changed card and stat names to `const char*` to improve memory usage. This makes them immutable and require to use constants.
- Removed the ability to change a stat name (for this dynamic use case we can remove and recreate a stat)
- refreshLayout() refactoring in order to avoid too many layout refresh requests when updating components dynamically: let the caller trigger a layout refresh once.
- Removed refreshStatistics() because it it not refreshing the stats only but all the updated cards also
- Removed update calls when adding / removing cards and stats in order to avoid trigger a sequence of full layout updates: this is u to the user to call refreshLayout() when he has finished
- Support both ESPAsyncWebServer fork from ESPHome and yubox-node-org (which as a better api for sending websocket buffers and dealing with concurrency)
@ayushsharma82
Copy link
Owner

Hi @mathieucarbou ,

Finally got time to review this PR. Everything looks great and I appreciate all the work went into this!

I've a few questions to understand the changes better though:

Do not update card values while another task (like the async_http task running the WS callback) is reading the cards to build the layout!

Q: The callbacks were supposed to be non-blocking. What was the limitation in regard to above?

Fork of AsynTCP from ESPHome

Q: Do they have a fork for ESP8266 too? ESP-DASH has support for both ESP8266 and ESP32.

Q: Please describe in a short note on how batching is implemented.


void refreshStatistics();

void refreshLayout() { sendUpdates(true); }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function definitions inside header file creates an issue when embedding ESP-DASH within a class. Moving definitions to cpp file is suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

embedding

Oh! I never experienced that...
In fact, I am using that a lot (these are usually the kind of functions that could be inlined)
In which situation ? Thanks!

@ayushsharma82 ayushsharma82 merged commit 13fedc4 into ayushsharma82:master Feb 2, 2024
1 check passed
@mathieucarbou
Copy link
Contributor Author

Do not update card values while another task (like the async_http task running the WS callback) is reading the cards to build the layout!

Q: The callbacks were supposed to be non-blocking. What was the limitation in regard to above?

Non-blocking does not mean we don't have any concurrency issues. Non-blocking for Dash means the async_http task from AsyncTCP will receive the websocket data, forward the message to the Dahs WS callback to be executed. Building a complete json layout of over 200 cards takes time and is now done in batches. If during this time the loop task is updating the components, while the layout is being built, some concurrent access on some memory zones can happen while pointers get dereferrenced.

Async frameworks are good, but usually what we see in most Arduino lib is a real lack of handling: mutex, volatile, etc. These issues are also more seen when we have frameworks such as ArduinoJson which are saving pointers for later use (serialise) or with dual core.

As for ESPAsyncWebServer, the original repo from me-no-dev is crippled with concurrent issues leading to heap crash. The websocket queue and queue control for example are not synchronisez, neither the Message Buffer API. the yunodebox fork aims at fixing that.

That's why I am maintaining now a fork of this fork (https://registry.platformio.org/libraries/mathieucarbou/ESP%20Async%20WebServer).

With a small app, only using 1 core, you don't see all that.

My app is using both cores, has several tasks, more than 200 cards, etc. So I am bumping into any concurrent issues, if any ;-)

Q: Do they have a fork for ESP8266 too? ESP-DASH has support for both ESP8266 and ESP32.

Yes of course!

The only problem is that they do not deploy in Arduino Registry.

The important point is to depend ONLY on ESP Async WebServer without specifying any dependencies of ESP Async WebServer and without specifying any "owner" for ESP Async WebServer, like it is in this PR. This allows more flexibility for users to point to the ESP Async WebServer fork they want, and change its dependencies to ESPHome.

Regarding Dash, the only thing to do would be to add a note in the documentation regarding the recommended dependencies.

Q: Please describe in a short note on how batching is implemented.

The batching is really simple and is controlled with 2 parameters:

  • DASH_JSON_SIZE (default 2048)
  • DASH_JSON_DOCUMENT_ALLOCATION which is 3x DASH_JSON_SIZE by default.

DASH_JSON_SIZE controls the maximum size of serialised json to send in each WS message. DASH_JSON_DOCUMENT_ALLOCATION is a parameter only available for ArduinoJson 6 users which controls the allocated amount of heap memory when building a DynamicJsonDocument. ArduinoJson 7 is now free of these allocation limits, so no need to specify it.

The updateLayout funciton will go through all the cards, charts and statistics and build the Json payload. As soon as the payload reaches DASH_JSON_SIZE or as soon as the Json document overflows (which means we went over DASH_JSON_DOCUMENT_ALLOCATION), then the last entry (which could be partially written) is then removed from the Json document, the remaining payload gets sent, and a new Json document is created for a new payload, starting with the item that was partially or not added.

Also, the send() method was improved to remvoe any use of String, which causes a copy of the buffer into WebsocketMessage by ESPASyncWebServer. To be more efficient, we have to serialise the data directly into a buffer which can be directly used without further copy.

@mathieucarbou
Copy link
Contributor Author

FYI - here is the app I am building: https://yasolr.carbou.me

@ayushsharma82
Copy link
Owner

FYI - here is the app I am building: https://yasolr.carbou.me

I like the use of emojis as tab icons! App looks great.

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.

Step is missing from slider cards ArduinoJson 7 Compatibility
2 participants