add support for up to 10 ESPNow remotes #4654
Conversation
WalkthroughThis change updates the handling of ESP-NOW remote MAC addresses throughout the codebase, transitioning from support for a single linked remote to supporting multiple linked remotes. The update modifies the data structure for storing MAC addresses, changes serialization and deserialization logic, and updates the web interface to allow users to manage multiple remotes. The ESP-NOW message processing logic is also updated to check against a list of authorized remotes. JavaScript and HTML in the WiFi settings page are revised to provide a user interface for adding, removing, and displaying multiple linked MAC addresses. Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wled00/data/settings_wifi.htm (1)
264-269: Well-designed UI for managing remote MACsThe UI shows the last seen device MAC address and provides an easy way to add it to the list. The implementation is clean and user-friendly.
Consider adding confirmation when removing a remote to prevent accidental deletions:
btn.onclick = function() { - removeRemote(row); + if(confirm('Are you sure you want to remove this remote?')) { + removeRemote(row); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wled00/cfg.cpp(2 hunks)wled00/data/settings_wifi.htm(2 hunks)wled00/remote.cpp(1 hunks)wled00/set.cpp(1 hunks)wled00/udp.cpp(1 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (7)
wled00/wled.h (1)
541-542: Implementation of multiple ESP-NOW remotes vector looks goodThe change from a single fixed-size array to a vector of fixed-size arrays is a clean way to support multiple remote devices. I see the original declaration is kept as a comment, which might help with understanding the change or potential backwards compatibility.
wled00/remote.cpp (1)
184-207: Removed sender validation from handleWiZdataThe sender validation logic that was previously in this function has been properly moved to the ESP-NOW receive callback in udp.cpp. This is a good architectural change, as it centralizes the MAC address validation in a single place.
wled00/xml.cpp (1)
219-222: Client-side remotes initialization implementation is correctThe code now correctly initializes the client-side list of remotes by:
- First calling
resetRemotes()to clear any existing entries- Then iterating through the vector to add each remote with a unique identifier ("RM0" through "RM9")
This approach properly supports the new multi-remote functionality.
wled00/udp.cpp (2)
962-973: Correctly implemented multi-remote validationThis is a key change that enables support for multiple ESP-NOW remotes. The code now:
- Iterates through all stored MAC addresses in the
linked_remotesvector- Checks if the sender's MAC matches any of the authorized remotes
- Rejects messages from unauthorized senders with appropriate debug logging
This validation logic is more robust than the previous single-remote check.
975-979: WiZ remote data handling is unchangedThe core functionality for handling WiZ remote data is preserved after the validation check, maintaining compatibility with existing remotes while enabling support for multiple devices.
wled00/cfg.cpp (1)
742-745: Correctly serializes the remote MACs to JSON arrayThe implementation properly serializes the vector of linked remotes to a JSON array, which matches the deserialization logic.
wled00/data/settings_wifi.htm (1)
200-200: Good form submission handlingAdding the
onsubmitevent handler ensures the MAC array is updated before form submission, which is essential for proper data serialization.
wled00/data/settings_wifi.htm
Outdated
| Paired Remote MAC: <input type="text" name="RMAC" minlength="12" maxlength="12"><br> | ||
| Last device seen: <span class="rlid" onclick="d.Sf.RMAC.value=this.textContent;" style="cursor:pointer;">None</span> <br> | ||
| Last device seen: <span class="rlid">None</span> | ||
| <button type="button" class="btn btn-xs" onclick="addRemote('RM'+remoteCount,this.parentElement.querySelector('.rlid').textContent)">Add</button><br> |
There was a problem hiding this comment.
You could use id property in last device seen <span> and then use shorter form gId("rlid").
There was a problem hiding this comment.
I tried this, since xml.cpp modifies it, it must be a class no? I cann add an id on top.
There was a problem hiding this comment.
class is used for formatting purposes, id for identification. Each element must have unique id if it is used.
You can assign multiple classes to an element (and they are inherited) but id is specific to the element (tag).
There was a problem hiding this comment.
if I remove the class and replace it with id, the xml.cpp update does not work.
this does work:
Last device seen: <span class="rlid" id="ld">None</span>
<button type="button" onclick="aR('RM'+rC,gId('ld').textContent)">+</button><br>
- reverting name to `linked_remote` - ESPNow remote list is now hidden if unchecked - shortened java script function names and variables to save flash - removed now obsolete settings in xml.cpp - correct checking of valid hex string for remote list in java script
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/data/settings_wifi.htm (2)
156-185: Consider more descriptive function and variable names.While the function effectively adds a remote MAC to the list with proper validation, the variable and function names (aR, rC) are very terse and not self-descriptive. Using more meaningful names would improve code readability and maintainability.
-var rC = 0; // remote count +var remoteCount = 0; // remote count -function aR(id, mac) { +function addRemote(id, mac) { if (!/^[0-9A-F]{12}$/i.test(mac)) return; // check for valid hex string - if (rC >= 10) return; + if (remoteCount >= 10) return; // Rest of the function remains the same - rC++; + remoteCount++; - uR(); + updateRemoteList(); }Also, update references to this function elsewhere in the code (like line 273).
186-190: Consider adding user feedback when removing a remote.The removal function works correctly, but there's no feedback to the user when a remote is removed. Consider adding a brief notification or visual feedback.
-function rR(e) { +function removeRemote(e) { e.remove(); - uR(); + updateRemoteList(); + // Optional: Show brief feedback + let notification = cE('div'); + notification.className = 'notification'; + notification.innerText = 'Remote removed'; + document.body.appendChild(notification); + setTimeout(() => notification.remove(), 2000); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/cfg.cpp(2 hunks)wled00/data/settings_wifi.htm(2 hunks)wled00/xml.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/cfg.cpp
- wled00/xml.cpp
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (6)
wled00/data/settings_wifi.htm (6)
139-139: Using setTimeout for DOM initialization is appropriate here.The use of setTimeout to delay calling tE() ensures the DOM has loaded before attempting to access elements, which is a good practice in this context.
142-149: Good implementation of ESP-NOW UI toggle functionality.The tE() function effectively toggles the visibility of the remote list UI based on the checkbox state. The function has clear comments and handles the DOM manipulation appropriately.
150-155: Well-implemented reset function for remotes list.This function correctly clears the remote list and updates the internal state, with proper comments explaining its purpose and origin.
191-202: Well-implemented function for updating the remote list.This function correctly collects all MAC addresses, validates them, and updates the hidden form field with a JSON string. The return value ensures the form submission continues.
207-207: Good use of form onsubmit handler.Adding the onsubmit handler ensures the hidden input is updated with the current list of remotes before form submission, which is essential for proper data handling.
268-279: Well-structured UI for managing ESP-NOW remotes.The UI implementation effectively displays the last seen device and provides a clean interface for managing linked MAC addresses. The organization of elements with appropriate containers and the hidden input for form submission is well done.
I notice you've addressed the previous feedback about using an ID for the last device span by adding
id="ld"to the span element.
|
in the html files, indentation seems to be messed up here on git, but it displays right in vscode. Do I need to change a setting or is this a github thing? |
It is not. HTML/CSS/JS files use tabs, C/C++ files use spaces. Fix that in your VSC configuration and you are good. |
|
One more thing that sprung into mind: Instead of "Add" and "Remove" buttons you could use familiar "+" and "-" buttons. |
…nVariant, replaced buttons with +/-
|
I think I have it all fixed and ready now. if ESPNow is disabled, the list is hidden (but still kept in config if it exists) |
| @@ -136,12 +136,75 @@ | |||
| getLoc(); | |||
| loadJS(getURL('/settings/s.js?p=1'), false); // If we set async false, file is loaded and executed, then next statement is processed | |||
| if (loc) d.Sf.action = getURL('/settings/wifi'); | |||
| setTimeout(tE, 500); // wait for DOM to load before calling tE() | |||
There was a problem hiding this comment.
This should not be needed here. You can add call to tE() in xml.cpp if it is necessary to be invoked when all fields are set.
There was a problem hiding this comment.
I tried many variations, including calling tE() from xml.cpp. this variant, although a bit of a hack, is the only one that consistently worked. tE() was always evaluated too early, not properly showing/hiding the section.
maybe I did it wrong, you can try if you can get it working right with an xml.cpp call.
Since I have very little knowledge on HTML and java script I did the UI update with a lot of help from AI's.
Please check the code for any mistakes or possible improvements. I tested it and it does work but I really do not know if there is better ways to do the java part.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor