Skip to content

Add settings and OTA lock state to info #4279

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.

Sign up for GitHub

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

Open
willmmiles wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add settings and OTA lock state to info #4279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3fa9aa9
Publish settings and OTA state in info
willmmiles Nov 13, 2024
702fb0e
Update lock state property names
willmmiles Nov 14, 2024
ee71e62
whitespace fix
willmmiles Jan 24, 2025
cb47316
Update lock state broadcasts
willmmiles Jan 25, 2025
09e2c74
Another whitespace fix
willmmiles Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions wled00/cfg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@blazoncek blazoncek Jan 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stateChanged = true;
}

#ifdef WLED_ENABLE_DMX
Expand Down
3 changes: 3 additions & 0 deletions wled00/json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,8 @@ void serializeInfo(JsonObject root)
os += 0x40;
#endif

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

@blazoncek blazoncek Jan 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A shorter text would be preferrable to reduce flash usage.


//os += 0x20; // indicated now removed Blynk support, may be reused to indicate another build-time option

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

@blazoncek blazoncek Jan 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@netmindz netmindz Jan 26, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#endif
root[F("opt")] = os;

Expand Down
2 changes: 2 additions & 0 deletions wled00/set.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@blazoncek blazoncek Jan 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@netmindz netmindz Jan 26, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Moustachauve Moustachauve Jan 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stateChanged = true;
Comment on lines +590 to +591

@coderabbitai coderabbitai Bot Apr 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use interface-update signaling only for security settings changes.

At Line [591], setting stateChanged = true for OTA/PIN settings mixes config updates with runtime state-change flow. Keep the WS/interface update trigger, but do not mark runtime state as changed.

Suggested fix for this segment 
       interfaceUpdateCallMode = CALL_MODE_WS_SEND;
-      stateChanged = true;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
interfaceUpdateCallMode = CALL_MODE_WS_SEND;
stateChanged = true;
interfaceUpdateCallMode = CALL_MODE_WS_SEND;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@wled00/set.cpp` around lines 590 - 591, The code is incorrectly marking
runtime state as changed when only security/OTA or PIN configuration is updated;
leave the WebSocket/interface signaling (interfaceUpdateCallMode =
CALL_MODE_WS_SEND) but remove or conditionally avoid setting stateChanged = true
for OTA/PIN-only changes. Update the handler that sets interfaceUpdateCallMode
and stateChanged so it sets interfaceUpdateCallMode = CALL_MODE_WS_SEND
unconditionally for config/security updates but only assigns stateChanged = true
when a true runtime state change occurs (i.e., not for OTA or PIN config flags);
reference the existing interfaceUpdateCallMode, CALL_MODE_WS_SEND, and
stateChanged symbols to implement this conditional behavior.

}
}

Expand Down
6 changes: 5 additions & 1 deletion wled00/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,11 @@ void checkSettingsPIN(const char* pin) {
if (!correctPIN && millis() - lastEditTime < PIN_RETRY_COOLDOWN) return; // guard against PIN brute force
bool correctBefore = correctPIN;
correctPIN = (strlen(settingsPIN) == 0 || strncmp(settingsPIN, pin, 4) == 0);
if (correctBefore != correctPIN) createEditHandler(correctPIN);
if (correctBefore != correctPIN) {
createEditHandler(correctPIN);
interfaceUpdateCallMode = CALL_MODE_WS_SEND;
stateChanged = true;
}
lastEditTime = millis();
}

Expand Down
1 change: 1 addition & 0 deletions wled00/wled.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@blazoncek blazoncek Jan 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing stateUpdated if we want same behaviour as elsewhere.

}

// reconnect WiFi to clear stale allocations if heap gets too low
Expand Down