moving pin high and low out of the define, handling low level rs485 #91
Conversation
|
Warning Rate limit exceeded@limpkin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note
|
| Cohort / File(s) | Summary |
|---|---|
Module IO & RS485 src/MoonBase/Modules/ModuleIO.h |
Added #include for UART driver; new pinRS485TX/RX/DE trackers (init UINT8_MAX); extended readPins() to parse RS485_DE/RX/TX; when all three present, install/configure UART1 for RS‑485 half‑duplex, assign pins, and run an optional Modbus‑style test/log exchange. |
Board enum / preset mapping src/MoonBase/Modules/ModuleIO.h |
Renamed enum entry board_SE16V2 → board_LightCrafter16 and switched board preset logic to use board_LightCrafter16 where appropriate. |
Driver exposure & allocation src/MoonLight/Modules/ModuleDrivers.h |
Gate exposure and allocation of layouts on board preset: expose/instantiate LightCrafter16Layout for board_LightCrafter16 and SE16Layout for board_SE16V1 (conditional allocation/exposure). |
New layout implementation src/MoonLight/Nodes/Layouts/L_SE16.h |
Added LightCrafter16Layout class (inherits Node) with static accessors, config/state (pinsAreColumns, ledsPerPin), setup() controls, addStrip(), and hasOnLayout()/onLayout() to build per-strip LED mapping (SE16-like traversal). |
Documentation updates docs/moonbase/inputoutput.md, docs/moonlight/layouts.md |
Added RS‑485 GPIO usage notes and new LightCrafter16 board/layout docs/images; adjusted SE16 docs (image markup and vendor name), removed DMX from planned list. |
Misc / manifest platformio.ini (metadata) |
No functional code changes reported in diff metadata. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant ModuleIO
participant GPIO_Scan as GPIO Scan
participant UART1 as UART1 Driver
participant RS485_Device as RS‑485 Device
ModuleIO->>GPIO_Scan: readPins() → detect RS485_DE / RS485_RX / RS485_TX
GPIO_Scan-->>ModuleIO: return pinRS485DE / pinRS485RX / pinRS485TX
alt All RS485 pins present
ModuleIO->>UART1: install driver & configure (baud/parity, half‑duplex)
ModuleIO->>UART1: assign TX/RX pins and DE control GPIO
UART1-->>ModuleIO: ready
opt Demo test enabled
ModuleIO->>UART1: send Modbus‑like request (DE asserted)
UART1->>RS485_Device: transmit
RS485_Device-->>UART1: response
UART1-->>ModuleIO: deliver response → log/parse
end
else Missing pins
ModuleIO->>ModuleIO: skip RS485 init
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- SE16V2 / Lightcrafter16 board support #86 — overlaps ModuleIO RS‑485 pin renaming, board preset mapping, and UART initialization changes.
- Update board definitions #73 — related changes to ModuleIO pin usage splitting RS‑485 TX/RX and board preset handling.
- v0.7.0 preparations #76 — edits to ModuleIO board presets and IO parsing that intersect this PR’s changes.
Poem
🐰 I hop on copper, nibble code at night,
Three pins now whisper, RS‑485 takes flight,
UART1 winks in half‑duplex tune,
LightCrafter rows sparkle under moon,
A rabbit cheers — firmware glows bright!
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 18.18% 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 partially addresses the changeset by referencing RS485 handling, but omits the significant addition of LightCrafter16 board support and layout, which is a major aspect of this changeset. |
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
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonBase/Modules/ModuleIO.h
🔇 Additional comments (3)
src/MoonBase/Modules/ModuleIO.h (3)
18-18: LGTM!The UART driver include is correctly added to support the RS485 functionality.
320-322: LGTM!Pin mappings correctly updated to RS485 usage for the SE16V2 board configuration.
606-608: LGTM!Member variable declarations are consistent with the existing pattern for other pin tracking variables in the class.
src/MoonBase/Modules/ModuleIO.h
Outdated
| ESP_LOGI(ML_TAG, "Request sent"); | ||
|
|
||
| // Wait for response (timeout 1 second) | ||
| int len = uart_read_bytes(UART_NUM_1, response, 128, pdMS_TO_TICKS(100)); | ||
|
|
||
| if (len > 0) { | ||
| ESP_LOGI(ML_TAG, "Answer recceived: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); |
There was a problem hiding this comment.
Minor: Typo and inconsistent logging macros.
- Line 731, 737, 740, 743: Uses
ESP_LOGI/ESP_LOGWwhile the rest of the file usesEXT_LOGD/EXT_LOGV. - Line 737: Typo "recceived" → "received".
🔎 Proposed fix
uart_write_bytes(UART_NUM_1, (const char*)request, sizeof(request));
- ESP_LOGI(ML_TAG, "Request sent");
+ EXT_LOGD(ML_TAG, "Request sent");
// Wait for response (timeout 1 second)
int len = uart_read_bytes(UART_NUM_1, response, 128, pdMS_TO_TICKS(100));
if (len > 0) {
- ESP_LOGI(ML_TAG, "Answer recceived: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]);
+ EXT_LOGD(ML_TAG, "Answer received: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]);
float humidity = ((float)response[3])*256 + (float)response[4];
float temperature = ((float)response[5])*256 +(float)response[6];
- ESP_LOGI(ML_TAG, "humidity: %f temperature: %f", humidity/10, temperature/10);
+ EXT_LOGD(ML_TAG, "humidity: %f temperature: %f", humidity/10, temperature/10);
// Process registers here (response[3] to response[6] contain the data)
} else {
- ESP_LOGW(ML_TAG, "No response from sensor");
+ EXT_LOGW(ML_TAG, "No response from sensor");
} 📝 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.
| ESP_LOGI(ML_TAG, "Request sent"); | |
| // Wait for response (timeout 1 second) | |
| int len = uart_read_bytes(UART_NUM_1, response, 128, pdMS_TO_TICKS(100)); | |
| if (len > 0) { | |
| ESP_LOGI(ML_TAG, "Answer recceived: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); | |
| EXT_LOGD(ML_TAG, "Request sent"); | |
| // Wait for response (timeout 1 second) | |
| int len = uart_read_bytes(UART_NUM_1, response, 128, pdMS_TO_TICKS(100)); | |
| if (len > 0) { | |
| EXT_LOGD(ML_TAG, "Answer received: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); | |
| float humidity = ((float)response[3])*256 + (float)response[4]; | |
| float temperature = ((float)response[5])*256 +(float)response[6]; | |
| EXT_LOGD(ML_TAG, "humidity: %f temperature: %f", humidity/10, temperature/10); | |
| // Process registers here (response[3] to response[6] contain the data) | |
| } else { | |
| EXT_LOGW(ML_TAG, "No response from sensor"); | |
| } |
🤖 Prompt for AI Agents
In src/MoonBase/Modules/ModuleIO.h around lines 731-737, the logging calls use
ESP_LOGI/ESP_LOGW and contain a typo "recceived"; change those calls to the
file's standard EXT_LOGD/EXT_LOGV variants and correct the message to
"received". Specifically replace the ESP_LOGI that logs "Request sent" and the
response print call with the matching EXT_LOGD/EXT_LOGV macros used elsewhere,
and update the format string text to "Answer received" (keeping the existing
byte-format arguments intact).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/MoonBase/Modules/ModuleIO.h (2)
725-748: Minor: Fix typo in log message.Line 738 contains a typo: "recceived" should be "received".
🔎 Proposed fix
- EXT_LOGD(ML_TAG, "Answer recceived: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); + EXT_LOGD(ML_TAG, "Answer received: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]);
706-724: Minor: Check if UART driver is installed before deletion.The code calls
uart_driver_delete(UART_NUM_1)unconditionally on line 719. While this works in practice (returnsESP_FAILif not installed, and execution continues), it's not explicit error handling. Best practice is to check if the driver is installed first.🔎 Recommended fix
// Check if all RS485 pins are specified if (rs485_ios_updated && (pinRS485TX != UINT8_MAX) && (pinRS485RX != UINT8_MAX) && (pinRS485DE != UINT8_MAX)) { EXT_LOGD(ML_TAG, "RS485 init with pins %d %d %d", pinRS485TX, pinRS485RX, pinRS485DE); // test code to be replaced with functional code. use UART1 as UART0 is (AFAIK) always for debug serial uart_config_t uart_config = { .baud_rate = 9600, .data_bits = UART_DATA_8_BITS, .parity = UART_PARITY_DISABLE, .stop_bits = UART_STOP_BITS_1, .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, // Flow control handled by RS485 driver .source_clk = UART_SCLK_DEFAULT, }; - uart_driver_delete(UART_NUM_1); + if (uart_is_driver_installed(UART_NUM_1)) { + ESP_ERROR_CHECK(uart_driver_delete(UART_NUM_1)); + } ESP_ERROR_CHECK(uart_driver_install(UART_NUM_1, 128 * 2, 0, 0, NULL, 0));Based on learnings, this follows ESP32 development best practices for explicit driver state management.
🧹 Nitpick comments (1)
src/MoonBase/Modules/ModuleIO.h (1)
737-737: Optional: Consider usinglen >= 9for clarity.The condition
len > 8is functionally correct butlen >= 9would be more explicit about the minimum required bytes for a valid Modbus response (address + function + byte_count + data[4] + crc[2] = 9 bytes).🔎 Proposed change
- if (len > 8) { + if (len >= 9) { // Modbus response: addr(1) + func(1) + byte_count(1) + data(4) + crc(2)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonBase/Modules/ModuleIO.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonBase/Modules/ModuleIO.h
🔇 Additional comments (3)
src/MoonBase/Modules/ModuleIO.h (3)
18-18: LGTM!The UART driver header is required for the RS485 functionality and is properly included.
320-322: LGTM!The pin assignments correctly reflect RS485 usage instead of generic serial, and include the necessary direction enable (DE) pin for RS485 half-duplex operation.
676-704: LGTM with prerequisite fix.The GPIO and RS485 pin parsing logic is well-structured. The handling of
pin_Highandpin_Lowcorrectly validates GPIO capability before configuring. The RS485 pin detection properly sets the flag and member variables. However, this depends on the critical fix above (resetting RS485 variables before parsing).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/MoonBase/Modules/ModuleIO.h (1)
735-739: Minor: Comment/code mismatch and typo.
- Line 735 comment says "timeout 1 second" but
pdMS_TO_TICKS(100)is 100ms.- Line 739 has typo "recceived" → "received".
🔎 Proposed fix
- // Wait for response (timeout 1 second) + // Wait for response (timeout 100ms) int len = uart_read_bytes(UART_NUM_1, response, 128, pdMS_TO_TICKS(100)); if (len > 8) { - EXT_LOGD(ML_TAG, "Answer recceived: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); + EXT_LOGD(ML_TAG, "Answer received: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]);
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Layouts/L_SE16.h (2)
90-90: UnusedmirroredPinsmember variable.The
mirroredPinsmember is declared but never used—the control is commented out insetup()andonLayout()doesn't reference it. Consider removing it to avoid confusion, or uncomment the control if the feature is intended.🔎 Proposed fix
- bool mirroredPins = false; bool pinsAreColumns = false; uint16_t ledsPerPin = 10;
100-110: Consider extracting sharedaddStriplogic.The
addStrip()implementation is identical toSE16Layout::addStrip(). If more SE16 variants are expected, consider extracting this to a common base class to reduce duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/MoonBase/Modules/ModuleIO.hsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Nodes/Layouts/L_SE16.h
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Applied to files:
src/MoonBase/Modules/ModuleIO.h
🧬 Code graph analysis (2)
src/MoonBase/Modules/ModuleIO.h (1)
src/MoonLight/Modules/ModuleDrivers.h (1)
readPins(34-68)
src/MoonLight/Modules/ModuleDrivers.h (1)
src/MoonLight/Nodes/Layouts/L_SE16.h (1)
SE16V2Layout(84-131)
🔇 Additional comments (8)
src/MoonLight/Nodes/Layouts/L_SE16.h (1)
112-130: LGTM!The layout logic correctly implements a 16-strip configuration with serpentine traversal for the SE16V2 board.
src/MoonLight/Modules/ModuleDrivers.h (2)
104-104: LGTM!Properly exposes the new
SE16V2Layoutin the layout selection dropdown.
134-134: LGTM!Correctly adds
SE16V2Layoutto the node allocation chain following the existing pattern.src/MoonBase/Modules/ModuleIO.h (5)
18-18: LGTM!Proper inclusion of ESP-IDF UART driver header for RS485 functionality.
320-322: LGTM!Correctly renames the pin assignments to reflect their RS485 purpose, aligning with the new RS485 initialization logic.
607-611: Good: RS485 pins are now local variables.This addresses the previous concern about stale values—pins are properly reset on each
readPins()call. Note that if you later need to track whether the UART has already been configured with the same pins (to avoid unnecessary reconfiguration), you'll need to promote these to member variables with explicit reset logic.
707-724: LGTM! Previous UART reinstallation issue addressed.Calling
uart_driver_delete()beforeuart_driver_install()correctly prevents the crash on subsequentreadPins()calls. Theuart_driver_delete()gracefully handles the case where no driver is installed.
726-749: Demo code looks reasonable for testing.The Modbus RTU request/response handling follows the expected protocol format. Since this is test code behind a preprocessor guard, the implementation is acceptable for validation purposes.
|
All looks fine, Could you add a commit to update the documentation:
|
|
Could you also add this in ModuleDrivers.h, so board specific layouts are only shown if layout is chosen: addNodes: addNode: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/MoonBase/Modules/ModuleIO.h (1)
726-750: Fix typo in demo code log message.Line 739 contains a typo: "recceived" should be "received".
Additionally, while
len > 8is functionally correct for validating a 9-byte Modbus response, usinglen >= 9would be more explicit and align with the Modbus RTU response structure (addr + func + byte_count + 4 data bytes + 2 CRC bytes).🔎 Proposed fix
- if (len > 8) { - EXT_LOGD(ML_TAG, "Answer recceived: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); + if (len >= 9) { // Modbus response: addr(1) + func(1) + byte_count(1) + data(4) + crc(2) = 9 bytes + EXT_LOGD(ML_TAG, "Answer received: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); float humidity = ((float)response[3])*256 + (float)response[4]; float temperature = ((float)response[5])*256 +(float)response[6]; EXT_LOGD(ML_TAG, "humidity: %f temperature: %f", humidity/10, temperature/10);
🧹 Nitpick comments (1)
docs/moonlight/layouts.md (1)
48-55: Consider hyphenating "16 channel" for clarity.The documentation is clear and correctly describes the LightCrafter16 layout. However, "16 channel LED strip driver" should be hyphenated as "16-channel LED strip driver" per standard English grammar conventions for compound modifiers.
🔎 Proposed fix
### LightCrafter16 -16 channel LED strip driver by Stephan Electronics +16-channel LED strip driver by Stephan ElectronicsAs per static analysis hints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
firmware/installer/images/esp32-s3-lightcrafter16.jpgis excluded by!**/*.jpgfirmware/installer/images/esp32-s3-stephanelec-16p.jpgis excluded by!**/*.jpg
📒 Files selected for processing (5)
docs/moonbase/inputoutput.mddocs/moonlight/layouts.mdsrc/MoonBase/Modules/ModuleIO.hsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Nodes/Layouts/L_SE16.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Nodes/Layouts/L_SE16.h
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Applied to files:
src/MoonBase/Modules/ModuleIO.h
🧬 Code graph analysis (2)
src/MoonLight/Modules/ModuleDrivers.h (2)
src/MoonBase/Module.h (1)
ModuleState(47-137)src/MoonLight/Nodes/Layouts/L_SE16.h (2)
SE16Layout(15-81)LightCrafter16Layout(84-129)
src/MoonBase/Modules/ModuleIO.h (1)
src/MoonLight/Modules/ModuleDrivers.h (1)
readPins(34-68)
🪛 LanguageTool
docs/moonlight/layouts.md
[grammar] ~40-~40: Use a hyphen to join words.
Context: ...ine into a larger fixture. ### SE16 16 channel LED strip driver by Stephan Elec...
(QB_NEW_EN_HYPHEN)
[grammar] ~50-~50: Use a hyphen to join words.
Context: ...th) = ledsPerPin ### LightCrafter16 16 channel LED strip driver by Stephan Elec...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (13)
src/MoonLight/Modules/ModuleDrivers.h (2)
102-107: LGTM! Board-specific layout exposure implemented correctly.The conditional exposure of SE16Layout and LightCrafter16Layout based on board preset aligns with the PR objectives and follows the requested pattern from the PR comments.
135-140: LGTM! Board-specific layout allocation implemented correctly.The conditional allocation logic mirrors the exposure pattern and correctly uses the existing
checkAndAllocpattern for node instantiation.docs/moonlight/layouts.md (2)
29-29: Documentation added for LightCrafter16 layout.The new table entry correctly documents the LightCrafter16 layout. Note that it currently uses the same preview image as SE16 - verify this is intentional or update with board-specific imagery when available.
40-47: LGTM! Vendor name corrected and image format standardized.The correction from "Stephane" to "Stephan" Electronics and the switch to Markdown image syntax improve documentation accuracy and consistency.
docs/moonbase/inputoutput.md (3)
36-37: LGTM! Pin usage documentation added for new GPIO and RS-485 functionality.The documentation clearly describes the new High/Low GPIO control and RS-485 pin assignments, aligning with the code changes in ModuleIO.h.
90-94: LGTM! SE16 v1 documentation enhanced with availability and community info.The added context about limited production (5 boards) and Discord community contact improves user guidance and aligns with the requested documentation improvements from PR comments.
97-102: LGTM! LightCrafter16 board documentation added.The new section provides essential setup information for the LightCrafter16 board and correctly references the installer board selection and future documentation location.
src/MoonBase/Modules/ModuleIO.h (6)
18-18: LGTM! UART driver header added for RS485 support.The inclusion of
driver/uart.his necessary for the RS485/UART functionality implemented in this PR.
84-84: LGTM! Board enum updated to board_LightCrafter16.The enum name change correctly reflects the board rebranding and aligns with documentation and layout changes across the codebase.
313-331: LGTM! LightCrafter16 board preset configuration is comprehensive.The board configuration correctly sets up LED pins, RS485 communication pins, voltage/current monitoring, SPI ethernet interface, and other peripherals. The explicit GPIO initialization for pin 3 is well-documented with comments explaining its necessity for ethernet module initialization.
608-610: LGTM! RS485 pin variables now properly scoped and initialized.Moving the RS485 pin variables to local scope within
readPins()correctly addresses the previous concern about stale values persisting across calls. This follows the same pattern used elsewhere in the codebase for pin variables.
677-705: LGTM! GPIO and RS485 pin detection logic is clear and correct.The loop properly handles:
- Immediate configuration of High/Low GPIO pins with appropriate direction and level
- Detection and recording of RS485 pins for subsequent UART initialization
- Flag tracking to enable conditional UART setup
707-725: LGTM! RS485 UART initialization correctly handles driver reinstallation.The UART initialization properly addresses the previous critical issue by calling
uart_driver_delete(UART_NUM_1)beforeuart_driver_install(), preventing program aborts fromESP_ERR_INVALID_STATE. The RS485 half-duplex configuration and pin assignments are correct.✅ Addresses previous review concern about UART driver reinstallation crash.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/MoonBase/Modules/ModuleIO.h (1)
720-724: Consider checking driver installation state before delete.While calling
uart_driver_delete()beforeuart_driver_install()prevents the reinstallation crash,uart_driver_delete()returnsESP_ERR_INVALID_STATEif the driver isn't installed. This is benign since there's noESP_ERROR_CHECK, but for cleaner behavior:🔎 Proposed improvement
// Check if all RS485 pins are specified if (rs485_ios_updated && (pinRS485TX != UINT8_MAX) && (pinRS485RX != UINT8_MAX) && (pinRS485DE != UINT8_MAX)) { EXT_LOGD(ML_TAG, "RS485 init with pins %d %d %d", pinRS485TX, pinRS485RX, pinRS485DE); // test code to be replaced with functional code. use UART1 as UART0 is (AFAIK) always for debug serial uart_config_t uart_config = { .baud_rate = 9600, .data_bits = UART_DATA_8_BITS, .parity = UART_PARITY_DISABLE, .stop_bits = UART_STOP_BITS_1, .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, // Flow control handled by RS485 driver .source_clk = UART_SCLK_DEFAULT, }; - uart_driver_delete(UART_NUM_1); + if (uart_is_driver_installed(UART_NUM_1)) { + uart_driver_delete(UART_NUM_1); + } ESP_ERROR_CHECK(uart_driver_install(UART_NUM_1, 128 * 2, 0, 0, NULL, 0));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/moonlight/layouts.mdsrc/MoonBase/Modules/ModuleIO.h
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
src/MoonBase/Modules/ModuleIO.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Applied to files:
src/MoonBase/Modules/ModuleIO.h
🧬 Code graph analysis (1)
src/MoonBase/Modules/ModuleIO.h (2)
src/MoonLight/Modules/ModuleLightsControl.h (1)
readPins(211-527)src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
readPins(40-424)
🪛 LanguageTool
docs/moonlight/layouts.md
[grammar] ~50-~50: Use a hyphen to join words.
Context: ...th) = ledsPerPin ### LightCrafter16 16 channel LED strip driver by Stephan Elec...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/layouts.md
29-29: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
🔇 Additional comments (8)
src/MoonBase/Modules/ModuleIO.h (6)
18-18: LGTM!The
driver/uart.hinclude is correctly added to support the new RS485 UART functionality.
84-84: Breaking change: Board enum value renamed.The rename from
board_SE16V2toboard_LightCrafter16will break any persisted configurations or code referencing the old enum name. Ensure this is intentional and that users are aware existing board selections may need reconfiguration.
313-331: LGTM!The LightCrafter16 board preset is well-structured with appropriate pin assignments for RS485 (TX on 17, RX on 18, DE on 46) and other peripherals. The immediate GPIO configuration for pin 3 (WIZ850_nRST) with the explanatory comments is helpful for understanding the boot-time requirement.
608-610: LGTM - RS485 pins now use local scope.Making the RS485 pin variables local to
readPins()addresses the previous concern about stale values. Since these pins are only needed during UART configuration within this function, local scope is appropriate and eliminates the need for explicit reset logic.
677-705: LGTM!The RS485 pin parsing logic correctly iterates the pin configuration, sets the
rs485_ios_updatedflag when any RS485 pin is found, and stores the GPIO values for later UART configuration.
726-749: Demo code is well-guarded.The Modbus RTU demo code is properly wrapped in
#ifdef DEMOCODE_FOR_SHT30_SENSOR, ensuring it won't execute in production builds. The response length check (len > 8) and structured logging are appropriate.docs/moonlight/layouts.md (2)
29-29: LGTM!The LightCrafter16 row is correctly added to the layouts table with consistent formatting.
Note: The static analysis hint about table pipe style is a false positive—the existing table already uses leading and trailing pipes consistently.
40-42: LGTM - Vendor name corrected.The vendor name is now correctly spelled as "Stephan Electronics" and the image uses consistent Markdown syntax.
2 participants
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.