Some feedback on the framework so far

[proddy]

Hi Rick, I finally found some time to start porting my project over to the latest esp8266-react library and was jotting down some notes along the way which I have shared below. All very trivial and somewhat noddy observations so no need to address them all. Everything is working flawlessly so thanks again for the amazing work.

---

General UI Cosmetics & wording

• mixup between "log in" wording and the "sign in" button. Also with "sign out" button and text that says "all users will be logged out". Recommend just use log.
• hitting save and you get 'changes successfully provided'. Better to use 'settings saved' and drop the word 'successfully'.
• prevent upper casing entry fields (gets mixed up when password). https://medium.com/paul-jaworski/turning-off-autocomplete-in-chrome-ee3ff8ef0908
• System stats that shows byte values is not very intuitive (e.g. Flash chip Size of 4,193,304 bytes instead of 4MB or Sketch size is 1,126,400 bytes etc). Use Kb?
• Sometimes horizontal ellipsis "..." have a spacing, e.g. Loading … vs Loading…

MQTT service

• please add QOS 0,1,2. Just as a setting.

WiFi Service

• scan networks sometimes shows doubles when in AP mode. I haven't looked into why.
• http/s would be a nice to have. Some ideas at https://maakbaas.com/esp8266-iot-framework/logs/https-requests/

Security Service

• in UI 'JWT Secret' is also a bit of a techie word if you don't understand JSON and web tokens. Perhaps rename this? Not sure to what though.

System Status

• rename Sketch size (and old Arduino word) to firmware size? I know its the official ESP term, but just sounds funny for newcomers if you're not into coding.
• rename Flash "chip size" to RAM, or Flash Memory
• File system, better to show % free instead of bytes free
• CPU Frequency vs Flash Chip Speed values. Better use the same naming, like clock speed?
• Free heap is a little misleading as it's calculated after the json package is created in mem and send to the web server.

My wish list & other ideas

• would be nice to separate out any custom specific code from interface and framework, so when a new version of the library comes out you can just dump in the new folders (like project) and npm build. It does require some tricky scripting like using gulp to move and concatenate files
• in your demo code, rename server to WebServer (e.g. AsyncWebServer webServer(80)), as there may be many servers and services.
• rename the whole project! The name esp8266-react shows the technology used but now it's not only limited to the ESP8266. Also it's become a full framework so why not give it a fancy name like they did with homie, espurna, tasmota, esphome, etc. Something like ESPWats?
• add version control to the library. Either in a library properties file on version.h/VERSION.md. Something to reference when updates are applied. Can also be shown in the system settings page.
• Please add logging, or extrapolate out to another stub service. I need to manually remove all the Serial.* lines each time and there are over 50 of them. The Serial conflicts with my own UART code.
• Please make the switch to littleFS in ESP8266. It's easy as just using a few #defines and #ifdefs. Also you can't use both LittleFS and SPIFFS on the same ESP so anyone wanted to use your framework but already using the FS (littleFS as its now default) will run into issues.
• Usually, there will be only a single project, so no need for /project/. REACT_APP_PROJECT_PATH could be the root. For example a URL like "//esp8266-react/project/demo/information" can be shortened to just "//esp8266-react/demo/information". I don't expect the ESP to host multiple projects.

Building

• replace node32s in platformio with esp32dev, which is generic to all esp32 boards
• I couldn't get npm to build correctly on OSX (npm vulnerabilities and fixing modifies the package lock file). I'll need to do some further more investigation here.

Documentation

• examples for updating the stateservice is missing return value (return StateUpdateResult::), e.g.

  esp8266React.getWiFiSettingsService()->update([&](WiFiSettings& wifiSettings) {
  wifiSettings.ssid = "MyNetworkSSID";
  wifiSettings.password = "MySuperSecretPassword";
  return StateUpdateResult::CHANGED;
}, "myapp");

• Add which getters/settings variables are available in the SettingsServices, e.g. getWiFiSettingsService, getAPSettingsService, getNTPSettingsService, getMqttSettingsService. Makes it easier than browsing through the header files.
• Location of project files is incorrect in demo. interface/src/project instead of interface/project
• Project demo code is slightly out of sync with the README and what is mentioned in the web code. E.g. the blinking LED example from earlier builds.

[rjwats]

Some good observations about consistancy/naming, very grateful, thanks. I'll work through the consistancy issues you mentioned and have a think about some of the naming, it's good to have a second set of eyes on things.

Some quick comments on a few of your points other:

• Logging: I made decent progress on this, but after originally saying I was only going to look at Serial ended up taking it too far and implemented FS and WebSocket logging too. It was a very rough experiment but the logging abstraction was OK. I'll pare it back to Serial only, as was my original intention, this will allow Serial logging to be disabled (build+run time) and log messages to observed by alternative log handlers. Will raise a PR for you to comment on later this week.

• MQTT QOS: This was omitted from MQTT settings as it was deemed a project specific setting - a global QoS setting does not make much sense really as QoS is applied on per pub/sub basis. If the user needs to control QoS level it's probably best to keep this with your projects MQTT settings (in the demo project I would have put this in LightMqttSettingsService had I needed it).

• LittleFS: Still waiting on PIO to support this properly, can't upload a LittleFS image from PIO ATM. I'm not convinced anyone uses the framework without the PROGMEM_WWW flag asserted anyway, but it is documented and that change would skupper them. If it wasn't for the refernce to this in SystemStatus, you'd be able to swap it out in main.c with no ill effect. I was a bit hesistant to reference the file system statically in SystemStatus, but it was the easiest way - yuck:

If I dropped support for the non-progmem approach this would be easier.... thoughts? I kinda think PROGMEM_WWW may as well be the only supported approach now. Fewer moving parts, easier updates... humm.

• ProjectRouting & ProjectMenu: The intention of this is to allow you to have multiple tabs dedicated to your project in the top section of the app menu. The demo project doesn't use it like this so it doesn't communicate the intent particuarly well. It might be a good idea to separate the settings+info from the control to make this clearer. Will look at this.

[rjwats]

Save messages: I agree that more contextual messages would be better. It was intentional for the RestController to have a generic message (which is why it doens't say "settings"). If a RestController is used for controlling a light, or a pump it's not really "settings" in my view.

Might be a good idea to change this to "Update successful." for now, however:

I think having specific messages per-component would be nice, so you can say "WiFi settings updated" or "Access point settings updated" for example. At this point having a wrapper to handle the REST communication becomes less useful and each controller component may as well handle it's own communication to the back end (and success/error responses). Also if doing this, it might be worth thinking about adding Axios to the framework, it really is much nicer to deal with than fetch (I'd never use fetch if bundle size wasn't such a concern) and now we have fetch and XHR (for the upload feature) Axios is more warranted than it was before (obviously bundle size is still a concern).

[rjwats]

"Add which getters/settings variables are available in the SettingsServices, e.g. getWiFiSettingsService, getAPSettingsService, getNTPSettingsService, getMqttSettingsService. Makes it easier than browsing through the header files."

What does it mean when you say "in the SettingsServices"? Are you talking about documenting the accessor functions on the ESP8266React object? Because that is already documented here. Did you mean something else?

[rjwats]

Some minor updates: https://github.com/rjwats/esp8266-react/pull/167/files

Might find more time tomorrow to look at the autocomplete + formatting stuff you mentioned.

[proddy]

thanks for considering some of the recommendations.

Logging

I do have my own implementation of logging (console & syslog) so I would probably not use esp8266-react's, so please consider making this a feature toggle too. What could be useful is registering triggers on particular services, e.g. when a WiFi STA is established, or an OTA update starts.

What does it mean when you say "in the SettingsServices"? Are you talking about documenting the accessor functions on the ESP8266React object? Because that is already documented here. Did you mean something else?

Yes, I was after which settings I could get/set from the services objects.

PROGMEM_WWW

Yup, drop the SPIFFS file upload. All in memory!

While I continue on my project here are some more findings:

• Nice to have: A way to register triggers/handlers on a particular service event, not just a state change. For example when a WiFI STA connection is made, or an OTA update starts/stops, or a Restart service is called. There are always things I need to do before and after such actions.
• The AP passphrase must be at least 8 characters long or the ESP32 will assert an error. Perhaps mention this somewhere in the factory.ini file.
• In the doc under 'theming the app' with the example using dark mode, the theme.palette.info.main value of blueGrey[900] will not show as it matches the background. You'll get solid grey avatar icons.
• btw, I fixed the build on OSX with upgrading npm npm -g install npm
• I had some situations where the AP name reverted back to the ESP device ID. I'll try and reproduce.
• Screen refresh takes quite a bit of time on slow networks with an ESP8266 while it loads the icon in the menu header (this PR). My PNG is only 256x256.

[rjwats]

The arduino core gives you AP / WiFi events:

https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/generic-class.html
https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/examples/WiFiClientEvents/WiFiClientEvents.ino

Does get you what you need already for the connectivity events? I'm not sure I'd bother wrapping already wrapped functionality here (assuming that's what you're suggesting?)

Will think about callbacks for OTA/Restart events. What's the use-case in particular?

On the TLS front, this is far too memory hungry for the ESP8266, you really need ~20k free just for the handshake and under ESP32 the async TCP lib doesn't support it yet (there's an open PR, but master hasn't had an update there for 10 months). Seriously considering doing a port of the framework using IDF, lots of active development going on there.

Yes, I was after which settings I could get/set from the services objects.

Oh OK, I was confused by the references to getWiFiSettingService etc... so you were saying: "Document the pubic APIs in the frameworks settings service classes". Might be one for a differnt .md file or some external documentation perhaps.

In the doc under 'theming the app' with the example using dark mode, the theme.palette.info.main value of blueGrey[900] will not show as it matches the background. You'll get solid grey avatar icons.

blueGrey[500] looks a little better.

The AP passphrase must be at least 8 characters long or the ESP32 will assert an error. Perhaps mention this somewhere in the factory.ini file.

Thx - handn't seen this before. Will add a comment and a min length validator on the AP Settings form too - no harm in enforcing a min password length in the UI for all devices.

Screen refresh takes quite a bit of time on slow networks with an ESP8266 while it loads the icon in the menu header (this PR). My PNG is only 256x256.

Not experienced this myself, is that with master? Here are my typical loading times are as follows:

ESP8266:

ESP32:

[proddy]

The arduino core gives you AP / WiFi events
Will think about callbacks for OTA/Restart events. What's the use-case in particular?

Yes, this is what I'm using now. The three particular use cases I had were a) so that all service events could be logged with my own logger b) one of my projects needs to suspend other background services such as the UART during an OTA upload or it will fail and c) I need to flush stuff before a restart. I also noticed that most times an OTA upload via pio or espota.py fails on an ESP32. It may be an idea to suspend all non-core services during an upload, like mqtt. Then again it could just be my code.

On the TLS front, this is far too memory hungry for the ESP8266, you really need ~20k free just for the handshake and under ESP32 the async TCP lib doesn't support it yet (there's an open PR, but master hasn't had an update there for 10 months). Seriously considering doing a port of the framework using IDF, lots of active development going on there.

I agree, it wouldn't fit in the ESP8266. I just wanted to share the library for reference. I'd gladly help port over to IDF

Screen refresh takes quite a bit of time on slow networks with an ESP8266 while it loads the icon in the menu header (this PR). My PNG is only 256x256.
Not experienced this myself, is that with master? Here are my typical loading times are as follows:

I'll run some tests again and compare the timings. It is physically visible in my project. Sometimes it even shows as a broken image and can take 1-2 seconds to load. Again, its most likely something in my code so I'll to try and reproduce on a clean master build.

[rjwats]

Sure, I can understand wanting to suspend processes during an OTA update. I'll raise an issue for that.

I have experienced WiFi/AP connection & transfer speed issues in the past when the main loop blocks for too long. Probably as a result of the output buffers being starved if the async tcp code isn't called frequenctly enough, maybe a strategicly placed yield() will resolve that issue?

[proddy]

I tried with a yield() and also with a delay(1) in the main loop and OTA continues to fails most of the time. Only on the ESP32 though, probably because the image is quite large (I never worked out why it uses 2x progmem compared to the ESP8266). I'll experiment with disabling some of the services and see if it helps.

[rjwats]

I figured ESP32 firmwares are "just larger" putting it down to the core. Even compiling this example gives wildly differnt firmware sizes:

https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/examples/CaptivePortal/CaptivePortal.ino  

ESP8266:

ESP32

If you figure anything out, I'd be happy to hear it!

[proddy]

yeah for some reason the progmem is just double that on the ESP32. There are other reports on the web about this but I couldn't find a reason for it. Probably an Arduino core quirk or something with the partition chunks.

Another minor observation today: on ESP32, after erasing flash, uploadig firmware, joining AP (no SSID set yet), in Wifi Status page the status shows 'Unknown'. Nicer if it would be WIFI_STATUS_NO_SSID_AVAIL or WIFI_STATUS_DISCONNECTED?

[rjwats]

Yeah I spotted that one too, will raise an issue.

…

On Tue, Jul 7, 2020 at 2:36 PM Proddy ***@***.***> wrote: yeah for some reason the progmem is just double that on the ESP32. There are other reports on the web about this but I couldn't find a reason for it. Probably an Arduino core quirk or something with the partition chunks. Another minor observation today: on ESP32, after erasing flash, uploadig firmware, joining AP (no SSID set yet), in Wifi Status page the status shows 'Unknown'. Nicer if it would be WIFI_STATUS_NO_SSID_AVAIL or WIFI_STATUS_DISCONNECTED? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKE4VBYMPI32L5MU7TTHYLR2MQGLANCNFSM4OP4ZU3Q> .

[proddy]

there's a hardcoded default SSID in WifiSettingsController.tsx. Can it revert to the hostname if non is set?

[rjwats]

Looks easy enough to fix... good catch.

…

On Wed, Jul 8, 2020 at 10:32 AM Proddy ***@***.***> wrote: there's a hardcoded default SSID in WifiSettingsController.tsx. Can it revert to the hostname if non is set? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKE4VDZQMSYNOUAOQZLYK3R2Q4K3ANCNFSM4OP4ZU3Q> .

[rjwats]

Created: #172

[proddy]

Will think about callbacks for OTA/Restart events. What's the use-case in particular?

Yes, this is what I'm using now. The three particular use cases I had were a) so that all service events could be logged with my own logger b) one of my projects needs to suspend other background services such as the UART during an OTA upload or it will fail and c) I need to flush stuff before a restart. I also noticed that most times an OTA upload via pio or espota.py fails on an ESP32. It may be an idea to suspend all non-core services during an upload, like mqtt. Then again it could just be my code.

My ESP32 image is 1.5MB (twice the size of the ESP8266 thanks to the weird Flash mem issue), and OTA fails. I modified my code to disable all other non-core functions during an upload but OTA upload still fails after 40%. Turns out that the Task Watchdog kills the OTA process because it's keeping the other processes from running. A simple fix is to add a delay(1) to the OTA update handler _arduinoOTA->onProgress().

[proddy]

can close this.