search

Juerd / ESP-WiFiSettings

WiFi Manager for the ESP32 Arduino environment
Other
168 stars 35 forks source link

HTTP Portal for changing non wifi settings while running #23

Closed DTTerastar closed 3 years ago

DTTerastar commented 3 years ago

This ports the portal to an optional always on http server. It still requires a reboot to apply settings changes (but so do most routers). It could still use a login page for certain use cases. But it is working for my project... It was the #1 most requested feature from users...

DTTerastar commented 3 years ago

Any interest in merging? If not just close, I won't feel bad, I understand you have principles :)

Juerd commented 3 years ago

Hi, thanks for the changes. I can't currently spend time and effort on this to verify your work, and there are some concerns that I don't know how to express properly.

Four things that come to mind when quickly scrolling through the diffs:

  1. The #ifdef is the wrong way around; the default should be to allow security. I don't mind if people explicitly choose to disable that, but this functionality should never be off by default.
  2. The http portal in the upstream library depends on the WPA of the access point for security; this PR adds a method that has neither authentication nor encryption. You say "it could still use a login page", but I would consider that the most minimal level of security required for merging.
  3. Floats are a different topic and I would like a separate branch with a separate PR for that. I'm not sure the way it's implemented in this PR even works well because if I recall correctly, the step attribute doesn't just change how the spinner buttons work, but also the allowed values, i.e. 0.015 would be disallowed if step=0.01, but 0.01 and 0.02 would be allowed. I think the step should be any to accommodate for multiple use cases.
  4. The PR has whitespace style changes. Please don't do that. Keep to the original style of whatever upstream software you're submitting changes to. You're not the only one with a strong opinion of what code should look like... :)
DTTerastar commented 3 years ago

I apparently got a few changes after the PR included since I used master to submit (I'm only used to doing stuff in the same repo at work).

  1. Yes, that disabling of the password isn't meant to be in this PR.
  2. Yes, I knew your security qualms thus why I didn't spend much time on this PR.
  3. I can submit this seperate
  4. I have my vs code format on save so it's practically impossible to stop it... Oh well
Juerd commented 3 years ago

re 4: VS Code has "save without formatting". The keyboard shortcut is different depending on the platform. https://newbedev.com/how-to-exclude-files-from-format-on-save-in-vscode

I highly recommend saving without formatting, or disabling this feature altogether, if you submit changes to projects with a different code style. There are many disagreements over code style, but there is broad consensus that it should at least be consistent within a single file.

ameerhazo commented 1 year ago

Hello @DTTerastar, is the always on HTTP configuration portal feature that you implemented working? Thank you :D

DTTerastar commented 1 year ago

Yes, it works fine. I converted it to AsyncWebServer as well: https://github.com/ESPresense/AsyncWiFiSettings

ameerhazo commented 1 year ago

Hello @DTTerastar, thank you so much :D Keep up the good work 👍

ameerhazo commented 1 year ago

Hi @DTTerastar, I've tried the code from this repo that you tried to merge aswell as this repo https://github.com/ESPresense/AsyncWiFiSettings but unfortunately both of them doesnt really work on my end. I posted an issue on the your Async repo aswell to ask about the error that I'm getting. Thanks for your work :D