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

v3 tracking issue #87

Open
1 task
nrwiersma opened this issue Aug 4, 2020 · 11 comments
Open
1 task

v3 tracking issue #87

nrwiersma opened this issue Aug 4, 2020 · 11 comments
Milestone

Comments

@nrwiersma
Copy link
Owner

This issue will be used to track to work on v3 of ConfigManager. The idea will evolve and be discussed here.

The ideas around this are from the collaboration between @nrwiersma and @cgmckeever.

General Idea

Split ConfigManager into roughly 3 parts:

  • Configuration Management
  • Wifi Management
  • HTTP interaction

When the Wifi and HTTP sections can we turned off or a custom class used to suit the needs of the user, however a "simple" option should remain leading to the use of ConfigManager as it exists today. This can be achieved using Abstract Classes (interfaces) around the Wifi and HTTP sections.

Questions

  • If HTTP is turned off, how does Wifi Management work. We cannot set the ssid and password?
@nrwiersma nrwiersma added this to the v3 milestone Aug 4, 2020
@cgmckeever
Copy link
Collaborator

I think making the three pieces separate components is the way to go. Im not 100% sure that the interface to the HTTP module is any better than having a solid interface in ConfigManager ie:

  • asJson
  • fromJson
  • updateParamater
  • storeWifiSettings(ssid, password, false);

Then anyone can hit configManager from HTTP/other (who knows right?) .. maybe Im over simplifying this

void ConfigManagerHTTP::handleSettingsGetREST() {
  JsonObject obj =   configManager.asJson();
  String body;
  serializeJson(obj, body);

  DebugPrintln(body);
  server->send(200, FPSTR(mimeJSON), body);
}

If HTTP is turned off, how does Wifi Management work. We cannot set the ssid and password

I say, first break out the components ... then think about how you turn off/on and what the edge cases are ... You talked me out of the ability to turn off the AP mode once already .. maybe until something is set it goes into AP mode for an initial setup .. There could also be a check box in the AP config ConfigManager Wifi Connection .. If it isnt checked, the station mode is skipped.

Thus, always have AP mode for setting the Wifi things, and then everything else is the optional (station mode, HTTP)

@cgmckeever
Copy link
Collaborator

cgmckeever commented Aug 4, 2020

As I got back to my code now, I remember a use case for disabling HTTP in AP mode:

I want a device that I doesnt connect to the internet, but I can connect to and serve HTTP ...

The current code path we just merged should allow this, jsut wanted to point out the use case of not needed to configure Wifi is a thing.

@nrwiersma
Copy link
Owner Author

Fair enough. It is something we should account for.

At the moment I am busy ripping things apparent to see where the interlinking is. I would like to have ConfigManager know as little as possible about the other parts, and largely let each part drive itself but at the same time not make things more complex in a simple use case. I think this is the bulk of the work in this.

I will open some branches when I have something to propose.

@cgmckeever
Copy link
Collaborator

Work of the maker - thank you

@nrwiersma
Copy link
Owner Author

So it seems the parameters are a difficult concept to handle in C++ in a generic way. Specifically if we remove Json which is desirable. My thoughts moved to a get and set method on ConfigManager, and while the set is possible, the get is problematic, as you need to know the type you think it first, and we obviously dont know that.

Currently my idea is to use a very generic parameter like:

class parameter {
  const char* name;
  type_info type;
  void* ptr;
  size_t size;
}

and offload all the functionality into ConfigManager, so it would look like:

class ConfigManager {
public:
  template <typename T>
  void add(const char* name, T* ptr, PM mode);
  template <>
  void add(const char* name, char* ptr, size_t len, PM mode); // This is a template specialisation for `add` for `char*`
  template <typename T>
  bool is<T>(const char* name);
  template <typename T>
  T as(const char* name);
  // char* as specialisation 
  template <typename T>
  void set<T>(const char* name);
  // char* set specialisation
  
  void begin();
}

it would mean we have to be specific about the types we allow, which would not be a bad thing. The only ugly thing would be creating the json document, cause for each param, we would need to try every type we support with is<T> to check and then cast.

Still looking into this more.

@cgmckeever
Copy link
Collaborator

set is possible, the get is problematic

What if get is just getting the config struct back? OR ... what if there is no get? the main code should know about the config struct(s) ? ConfigMangers real task is the setting IMO ? and also the thing that the main code can easily trip over.

@nrwiersma
Copy link
Owner Author

So there are 2 ways of thinking about this:

  1. ConfigManager drops the parameters (in effect) and they move to the HTTPServer. ConfigManager will basically becomes (logic wise) the begin and the save functions (this is actually a really tiny amount of code). Other functions may exist, but they will mostly be proxy to other pieces.
  2. We no longer force you to save entire structs, and instead make parameters in ConfigManager that allows getting and setting info. The HTTPServer will ask ConfigManager for params to build and save json data. It means we can now save and restore arb pieces of data across multiple structs, or perhaps just free floating.

What I am exploring above is option 2 to understand its feasibility. I find it an interesting option as Wifi and HTTP feed off of ConfigManager, making it a central player. In option 1 ConfigManager is playing a mostly proxy role in the system (ie the HTTPServer could function entirely without ConfigManager).

@cgmckeever
Copy link
Collaborator

ConfigManager drops the parameters (in effect) and they move to the HTTPServer. ConfigManager will basically becomes (logic wise) the begin and the save functions (this is actually a really tiny amount of code). Other functions may exist, but they will mostly be proxy to other pieces.

This does not feel right to me ...

parameters in ConfigManager that allows getting and setting info

Yes ... many yes

HTTPServer will ask ConfigManager for params to build and save json data

YES

Option 2 sounds more of what I was thinking. I'd treat ConfigManager as, well, only the manager of the configs ... I know, not creative. Setting and Storing ... The how they are used and interfaced with is them independent. If there is a Web UI to update cool, if the main project code has a bunch of logic and checks, cool. I almost feel Wifi manager is just told its SSID/Password and doesnt care where those come from or are stored. Meaning, it can function on its own with some parameters passed in.

@nrwiersma
Copy link
Owner Author

So at this point this is thwarting me a bit. Partially cause my brain is stuck in a very Go way of solving this problem, and partially due to the distractions in RL. It has occurred to me along the way that the is and as is less useful, and we should instead look at a function that gets all params, perhaps typed into some kind of variant. As @cgmckeever has pointed out, the user will never use the get style methods, and the HTTPServer will need a getAll kind of function to work.

@cgmckeever
Copy link
Collaborator

I was hoping to snoop :(

what is the is and as comment?

The settings are already available via the config struct. Maybe not the WIFI settings, but I cant think of any reason to really need that, other than to display what SSID its connected to (but thats low priority)

I struggled with how to get all the variables in a way that doesnt upset C++ string typing ;) ... getAll would be nice, but hits this wall unless its gotten as JSON or nested in some map of names that can then be used to access the config. But I think C++ ability to have variables as name (ie config.VARIABLE) is limited, or non-existent?

@nrwiersma
Copy link
Owner Author

I have committed a wip on v3 to show where my brain is on this. I am still very much playing with the struct of this to understand how it could fit together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants