Revert disable OTA logic & optional Arduino OTA #4748
Conversation
- new compile flag WLED_ENABLE_AOTA - modify WLED_CONNECTED macro - bugfix in Network isConnected() when static IP is set
WalkthroughThe changes standardize and invert the logic for enabling Over-The-Air (OTA) updates by introducing the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learningswled00/wled.h (9)🧬 Code Graph Analysis (1)wled00/wled.h (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@KrX3D can you verify/check this PR? |
|
i tested it with both in all combinations and it always worked, it compiled and could be uplaoded via usb but it took me some time to figure out that i need to uncheck "Only allow update from same network/WiFi" and only looking atthe log i could find out that i need to uncheck it: since i could select the BIn file and hit update, and for 2 devices (both an s2 mini) i got 2 results.
and need to manually reset the device. i dont know if it really was flashed
for both it is not clear that the checkbox creates the problem. for 2. i think it is
for 1. not sure why it behaves differently, both are an s2 mini and are flashed with exactly the same bin file and are located in the same VLAN |
|
Thank you @KrX3D Since I am unable to perform any testing ATM due to not having access to an ESP device I have to rely on willing users to do the testing. This PR only introduces ability to completely disable OTA updates (Access denied) or additionally enable Arduino OTA (Arduino protocol UDP 3232 IIRC). Arduino OTA requires about 10k of flash and is seldom used (mostly with developers) while HTTP OTA requires 4k (14k if both are included). Your issues might be related to browser cache. |
- added OTA section toggle - added ignore upload - fix for missing "NoXXX" ID - rely on 404 for /dmxmap
|
i tested those new changes and they seems also ok. a couple more things. if i have checked Only allow update from same network/WiFi: and try tro flash it from a different vlan it takes like i said around 10-15 sec. during that time i get around 700 lines of: and than the esp just hangs up and i get the page shown on the picture in my last post. and i found out why i get on those 2 devices different results. the one where i get the picture in my last post i connected via IP 192.168.XXX.XXX and HTTP which also give the 700 lines and by getting the access denied the log only gets 3 lines and not 700, and the esp also doesnt hang up so its not an:
and also it would be good to have on both ways the Access denied message
its ok that the checkbox hindes to upload from another vlan, what i wanted to suggest was that the "Access denied" which just looks like this:
should also mention WHY it is denied, and so it would be good to have that same message as in the log like: so that also new users would understand why it doesnt work |
|
The primary goal of this PR (and original #4700) is related to preventing OTA from untrusted sources. This PR removes Arduino OTA from standard builds, freeing up couple of kB or flash and RAM. It also fixes a few underlying bugs. It does not change original behaviour of #4700 which is at the source for comments from @KrX3D . So IMO notifying user about inability to update OTA from another VLAN is of secondary importance (and can be described in KB if necessary). Are there any other objections to this PR or should it be merged to wrap what was started in #4700? |
|
no objections from me |
|
Given we have moved from Arduino to PlatformIO, we should be removing Arduino OTA entirely. We say the only way to build is to use platformio so, no point maintaining an update method used by Arduino |
|
With this PR enabling Arduino OTA (protocol) is optional and not enabled by default. |
|
I thought the esp OTA was different to the Arduino OTA, if was wrong about that, then yes we keep support Can you give an example of your platformio config where you have this working? |
|
UI OTA works using HTTP while ArduinoOTA has its own protocol (and port in use). You can update via wget with first method while using espota.py tool for second. EDIT: Here you go: |
With #4700 WLED_DISABLE_OTA did not entirely disable OTA, just Arduino OTA. This PR reverses that and introduces new compile flag WLED_ENABLE_AOTA (undefined by default) which (if set) enables Arduino OTA. Otherwise only HTTP OTA is available.
WLED_CONNECTED macro is modified to call
Network.isConnected()instead of duplicating its code.Network.isConnected()is updated to account if static IP is set for wired connection.Summary by CodeRabbit