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

WIP: reduce stack buffer usage for AudioReactive settings page #94

Closed
wants to merge 1 commit into from

Conversation

softhack007
Copy link
Collaborator

@softhack007 softhack007 commented Dec 10, 2023

Currently we cannot add any more options to the AudioReactive settings page, as the buffer used by oappend() is very limited and cannot be expanded any more (its placed into the task stack, not into the heap ... for historical reasons and to help tiny 8266).

This is an attempt to work around the problem, by shortening the strings that are passed into oappend(). A typical call looks like this: oappend(SET_F("dd=addDropdown('AudioReactive','config:AGC');"));. We can shorten the JS function names like this

  • adI = addInfo
  • adD = addDropdown
  • adO = addOption
  • adF = addField

before: String buffer usage: 3540 of 3871 bytes
after: String buffer usage: 3158 of 3871 bytes
==> 382 bytes saved

Actually its even possible to save more buffer space, if "AudioReactive" gets replaced by "AR": String buffer usage: 2784 of 3871 bytes
==> another 368 bytes of potential savings

To be discussed:
a) depending on predefined PINs from the buildenv, stack buffer usage can be bigger because descriptions get longer.
b) source code is harder to read after replacing names; so maybe better that oappend() peforms the replacement behind the scenes, without needing to modify audioreactive code.
c) Changing "AudioReactive" to "AR" in parameters causes problems in other areas; so maybe that could be another behind-the-scenes optimimization done by oappend. It would then need to be "undone" in the JS part, so that parameters with "AR" become "AudioReactive" again.

actually for point c) i don't have an idea for implementation yet - I'm still a "hobby coder" when it comes to JavaScript 😅

@netmindz, @ewoudwijma and other devs: I would be happy to get your opinions, ideas, and possible alternatives.
Right now we are at the limits and cannot add new options to the AudioReactive settings page any more.

- adI = addInfo
- adD = addDropdown
- adO = addOption
 - adF = addField
@softhack007 softhack007 marked this pull request as draft December 10, 2023 19:18
@netmindz
Copy link
Collaborator

I didn't know what oappend was limited in length.

Definitely agree that shortening the names makes it harder to understand what is going on, it's already pretty difficult.

Use of more user friendly constants who's value contains the shorter name sounds like a good compromise.

In order to not break people's existing config files we would definitely need to swap to the longer name when saving /loading the config

softhack007 added a commit that referenced this pull request Dec 11, 2023
we can use `-D CONFIG_ASYNC_TCP_TASK_STACK_SIZE=xxxx` to increase the stack size of AsyncWebserver - default is 8196.
This should give us a bit of headroom for new UI items.

We still need PR #94 for more savings.
@softhack007
Copy link
Collaborator Author

Note to self:

  • use "A~" instead of "Audioreactive", but only in the short-named functions (see first post).
  • The short functions will then expand (replace) any "A~" inside parameter strings back to "Audioreactive".

-> Fully transparent, and no problems with user settings or anything.

@ewoudwijma
Copy link
Collaborator

Sounds good! Shouldn't we do it consistently for all usermods?
Maybe use a 2 or 3 letter abbreviation?

Djelibeybi pushed a commit to Djelibeybi/WLED-MM that referenced this pull request Jan 15, 2024
we can use `-D CONFIG_ASYNC_TCP_TASK_STACK_SIZE=xxxx` to increase the stack size of AsyncWebserver - default is 8196.
This should give us a bit of headroom for new UI items.

We still need PR MoonModules#94 for more savings.
@softhack007
Copy link
Collaborator Author

Sounds good! Shouldn't we do it consistently for all usermods?
Maybe use a 2 or 3 letter abbreviation?

Indeed this would be good. On the c++ side, we could do the replacement "behind the scenes" inside oappend(). On the JS side, String replace() could be used for "unpacking" abbreviations.

https://www.w3schools.com/jsref/jsref_replace.asp

@softhack007
Copy link
Collaborator Author

oappend(SET_F("dd=addDropdown('AudioReactive','config:AGC');"));. We can shorten the JS function names like this

  • adI = addInfo
  • adD = addDropdown
  • adO = addOption
  • adF = addField

The principle of this PR seems good, however in the next days I'll do a fresh restart using latest MDev.

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.

3 participants