Espeasy: Code cleanup

[psy0rz]

There is still lots of dangerous code in ESPEasy.

I propose a generic code cleanup, this is a copy of a comment i made earlier in an older issue:

• use strlcpy instead of strncpy. strncpy is nasty, see https://linux.die.net/man/3/strncpy. When using strncpy, the size always needs to be one less than the buffer (sizeof()-1) , and you need to make the last byte always \0. We should probably replace all str*cpy stuff by strlcpy, or regular Strings.

• All the temporarty static log-buffers are dangerous: if we printf something to it that ends up too big after formatting we have a buffer overflow and crashes/instability.

• Use "IPAddress ip;" for ip-adresses and convert to string with ip.toString() when needed.

• This gets copypasted all over the place and is horrible and unnecessary and dangerous:

strcpy_P(tmp, PSTR("HTTP : connecting to "));
sprintf_P(log, PSTR("%s%s using port %u"), tmp, host, ControllerSettings.Port);

Since recent versions of ESPEasy this should be changed to:

addLog(LOG_LEVEL_DEBUG, String(F("HTTP : connecting to "))+ControllerSettings.getHostPortString());

• addLog now also supports F() strings, so temporary buffers are no longer neccesary

• This is something i also see a lot when reading network/http responses (also bad code that is copy/pasted a lot):

  ```
  while (client.available()) {
  String line = client.readStringUntil('\n');
  line.toCharArray(log, 80);
  addLog(LOG_LEVEL_DEBUG_MORE, log);
  if (line.substring(0, 10) == F("HTTP/1.1 2")

Replace with :

while (client.available()) {
String line = client.readStringUntil('\n');
addLog(LOG_LEVEL_DEBUG_MORE, line);
if (line.startsWith(F("HTTP/1.1 2")))
{
```

• There are FIXME's at some places, we should review them and see if we want to fix some of these for 2.1. Some are to replace functions that do iffy stuff like converting a c-string ip-adress to 4 bytes. There are nice clean functions to do that. (in that specific case IPAddress.fromString() for example) We might want to try to get rid of things like that to get cleaner code and less bugs.

[TD-er]

Can even done with less code:
addLog(LOG_LEVEL_DEBUG, String(F("HTTP : connecting to "))+ControllerSettings.getHostPortString());

See #629

[psy0rz]

ahhh right the nice new stuff you've added :)

SHOULD even be done with less code. i'll edit my first comment to reflect it.

[TD-er]

Another thing that's bothering me is Strings are often already created and not being used.
For example for logs but the log level of all 3 log outputs is lower than the loglevel set in the addLog.

Also often you know how long a string is going to be, so do a reserve on it as soon as possible. That prevents heap fragmentation.

[TD-er]

Another thing we should do is to use enums (instead of #defines) to reflect states like the plugin_load, _save, etc.
When using enums, the compiler can warn us when not all cases are being handled.

[TD-er]

Also the ESPEasy.ino and Misc.ino are quite long.
Should split based on tasks / structs / formatting functions etc.

Frequently used operations on structs should be part of the struct itself (object oriented), like I did with the ControllerSettings.

Also structs which are being stored, should be marked accordingly along with a description how much space is left to extend the struct and what the implications are when things get extended.
Also moving member variables around is a big no-no in those classes/structs, which should be mentioned accordingly. Perhaps store these in a separate file?

[TD-er]

There is a lot of repetition of the same statements in the same file.
For example [index - 1] is being used 67 times in WebServer.ino.
Using a different name and don't repeat the "-1" would save a lot of chances for error.

These are now taken care of in #700, but similar ones may still exist in the rest of the code.

[s0170071]

There should also be some code addressing hardware faults like those failing flashes. I was thinking about CRC checks of code and spiffs.
Checking the code should be done piece-wise in regular intervals.
I had a faulty configuration once and had a hard time getting rid of it because the Esp did not start properly.

[psy0rz]

@s0170071 good point. at least we now check the error codes when reading/writing to flash and give feedback. (this wasnt even done as well before)