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

Allow ability to turn off built-in webserver #86

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

cgmckeever
Copy link
Collaborator

@cgmckeever cgmckeever commented Aug 2, 2020

Tested

Multi-configurations on both ESP8266 and ESP32

Disclaimer

I'd release this as an Release Candidate before promotion ;) .. I did get under the hood a bit

Objective

To provide optional settings to disable ConfigManagers built-in Wifi and HTTP server.

  • Not be too destructive of the core library

Reason

I want to use another package that has a conflicting server, and didn't necessarily want to have the device running multiple all the time.

End Result

To additional methods provided that will one-time-only (ie no toggling out of it, as it is usually done during setup) disable either Wifi or HTTP, but allow either to also continue to work if they are not both turned off (ie, HTTP will still serve if WIFI has been disabled)

configManager.disableWIFI();
configManager.disableHTTP();

Of course, this is Buyer Beware, as you can't really have a useful HTTP thing without WIFI, so you would need to have custom WIFI or HTTP methods of your own.

Abstracted out methods used by ConfigManagers HTTP

For the HTTP calls (ie /scan), I unrolled some the underlying code so they are more readily exposed to other processes

Example Code

Example code provided which should easily let one test the code out, as well as try the different disabling configurations.

DebugPrintln(WiFi.localIP());
this->mode = basic;

if (!this->disabledWIFI) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the bulk of the change

@cgmckeever
Copy link
Collaborator Author

Moving this to a WIP -- persistence of WIFI configs seems to not be a thing for some reason

@cgmckeever cgmckeever marked this pull request as draft August 2, 2020 21:39
@cgmckeever cgmckeever marked this pull request as ready for review August 3, 2020 02:47
@cgmckeever
Copy link
Collaborator Author

Moving this to a WIP -- persistence of WIFI configs seems to not be a thing for some reason

OK, this was dumb, ConfigManager was working, tested against the other examples. Turns out, I didnt understand how setting a variable and writing it would blow up the flash memory

Dont do this

config.VARIABLE = 12345;
configManager.save();

💥 there goes your flash memory ...

With that, I finally added a way to update the settings from code versus the browser .. it may not be pretty, but it appears to work

configManager.updateParameter("someNumber", 47);
configManager.updateParameter("deviceName", "First Load");
  template <typename T>
  void updateParameter(const char* name, T value) {
    std::list<BaseParameter*>::iterator it;
    for (it = parameters.begin(); it != parameters.end(); ++it) {
      if ((*it)->getName() == name) {
        DynamicJsonDocument doc(1024);
        JsonObject json = doc.createNestedObject();
        json[name] = value;
        (*it)->fromJson(&json);
        break;
      }
    }

    writeConfig();
  }

I once again tried to turn the objects parameter store into a map, only to realize maps arent available via Arduino ...

DebugPrintln(WiFi.localIP());
DebugPrintln(F("\""));

EEPROM.get(MAGIC_LENGTH + SSID_LENGTH, password);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attempt to grab ssid/password after validate magic

JsonObject json = doc.createNestedObject();
json[name] = value;
(*it)->fromJson(&json);
break;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly hacky (?) ... couldnt get the Type definition into BaseParameter for the different types, so went the good old way of changing it to JSON and using the already set up update methods!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disregard everything I wrote ... It appears I chased my tail, as there is some weird issue if you update a setting continually in the loop I thought it was the way I was doing updates/saves ... Ill not squash this commit in the event you like this updateParameter hook ... it does abstract away from of how to handle the different types .. maybe ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one benefit to this method is that it does the type casting for you .. and you cant try to jam a random as an int ... no bueno

template <typename T>
  void updateParameter(const char* name, T value) {
    std::list<BaseParameter*>::iterator it;
    for (it = parameters.begin(); it != parameters.end(); ++it) {
      if ((*it)->getName() == name) {
        DynamicJsonDocument doc(1024);
        JsonObject json = doc.createNestedObject();
        json[name] = value;
        (*it)->fromJson(&json);
        break;

@cgmckeever
Copy link
Collaborator Author

Dont do this

config.VARIABLE = 12345;
configManager.save();

💥 there goes your flash memory ...

This appears to be some odd issue when updating a setting continually in loop which I was just toying around with, versus any real use case .

@cgmckeever
Copy link
Collaborator Author

cgmckeever commented Aug 3, 2020

Dont do this

config.VARIABLE = 12345;
configManager.save();

💥 there goes your flash memory ...

This appears to be some odd issue when updating a setting continually in loop which I was just toying around with, versus any real use case .

This is resolved ... setting an int to a random kills things .. including my Sunday afternoon

void loop() {
  configManager.loop();
  config.led = random(300);
  configManager.save();
}

@nrwiersma
Copy link
Owner

Hey. Wow, that is a change in deed. Some starting questions around the idea:

  • Should you not want us to handle WiFi, is the AP even a thing anymore. ie. you disable Wifi, we will never do AP mode? Or if you disable it, we assume AP mode and just leave out the connection?
  • I kinda disagree with the idea of "disabling HTTP" cause you in effect disable ConfigManager, right? (naming issue)

Arguably I can see how disabling the WiFi might be interesting for someone, but from a quick naive PoV I think disbaleWifi + disableHTTP is like not having ConfigManager at all (Perhaps I have the wrong idea, or there is a naming issue). We also need to think carefully about starting mode when WiFi is disabled. It is not clear to me what happens here.

I do however take your point that you want to use a different web server to what is built in. Perhaps another way to look at disableHTTP is to stop using the WebServer and instead use an interface. We can by-default provide 1 for ESP8266 and ESP32, but should you want to provide your own, this would be allowed. This would allow for what you want, and remove the disabledHTTP idea entirely. Thoughts?

@cgmckeever
Copy link
Collaborator Author

Happy Monday -- my coffee is brewing, so please bear with me ;)

I kinda disagree with the idea of "disabling HTTP" cause you in effect disable ConfigManager, right? (naming issue)

I think the elegance is how you manage configuration in the SPIFFS is really the power of ConfigManager. The web interface is awesome and useful, but to me the memory management of the configs is essential and truly the work of art (IMHO)

Should you not want us to handle WiFi, is the AP even a thing anymore. ie. you disable Wifi, we will never do AP mode? Or if you disable it, we assume AP mode and just leave out the connection?

I could easily be talked out of even including disableWIFI. I already had Wifi working in the other project, so it was simpler to be able to toggle it in ConfigManager. The issue with the assumption that AP mode should spin up is that you dont know the reason its being disabled, so it could be conflicting with an already established connection (my case). There could be a 3-way switch, NO-WIFI, AP-MODE, ALL --- that should not be hard, but does add to the complexity of having a lot fo switches in software.

I think disbaleWifi + disableHTTP is like not having ConfigManager at all

This is an odd combination, but again, the power and beauty of ConfigManager in my perspective is the actual management of memory based persistence of configuration. There are a bunch of other projects that already have UI, but could benefit just from the ease of use of the configuration store. I could be wrong ...

Someone would really need/want to understand why they turned both those off.

We also need to think carefully about starting mode when WiFi is disabled

I think if Wifi is disabled, this is just like any of project, its an island. But I feel since both WIFI and HTTP disables are optional, you really need a reason to use them.

is to stop using the WebServer and instead use an interface

Intrigued and curious, but not following. I would argue that outside of Strings changing the configs outside of the HTTP is simple, minus the consequence of the silent eradication of all config memory when you type-mismatch (my struggle of a random into an int). Ive tried a couple times to expose the Parameters directly from ConfigManager:: but outside of the json hack, Ive never come up with a good method

@nrwiersma
Copy link
Owner

So I never considered the idea of just using it for storing config, like it never even crossed my mind. So my first set of questions have been answered. I finally understand what you are aiming at.

Intrigued and curious, but not following.

So my point here is to allow any web server adapter that anyone can think of, as long as it implements the methods we want, by way of an interface. Where we are starting and binding to a webserver, we instead make use of an interface that exposes the webserver to ConfigManager. We would then have a class for ESP8266 and anther for ESP32. If someone else prefers to provide their own they can, and setting it to nil will not use the webserver at all. (This was my first thought, but understanding more about the use case, perhaps it is not what we need).

silent eradication of all config memory when you type-mismatch

Well that is not great. Not sure what to do about it though. Must say, i have never tried this before.

@cgmckeever
Copy link
Collaborator Author

silent eradication of all config memory when you type-mismatch

Well that is not great. Not sure what to do about it though. Must say, i have never tried this before.

I will answer the rest in a separate thread, but if you take the saved_config demo and just add

config.someIntParam = random(300);
configManager.save();

On next reboot you have cleared the memory ... Im not exactly sure why, but I think its cause you are jamming too much in? or the offsets are wrong?

I tried for a cleaner parameter update. I tried having addParameter return its parameter object, and then a developed could track these (if they desired) and do something like this:


ConfigStringParameter deviceConfig = addParameter("deviceName", "Thing 1");
 
...

deviceConfig.update("Thing 2");
configManager.save();

and in this case, those classes have strict typing of each member ... I just could not get the addParameter to behave with its return ...

@cgmckeever
Copy link
Collaborator Author

Intrigued and curious, but not following.

So my point here is to allow any web server adapter that anyone can think of, as long as it implements the methods we want, by way of an interface. Where we are starting and binding to a webserver, we instead make use of an interface that exposes the webserver to ConfigManager. We would then have a class for ESP8266 and anther for ESP32. If someone else prefers to provide their own they can, and setting it to nil will not use the webserver at all. (This was my first thought, but understanding more about the use case, perhaps it is not what we need).

Thats where I thought you were going, my brain is slow on seeing how they are all tied together ... I think its something like:

  • You pass in the webserver interface which probably has a method for request/response and some possible mime/codes etc
  • ConfigManager uses that instead to serve its pages

I like this for AP mode, I think it gets convoluted (overkill) for API mode .. reason being, I see API mode being a more custom defined page and not just a settings editor. I could not be thinking completely through this.

To an earlier point, if you are going to use ConfigManager for Wifi Handling, why fold in so many layers of the interface to expose a 2 field configuration? I'd just use AP mode, its credentialAPI and call it a cay, simple and efficient ..

With that, maybe its better to just have the developer stop the onboard server when desired? .. of course I think of this now ... so the flow in our co-mingled use case would be:

  • Start in AP mode to bootstrap the wifi config
  • Reboot, project code see's its out of AP mode and configManager.stopServer()
  • Continual on with their world ..

In this world, they get the implicit ability to not use HTTP without disabling it upfront. I dont know if that is any better from a memory/performance perspective over the current paradigm BUT its less code churn/toggles on ConfigManager with pretty much the same result ... GAWD that would have been a simpler PR ....

However .. that leads me to ask if your interface idea is the best of all -- but IMO, for the use case, the potential ability to achieve it with a stop(), and not fully understanding if anyone would just want to serve vanilla pages through it for the config -- I feel that is a lot of churn for limited result ;) .. that may be me being both lazy and not-fully-tracking .....

@nrwiersma
Copy link
Owner

Perhaps you are right, the only benefit i see to having things split out and included via interface is compile size. If you dont use it, you dont pay the cost in the binary. Thinking along these lines, the natural split would be WifiManager, HTTPManager and ConfigManager which binds them all. All WiFi related code in WiFiManger, the 2 provided HTTPManagers and ConfigManager which by default has the "correct" instances. If you turn something off, the code is tree shaken out. Thoughts?

@cgmckeever
Copy link
Collaborator Author

I started down that path of peeling our the HTTP stuff .. and it got deep quick. Im leaning on the side that this would be an excellent game of code-golf, but the overall benefits would be more for personal puzzle-solving than anything? ...

What are your thoughts on my over-simplified 'just stop the webserver when you dont need it' ... I could easily re-shuffle the PR for that, remove the disableWifi .. and see what that looks like. I think there would be a significant less amount of churn for this PR will a very

It does 98% suck that the server that is embedded in ConfigManager has naming conflicts, I was able to work around that via a namespace, but Ive also seen that fail miserably trying to namespace libraries that other things aggressively assume are named one thing ... and for that I love the segmented module approach ..

BUT for v1 of this I can quickly rework to stop the server, some code clean that is in this PR .. and see where it lands?

As always, thank you for talking me off the cliff .. good stewardship of code protection with ConfigManger ;)

@nrwiersma
Copy link
Owner

Thanks, it is always great to have the discussion 😄

I think you are ultimately correct. While the reshuffle is perhaps a better direction, it is better in a major version. So lets do stop and go from there. 👍

@cgmckeever cgmckeever force-pushed the cgmckeever/http branch 3 times, most recently from 2a8b49d to 89299a2 Compare August 3, 2020 16:05
@cgmckeever
Copy link
Collaborator Author

@nrwiersma updated per discussion, much thinner...

compiles -- UNTESTED ... open for feedback

@cgmckeever cgmckeever changed the title Allow Disable of ConfigManager builtin Wifi and/or HTTP Allow ability to turn off built-in webserver Aug 3, 2020
src/ConfigManager.h Show resolved Hide resolved
src/ConfigManager.h Show resolved Hide resolved
@@ -73,9 +72,13 @@ class ConfigParameter : public BaseParameter {

ParameterMode getMode() { return this->mode; }

void update(T value) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍 It gives me an idea for the future to move JSON out of ConfigManager.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be awesome, a pure update hook of the data

src/ConfigManager.cpp Show resolved Hide resolved
src/ConfigManager.cpp Outdated Show resolved Hide resolved
@nrwiersma
Copy link
Owner

The code is looking pretty good. Nice work 👍

WIP

library wrangling

whitespace

no working CM http

no working CM http

almost all working .. WTF is up with the scan packer

SCANNER

return minJSON

final touches

quick clean

flash memory working WTF

add ability to update settings

add ability to update settings

disregard everything I said about updating

start/stop webserver

start/stop webserver

start/stop webserver

start/stop webserver

start/stop webserver

cleanup
@cgmckeever cgmckeever marked this pull request as ready for review August 3, 2020 23:30
@cgmckeever
Copy link
Collaborator Author

The code is looking pretty good. Nice work 👍

@nrwiersma tested on ESP8266/32 everything works as expected. Shutting down of the HTTP is nicer than expected, and less code mucking .. good call ...

Added to the README.md and some minor sweep up. Let me know if there are other areas of concern/change

@nrwiersma
Copy link
Owner

Thanks very much. Nice work 👍

@nrwiersma nrwiersma merged commit 7cfbcad into nrwiersma:master Aug 4, 2020
@cgmckeever
Copy link
Collaborator Author

@nrwiersma MUCH THANKS!

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.

None yet

2 participants