TD-er commented 4 years ago

There has been a lengthy discussion already about how to implement the patch to use 24 tasks on ESP8266 ESPEasy. However this was posted below a pull request, which makes it hard to follow and rather invisible to others unless they track all changes and updates on this repo.

So let's continue here.

The most useful parts of the discussion:

Arduino IDE compilation was (again) broken by my attempt to clean up the code.
Splitting defines into separate files made it hard to apply a user defined patch for 24 tasks which had been working for a while.
How to add/suggest some kind of universal define to make switching to a different set of tasks more reliable and reproducible
Other defines a user may want to set using a simple header file for creating a personalized build.

@uzi18 has some set to define 24 tasks: https://github.com/letscontrolit/ESPEasy/commit/f7e0d7cfdde65d9503ff08fd92d48681dc8da702#commitcomment-35122941

#define TASKS_MAX                          24
#define DAT_OFFSET_CONTROLLER            4096    // 0x1000 each controller = 1k, 4 max
#define DAT_OFFSET_CUSTOM_CONTROLLER     8192    // 0x2000 each custom controller config = 1k, 4 max.
#define DAT_OFFSET_TASKS                16384    // 0x4000 each task = 2k, (1024 basic + 1024 bytes custom), 12 max

@clumsy-stefan has another (incompatible) one: https://github.com/letscontrolit/ESPEasy/commit/f7e0d7cfdde65d9503ff08fd92d48681dc8da702#commitcomment-35126444

#define TASKS_MAX                          24 // max 12! 
#define DAT_OFFSET_CONTROLLER            DAT_OFFSET_TASKS + (DAT_TASKS_DISTANCE * TASKS_MAX)                        // each controller = 1k, 4 max 
#define DAT_OFFSET_CUSTOM_CONTROLLER     DAT_OFFSET_CONTROLLER + (DAT_CUSTOM_CONTROLLER_SIZE * CONTROLLER_MAX)  // each custom controller config = 1k, 4 max

TD-er commented 4 years ago

I was more thinking of places where it is actually being used. So either in .cpp files, or in .ino files.

clumsy-stefan commented 4 years ago

sure, sorry, wasn't meant tooo serious.....

uzi18 commented 4 years ago

@TD-er

#define DAT_OFFSET_CONTROLLER            4096    // 0x1000 each controller = 1k, 4 max
#define DAT_OFFSET_CUSTOM_CONTROLLER     DAT_OFFSET_CONTROLLER + (DAT_CONTROLLER_SIZE * CONTROLLER_MAX)  // each custom controller config = 1k, 4 max
// note not DAT_CUSTOM_CONTROLLER_SIZE
#define DAT_OFFSET_TASKS                16384    // 0x4000 each task = 2k, (1024 basic + 1024 bytes custom), 12 max

so we only change DAT_OFFSET_CONTROLLER and DAT_OFFSET_TASKS and DAT_OFFSET_CUSTOM_CONTROLLER is not correct in @clumsy-stefan - to be updated like above

maybe we can use 3rd option like just tasks 12+ store at the end of the file so it will be more/less backword compatible but need some logic load/save routines (incompatibility only in basic settings)

with https://github.com/letscontrolit/ESPEasy/commit/6277a8fad249cefc6ed8860b70f0f32b528603a1 you can't build 24 tasks version for esp32

TD-er commented 4 years ago

I had the issues described by @clumsy-stefan when trying his offsets. With the 'fixed' settings of @uzi18 I had crashes, when accessing something related to the tasks > 12

With these offsets used by @uzi18 it all seems to work just fine

#define TASKS_MAX                          24
#define DAT_OFFSET_CONTROLLER            4096    // 0x1000 each controller = 1k, 4 max
#define DAT_OFFSET_CUSTOM_CONTROLLER     8192    // 0x2000 each custom controller config = 1k, 4 max.
#define DAT_OFFSET_TASKS                16384    // 0x4000 each task = 2k, (1024 basic + 1024 bytes custom), 12 max

These settings look like this:

And the settings used by @clumsy-stefan (his screenshot):

TD-er commented 4 years ago

Please note, in the last commit, I have used the format @uzi18 uses for 24 tasks layout.

After dinner I can have another look at it.

TD-er commented 4 years ago

Hmm, just an idea here... What if we place the offsets inside the settings? Then reading the settings file will determine the position of the offsets. This way we can make the settings interoperable between nodes without having to change the build itself. At reset to factory default we can make a selection to change the nr. of tasks.

It will break backwards compatibility if you change the nr. of tasks.

uzi18 commented 4 years ago

we can but we tried to avoid it, we have also in basic settings cfg file version so we can upgrade it in the fly but it is one way ticket, we can also warn user to do backup of old settings before upgrade or copy file and use new one

TD-er commented 4 years ago

Yep, setting it to a different value will break compatibility. Maybe we could also have it changed back into the old version:

loose all tasks > 12
provided enough free space on SPIFFS.
Conversion can only be done at boot, right before the settings will be made active.

Converting the settings is just a matter of moving to another offset of a block of data. So we can split it and copy to a new file and rename the files.

This can also be used to convert from/to ESP32 settings.

I think we can then also change some sizes to make room for future upgrades?

This is the layout used on ESP32:

Edit: Hmm the settings struct on ESP32 is not the same size, so 1-to-1 conversion is not possible. But if the settings file has the offset, we could extend the "Settings Archive" feature to copy task definitions from it instead of the entire file.

clumsy-stefan commented 4 years ago

What if we place the offsets inside the settings?

That would be a cool thing with the caveats that @uzi18 wrote...

Please note, in the last commit, I have used the format @uzi18 uses for 24 tasks layout.

So this would break backwards compatibility?

I had the issues described by @clumsy-stefan when trying his offsets.

I still don'tunsderstand why these things worked before the splitting of the files... this is what concerns me most. It did work without issues before, where else are such issues introduced with splitting the files that we don't captured (yet)?

For example the SHT30 problem, why it only happens after the cleanup commit? or why it only happens with task nr. 13?

TD-er commented 4 years ago

What if we place the offsets inside the settings?

That would be a cool thing with the caveats that @uzi18 wrote...

I don't think the caveats are that much of an issue, with proper warning and a "convert to 12 tasks" as fall back.

Please note, in the last commit, I have used the format @uzi18 uses for 24 tasks layout.

So this would break backwards compatibility?

With your 24 tasks settings. So it was more of a warning to you.

I had the issues described by @clumsy-stefan when trying his offsets.

I still don'tunsderstand why these things worked before the splitting of the files... this is what concerns me most. It did work without issues before, where else are such issues introduced with splitting the files that we don't captured (yet)?

That's the whole reason I've been working on this the whole day, since I have the same feeling. I had entirely different plans for today.

For example the SHT30 problem, why it only happens after the cleanup commit? or why it only happens with task nr. 13?

Can you test on a node where starting with clean settings isn't really an issue, to see if it still is an issue with the current parameters (as in my last commit)

clumsy-stefan commented 4 years ago

Can you test on a node where starting with clean settings isn't really an issue, to see if it still is an issue with the current parameters (as in my last commit)

tried this, completely factory resetted node with just an SHT30 connected...

same issues as before: task nr. 13 goes blank when pressing "submit" and SHT30 can't be read...

uzi18 commented 4 years ago

@TD-er @clumsy-stefan isn't same issue as with dummy plugin?

clumsy-stefan commented 4 years ago

@uzi18 not sure if I understand? what you mean?

I just use the dummy plugin for demonstration, it does not happen with the dummy plugin on any other task, only if it's on task nr. 13

the same happens eg. with system plugin:

it does not happen on any other task, eg. 17:

uzi18 commented 4 years ago

dummy has got one bug with settings saveing (this should be fixed with my PR) but we found also another issues with settings after digging in code (only possible to use same output data type on all).

clumsy-stefan commented 4 years ago

as you can see it also happens with the system plugin... and it ONLY happens on task nr. 13 and no other one... also it does not explain the issue with SHT30... all of these things that have not been there before the cleanup merge..

uzi18 commented 4 years ago

dummy issues are before these changes, did not pull from git tree for some days, it is affected also on 12task version.

clumsy-stefan commented 4 years ago

did you read above? It's not (only) the dummy plugin and it happens only on task nr 13!!!! It's any plugin (in above screenshot the system plugin) and the SHT30.... however, let's see what @TD-er finds..

uzi18 commented 4 years ago

@clumsy-stefan no dummy have this problem on every task nr., try with my change only one line of code: https://github.com/letscontrolit/ESPEasy/pull/2601/commits/359fecff112669f1a15bf57969940a76e2052e6b

clumsy-stefan commented 4 years ago

@uzi18 sorry, but I think you don't understand... in my test it happens only on task nr 13 but with all different kind of plugins...otherwise it works fine, except for example the sht30 plugin....

TD-er commented 4 years ago

That's strange. I have it with the standard Custom.h settings and set the flag for 24 tasks.

With the settings suggested by @clumsy-stefan I got exactly as he described. Are you sure you don't have your settings also in the Custom.h file?

clumsy-stefan commented 4 years ago

excerpt from my Config.h:

#define USE_NON_STANDARD_24_TASKS
/*
#ifdef TASKS_MAX
#undef TASKS_MAX
#endif
#define TASKS_MAX                          24 // max 12! 

#ifdef DAT_OFFSET_CONTROLLER
#undef DAT_OFFSET_CONTROLLER
#endif
#define DAT_OFFSET_CONTROLLER            DAT_OFFSET_TASKS + (DAT_TASKS_DISTANCE * TASKS_MAX)                        // each controller = 1k, 4 max 

#ifdef DAT_OFFSET_CUSTOM_CONTROLLER
#undef DAT_OFFSET_CUSTOM_CONTROLLER
#endif
#define DAT_OFFSET_CUSTOM_CONTROLLER     DAT_OFFSET_CONTROLLER + (DAT_CUSTOM_CONTROLLER_SIZE * CONTROLLER_MAX)  // each custom controller config = 1k, 4 max 
*/

#endif

as you see, it's commented out... except the new define...

TD-er commented 4 years ago

Hmm I made a clean build here and now I do have the same issues as you describe. I will try to replicate what I did before dinner.

N.B. it also happens with the Switch plugin, so it is not related to the DHT3x plugin.

If I remember correctly it may also have been a define of the 24 task parameter at PIO level.

clumsy-stefan commented 4 years ago

Hmm I made a clean build here and now I do have the same issues as you describe.

at least no strange magic going on on my nodes 😄

I think you need to find what esentially changed before and after the cleanup merge...

thanks a lot for your help and efforts!!! really appreciated!!!

TD-er commented 4 years ago

I really think we're running into another bug regarding Arduino compilations. I have now set all defines in the code to set the TASKS_MAX to 24 and it is working perfectly fine. So #ifdef stuff to toggle defines is just not working like it should be.

The value of TASKS_MAX is clearly not used consistently throughout the compilation process (very likely the preprocessor)

So that's another day gone looking at something that's clearly a bug.

This makes you wonder what else is there in the code which is causing issues and is actually a compiler bug here.

I don't yet know what is actually going wrong, so I don't know how or where to report it. But if this is the fix:

  #ifndef TASKS_MAX
    #ifdef USE_NON_STANDARD_24_TASKS
     #define TASKS_MAX                          24
    #else
     #define TASKS_MAX                          24
    #endif
  #endif

Then there is clearly something wrong here. Apparently it is not (only) a bug in PlatformIO here, but clearly something is terribly wrong here when combining generated .cpp code from Arduino stuff, combined with defines in .h files.

I will now take a shower and go to bed and sleep on it what to do here. I think the safest way is to move all defines a user wishes to make to a single file. So either include that Custom.h file or use the ESPeasy files and no #ifndef....#def stuff, just have to make sure it is defined in the Custom.h or else it will not build.

Other way is to do it in PlatformIO and let it generate from a Python script, but then Arduino IDE support is gone.

But this way is just not working. It just is buggy as hell.

clumsy-stefan commented 4 years ago

first of all, thanks a lot for all your help, debugg and time you're investing. this is really much appreciated (by everyone Iguess)!!!

now for the issue, the (temporary) fix seems rather simple, so I?ll try that. Now for the last changes, could you revert back to the original laysout/offsets? I think there is currently no need to completely change the config file (offsets) if it's working with the original ones and the bug is somewhere completely different. also because we probably shouldn't change in multiple places at the same time!

have a good day!!!

clumsy-stefan commented 4 years ago

quick update: I can confirm @TD-er 's findings, changing the TASKS_MAX define to 24 in the above definitions (and revrting to the old offset's) makes everything working again, except that murphy decided to kill my SHT30 on my test node (still t.b.c.)...

However the uncertainity still remians in what other places where we have conditional defines the same problem occurs (but did not yet surface)... this could explain the larger-than-expected sizes for example...

uzi18 commented 4 years ago

@TD-er should move all files to h/cpp, and check durring build if values of TASK_MAX changed somewhere

TD-er commented 4 years ago

@uzi18 Well that's probably what should be done, but it's not what I'm going to do. Problem is that we simply don't know yet what exactly causes these issues and I already lost a lot of time in this, which is very likely a bug in how Arduino converts its code to .cpp.

I will collect all defines which may be user defined and determine structs and put them in ESPeasy.ino. (not in .h file) So then we will be sure they are being dealt with in a predictable manner and later I will decide on how to use the other core related stuff.

Something similar is also causing issues with forward declarations and global defined variables. So it is good we now have somewhat clear what is happening here so we can make sure to use a work around for this. But believe me that I have spent a lot of thinking about these issues last night and it feels a bit frustrated to realize we probably have a lot more hard to reproduce issues which are very likely related to these issues.

uzi18 commented 4 years ago

If you move all other ino files to h/cpp/c you probably will resolve all these problems related to conversion and Arduino builder support this, just one more step left. It is good part of work and your cplugin queue macros will work one more time as expected :)

TD-er commented 4 years ago

Thing is you're also converting all plugins and controllers then. That's kind of breaking with the origin of the project. Not sure yet if that's wise to do. The project is setup to have the plugins part completely separated. Nothing defined in a plugin will be used elsewhere. So I think the plugins can remain .ino files.

clumsy-stefan commented 4 years ago

Nothing defined in a plugin will be used elsewhere. So I think the plugins can remain .ino files.

Are no crossreferences at all in Plugins? Especially MQTT stuff that is all over the project (or is that only in controllers)? Serial or GPIO stuff (Switch)?

Even if you would convert everything. can you be sure that these issues are gone then?

TD-er commented 4 years ago

Those parts use external files, some of which are already in .h/.cpp files and some still in .ino files. The only tricky part may be the global defined variables. Thing is with C++, if you define global variables (not members of a class/struct) in a .h file and you include that file more than once (even with #ifdef ... #define wrappers), you will create multiple instances of that same object. So then you have two objects with the same name. That may give strange results. So for example the ESPEasy-glbals.h file should really only be included once or else you will break everything. Or you declare them in a .ino file, but then you cannot have an easy access to them from within .h/.cpp files.

So we do keep having possible pitfalls for issues like these as long as we keep merging .ino and .h/.cpp files in the same project.

As some kind of temp work around I created the fwdecls.h file to have some functions forward declared in a .h file so we can call them from within .h/.cpp files while the functions are still declared in .ino files. That's the only way I could think of to not converting them all and still have a working project. Well that was working fine, so it seems, until you showed the effects of the Arduino conversion + C++ preprocessor.

So for now I really don't know what will be the best here to do.

uzi18 commented 4 years ago

@TD-er another solution - before build (with python) create new dir src_effective copy all from src dir there all .ino plugins copy to .h files, next generate one file all_plugins.h with one line "#include "plugin001.h" per plugin, next at EspEasy.h add before first real function #include "all_plugins.h"

or just have all ino plugins in one known dir to convert

and we need to change build dir to another or change src dir name ;)

TD-er commented 4 years ago

@uzi18 That may work, but that's not how I would like it to work. If something breaks in that process, then it is a hell to debug. I'm not masochist enough for it I guess ;)

I was also thinking about another work-around, since the issue we're seeing appears to be related to multi-step interpretation of defines (actually replacing the defines with text strings by the preprocessor). My idea is to have the 24 or 12 first set into another define itself and only using that define to initialize the TASKS_MAX value.

uzi18 commented 4 years ago

one big advantage of all plugins/functions included in one file is better optimisation by compiler

this idea is only for ino plugins, controllers can be moved to h/cpp and I take here into account all core is h/cpp already - take it only as compatibility layer with plugins

TD-er commented 4 years ago

one big advantage of all plugins/functions included in one file is better optimisation by compiler

Not entirely sure, since the Arduino environment is also trying to do that in their build process. They put it in .cpp file, which ofcourse need forward declarations like it was in a .h file. Problem with functions defined in a .h file is that those functions are always compiled as if they were declared inline. Which effectively means the function is not wrapped in a function call, but its internal is just copied into the function that calls it.

That's fine for functions which are really small like just returning a member value or something like that. But for larger functions it does mean the executable will grow with every other call to that function from another location in the code.

A bit like template functions. For every new template argument there will be a new instance of the compiled code in the binary.

uzi18 commented 4 years ago

even simpler just include every pl ugin without conversion, with '''

ifdefs USES_Pxxx

include "pluginxxx.ino"

endif

'''

clumsy-stefan commented 4 years ago

before build (with python) create new dir src_effective copy all from src dir there all .ino plugins copy to .h files, next generate one file all_plugins.h with one line "#include "plugin001.h" per plugin, next at EspEasy.h add before first real function #include "all_plugins.h"

this sounds really ugly (sorry not to take it personally) and for sure breaks building in every other environment than PIO. I guess....

what about putting the user-changable defines in one file and then just check if it's defined at all or throew error otherwise at build time (I think @TD-er already suggested something like this...

uzi18 commented 4 years ago

already wrote it can be more simple and Arduino builder compatible

ifdefs USES_Pxxx

include ".. /plugins_ino/pluginxxx.ino"

endif

here: https://github.com/letscontrolit/ESPEasy/issues/2621#issuecomment-532694666

uzi18 commented 4 years ago

@clumsy-stefan not a problem, just trying to put some ideas, @TD-er did lot's of work with code and testing with you and it is better to win this battle now than try another (random) time with wasting resources.

clumsy-stefan commented 4 years ago

already wrote it can be more simple and Arduino builder compatible

Hmm... but this doesn't affect on how the whole storage part is handled, where the issues with (wrongly defined) TASKS_MAX occurs... The Plugins are not the issue... or probably I misunderstand that comment...

TD-er commented 4 years ago

I think we're also slightly diverting in what each of us is trying to say :)

I think (hope?) keeping the plugins in .ino will not cause any of these issues. If that appears to be true, then it should not be needed (yet?) to convert those to .h/.cpp too. At compile time converting to some .h or .cpp is not the way to go, since that's also what Arduino is doing and they have (hopefully) far more knowledge about this than we have and their method is also not working like it should. (or my knowledge of C++ has some serious short comings)

If in the end we must convert the plugins to .h/.cpp too, then so be it, but then I will do it manually. This conversion will be slightly automated, but not automated as in a full automated conversion during each build. Like I said that will then cause major headaches when something unexpected is happening.

What really bothers me by the way, is that I have the feeling I'm tracking bugs in a lot of 3rd party code for the last 2 years which is holding this project's progress back for a long time.

Build issues (PlatformIO and now Arduino IDE so it seems)
WiFi issues.
LWIP issues.
esp8266/Arduino issues.

These are happening at way too many levels, so new features, bugfixes and code optimizations in our own code are pushed back over and over again.

clumsy-stefan commented 4 years ago

@TD-er couldn't agree more... but I guess that's the problem using open source community code and libraries... you never now what's happening at other places...

still I think you (we) did make quite some advance over the last month's (years) probably not as fast as expected, but quality wise it has improved a lot. Eg. my nodes are running for weeks now (rather than hours or days) as some months ago!!! so just keep up the good work!! 😄

TD-er commented 4 years ago

but I guess that's the problem using open source community code and libraries... you never now what's happening at other places...

Well, at least with open source you can see what's going on. I've also had to work around bugs in proprietary code and that's debugging with a black box.

clumsy-stefan commented 4 years ago

offtopic... but yes, that's why I also use more or less exclusively opensource and try to contribute if possible...

TD-er commented 4 years ago

Just an update, will commit my updated files soon.

I'm still not convinced that the Arduino preprocessor is off the hook here, since the hiding of compiler errors is caused by the way the Arduino code conversion operates.

On the other hand, the way we use the .h/.cpp files along with Arduino .ino files is also incorrect.

So, what is happening here?

The scope of a #define is only local to the file in which it is defined. This means that if it is defined in a .cpp file, it is only present in that file, from the point where it was defined, until it is undefined, or the end of the file has been reached.

If you define something in a .h file, you cannot expect it to be present in other files, unless you include that .h file.

So by including the Custom.h file, the content of that file is copied by the preprocessor into the .cpp file that's being compiled. This is something you really should understand to see all the implications of it. It also has to do with variables defined in that file, not being in the scope of a class/struct. For example, if you include file Foo.h which has variable int bar; declared to be globally defined, then every .cpp file where this Foo.h is included will have an instance of that same variable bar. So there are several versions of the variable with the same name.

Now back to the #define issues, combined with working in Arduino environment. When compiling code in the Arduino environment, the .ino files are being concatenated one after eachother to a single .cpp file and the function declarations are being forward declared at the start of the file. So at some time, the .h files are included and thus copied into the generated .cpp file by the preprocessor. Meaning, the define will occur at some somewhat random position in the code. All code following the initial define will have the initial value of the define replaced in the .cpp file. Then the preprocessor will start over and tries to replace the other occurrences of the defined macro with the value of that macro, known at that time. (!!). So what if we have some defined depending on USE_NON_STANDARD_24_TASKS, which is only known when we include the Custom.h? Then the preprocessor may have replaced occurrences of your macro without knowing USE_NON_STANDARD_24_TASKS was defined. So essentially the value for TASKS_MAX, which is depending on USE_NON_STANDARD_24_TASKS may have had the value 12 at first and 24 later on during the replacement actions of the preprocessor.

So what do we need to do here?

include "ESPEasy_common.h" whenever you define something that may be defined also in Custom.h (which is included in ESPEasy_common.h ) (Do not include it in Custom.h)
include the .h file which has the define you need, in any file you need it, right at the start of the file.
Try to include as few .h files in a .h file as possible. Forward declare where possible and include .h files needed in .cpp files.

I will think about what to do with the global defined variables in ESPEasy-Globals.h. C++17 has the option to define these using inline, which does help the linker to link those "global" variables. which does allow to include the ESPEasy-Globals.h multiple times. But I'm not sure how many compilers already support this and it is still not really an elegant solution. The best way would be to have a single (singleton?) object which has get/set functions to allow access to these variables, but then I think any file using them may need to be updated. Or maybe I just declare them as extern and add a .cpp file for it to actually declare them.

Anyway, I am now more confident we're back on track here and maybe we also do fix some stuff without knowing it ;) It does explain very well why sometimes a build is working fine and sometimes it isn't.

Edit: Almost forgot to mention why I don't think Arduino's process is off the hook here. The way how Arduino environments are being compiled does allow for very strange situations and since eventually the defines are made clear, they do not end up as being unknown. Thus effectively suppressing compiler errors.

One way this could be prevented is by not only collecting all function declarations and echo them as forward declarations in the generated .cpp file. But the same could be done by all #include statements in .ino files. If those are put in the beginning of the generated .cpp file, then defines will be known to the preprocessor right at the beginning

clumsy-stefan commented 4 years ago

A quick first check with a couple of nodes show that the new commits seem to work fine. Currently I can't see any strange things going on... will leave it running on these nodes and do some checking during the day.

@TD-er great work!!!

clumsy-stefan commented 4 years ago

ESP32 won't compile with USE_NON_STANDARD_24_TASKS due to line 66 in src/DataStructs/SettingsStruct.h. I guess the line should check for #if defined(USE_NON_STANDARD_24_TASKS) && defined(ESP8266).

TD-er commented 4 years ago

Will fix it, but why would you build ESP32 with USE_NON_STANDARD_24_TASKS? To make settings exchangeable? If so then it should not be limited to ESP32 I guess.

clumsy-stefan commented 4 years ago

hmm.. right, I guess after changing all the defines in my Config.h probably this is was defined even though it shouldn't... however the assert fails, as TASKS_MAX for ESP build is set to 32 regardless of USE_NON_STANDARD_24_TASK (which is ok), but then you should check everywhere for ESP8266 everywhere.

but the exchangable thing is a good point, why not...

letscontrolit / ESPEasy

Discussion on how to define 24 task settings #2621

ifdefs USES_Pxxx

include "pluginxxx.ino"

endif

ifdefs USES_Pxxx

include ".. /plugins_ino/pluginxxx.ino"

endif