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

[#1591] Move system timers to main scheduler #1594

Merged
merged 3 commits into from Jul 21, 2018

Conversation

TD-er
Copy link
Member

@TD-er TD-er commented Jul 20, 2018

And fix longpulse command

With other plugins active, like the OLED Framed, the set period of longpulse_ms may fluctuate and durations below 100 msec are quite useless.

However, when running just a few sensors, the set pulse width is quite accurate.
espeasy_pulsewidth_ms_50
See right column, "-PulseWidth (NW)"

image

Must still check a few other plugins.

And fix longpulse command

With other plugins active, like the OLED Framed, the set period of `longpulse_ms` may fluctuate and durations below 100 msec are quite useless.

However, when running just a few sensors, the set pulse width is quite accurate.

Must still check a few other plugins.
@TD-er TD-er added the Category: Stabiliy Things that work, but not as long as desired label Jul 20, 2018
@Grovkillen
Copy link
Member

Grovkillen commented Jul 20, 2018 via email

@TD-er
Copy link
Member Author

TD-er commented Jul 20, 2018

Always nice to have some visual debugger while coding :)
It can also make very colorful images showing the jitter.
Maybe I will let it run for a while this evening to get one of those.

Would also be nice if someone could test it.

@Grovkillen
Copy link
Member

I'm feeling a little off the loop right now. We got really hot weather, I'm on holidays, and the Swedish fires are "only" 70km away from our house. I will hopefully get more time next month 👍

@Grovkillen
Copy link
Member

(so I can help out more I mean)

@clumsy-stefan
Copy link
Contributor

compiled it with this PR and flushed some uints... will report tomorrow or so..

These timers were not used.
…val.

All enabled plugins have their own timer added to the scheduler.
The `PLUGIN_INIT` function call is used to reschedule when it was added, or enabled.
The set interval for the plugin will be used to set the new entry in the scheduler.
@clumsy-stefan
Copy link
Contributor

up to 04b631e it works fine... I'm adding the new commits now and retest...

@clumsy-stefan
Copy link
Contributor

clumsy-stefan commented Jul 21, 2018

had them running for a couple of hours now, all nodes seem to run fine, the only difference I'm seeing is in CPU-Load and Memory consumption, see attached graphs (updated the unit this morning about 9 o clock)!
image
image

@v-a-d-e-r
Copy link

v-a-d-e-r commented Jul 21, 2018

Can confirm this. Seems to run stable so far, but CPU load is too high with 10-20% and NO enabled controller and NO active task! That can not be...

@Grovkillen
Copy link
Member

Maybe we should "tare" the percentage with nothing activated and a clean install?

@clumsy-stefan
Copy link
Contributor

if I understood correctly this is anyway not an effective "load" which is shown, but some indication on the actual behaviour.

however, just from a sunjective point of view I got the impression that especially the web-interface is a bit less responsive with this change... but again, this is not measured but just my feeling....

@TD-er
Copy link
Member Author

TD-er commented Jul 21, 2018

@clumsy-stefan Those charts, do they show the change between the earlier build using the new scheduler and the last additions I made?

@v-a-d-e-r
The scheduler now looks into a list of things to do and I count the time (in microseconds) it does not schedule jobs.
Still not all has moved to this scheduler and it is running background tasks on every loop.
Also calls to "delay()" from within code adds up to the "non-idle time", but the CPU is not really doing much. (background tasks are also handled when calling delay())

The general idea is a lot of things still use loops to see if there is something to be done.
Moving the things to-do to this scheduler means we only have to spend time on them when needed.
Also I fixed a number of "lookup" things already by simply keeping some kind of cache. I am sure there are still some left.
One example is the call for "run N/second". These calls still look at all tasks to see if they are enabled and if there is something to do.
I will move these to separate jobs in the queue, so we no longer have to check if there is something to be done.
This may lead to some other strange side effect, that the apparent CPU-load may appear higher with enabled plugins, but lower CPU-load when no plugins enabled.

In short, there is still a lot of room for improvement and a lot of cycles are currently still wasted.
But for now I believe the displayed CPU-load is closer to the truth than it was, since it is now based on idle time.

@clumsy-stefan About responsiveness.
I had the same feeling, but that was already before this schedule was added. Maybe to do with a newer core library. It is also consuming more memory since that latest update.
The JSON pages still reply as fast as before, based on the timings of the Network tab on the inspect window in Chrome.
But the Logviewer is hardly working at this moment. Not sure what's happened there.
Maybe some limit on accepted simultaneous connections? Or the JavaScript does not recognize a reply as a reply on its request?

@v-a-d-e-r
Copy link

@TD-er: "background tasks are also handled when calling delay()" --> That info is new for me. What about the "delayBackground" command now? Obsolete? What's the differences?

@clumsy-stefan
Copy link
Contributor

@TD-er yes, graphs show before and after commit 447e958

@TD-er I always use the latest GIT for the ESP Core library. However I had the feeling, that memory usage got better (eg. more free memory) for GIT versus 2.4.1 core... but can't really say about responsiveness, especially since when it got more laggy....

btw: about the core version, there were changes in the tone/frequency generation handling and some issues about memory corruption etc. since a few commits there is a tone() function in the core which conflicts with the function included in the ESPCore... probably these issues when playing frequencies on some pin's that have been fixed in the core could also occur in the ESP function and is woth looking at?
see:
esp8266/Arduino@e6af980 and related issues..

@TD-er
Copy link
Member Author

TD-er commented Jul 21, 2018

There are several background tasks.
In the core library, when calling yield() or delay(...), some tasks assigned to another ring are being performed.
For example maintaining WiFi connection, handling incoming data from the IP-stack, etc.

When you don't serve those on a regular base, you will loose connection, buffers will overflow, etc.
And if the interval between those calls is long enough, the Soft-Watchdog will kick in. If that doesn't happen, the hardware Watchdog will reset the node.

So that's the core library part of the background tasks.
Our own void backgroundtasks() is still needed, but I may want to move this to the scheduler part.
It is about handling web- and DNS-requests, calling plugins to look for their serial inputs, etc.
These should all be served via scheduled jobs, but also the serial part should be a separate C++ object that will manage all time critical parts, keeping sync, verify when packet is complete, etc.
So all those will in the end move to a central class managing the communications and scheduling an event-handling job for the appropriate plugin. This will make responses a lot faster and takes the hard part out of the plugins.

@TD-er TD-er moved this from High priority to In Progress in Improve Timing Jul 21, 2018
@TD-er TD-er merged commit 7e67ce5 into letscontrolit:mega Jul 21, 2018
Improve Timing automation moved this from In Progress to Closed Jul 21, 2018
@TD-er TD-er deleted the bugfix/longpulse branch July 21, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Stabiliy Things that work, but not as long as desired
Projects
Improve Timing
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants