Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use flashed options after a new flash #2149

Merged
merged 7 commits into from Apr 28, 2023

Conversation

pkendall64
Copy link
Collaborator

@pkendall64 pkendall64 commented Apr 1, 2023

When flashing a device, it should always use the options supplied in configurator.

When flashing we now generate a random string which is written to the option JSON object. If this is different to the one on the SPIFFS partition then we use the flashed one. This means a user can modify their settings in the web UI and the new discriminator string is written to the SPIFFS and will use the SPIFSS options until the device is flashed again.

Check the "device-id" when saving options and if it is different it will return an error telling the user they need to refresh.
Disables the "Save & Reboot" button until a config has been read from the device.

Also fixes airport settings not being done when using the binary_configurator (for cloudbuilds).

This required an upgrade to the base ESP8266 platform as there was a bug when serialising JSON to the SPIFFS filesystem! It would write the correct number of bytes, but the last few were 0xFF ?! The code change was required because we now check the device-id on options save.

Fixes #2121 and #2120

Test 1

  1. Flash a TX or RX with or without a passphrase
  2. Go to the web-UI and change the passphrase
  3. Reboot the TX or RX and check in the web-UI that the new UID for the passphrase is shown
  4. Flash the TX or RX again, with the original passphrase
  5. Verify that the web-UI shows the UID for the original passphrase

Test 2

  1. Flash 2 TX or RX modules without home network
  2. Start device 1, and connect to the web UI
  3. Leave the web-page open and power off device 1 and power on device 2
  4. Connect to the AP of device 2 making sure not to refresh the browser
  5. Try to save settings and you should get an error about a mismatched device and to refresh the browser

@pkendall64 pkendall64 marked this pull request as draft April 2, 2023 20:27
@pkendall64 pkendall64 linked an issue Apr 2, 2023 that may be closed by this pull request
@pkendall64 pkendall64 marked this pull request as ready for review April 2, 2023 22:45
# Conflicts:
#	src/html/index.html
#	src/lib/OPTIONS/options.cpp
@JyeSmith
Copy link
Member

JyeSmith commented Apr 5, 2023

Test 1 and 2 both work as expected.
image

@CapnBry
Copy link
Member

CapnBry commented Apr 27, 2023

This is an interesting solution to the confusion people are having with the webui options overrides. However, in the interest smaller code and less RAM, isn't a 32-bit random integer as functionally effective as 16 bytes of random data? Someone calculate the odds of getting the exact same 32 bits of random twice in a row AND also having edited their config.

  • In code it is just uint32_t, 4 bytes instead of 25. Also note that the variable needs a comment. firmware-differentiator of course, everyone knows what those are and what it is used for 😄
  • No need for constructing convenience String classes to compare the values
  • Simple assignments work
  • More secure (lols) because dealing with strings etc etc

Confusion

Back to the confusion of the webui overrides, what's missing from this is information. The user is presented with a box with no other information, so of course they think they're editing the actual value and not overriding it. Under "Runtime Options" on the webui how about something like:

Use the form below to override the options provided when firmware was flashed. These changes will persist across reboots, but will be reset if the firmware is updated with different values.

Additionally, the indicator of uidtype is virtually invisible on casual glance. I believe it would be beneficial if it stood out more with color. Ideally I'd like to see it right before the UID value itself, same font size, in a colored box proudly displaying what the user is working with. The standard blue for Flashed, orangey for On Loan, red for Overriden / Not set, green for Traditional bind? If the CSS is too hard to make that work, just changing the text in the label is fine, with 5px padding each side so the color stand out more.

Issues

I'm testing this and when I override the UID, the value changes after reboot but is always says "Flashed" as the UID type. Also, should the "& Reboot" be removed from the Save & Reboot button? It only saves, not reboots. The reboot comes on the followup dialog.

That's all I got for now because I gotta get on a callllll!

@pkendall64
Copy link
Collaborator Author

@CapnBry I did most of the things you mentioned above. With the exception of the styling around the UID type.
I think that would be a bit more work than I want to do at this stage.

@pkendall64
Copy link
Collaborator Author

pkendall64 commented Apr 28, 2023

Ok a few more changes:
A note block at the top of the options about the reset and bind-phrase.
Screenshot 2023-04-28 at 4 58 20 PM

Also, colors/badges and text around the UID
Screenshot 2023-04-28 at 12 47 59 PM
Screenshot 2023-04-28 at 12 49 16 PM
Screenshot 2023-04-28 at 12 49 45 PM
Screenshot 2023-04-28 at 12 50 15 PM
Screenshot 2023-04-28 at 12 52 02 PM

@CapnBry
Copy link
Member

CapnBry commented Apr 28, 2023

DUDE! This is amazing. You went above and beyond again on this implementation. Not only does your new functionality work, but the webpage now explains it, and shows it clearly. THIS!

I'm not sure about the box around the descriptive text, since none of our other descriptions have boxes so it adds a new visual style that nothing else follows. It also pushes the important part of the page further down especially with the text split.
image

Had a ball testing by flashing with bindphrase, overriding to give it to a second radio, loaning it to a third radio, then flashing with bindphrase again (loan status stayed), returned the loan and it was back to the flashed bindphrase, exactly what I expected. Also tested on a TX, flashed, override, cleared (worked), override'd again, flashed (reset to flashed UID)-- perfect.

We might consider clearing the On Loan status on firnware-differentiator mismatch as well at some point. While logically it makes sense that a loan should persist, a user might be confused that the flashing didn't wipe it back to using the flashed UID since now our paradigm is "flashing will override any override", of which a loan could be considered.

Fudging gold medals to you for this PR though, man. Not only is the functionality so much easier to work with, it is so obvious to the user as well.

@pkendall64
Copy link
Collaborator Author

Thanks @CapnBry. I removed the panel around the descriptive text as you are right it takes up too much space.

@pkendall64 pkendall64 merged commit 7fec80e into ExpressLRS:master Apr 28, 2023
38 checks passed
@pkendall64 pkendall64 deleted the reset-options-on-flash branch April 28, 2023 20:01
@hydra
Copy link
Contributor

hydra commented May 3, 2023

Great work guys, I'll give feedback as appropriate when I next update my TX firmware. This should definitely cut-down on the support requests and users being unable to bind after upgrading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants