-
Notifications
You must be signed in to change notification settings - Fork 37
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
Update tasks #371
Update tasks #371
Conversation
Hi, I disagree with letting users configure the stack sizes. To set the stack size, you need understanding of the underlying code, and if set incorrectly can easily throw the ESP into a boot-loop. Then users will open issues here and we'll have to explain you have to factory reset the ESP to get it working again. I see it can be useful for debugging, I'd suggest to only show it when debugging has been enabled by calling the "/debugon" endpoint, so it's clear this is and advanced debug setting, and not meant for the casual user. If more stack size is needed to to more retrieved and published entries, the stack size should by default be adjusted dynamically by allocating a certain number of bytes for each entry. Also, there should be a limit of maximum entries you can set, because if it could blow up the stack size too a point that also results in a boot loop. |
I understand your hesitance. I propose the menu item is only visible on the homepage when /debugon is enabled (which is still not documented btw) but allow direct access to /advanced for those who know the url (and for now not document it). I will also create a bootloop counter that resets these settings to default when 10 restarts in a row are detected before the ESP is up for 2 minutes. This should break boot loops automatically or allow you to change the settings yourself as the system remains up for more than 2 minutes. Keypad and Time Control lack a maximum in master at this time, so currently this will definitely blow up the task if you have too many entries in your Nuki device. Dynamically allocating stack based on the amount of entries is non trivial because the required stack size does not scale linear based on the total amount of auth log + keypad + time control entries. It will also interfere with setting custom stack sizes. A solution could be to add a dynamic amount only when setting very high number of entries and when the stack size is set to default. Will work some more on this. |
Sounds good. I know calculating the stack size isn't that easy, I guess we can only do an educated guess. I'd like to call the endpoint something more obvious than "advanced", like developer settings so that it's clear for the average user not to mess with this. |
04019b9
to
09ddeeb
Compare
I called it advanced because I don't believe it should necessarily only be a developer option. Not publicly listing it on the webpage and README, the big red warning should you land on the page and the auto enabled boot loop mitigator should be enough to deter the average user imho. I have setup minimum and maximum values for all changeable settings. I got the settings below running stable on a ESP32 D1 Mini (ESP32-WROOM-32) with presence detection disabled. |
Merged. Let's see wether or not this will create issues :D |
-Disable the presence detection task when presence detection is disabled in settings to save RAM and CPU cycles.
-Allow enabling and disabling the webserver using MQTT to save RAM and CPU cycles.
-Add an Advanced configuration menu to the webserver with options to change the amount of processed/retrieved auth log/keypad/time control entries and change the stack size for the 3 different tasks.