-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
support iotorero ethernet controller #4860
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
Conversation
WalkthroughThe Ethernet Type dropdown in wled00/data/settings_wifi.htm was updated: option value 6 was relabeled to "IoTorero/ESP32Deux/RGB2Go" and repositioned directly after the "None (0)" option; the original entry for value 6 was removed from its previous position. No other logic or structure changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~6 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
wled00/const.h (2)
355-355
: Keep count and initializer array in lockstep (add a compile-time guard).Bumping WLED_NUM_ETH_TYPES to 14 looks correct given the added IoTorero entry. To prevent future mismatches between this constant and ethernetBoards[] size, add a static_assert right after the array definition in network.cpp.
Add this (outside the changed lines, in wled00/network.cpp right after ethernetBoards[]):
// Guard against mismatches between enum space and initializer table static_assert( WLED_NUM_ETH_TYPES == (sizeof(ethernetBoards) / sizeof(ethernetBoards[0])), "WLED_NUM_ETH_TYPES mismatch with ethernetBoards[] entries" );
370-370
: Unify macro casing with the rest (prefer screaming snake).Other Ethernet macros use all-caps with underscores. Consider standardizing the new one to WLED_ETH_IOTORERO and keeping a temporary alias to avoid breaking any in-flight references.
Apply this diff:
-#define WLED_ETH_IoTorero 13 +#define WLED_ETH_IOTORERO 13 +// Temporary alias for consistency with existing PRs/forks that may reference mixed-case name +#define WLED_ETH_IoTorero WLED_ETH_IOTOREROwled00/data/settings_wifi.htm (1)
261-261
: Label and ordering nit: match existing naming style and position.
- Label: Other entries avoid hyphens in product names; suggest “IoTorero Ethernet Controller” for consistency.
- Order: Consider positioning the option near related entries (by numeric value or alphabetically). Placing value="13" after value="12" (LILYGO T-POE Pro) keeps the list stable and easier to scan.
Apply this diff for the label (reordering is optional and not shown here):
- <option value="13">IoTorero-ETH-Controller</option> + <option value="13">IoTorero Ethernet Controller</option>wled00/network.cpp (1)
149-157
: Verify IoTorero pinout, PHY address, and clock mode before merging.The entry matches common LAN8720 setups (MDC=23, MDIO=18, ETH_CLOCK_GPIO17_OUT) and uses eth_power = -1 (no power/reset pin). Please confirm against the board’s schematic:
- PHY address: 0 vs 1 often varies by board; ensure 1 is correct here.
- eth_power: If the board exposes a PHY power/reset pin, set it accordingly instead of -1 to benefit from the reset logic.
- Clock: Confirm the board requires GPIO17_OUT (50MHz output) vs GPIO0_IN/OUT.
Optionally, add a link comment to the board’s schematic/product page (similar to the LilyGO block) for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
wled00/const.h
(2 hunks)wled00/data/settings_wifi.htm
(1 hunks)wled00/network.cpp
(1 hunks)
Maybe just add to "ESP32Deux/RGB2Go Tetra" since it's the same def? |
yeah if @kilrah is correct, just update the name in the dropdown, don't add a duplicate type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mo need for new definition, just updated naming
Add support for IoTorero Ethernet controller
https://www.athom.tech/blank-1/ethernet-wled-esp32-addressable-dmx-led-strip-controller
Summary by CodeRabbit