Closed proddy closed 3 years ago
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.
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).
"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?
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.
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:
npm -g install npm
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:
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.
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?
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.
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!
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?
Yeah I spotted that one too, will raise an issue.
On Tue, Jul 7, 2020 at 2:36 PM Proddy notifications@github.com 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 https://github.com/rjwats/esp8266-react/issues/166#issuecomment-654865196, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VBYMPI32L5MU7TTHYLR2MQGLANCNFSM4OP4ZU3Q .
there's a hardcoded default SSID in WifiSettingsController.tsx
. Can it revert to the hostname if non is set?
Looks easy enough to fix... good catch.
On Wed, Jul 8, 2020 at 10:32 AM Proddy notifications@github.com 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 https://github.com/rjwats/esp8266-react/issues/166#issuecomment-655406423, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VDZQMSYNOUAOQZLYK3R2Q4K3ANCNFSM4OP4ZU3Q .
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()
.
can close this.
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
Loading …
vsLoading…
MQTT service
WiFi Service
Security Service
System Status
My wish list & other ideas
Building
Documentation
interface/src/project
instead ofinterface/project