Update board definitions #73
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Core header / enums src/MoonBase/Modules/ModuleIO.h |
Added pin_RS485_TX, pin_RS485_RX, pin_Dig_Input to IO_PinUsage. Replaced board_MHCD0/board_MHCP4Nano with board_MHCV43/board_MHCP4NanoV1 in IO_Boards. |
UI controls & presets src/MoonBase/Modules/ModuleIO.h |
Renamed public control jumper1 → switch1, added switch2. Updated setupDefinition UI strings to "MyHome‑Control V43 controller" and "MyHome‑Control P4 Nano Shield V1.0". |
Board preset defaults & pin mappings src/MoonBase/Modules/ModuleIO.h |
Implemented setBoardPresetDefaults for board_MHCV43 and board_MHCP4NanoV1 with new LED pin arrays, RS‑485 TX/RX and Digital Input mappings, and conditional I2S/Line‑In behavior gated by switch2. Added conditional 8/16‑pin LED mappings influenced by switch1. |
Runtime update handling src/MoonBase/Modules/ModuleIO.h |
onUpdate now reacts to switch1 and switch2 changes to rebuild state; preserved previous jumper-related logic mapping to new switches. |
Documentation docs/moonbase/inputoutput.md |
Replaced Jumper1 with Switch1, introduced Switch2, and updated descriptions and pin-mapping examples (MHCP4NanoV1) for LED layouts and I2S/Line‑In configurations. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20–30 minutes
- Pay attention to all usages of the renamed/added enum values across the codebase.
- Review
setBoardPresetDefaultsbranches for correct LED arrays, RS‑485/Digital Input pin assignments, and theswitch2gating for I2S/Line‑In. - Verify UI control IDs, default values, and persistence handling for
switch1/switch2. - Check any JSON/UI strings for localization or rendering issues (emoji → plain text changes).
Poem
🐰
I hop through pins both old and new,
Two little switches guide the view,
TX and RX find tidy tracks,
LEDs march forth, no jumper lacks,
A tiny board, a brighter cue ✨
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 'Update board definitions' is directly related to the main changes in the PR. The changeset focuses on updating board enums (replacing board_MHCD0/board_MHCP4Nano with board_MHCV43/board_MHCP4NanoV1), board presets, pin usage taxonomy, and related configuration logic. |
✨ 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(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonBase/Modules/ModuleIO.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
🔇 Additional comments (4)
src/MoonBase/Modules/ModuleIO.h (4)
189-191: LGTM: Pin usage labels correctly match new enum entries.The UI labels for RS-485 TX, RS-485 RX, and Digital Input are properly aligned with the new pin type enums.
464-476: LGTM: Line-In vs Mic configuration correctly implemented.The switch2 conditional properly handles two distinct I2S pin configurations: Line-In mode (pins 33, 26, 32, 36) when enabled, and built-in Mic mode (pins 10, 11, 12, 13) when disabled. This aligns with the PR objectives.
556-559: LGTM: Switch change handling correctly triggers board rebuild.The logic properly detects switch1/switch2 changes and triggers a board preset rebuild by marking the configuration as unmodded. This ensures pin assignments are recalculated based on the new switch states.
292-292: LGTM: Consistent rename from jumper1 to switch1.The reference has been properly updated to use the new switch1 naming, maintaining consistency with the control definition changes.
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonBase/Modules/ModuleIO.h(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonBase/Modules/ModuleIO.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
🔇 Additional comments (2)
src/MoonBase/Modules/ModuleIO.h (2)
134-137: Good rebuild trigger forUseLineIn, but ensure state coherence formodded
Rebuilding presets whenjumper1/UseLineInchanges makes sense. Just ensure the UI and any persisted state don’t briefly display “modded=true” while the preset rebuild is pending (since you set_state.data["modded"]=falsewithout an immediateupdate()here).Also applies to: 508-512
383-397: Verify GPIO0 as I2S MCLK against MHCV43 board design documentationGPIO0 is an ESP32 strapping pin that controls boot mode selection during power-up. While the MHCV43 board design may safely handle this through proper pull-up/pull-down configuration, confirm the board's schematic or datasheet verifies GPIO0 can be driven for I2S MCLK without affecting boot behavior. If not documented, consider documenting this constraint for users or moving MCLK to a non-strapping GPIO pin.
| pin_RS485_TX, | ||
| pin_RS485_RX, | ||
| pin_Dig_Input, // Digital Input pin type. May contains some protection circuit | ||
| pin_Reserved, | ||
| pin_count | ||
| }; |
There was a problem hiding this comment.
Enum value renumbering can break persisted configs; add migration or explicit values
Adding pin_RS485_TX/pin_RS485_RX/pin_Dig_Input and renaming boards will shift underlying enum integers. If usage / boardPreset are stored as ints in _state or persisted to flash, existing installs may silently map to the wrong pin types/boards after upgrade.
Suggested mitigation (pick one):
- Assign explicit numeric values to all enum entries (stable ABI), or
- Implement a migration step when loading state (old values → new values), including
pin_RS485→pin_RS485_TX/RXmapping andboard_MHCD0/board_MHCP4Nano→ new IDs.
Also applies to: 79-87
🤖 Prompt for AI Agents
In src/MoonBase/Modules/ModuleIO.h around lines 61-66 (and similarly for the
enum block at 79-87), the recent insertion/rename of pin/board enum members will
change their implicit integer values and break persisted configs; either assign
explicit numeric values to every enum entry to lock their ABI, or add a
migration path when loading persisted state that maps old integer values to the
new ones (e.g., pin_RS485 → pin_RS485_TX/pin_RS485_RX mapping and
board_MHCD0/board_MHCP4Nano → new IDs) and bump/record a state schema version;
implement the chosen approach consistently for both pin and board enums and
update any serialization/deserialization code and unit tests accordingly.
src/MoonBase/Modules/ModuleIO.h
Outdated
| } else if (boardID == board_MHCP4NanoV1) { // https://shop.myhome-control.de/ABC-WLED-ESP32-P4-Shield/HW10027 | ||
| object["maxPower"] = 100; // Assuming decent LED power!! | ||
| if (_state.data["jumper1"]) { // on | ||
| uint8_t ledPins[8] = {21, 20, 25, 5, 7, 23, 8, 27}; // 8 LED_PINS | ||
| uint8_t ledPins[6] = {21, 20, 25, 5, 23, 27}; // Always LED Pins independend from switches | ||
| for (int i = 0; i < sizeof(ledPins); i++) pinAssigner.assignPin(ledPins[i], pin_LED); | ||
| if (_state.data["jumper1"]) {// on | ||
| // per default used as LED Pins | ||
| pinAssigner.assignPin(3, pin_RS485); | ||
| pinAssigner.assignPin(4, pin_RS485); | ||
| pinAssigner.assignPin(22, pin_RS485); | ||
| pinAssigner.assignPin(24, pin_RS485); | ||
| pinAssigner.assignPin(2, pin_Exposed); | ||
| pinAssigner.assignPin(46, pin_Exposed); | ||
| pinAssigner.assignPin(47, pin_Exposed); | ||
| pinAssigner.assignPin(48, pin_Exposed); | ||
| } else { // off - default | ||
| uint8_t ledPins[16] = {21, 20, 25, 5, 7, 23, 8, 27, 3, 22, 24, 4, 46, 47, 2, 48}; // 16 LED_PINS | ||
| pinAssigner.assignPin(3, pin_RS485_TX); | ||
| pinAssigner.assignPin(4, pin_RS485_TX); | ||
| pinAssigner.assignPin(22, pin_RS485_TX); | ||
| pinAssigner.assignPin(24, pin_RS485_TX); | ||
| pinAssigner.assignPin(2, pin_Dig_Input); | ||
| pinAssigner.assignPin(46, pin_Dig_Input); | ||
| pinAssigner.assignPin(47, pin_Dig_Input); | ||
| pinAssigner.assignPin(48, pin_Dig_Input); | ||
| } else { // off - default | ||
| uint8_t ledPins[8] = {3, 22, 24, 4, 46, 47, 2, 48}; // 8 LED_PINS | ||
| for (int i = 0; i < sizeof(ledPins); i++) pinAssigner.assignPin(ledPins[i], pin_LED); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full context around lines 398-432 in ModuleIO.h
wc -l src/MoonBase/Modules/ModuleIO.hRepository: MoonModules/MoonLight
Length of output: 100
🏁 Script executed:
# Get the relevant section with context
sed -n '380,440p' src/MoonBase/Modules/ModuleIO.hRepository: MoonModules/MoonLight
Length of output: 3310
🏁 Script executed:
# Search for pin_RS485_TX and pin_RS485_RX definitions
rg "pin_RS485_T[XR]|pin_RS485_RX" -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 1048
🏁 Script executed:
# Search for all usages of pin_RS485_TX and pin_RS485_RX to understand the pattern
rg "pin_RS485" -B 1 -A 1Repository: MoonModules/MoonLight
Length of output: 799
🏁 Script executed:
# Check if there are other boards using RS485 to understand the pattern
rg "pin_RS485_TX|pin_RS485_RX" -B 5 -A 5Repository: MoonModules/MoonLight
Length of output: 1911
🏁 Script executed:
# Look for MHCP4NanoV1 or board_MHCP4NanoV1 references
rg "MHCP4NanoV1|HW10027" -B 3 -A 10Repository: MoonModules/MoonLight
Length of output: 8325
🏁 Script executed:
# Check if there's any downstream code that handles or validates pin_RS485_TX/RX assignments
rg "pin_RS485_TX|pin_RS485_RX" -B 3 -A 3 --type cpp --type hRepository: MoonModules/MoonLight
Length of output: 1348
🏁 Script executed:
# Look for any comments or usage in other files that might explain the intent
find . -type f \( -name "*.cpp" -o -name "*.h" -o -name "*.md" \) -exec grep -l "RS.485\|RS-485\|rs485" {} \;Repository: MoonModules/MoonLight
Length of output: 129
🏁 Script executed:
# Check git history to see when pin_RS485_TX/RX were introduced
git log --oneline -S "pin_RS485_TX" -- src/MoonBase/Modules/ModuleIO.h | head -20Repository: MoonModules/MoonLight
Length of output: 97
🏁 Script executed:
# Get the exact commit to understand what changed
git show --stat ec39c78Repository: MoonModules/MoonLight
Length of output: 28220
🏁 Script executed:
# Check if there are any UART or serial configurations that might explain multiple TX pins
rg "UART|Serial|uart|serial" src/MoonBase/Modules/ModuleIO.h -B 2 -A 2 | head -40Repository: MoonModules/MoonLight
Length of output: 1199
Incomplete RS-485 pin configuration: only TX assigned, RX missing
The board_MHCP4NanoV1 jumper1 branch assigns 4 GPIOs (3, 4, 22, 24) to pin_RS485_TX but provides no pin_RS485_RX assignments. Given that distinct pin_RS485_TX and pin_RS485_RX types exist and other boards correctly pair TX/RX pins (e.g., board_TroyP4Nano), this inconsistency suggests incomplete configuration.
Either assign RX pins to match the TX assignments, or clarify whether these pins represent a shared RS-485 bus connector group and rename the types accordingly.
Also applies to: 416-432
🤖 Prompt for AI Agents
In src/MoonBase/Modules/ModuleIO.h around lines 398-415 (and similarly for lines
416-432), the RS-485 GPIOs are only being assigned as pin_RS485_TX (TX) while RX
assignments are missing; update the configuration to either assign corresponding
RX pins for the same GPIOs (call pinAssigner.assignPin(<pin>, pin_RS485_RX>) for
the matching pins) or, if these pins are intentionally a shared bus, change the
pin type to a shared RS-485 type consistently (and update other boards to
match). Ensure each physical RS-485 connector pin used for receive is explicitly
registered as pin_RS485_RX wherever TX pins are registered.
Docs ==== - Inputoutput: describe switch 1 and 2 - ModuleIO: Format document + jumper/uselinein to switch 1 and 2 (board independent), board_MHCV43: remove double entry, board_MHCV43: order of LED pins
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/MoonBase/Modules/ModuleIO.h (4)
79-87: [Persisting Issue] Board enum renumbering will break persisted board selectionsSame issue as the pin enums: renaming
board_MHCD0→board_MHCV43andboard_MHCP4Nano→board_MHCP4NanoV1shifts all subsequent board enum values, corrupting persistedboardPresetdata on upgrade.
61-66: [Persisting Issue] Enum renumbering will break persisted configurationsThis issue was flagged in a previous review and remains unaddressed. Adding
pin_RS485_TX,pin_RS485_RX, andpin_Dig_Inputshifts all subsequent enum values, which will corrupt persisted pin usage data when users upgrade.
383-392: [Persisting Issue] Non-idiomatic sizeof() loop boundsA previous review flagged these loops as critical issues and provided explicit fixes using range-based for loops. While the code happens to work for
uint8_tarrays (wheresizeof(array)equals element count), it's fragile and non-idiomatic.Apply the previously suggested fix:
- uint8_t ledPins[] = {12, 13, 16, 18}; // 4 LED_PINS - for (int i = 0; i < sizeof(ledPins); i++) pinAssigner.assignPin(ledPins[i], pin_LED); + uint8_t ledPins[] = {12, 13, 16, 18}; // 4 LED_PINS + for (uint8_t gpio : ledPins) pinAssigner.assignPin(gpio, pin_LED); pinAssigner.assignPin(32, pin_I2S_SD); @@ -15,7 +15,7 @@ pinAssigner.assignPin(0, pin_I2S_MCLK); - uint8_t exposedPins[] = {4, 5, 17, 19, 21, 22, 23, 25, 26, 27, 33}; - for (int i = 0; i < sizeof(exposedPins); i++) pinAssigner.assignPin(exposedPins[i], pin_Exposed); // Ethernet Pins + uint8_t exposedPins[] = {4, 5, 17, 19, 21, 22, 23, 25, 26, 27, 33}; + for (uint8_t gpio : exposedPins) pinAssigner.assignPin(gpio, pin_Exposed);
396-410: [Persisting Issues] sizeof() loops and incomplete RS-485 configurationTwo critical issues from previous reviews remain unaddressed:
sizeof() loop bounds: Lines 398 and 409 use
sizeof(ledPins)which happens to work foruint8_tbut is fragile.Missing RS-485 RX pins: Lines 399-402 assign 4 GPIOs to
pin_RS485_TXbut provide nopin_RS485_RXassignments. RS-485 requires both TX and RX for bidirectional communication.Apply the previously suggested fixes:
if (_state.data["switch1"]) { // on - uint8_t ledPins[] = {21, 20, 25, 5, 7, 23, 8, 27}; // 8 LED pins in this order - for (int i = 0; i < sizeof(ledPins); i++) pinAssigner.assignPin(ledPins[i], pin_LED); + uint8_t ledPins[] = {21, 20, 25, 5, 7, 23, 8, 27}; // 8 LED pins + for (uint8_t gpio : ledPins) pinAssigner.assignPin(gpio, pin_LED); pinAssigner.assignPin(3, pin_RS485_TX); + pinAssigner.assignPin(3, pin_RS485_RX); // Add RX assignment pinAssigner.assignPin(4, pin_RS485_TX); + pinAssigner.assignPin(4, pin_RS485_RX); // Add RX assignment pinAssigner.assignPin(22, pin_RS485_TX); + pinAssigner.assignPin(22, pin_RS485_RX); // Add RX assignment pinAssigner.assignPin(24, pin_RS485_TX); + pinAssigner.assignPin(24, pin_RS485_RX); // Add RX assignment pinAssigner.assignPin(2, pin_Dig_Input); @@ -410,7 +418,7 @@ } else { // off - default - uint8_t ledPins[] = {21, 20, 25, 5, 7, 23, 8, 27, 3, 22, 24, 4, 46, 47, 2, 48}; // 16 LED_PINS in this order - for (int i = 0; i < sizeof(ledPins); i++) pinAssigner.assignPin(ledPins[i], pin_LED); + uint8_t ledPins[] = {21, 20, 25, 5, 7, 23, 8, 27, 3, 22, 24, 4, 46, 47, 2, 48}; // 16 LED_PINS + for (uint8_t gpio : ledPins) pinAssigner.assignPin(gpio, pin_LED); }Note: If RS-485 pins are half-duplex and don't need separate RX assignments, clarify this in comments and consider renaming the pin type to reflect shared usage.
🧹 Nitpick comments (1)
docs/moonbase/inputoutput.md (1)
78-83: Fix grammar and markdown indentation for better readabilityStatic analysis identified minor issues:
- Line 78: "set also the switches" should be "also set the switches"
- Lines 78-83: Inconsistent list indentation (expected 2/4 spaces, actual 4/8)
Apply this diff to fix the grammar and indentation:
- * Switch1: (set also the switches on the board) + * Switch1: (also set the switches on the board) * off (default): 16 LED pins. * On: 8 LED pins, 4 RS-485 pins and 4 exposed pins * Switch2: * off (default): Pins 10, 11, 12, 13 used for build-in Mic over I2S, pin 7 and 8: I2C SDA and SCL * On: Pins 33, 25, 32, 36 used for Line in, pin 7 and 8: additional LED pins.For the markdown indentation, adjust the nested list items to use 2-space increments instead of 4-space increments per the MD007 linter rule.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/moonbase/inputoutput.md(3 hunks)src/MoonBase/Modules/ModuleIO.h(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonBase/Modules/ModuleIO.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
🪛 LanguageTool
docs/moonbase/inputoutput.md
[grammar] ~78-~78: Ensure spelling is correct
Context: ...100W LED power; change if needed. * Switch1: (set also the switches on the board) ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~78-~78: To make your writing flow more naturally, try moving ‘also’ before the verb.
Context: ...ower; change if needed. * Switch1: (set also the switches on the board) * of...
(ALSO_PLACEMENT)
[grammar] ~80-~80: Ensure spelling is correct
Context: ...D pins, 4 RS-485 pins and 4 exposed pins * Switch2: * off (default): Pins 10, 11, 12, 13 used for...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/moonbase/inputoutput.md
78-78: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
81-81: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
82-82: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
83-83: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
🔇 Additional comments (2)
src/MoonBase/Modules/ModuleIO.h (2)
412-424: LGTM: Switch2 I2S configuration logic is correctThe Line-In vs Mic pin configuration is properly implemented with clear conditional logic. The default (switch2 = false) correctly configures the built-in Mic pins.
500-503: LGTM: Switch update handler correctly triggers preset rebuildThe logic appropriately rebuilds the board preset when switch1 or switch2 changes, maintaining consistency with the existing pattern.
| control = addControl(controls, "switch1", "checkbox"); | ||
| control["default"] = false; | ||
|
|
||
| control = addControl(controls, "switch2", "checkbox"); | ||
| control["default"] = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add descriptive labels for switch controls to improve UX
The controls are named "switch1" and "switch2" without descriptions. Based on the PR objectives mentioning "UseLineIn" and the documentation showing these control RS-485/LED vs Line-In/Mic configurations, users need clear labels.
Apply this diff to add meaningful descriptions:
- control = addControl(controls, "switch1", "checkbox");
+ control = addControl(controls, "switch1", "checkbox", 0, UINT8_MAX, false, "RS-485 mode");
control["default"] = false;
- control = addControl(controls, "switch2", "checkbox");
+ control = addControl(controls, "switch2", "checkbox", 0, UINT8_MAX, false, "Use Line-In");
control["default"] = false;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MoonBase/Modules/ModuleIO.h around lines 132 to 136, the controls are
created as "switch1" and "switch2" with no descriptive labels; update the
control creation to provide clear, user-facing descriptions such as a
label/description property indicating their function (e.g., one control for
"UseLineIn (Line-In vs Mic)" and the other for "RS-485/LED mode selection"),
keeping the controls' types as "checkbox" and preserving the default value
false; ensure the descriptive text matches project terminology (UseLineIn /
RS-485 vs Line-In) so UI shows meaningful labels instead of "switch1"/"switch2".
|
I had pins 7 and 8 defined depending on UseLineIn switch (now as you changed it is switch2). The reason is that if the P4 nano build-In micro is used, these pins must be used as I2C bus to configure the build-in decoder. If the Line-In on the Shield is used, it does not require I2C configuration, then these pins 7 and 8 are free to be used as LED pins. |
2 participants
You can cherry-pick some parts and discard those that do not fit to your idea how it should be done.
The following changes proposed:
Summary by CodeRabbit
New Features
Updates
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.