Add some additional Features #33
Conversation
…to add custom AP name
There was a problem hiding this comment.
Overall looks great!
A couple of comments:
- Please use PlatformIO builtin lib_deps mechanism for adding dependencies on ArduinoJSON, here's an example: https://techoverflow.net/2021/01/29/esp32-minimal-json-webserver-example-for-platformio-espasyncwebserver/
- Please initialize wifi power to a sane default in settings. A good place to put this is in settings.cpp::nukeSetting(). Do some sanity checking of the values passed from the web form.
src/network.cpp
Outdated
| @@ -38,6 +41,35 @@ inline String uptimeString() { | |||
| return ret; | |||
| } | |||
|
|
|||
| DynamicJsonDocument parseOwieStatusData() { | |||
There was a problem hiding this comment.
Please name this function "generateOwieStatusJson()" or something like that as it doesn't do any parsing.
src/network.cpp
Outdated
| String(relay->getRegeneratedChargeMah()) + " mAh"; | ||
| status["UPTIME"] = uptimeString(); | ||
| status["CELL_VOLTAGE_TABLE"] = out; | ||
| return status; |
There was a problem hiding this comment.
A suggestion: In the spirit of better encapsulation, perform the serialization to string in this function instead of the call site as the caller doesn't require the JSON object.
src/network.cpp
Outdated
| // add aditional sanity checks, so that the power range is between 9 and | ||
| // 12 only! | ||
| if (wifiPower == nullptr || wifiPower->value().toInt() < 9 || wifiPower->value().toInt() > 12) { | ||
| request->send(400, "text/html", "Wifi Power range MUST be between 9 and 12."); |
There was a problem hiding this comment.
According to https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/generic-class.html#setoutputpower this can be anywhere from 0 and 20.5 with the docs suggesting that 17.5 is a good sweet spot. Maybe limit this from say 4 to 17?
There was a problem hiding this comment.
My first intention on this was only to raise the value only little bit, and to not allow a value lower as your hard coded "default". But I would not go as low as 4, since the top shilding of the Batterybox may prevent the Signal from spreading on a value of 4. On a XR the "default" 9 from you was just a little to less...
Therefore I suggest to limit this from say min. 8 to 17 with a default of 9?
src/settings.cpp
Outdated
| @@ -70,5 +70,7 @@ void disableFlashPageRotation() { getEeprom().rotate(false); } | |||
|
|
|||
| void nukeSettings() { | |||
| *Settings = DEFAULT_SETTINGS; | |||
| // set default value of the wifi power output | |||
| Settings->wifi_power = 9; | |||
There was a problem hiding this comment.
One thing that came to my mind is that we need to sanity test this setting on load. When users install this new version and the old protocol buffer gets read, wifi_power will be initialized to 0. You have to handle that case and clamp the setting value to the same limits as the ones you have in the web form check
There was a problem hiding this comment.
Since im very new to Device programming and relatively rusty on C++ can you give me some guidance, where I can / should check the loaded Settings value for sanity (and probably reset it to a sane value)?
The nukeSettings is only called if there are NO values in the eeprom or if somethig went wrong on decoding the eeprom value, right? So if old settings are loaded this won´t be called?
My intention would be putting the check and setting to a sane default value into the loadSettings() function, right before it returns after decoding into the Settings object. Right?
There was a problem hiding this comment.
Yeah for sure. Call the function sanitizeSettings() where you check wifi_power and clamp its values if necessary. Call it from both nukeSettings() instead of setting wifi_power directly and also from loadSettings() on line 43 in this file, right after "DPRINTF("Read and decoded settings, size = %d bytes.", len);"
I have added some additional features, I think are usefull.
This includes:
(Currently this is updated ever 1 second via the Browser calling the endpoint actively.
I´m planning to make this adjustable via the settings page, but this will probably take a little bit)
For this I intruduced the ArduinoJson.h file, making dynamic update way easier.
I also have updated the version number while developing, so this should be reversed or overriden!!
I still suggest that you add some kind of minor versioning when makuing updates on your code, since this helps a LOT to find out if the chip needs an update or not.