Add settings and OTA lock state to info #4279

willmmiles · 2024-11-14T03:25:16Z

As requested by @Moustachauve on Discord: add the settings and OTA lock states to the info struct, so UIs can direct users to an appropriate workflow for updating their devices.

I'm not 100% confident about the usage of interfaceUpdateCallMode in these contexts to induce websocket notifications. The notification subsystem currently seems a bit tangled to me. If this stands out as obviously wrong, please let me know.

Use "settingsLocked" and "otaLocked" to clearly convey the meaning and sense.

blazoncek · 2024-11-15T06:35:01Z

In fact it is convoluted. It took me 2 years to decipher it. 😄
If you want notifications to be broadcast, use: stateUpdated = true;

blazoncek

I'm ok by adding information that update may be locked (either via PIN or OTA password) but the same may be achieved by the app by polling/requesting the update page (it will produce PIN entry or 503 IIRC). So I'm not really sure it is needed.

The more important thing is if OTA is not compiled. That information is already in info object hidden in options/capabilities bitfield.

I also have considerations about JSON buffer size as it may get too small on ESP8266 since we are adding new items to info and state objects all the time.
And when @DedeHai 's particle system gets merged it will definitely be too small.

IMO the most important thing for @Moustachauve that is missing is the source location for correct binary and what compilation options/flags were used. This being due to users unintentionally updating customised version with plain from Github.

Usermods can be determined by checking info object, but some do not advertise their presence there. Nor in state object.
Perhaps enhancing options/capabilities bitfield would be better or provide a hook for usermods to advertise their presence.

blazoncek · 2025-01-26T08:01:12Z

wled00/cfg.cpp

@@ -643,6 +643,8 @@ bool deserializeConfig(JsonObject doc, bool fromFS) {
    CJSON(wifiLock, ota[F("lock-wifi")]);
    CJSON(aOtaEnabled, ota[F("aota")]);
    getStringFromJson(otaPass, pwd, 33); //normally not present due to security
+    interfaceUpdateCallMode = CALL_MODE_WS_SEND;


I do not think it is necessary to add this here as cfg is only read at boot time.

blazoncek · 2025-01-26T08:02:03Z

wled00/json.cpp

@@ -802,6 +802,8 @@ void serializeInfo(JsonObject root)
  os += 0x40;
  #endif

+  root[F("settingsLocked")] = !correctPIN;


A shorter text would be preferrable to reduce flash usage.

blazoncek · 2025-01-26T08:04:18Z

wled00/json.cpp

@@ -818,6 +820,7 @@ void serializeInfo(JsonObject root)
  #endif
  #ifndef WLED_DISABLE_OTA
  os += 0x01;
+  root[F("otaLocked")] = otaLock;


IMO the if enclosing is enough (and its bitfield of options).
Adding another key seems wasteful.

As the docs do not explain how you read this info out of that field, can you at least explain here how to read that value

blazoncek · 2025-01-26T08:06:14Z

wled00/set.cpp

@@ -587,6 +587,8 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage)
      wifiLock = request->hasArg(F("OW"));
      aOtaEnabled = request->hasArg(F("AO"));
      //createEditHandler(correctPIN && !otaLock);
+      interfaceUpdateCallMode = CALL_MODE_WS_SEND;


Do you think it is really necessart to broadcast state/info after settings are saved?
IMO not but I may be missing a usecase.

Yes it is. If this PR is that you know if the settings value, then you need to send the new value when you save

Indeed, it would allow an app using websockets to update its state right away

blazoncek · 2025-01-26T08:08:37Z

wled00/wled.cpp

@@ -155,6 +155,7 @@ void WLED::loop()
  if (strlen(settingsPIN)>0 && correctPIN && millis() - lastEditTime > PIN_TIMEOUT) {
    correctPIN = false;
    createEditHandler(false);
+    interfaceUpdateCallMode = CALL_MODE_WS_SEND; // schedule WS update


Missing stateUpdated if we want same behaviour as elsewhere.

netmindz · 2025-01-26T09:15:24Z

I'm ok by adding information that update may be locked (either via PIN or OTA password) but the same may be achieved by the app by polling/requesting the update page (it will produce PIN entry or 503 IIRC). So I'm not really sure it is needed.

Parsing the page looking for the pin entry feels very hacky and fragile

The more important thing is if OTA is not compiled. That information is already in info object hidden in options/capabilities bitfield.

Please can you share a link as where this is documented

I also have considerations about JSON buffer size as it may get too small on ESP8266 since we are adding new items to info and state objects all the time. And when @DedeHai 's particle system gets merged it will definitely be too small.

I'm not aware of why the PS would have this affect, please add info to the appropriate PR

IMO the most important thing for @Moustachauve that is missing is the source location for correct binary and what compilation options/flags were used. This being due to users unintentionally updating customised version with plain from Github.

Agreed, but again a can we please keep comments on the appropriate issue/pr so that we keep them all together.

Usermods can be determined by checking info object, but some do not advertise their presence there. Nor in state object. Perhaps enhancing options/capabilities bitfield would be better or provide a hook for usermods to advertise their presence.

This is where source repo and release name should be used, not expecting update apps to try and interpret. Please make any further comments on the appropriate PR/issue

blazoncek · 2025-01-26T10:17:01Z

Parsing the page looking for the pin entry feels very hacky and fragile

It is not. What if I know the PIN and want to enter it while updating?

Please can you share a link as where this is documented

KB see opt field. It says "for debugging" but is available (and filled with options) nevertheless. That field is available and documented as such since pre 0.11. Does it help other developers? It does not as such but it only requires updated documentation.

I'm not aware of why the PS would have this affect, please add info to the appropriate PR

https://discord.com/channels/473448917040758787/779395228624617512/1324429991101862049
https://discord.com/channels/473448917040758787/779395228624617512/1324422249603928169

I merely wanted to express my thoughts and shed a different light on the topic. Nothing more.

netmindz · 2025-01-26T14:28:36Z

Parsing the page looking for the pin entry feels very hacky and fragile

It is not. What if I know the PIN and want to enter it while updating?

I thought the question we are trying to answer is if a pin is required. So either the API supplies that info, as I belive is the intention here - correctly me if I have misunderstood @willmmiles. If I am understanding your alternative proposal correctly @blazoncek you are suggesting that we do not provide that via the API but instead @Moustachauve has to try and grab the HTML of the UI and see if there is an entry box for the PIN. That is both really nasty and also fragile

Please can you share a link as where this is documented

KB see opt field. It says "for debugging" but is available (and filled with options) nevertheless. That field is available and documented as such since pre 0.11. Does it help other developers? It does not as such but it only requires updated documentation.

Are you able to provide the updates to the doc that actually explain what that field is and how to use it if we are now intending 3rd party developers to use this rather than it just being there for our own internal debugging?

I'm not aware of why the PS would have this affect, please add info to the appropriate PR

https://discord.com/channels/473448917040758787/779395228624617512/1324429991101862049 https://discord.com/channels/473448917040758787/779395228624617512/1324422249603928169

I merely wanted to express my thoughts and shed a different light on the topic. Nothing more.

Yes I understand that, my point is that we need to be better organised and ensure we write our comments against the correct issue rather than go off on a tangent. Not everyone will be reading every issue/PR and even if they do, it makes it harder to keep track of items if when you go back to look at the issue/PR where you expect to see the comment and then can't find it as it was placed elsewhere

Moustachauve · 2025-01-26T16:41:41Z

WLED Native ideally would need only the api (and in the future, websocket api) to keep the state up to date.

I would like to not offer OTA to people who can't have updates (including PIN for now). This means that without this API field, I would have to send a request to the update page every 15 seconds to know if it's enabled now.

I don't have a strong preference for this, as it's not the most important use case, but it would be nice if everything was doable through an API and if all the settings/status of the device could be read/written through a standard API. This felt like a useful addition

blazoncek · 2025-01-26T17:57:40Z

1st off I did not create opt field, nor document it. I merely noticed its content and provided info here.
See:

WLED/wled00/json.cpp

Lines 782 to 811 in 61b9947

    
           uint16_t os = 0; 
        
           #ifdef WLED_DEBUG 
        
           os  = 0x80; 
        
             #ifdef WLED_DEBUG_HOST 
        
             os |= 0x0100; 
        
             if (!netDebugEnabled) os &= ~0x0080; 
        
             #endif 
        
           #endif 
        
           #ifndef WLED_DISABLE_ALEXA 
        
           os += 0x40; 
        
           #endif 
        
           //os += 0x20; // indicated now removed Blynk support, may be reused to indicate another build-time option 
        
           #ifdef USERMOD_CRONIXIE 
        
           os += 0x10; 
        
           #endif 
        
           #ifndef WLED_DISABLE_FILESYSTEM 
        
           os += 0x08; 
        
           #endif 
        
           #ifndef WLED_DISABLE_HUESYNC 
        
           os += 0x04; 
        
           #endif 
        
           #ifdef WLED_ENABLE_ADALIGHT 
        
           os += 0x02; 
        
           #endif 
        
           #ifndef WLED_DISABLE_OTA 
        
           os += 0x01; 
        
           #endif 
        
           root[F("opt")] = os;

Regarding availability of OTA: The information is in opt field as opt & 0x0001. If that evaluates as true (or >0) then OTA is available. If not, OTA is unavailable and calling /update (POST or GET) will yield 501.

Regarding PIN and OTA lock: If you try to call /update (POST with data for binary) and device is OTA locked you'll get 401, but PIN entry (HTTP) when PIN is used and is locked. If you call /update via GET and PIN is locked you'll be prompted with PIN entry as well. When PIN is unlocked (or not used) /update will behave as expected.

I am merely describing behaviour, don't want to suggest anything.
However I do feel that cramming too much into JSON will inevitably lead to removing support for ESP8266. This is something I do not want.

netmindz · 2025-01-26T18:26:43Z

Thank you for the further info @blazoncek

willmmiles added 2 commits November 12, 2024 23:34

Publish settings and OTA state in info

3fa9aa9

Update lock state property names

702fb0e

Use "settingsLocked" and "otaLocked" to clearly convey the meaning and sense.

willmmiles added the enhancement label Nov 14, 2024

netmindz changed the base branch from 0_15 to main December 16, 2024 13:26

willmmiles added 3 commits January 23, 2025 22:09

whitespace fix

ee71e62

Update lock state broadcasts

cb47316

Another whitespace fix

09e2c74

willmmiles changed the title ~~Draft: Add settings and OTA lock state to info~~ Add settings and OTA lock state to info Jan 25, 2025

willmmiles requested a review from blazoncek January 25, 2025 20:35

willmmiles self-assigned this Jan 25, 2025

blazoncek reviewed Jan 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add settings and OTA lock state to info #4279

Add settings and OTA lock state to info #4279

willmmiles commented Nov 14, 2024

blazoncek commented Nov 15, 2024

blazoncek left a comment

blazoncek Jan 26, 2025

blazoncek Jan 26, 2025

blazoncek Jan 26, 2025

netmindz Jan 26, 2025

blazoncek Jan 26, 2025

netmindz Jan 26, 2025

Moustachauve Jan 26, 2025

blazoncek Jan 26, 2025

netmindz commented Jan 26, 2025

blazoncek commented Jan 26, 2025 •

edited

Loading

netmindz commented Jan 26, 2025

Moustachauve commented Jan 26, 2025

blazoncek commented Jan 26, 2025

netmindz commented Jan 26, 2025

Add settings and OTA lock state to info #4279

Are you sure you want to change the base?

Add settings and OTA lock state to info #4279

Conversation

willmmiles commented Nov 14, 2024

blazoncek commented Nov 15, 2024

blazoncek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netmindz commented Jan 26, 2025

blazoncek commented Jan 26, 2025 • edited Loading

netmindz commented Jan 26, 2025

Moustachauve commented Jan 26, 2025

blazoncek commented Jan 26, 2025

netmindz commented Jan 26, 2025

blazoncek commented Jan 26, 2025 •

edited

Loading