SE16V2 / Lightcrafter16 board support #86
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
SE16V2 Board & RS485 Pin Support src/MoonBase/Modules/ModuleIO.h |
Added board_SE16V2 to IO_BoardsEnum and pin_RS485_DE to IO_PinUsageEnum. Exposed "Lightcrafter16" in setupDefinition UI. Implemented full SE16V2 preset defaults in setBoardPresetDefaults (maxPower, LED pins, RS485_DE, UART/SPI/PHY/ADC mappings, IRQ/CS pins). Extended runtime pin-assignment logic to initialize SE16V2-specific pins. |
Sequence Diagram(s)
(Skipped — changes are confined to enums, UI options, and board preset wiring without new multi-component control flow.)
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
- Update board definitions #73 — Modifies the same IO enums and board preset handling; closely related to the added
board_SE16V2andpin_RS485_DE.
Poem
🐰 I hopped a circuit, tiny and spry,
RS‑485 ready beneath the sky,
SE16V2 with LEDs aglow,
Pins lined up in tidy row —
A rabbit wink, and presets go! ✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'SE16V2 / Lightcrafter16 board support' directly and specifically describes the main change: adding board support for these two components, which aligns perfectly with the changeset. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonBase/Modules/ModuleIO.h
🔇 Additional comments (4)
src/MoonBase/Modules/ModuleIO.h (4)
63-63: LGTM!The addition of
pin_RS485_DE(Driver Enable for RS485) follows the existing pattern and is properly integrated with UI configuration and board presets.
81-81: LGTM!The board enum addition is properly placed and follows the existing pattern.
121-121: LGTM!The UI labels "Lightcrafter16" and "RS-485 DE" are clear and properly integrated into the configuration interface.
Also applies to: 195-195
309-311: Well-structured board configuration with comprehensive pin assignments.The SE16V2 preset correctly defines all necessary pins for the 16-channel board, including:
- 16 LED output pins
- RS485 communication with the new DE pin support
- SPI bus for WIZ850IO Ethernet module
- Voltage/current sensing
- Digital input and infrared receiver
The configuration mirrors the SE16V1 pattern while adding the new RS485_DE pin, which demonstrates good consistency.
Also applies to: 313-324
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonBase/Modules/ModuleIO.h
🔇 Additional comments (5)
src/MoonBase/Modules/ModuleIO.h (5)
63-63: LGTM!The RS485_DE pin usage enum addition is correctly placed alongside other RS-485 pin types and follows the existing naming convention.
81-81: LGTM!The board enum addition is correctly positioned after SE16V1 and follows the existing naming convention.
121-121: LGTM!The UI control option correctly corresponds to the new board_SE16V2 enum value and follows the pattern of using user-friendly names.
195-195: LGTM!The RS-485 DE pin usage UI option correctly corresponds to the new pin_RS485_DE enum and maintains consistency with existing RS-485 pin labels.
315-315: RS485_DE pin requires verification of configuration approach.The pin is assigned during board initialization (line 315) but unlike Ethernet pins (which are configured dynamically in readPins() and applied via EthernetSettingsService), RS485_DE has no corresponding configuration service or dynamic handling in readPins(). Verify whether RS485_DE requires similar service-based initialization like Ethernet pins, or if static assignment via pinAssigner is sufficient for this hardware.
| } else if (boardID == board_SE16V2) { | ||
| object["maxPower"] = 500; | ||
| uint8_t ledPins[] = {47, 21, 14, 9, 8, 16, 15, 7, 1, 2, 42, 41, 40, 39, 38, 48}; // LED_PINS | ||
| for (uint8_t gpio : ledPins) pinAssigner.assignPin(gpio, pin_LED); | ||
| pinAssigner.assignPin(3, pin_OnBoardLed); // WIZ580_nRST, needs to be high to access RS485_DE, VBUS_DET, WIZ580_nINT. Also drives an LED. | ||
| pinAssigner.assignPin(17, pin_Serial_TX); | ||
| pinAssigner.assignPin(18, pin_Serial_RX); | ||
| pinAssigner.assignPin(46, pin_RS485_DE); | ||
| pinAssigner.assignPin(0, pin_Dig_Input); // Native USB port vbus detection | ||
| pinAssigner.assignPin(5, pin_Voltage); // Input voltage | ||
| pinAssigner.assignPin(6, pin_Current); // Input current | ||
| pinAssigner.assignPin(13, pin_SPI_MISO); // WIZ850IO MISO | ||
| pinAssigner.assignPin(11, pin_SPI_MOSI); // WIZ850IO MOSI | ||
| pinAssigner.assignPin(12, pin_SPI_SCK); // WIZ850IO CLK | ||
| pinAssigner.assignPin(10, pin_PHY_CS); // WIZ850IO nCS | ||
| pinAssigner.assignPin(45, pin_PHY_IRQ); // WIZ850IO nINT | ||
| pinAssigner.assignPin(4, pin_Infrared); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for OnBoardLed handling and GPIO 3 usage
rg -n -C5 'pin_OnBoardLed' --type=cpp --type=h | head -100Repository: MoonModules/MoonLight
Length of output: 1725
🏁 Script executed:
#!/bin/bash
# Search for readPins function to understand pin handling
rg -n 'readPins|pin_LED|pin_OnBoardLed' --type=cpp --type=h | grep -E 'readPins|void.*pin|return.*pin' | head -50Repository: MoonModules/MoonLight
Length of output: 1083
🏁 Script executed:
#!/bin/bash
# Check the pin assignment logic to understand if pins can be driven low
rg -n -B5 -A10 'struct.*PinAssigner|assignPin' --type=cpp --type=h | head -80Repository: MoonModules/MoonLight
Length of output: 6271
🏁 Script executed:
#!/bin/bash
# Read the readPins() function implementation in ModuleIO.h
sed -n '599,700p' src/MoonBase/Modules/ModuleIO.hRepository: MoonModules/MoonLight
Length of output: 1800
🏁 Script executed:
#!/bin/bash
# Search for LED control logic and how OnBoardLed pins are driven
rg -n -A15 'pin_OnBoardLed|pin_LED' src/MoonLight/Modules/ModuleLightsControl.h | head -60Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if there's any initialization code for GPIO 3 or special pin setup
rg -n -B3 -A3 'GPIO.*3|pin.*3|pinMode.*3' --type=cpp --type=h | grep -v test | head -50Repository: MoonModules/MoonLight
Length of output: 4909
🏁 Script executed:
#!/bin/bash
# Search for ModuleLightsControl.h readPins implementation
sed -n '73,150p' src/MoonLight/Modules/ModuleLightsControl.hRepository: MoonModules/MoonLight
Length of output: 3373
🏁 Script executed:
#!/bin/bash
# Search for any code that might control GPIO 3 or OnBoardLed
rg -n 'digitalWrite|digitalWrite|pinMode.*3|pin_LED.*[0-9]|pin_OnBoardLed' --type=cpp --type=h | head -40Repository: MoonModules/MoonLight
Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Check if there's any driver or control module that reads pin_LED or pin_OnBoardLed
rg -n 'pin_LED|pin_OnBoardLed' src/MoonLight/Nodes/Drivers/ --type=cpp --type=hRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for initialization of GPIO 3 or pin setup for OnBoardLed
rg -n 'setUp|init|Setup|GPIO.*3|OnBoardLed' src/MoonBase/Modules/ModuleIO.h | head -30Repository: MoonModules/MoonLight
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Check if pin_OnBoardLed is used anywhere else in the codebase
rg -rn 'pin_OnBoardLed' --type=cpp --type=h .Repository: MoonModules/MoonLight
Length of output: 270
🏁 Script executed:
#!/bin/bash
# Verify there's no LED control for OnBoardLed specifically - check all references to pin_OnBoardLed
rg -B5 -A5 'pin_OnBoardLed' --type=cpp --type=hRepository: MoonModules/MoonLight
Length of output: 1648
🏁 Script executed:
#!/bin/bash
# Check ModuleDrivers.h completely for any pin_OnBoardLed handling
sed -n '1,200p' src/MoonLight/Modules/ModuleDrivers.h | grep -E 'pin_OnBoardLed|readPins|usage =='Repository: MoonModules/MoonLight
Length of output: 299
🏁 Script executed:
#!/bin/bash
# Double-check if there's ANY code that iterates through pins and checks for pin_OnBoardLed
rg -n 'usage.*pin_OnBoardLed|pin_OnBoardLed.*gpio|pin_OnBoardLed.*usage' --type=cpp --type=hRepository: MoonModules/MoonLight
Length of output: 47
GPIO 3 assigned as pin_OnBoardLed is never managed by any control logic; ensure this critical enable pin stays high.
The comment indicates GPIO 3 "needs to be high to access RS485_DE, VBUS_DET, WIZ580_nINT," yet assigning it as pin_OnBoardLed has no effect since no code in the codebase processes pin_OnBoardLed. This leaves GPIO 3 in an unmanaged state despite being critical for peripheral access.
Either remove the pin_OnBoardLed assignment if GPIO 3 should be unmanaged, or add explicit initialization to drive GPIO 3 HIGH and ensure it remains high throughout operation.
Also fix comment inconsistency: line 312 references "WIZ580_nRST" while lines 319–323 reference "WIZ850IO"—clarify which is correct.
🤖 Prompt for AI Agents
In src/MoonBase/Modules/ModuleIO.h around lines 308–324, GPIO 3 is assigned to
pin_OnBoardLed but no code drives or manages that pin, leaving the critical WIZ
reset/enable signal potentially floating; also the inline comments mix WIZ580 vs
WIZ850 naming. Fix by (1) explicitly reserving and initializing GPIO 3 as a
driven output and set it HIGH during board/module initialization (ensure it
cannot be later re-assigned or inadvertently driven low), or if the design
expects it unmanaged, remove the pin_OnBoardLed assignment and add a clear
comment stating it is intentionally unmanaged; and (2) make the comment names
consistent (choose and use either WIZ850IO or WIZ580_nRST across the block) so
the hardware signal reference is unambiguous.
There was a problem hiding this comment.
"add explicit initialization to drive GPIO 3 HIGH " << that's pretty much the comment is above.
tagging @ewowi
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
src/MoonBase/Modules/ModuleIO.h
Outdated
| object["maxPower"] = 500; | ||
| uint8_t ledPins[] = {47, 21, 14, 9, 8, 16, 15, 7, 1, 2, 42, 41, 40, 39, 38, 48}; // LED_PINS | ||
| for (uint8_t gpio : ledPins) pinAssigner.assignPin(gpio, pin_LED); | ||
| pinAssigner.assignPin(3, pin_OnBoardLed); // WIZ580_nRST, needs to be high to access RS485_DE, VBUS_DET, WIZ580_nINT. Also drives an LED. |
There was a problem hiding this comment.
Fix comment inconsistency: WIZ580 vs WIZ850IO.
Line 312 mentions "WIZ580_nRST" while lines 319-323 consistently reference "WIZ850IO" for the Ethernet module. Ensure the correct module designation is used throughout.
🔎 Suggested fix if WIZ850IO is correct
- pinAssigner.assignPin(3, pin_OnBoardLed); // WIZ580_nRST, needs to be high to access RS485_DE, VBUS_DET, WIZ580_nINT. Also drives an LED.
+ pinAssigner.assignPin(3, pin_OnBoardLed); // WIZ850IO_nRST, needs to be high to access RS485_DE, VBUS_DET, WIZ850IO_nINT. Also drives an LED.📝 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.
| pinAssigner.assignPin(3, pin_OnBoardLed); // WIZ580_nRST, needs to be high to access RS485_DE, VBUS_DET, WIZ580_nINT. Also drives an LED. | |
| pinAssigner.assignPin(3, pin_OnBoardLed); // WIZ850IO_nRST, needs to be high to access RS485_DE, VBUS_DET, WIZ850IO_nINT. Also drives an LED. |
🤖 Prompt for AI Agents
In src/MoonBase/Modules/ModuleIO.h around line 312, the comment uses
"WIZ580_nRST" which conflicts with the rest of the file that references the
Ethernet module as "WIZ850IO"; change the comment to use the correct module
designation (e.g., "WIZ850IO_nRST" or "WIZ850IO" as appropriate), and scan
nearby lines to ensure all mentions of WIZ580 are replaced with WIZ850IO for
consistent naming.
Every change in the future tries to preserve everything what is already working, so bugs aside this will indeed be the aim 🙂 |
2 participants
@ewowi I'm hoping these changes don't get modified now or in the future... but if they do please let me know to make sure we don't make breaking changes.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.