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

P021 state based extension #5085

Open
wants to merge 36 commits into
base: mega
Choose a base branch
from

Conversation

flashmark
Copy link
Contributor

As discussed previously I reworked P021 to support my floor heating pump functionality. This should be a backwards compatible upgrade from previous versions. I tried to make it as flexible as possible by adding several configuration options. Documentation is updated accordingly.

src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
@TD-er
Copy link
Member

TD-er commented Jul 13, 2024

See this screenshot from your docs:
image

These units of time might be perfect for your specific use case, but keep in mind the "level" plugin is a generic plugin.
So there might be enough use cases where a time set in hours (or even minutes) is too long.
Also it is quite confusing to have different units of time on the same page.

Better have it set as seconds.
Maye later we could add some input filter which can then be used on each input field where you set a time in seconds, which allows some input formatting like "1d3h5m10s" or whatever notation you wish.
The PLUGIN_WEBFORM_SAVE can then parse this format and convert it into seconds when saving.

But that's for later, so we can make it generally available for more plugins.

src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
@flashmark
Copy link
Contributor Author

Reworking the code. I saw quite some useful comments and lacking some time to fix it in a rush.

src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
src/_P021_Level.ino Outdated Show resolved Hide resolved
@TD-er
Copy link
Member

TD-er commented Aug 18, 2024

OK, I have no idea what values I have been looking at when concluding it would save 10k+....
Ton also tested my code and I also did a lot more testing and the gain is a lot less.
However I am convinced my code changes will make the code (for P001/P009/P019) more readable and probably a bit faster too.

@TD-er
Copy link
Member

TD-er commented Aug 22, 2024

I just cancel your build job and after the release build has finished, I will resume your GH Actions build
Otherwise I have to stay up a bit longer...

@TD-er
Copy link
Member

TD-er commented Aug 22, 2024

I just updated this PR to include merged I made right after the release build.
This should give a few kB extra room for ESP8266 builds.
Please do not fill those up immediately.

@flashmark
Copy link
Contributor Author

I will only try to reduce memory usage. With the memory reduction all builds are successful. I tested the plugin with and without reduced build size on a ESP32. I am not sure if tests on ESP8266 are required for a merge to mega. If such tests are required I need some time to arrange a test setup. Most of my spare modules are ESP32 and ESP32-C3. This weekend I will most time far away from my keyboard.

@tonhuisman
Copy link
Contributor

It's good to know if all is working as intended on an ESP8266, but if it already works fine on an ESP32, then most of the time it won't be problematic on an ESP8266. This plugin is not directly related to CPU performance or available memory, so not too much to worry about.

@TD-er
Copy link
Member

TD-er commented Aug 23, 2024

The most important thing to keep in mind is that you should only add new features of existing plugins to ESP32-builds and only to ESP8266 when it isn't a regular build (thus only custom builds should be able to run those new features.

As you've seen yourself, it is really hard to make it all fit again and it has taken me about 20 - 24 hours of work last week to gain these extra few kBytes of space in the build size of ESP8266.

Like I've mentioned before a number of times, I have spent many 100's of hours in the past to make it all fit on ESP8266 and I don't think I should keep doing this as it will result in hours lost which could have been spent on new features, bug fixes, etc.

Espressif already declared the ESP8266 as deprecated about 5 years ago, so we should just consider the ESP8266 as "feature frozen" for regular builds like "collection X" or "normal" etc.
New features should -where possible- still be possible on ESP8266 for "custom" builds, but no longer for other builds.

@flashmark
Copy link
Contributor Author

Sorry I am late in reply. Other stuff to do outside ESPeasy.

Problem with my changes to P021 was the fact I refactored it a lot to fit the new functionality. This makes it quite difficult to get the same memory footprint by "disabling" the new stuff. Is there a way to detect the ESP8266 builds to restrict the functionality even more? We could at least disable all options on the setup page and cut out the associated control part too. Would make documentation a bit more difficult as documented features are not available...

Discussion started with an new plugin fitting my timing related requirements. Suggestion was to add them to P021. Indeed it was a nice challenge to get it combined with backwards compatibility, but at the cost of some memory. We can still decide to create a new plugin and leave P021 unaltered. It will duplicate functionality, but the new plugin can be left out of all ESP8266 code.

With additional savings by cutting functionality on ESP8266 I think it is wise to do some testing with this code on a 8266 too. For now the optimized code is tested on ESP32 only. Please advise how to continue.

@tonhuisman
Copy link
Contributor

Currently all builds succeed, because we where able to (again) reduce the ESP8266 builds by a few kB (though I think we're close to rock-bottom now). I'd leave the new functionality in P021, and not create a new plugin.
It seems to work as intended, so no need to change again, it's fine as it is now 👍.

@TD-er
Copy link
Member

TD-er commented Aug 31, 2024

I think you should add a separate define which will then be used only within the P021 code.

#ifndef LIMIT_BUILD_SIZE
#define FEATURE_P021_EXTRAS  1
#else 
#define FEATURE_P021_EXTRAS  0
#endif

And this could then be used as wrapper for all these extra features like this:

#if (FEATURE_P021_EXTRAS)
....
#endif

In the documentation you could add some asterisk next to the title of some feature which is only included in non-limited builds.

@flashmark
Copy link
Contributor Author

I updated the code to remove new functionality on builds with limited flash sizes. Still to determine how to switch this on for all affected builds. Documentation is also updated to mark the unavailable parameters. Functionality is hand-tested for full & limited variants on ESP32.

@flashmark
Copy link
Contributor Author

I could not spend much time on this plugin for some time. Meanwhile the plugin is working for more than a week to control my floor heating pump without problems. Can somebody review the logic to switch the additional functionality on/off and advise on a final configuration? I am missing some experience in managing multiple build options. Probably we should left all esp8266 versions without new functionality and enable it on the other hardware.

@flashmark
Copy link
Contributor Author

Found a bug introduced due to the memory optimizations. I missed a condition check due to which extending the active phase after input became inactive was not working. I did manual testing on ESP8266 (Wemos D1) and ESP32. Software has been running for weeks at my place without issues except for not extending the active phase as described above. To be reviewed is the selection of the code size and supported functionality. For now it depends on LIMIT_BUILD_SIZE.

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