Consolidate ESP8266 & ESP32 calls and enable addition of other boards/connection methods

[kcranky]

Not really a bug report or "issue", just wanted to open a discussion on this. It's along the same lines as the "has connection"/"pull out wifi" discussed in #8

Description of the problem.
Maintaining multiple versions of the same sets of code is tough. I've looked into ways that the library might be able to be consolidated, but I haven't found anything that would map 1:1 directly, because the ESP8266 seems to use BearSSL whereas that is currently unsupported on the ESP32 (as far as I can tell). I'm also not sure if other methods of connectivity would be able to be implemented in a similar way, in which case having a "per-board" implementation seems to make a lot of sense (with perhaps pulling out what is common between them, such as certain functions like getWorkSpaces I've referenced in #7.

Take for example the current implementation of getUserData. The only difference is as follows:

// ESP8266
std::unique_ptr<BearSSL::WiFiClientSecure>client(new BearSSL::WiFiClientSecure);
client->setFingerprint(Fingerprint);
HTTPClient https;
https.begin(*client, BaseUrl + "/me");
https.addHeader("Authorization", AuthorizationKey, true);
      
//ESP32
HTTPClient https;
https.begin(BaseUrl + "/me", root_ca);
https.addHeader("Authorization", AuthorizationKey);

Describe the solution you'd like
Given that the differences between the methods are very minor as they stand, pulling out the connection method into it's own per-device function seems like a good call.

• A separate c++ file (let's call it "Connections.cpp") with #ifdefs for each board supported. For each board, a void connect(String &URL) function would be implemented.
• The Toggl class would have a global private HTTPClient object. For the 8266, the "WiFiClientSecure" unique pointer could likely also be pulled out of the connect() method (or left in, but it could no longer be a unique pointer in that case and would need to be cleared after each call). I don't know if headers need to be added for each call (for example setting the Authorization header - it'd be good if that only needed to be written once), but that could be left up to implementation.
• The example listed above would be converted to contain something similar to the following in the "Connections.cpp"

void Toggl::connect(){
    #if defined (ESP8266)
    https.begin(*client, URL);
    https.addHeader("Authorization", AuthorizationKey, true);
    # elif defined(ESP32)
    https.begin(URL", root_ca);
    https.addHeader("Authorization", AuthorizationKey);
    # endif
}

The BearSSL client for the ESP8266 code would be created as a global when the library initialized.

There would then only be a single getUserData function to maintain, and it would be easier to incorporate additional boards/connection methods. The following would work for any board/connection method when added:

...
connect();
https.addHeader("Content-Type", " application/json");
...

Memory
The above suggested implementation ends up with slightly less memory in the ROM (387680 as opposed to 387776), but I'm unsure of that HTTPClient variable (and WiFiClientSecure variable in the case of the ESP8266) sitting permanently instantiated. I'm not sure if it makes much of a difference either, I have neither the tools or knowledge to profile it, and it's beyond my knowledge if I'm honest. There's a case to be made for memory optimization in either scenario - with globals we are guaranteed we are able to make requests, with locals we have more memory for other things. I know embedded practice generally advises against globals, but sometimes they are unavoidable.

Additional comments
I think the movement to a single code base that easily allows boards and connection methods to be added is important, and that is the motivation of this discussion.
That being said, I think it is a trade off between memory management and complexity. I don't think the memory management would go too "off the wall" with a global HTTPClient, though I haven't run any in-depth profiling and I think it would be fine (though it would probably be worth asking someone more experienced and knowledgeable).
Another option entirely is to write something like a Python script to generate code for a list of boards as an alternative to the solution given above, but that feels like it would just be doing the job of the pre-processor anyway.

[JoeyStrandnes]

Finals went well and are now over so I should have somewhat more time for side projects :D

This would definitely make it easier to maintain the existing code and expand upon the project with new features or new boards. I never really expected anybody else to use the project besides myself so never though of making it modular. But I think its a great idea to bring the implementation into one file. Would probably be better to do so now before any major additions are made.

Just so I'm on the same page as you, the below image is my interpretation of the general structure. Toggl.cpp is what the Arduino user is interfacing with, the internal functions like getUserData interface through an translation layer "connection.cpp" that contains all different versions/translations between the board specific internet drivers. The translations are separated/chosen by precompiler instructions like #ifdef etc.

This seems like a good approach but I have one small concern. I think it would get cluttered if lots of different connection types are added. It isn't that big of an issue at the moment but might become one in the future if multiple new boards are added with different connection types.

It would at least be possible to change it in the future without too much hassle since it would be abstracted / hidden away.

While we are at it, the internet connection establishment could also be moved out of the initializer and add a separate function for the establishment, this function call would then be optional and allow advanced user to establish the connection as they please while still allowing novice users the convenience of just calling "connectToInternet()".

This issue is simmilar enough that it could probably be merged with issue #8?

I agree with you that this would be a small price to pay to make it a more scalable implementation.

[kcranky]

Glad to hear exams went well! 🎉

I've had some time to think about it - and I think a more elegant solution would be to have all the functions defined in Toggl.h. Toggl.cpp would have methods that are universal to the library regardless of board, and then, for each function that requires an implementation specific to the board, create a new cpp file that implements the connection() method, or whatever else might be required. Let me elaborate further with an example that has two methods - getID() and isTimerActive.

The Toggl.h file

#ifndef TOGGL_H
#define TOGGL_H
// Include relevant Arduino libraries
#include <Arduino.h>
#include <ArduinoJson.h>

//Include the Fingerprint and root_ca (kept seperate now for easier changes)
#include "TogglSSL.h"

//Include board level definitions
#include "TogglBoards.h"

class Toggl{
  public:
    Toggl(); // The initializer method - implemented on a per-board level
    
   //Get the individual account settings/data
  const uint16_t  getID();
  const bool      isTimerActive();

  private:
  const bool hasConnection(); // tests if there is a valid internet connection
  void connect(); // connects to the toggle server over SSL
  };
#endif

TogglBoards.h

This just contains all the include guards for boards supported in the library, to include the relevant board specific cpp files. It's a way of neatening up the top level header file, and it makes addition of additional boards easier.

Toggl.cpp

Toggl.cpp won't have an init method, as some boards might require power-on initialization, so pulling them out to a board level makes sense. In the getID() and isTimerActive() will make calls to hasConnection() (this is the part that is similar to point 2 in issue #8). If hasConnection() returns true, a call is made to connect() as I describe above.

Board specific cpp files.

These will be along the lines of "Toggl_ESP32.cpp" etc (as already exists), but they will only contain the code that is specifically unique to each board.

The process

Ultimately, it should end up that, to add a board, a user must create their own Toggl_board.cpp and include it in TogglBoards.h.

I hope that makes it clear? Let me know if there's anything else that might help explain it.