s00500 / ESPUI

A simple web user interface library for ESP32 and ESP8266
https://valencia.lbsfilm.at/midterm-presentation/
Other
923 stars 174 forks source link

Make initial UI update reliable (and 4 small fixes) #148

Closed iangray001 closed 2 years ago

iangray001 commented 2 years ago

This carries on the work of the chunkupdate branch in which the initial UI was moved from one massive websocket message into a stream of them. Whilst this did fix a number of issues, it was not a total fix. This, I believe, is.

Recall that the need for chunkupdate was that above a certain size of message, the websocket layer simply throws away the message. Well that also occurs if you send enough messages in quick succession. We can verify this by printing the value of ws->availableForWriteAll() before each message is sent. Eventually (in my tests after around 10 or so messages, so after ~100 controls), the return value of availableForWriteAll becomes false, indicating that the internal buffers are full and so any further messages sent to the layer are simply discarded. In the previous implementation, the client (browser) has no way of knowing that UI elements were lost. We cannot slow transmission (like with a delay or yield) to allow times for the buffers to clear because anything from the websocket layer is not in the loop context and so these functions will fail.

Fundamentally the "proper" way to handle this is to rebuild the UI protocol so that the client Javascript actually acknowledges messages and so the ESP and client coordinate to manage the transfer. That is what this PR does.

Now, the initial UI message is sent, and client.js processes it and then if more UI elements are to come, sends a request message back to the ESP, which then replies with the next group of UI elements. This continues until the entire UI is sent, and should allow for no limit to messages (aside from total system RAM). More details of the protocol are in the commit messages.

Also, fixes #121, fixes a bug where ControlColor::Dark was not getting correctly applied to panels, fixes programmatic update of sliders, and allows buttons to be programmatically updated.

thomastech commented 2 years ago

@iangray001

Hello Ian, thanks for another great contribution.

I performed a basic validation. Here's what I observed:

1. Further improve the reliability of UI elements: Not validated, need advice on how to test this.

Fix issue with switchers getting out of sync with the DOM if updated: Working.

2. Fix bug with updating slider values programatically: My sliders are still reverting back to the default position. Seem unresolved.

3. Allow runtime updates to button controls: I tried this in a button callback: ESPUI.updateControlValue(rdsRstID, "RESET!"); and the rdsRstID button did not change. Please advise on how to test this.

4. Fix a bug where the Dark colour wasn't being correctly applied: Untested.

5. Add a null check in updateControl: Working.

iangray001 commented 2 years ago
  1. It seems fine for me, are you sure? I'm doing this, which seems to work:
ESPUI.updateSlider(mainSlider, random(10, 400));
  1. Yes sorry that was vague on my part. In the original library you can update a control's value (so the value sent to the callback from the UI) but not the title. In the context of me fixing things, this now works for Buttons, and also therefore allows a button's CSS styles to be updated. Lets me do fun things like: Screenshot 2022-01-14 at 20 30 56

Live updating of titles is something else.

thomastech commented 2 years ago

Hi Ian,

  1. The slider issue is a real head scratcher. A summary of the problem is here: https://github.com/s00500/ESPUI/pull/147#issuecomment-1010281038

But to summarize, the problem does not occur because of a programmable change to the slider's value. Instead, I'm changing the Value on various Label Controls. For some reason updating a Label Control Value causes the Slider's position (if it has been moved) to revert back to its default position.

There's two Sliders in my App. If I change a Slider it reverts back to its default position as soon as a new Label Control Value is sent. For example, I have some Labels that get changed every few seconds courtesy of some timers. If I disable these Label updates the Slider acts normal. TL;DR: Changing a Label Value causes all Sliders to move. Labels are working fine, Sliders bad.

By the way, your example code sends "long" data Values to the Slider control. I didn't know that data type is accepted, I assumed the Values needed to be Strings. Haven't tried either way yet because first I need to get the Slider to stop its madness.

  1. OK, this update in intended for the inline css. As for Buttons, I understand updating its label (panel title text) is not supported. But how about updating its Value (text inside the button)? This text change magic can be done with a Label Control. Any chance I can do this Text Input Controls too? That's what my example (in previous post) is trying to do.
iangray001 commented 2 years ago

updateSlider takes ints so C will truncate the long down to int. You are right that internally ESPUI uses an unholy mix of char * and String for everything, but the updateX functions take more expected data types and convert them to Strings if required. See here.

I have finally managed to recreate the bug you've found. None of my test GUIs exposed it, but the short version is that because the sliders are from some 3rd party library and because ESPUI also uses Zepto, it is possible for Zepto's view of the DOM and the Slider libraries to get out of sync. I have added code to resynchronise the values and it fixes what I exposed. There may still be other sources of this, or this might fix it completely.

Can you re-pull this branch (https://github.com/iangray001/ESPUI/tree/chunkupdates2) and see if it helps?

thomastech commented 2 years ago

You are right that internally ESPUI uses an unholy mix of char * and String for everything, but the updateX functions take more expected data types and convert them to Strings if required.

Good to know. Too bad the addControl Slider functions don't support int Value args. That would eliminate the String conversions before the call.

I have finally managed to recreate the bug

I'll test your new changes later today.

New Bug: Yesterday I performed more validation and found another issue with this PR (did not occur in #147 or earlier). Hopefully you can recreate it.

Setup: Medium size project using 60 controls with menu Tabs, page separators, grouped controls, inline css, and the like. Win10-Pro/64, Latest Firefox.

Test: If two browsers access the device things get weird. To see the bug each browser must move about different ESPUI Tabs and perform browser page refreshes. Eventually the Controls begin to go wild. Entire groups of Separated Controls will duplicate, some Controls will experience huge vertical sizes, and some controls stop working. Both browsers remain in sync, i.e. they will see the same demonic layouts as they mutate in realtime.

thomastech commented 2 years ago

Update: I tried the new Slider fix. Firefox and Chrome are functional. Two issues: (1) As I move the Slider there are random times where I see flashes of the knob appearing at the previous position. My best guess is that the flashes coincide with the Label Control updates going on in the background. (2) When the mouse moves the Slider and continues to hold it at the new position, within a couple seconds the Slider snaps back to the previous position.

Unfortunately Microsoft Edge still has the Slider problem.

BTW, I was looking at the library's online docs and it says a little "value" bubble should appear above the slider knob when it is being moved. I don't see the bubble (tried FF, ME, Chrome). I didn't see it on the previous releases either, so not related to this fix. Maybe an outdated feature? ui_slider

iangray001 commented 2 years ago

Yes updates will snap the slider from under the mouse pointer. There might be a way to prevent this but it's lower priority.

Microsoft Edge...

Rofl, always trust Microsoft to break something... I'll see if I can find a Windows machine for testing.

Bubbles...

Yeah the docs are quite out of date and need a serious overhaul. As far as I can tell they have never done that. Once I've dealt with the main outstanding bugs I'll look at that.

thomastech commented 2 years ago

The main issue with the Slider is the odd behavior when you move it then hold for a moment. I had a workaround idea where I would suspend my Label updates during Slider use. But its callback remains dormant until after movement & release.

New Bug: You may have missed my earlier post: https://github.com/s00500/ESPUI/pull/148#issuecomment-1013733232

Edit: Just want to thank you again for the new features and improvements. Rather than report problems I wish I could contribute code fixes to the ESPUI library. Maybe one day I'll better understand the magic web stuff and be able to do that.

thomastech commented 2 years ago

Hello Ian (@iangray001)

Besides the repeating controls issue (triggered by two or more browsers), I ran into another chin-scratcher today.

The issue is that inline styles aren't applied on initial web access or anytime a refresh is made. But the inline styles are working if they are sent AFTER the web page is in view.

Reverting back to one of your previous ESPUI branches (timestamped Jan 13, 1:29PM) has restored inline styles. Hopefully you can recreate this in your test setup.

BTW, the Jan 13 version also has the repeating control problem. I think it was a works-in-progress for PR #148, but I am not sure.

iangray001 commented 2 years ago

inline styles aren't applied on initial web access

Yes I believe that I noticed this one myself and corrected it. It should be fixed.

I haven't looked at the duplicate controls yet. It's on the list.

I've added updating button labels in my own fork, I'll merge it in at some point soon.

I have got my ESP32 test rig up now and have been trying to replicate the "Too Many Messages" problem that many have spoken about but to no avail. When does it happen to you?

thomastech commented 2 years ago

I'll try out the new merge and report back. Hopefully tomorrow or sooner.

I readily observed the "Too Many Messages" before I fixed the slow ESP32 ping times. The warning would occur after approx three or more quick presses on a Button control. So let's say a ~3Hz rate.

Now that my ESP32 has fast Ping times (~5mS) it takes a lot more Button action to do it. I'd say > 5 rapid presses on a Button will invite the message.

In my testing the Button was sending only one Control update, so top layer message load was light. There must be a lot going on someplace deeper in the code that fills up the buffer. What is odd is that the ESP32 is suppose to have a 32 message buffer. Seems very strange that it is ever full, at least with what I'm throwing at it.

From what I read, filling the buffer means that some messages will be lost (no room to store them). But it appears to me that none are lost. That is to say, after I'm done mashing the Button I can see the UI actions continue for a couple seconds as the queue is emptied. And all presses are accounted for.

thomastech commented 2 years ago

I tested PR #149 and inline styles (re: issue mentioned above on Jan-17-2022) are working correctly. Thanks!