moving pin high and low out of the define, handling low level rs485 #91
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
RS485 support & pin parsing src/MoonBase/Modules/ModuleIO.h |
Added #include for UART driver; replaced Serial_TX/Serial_RX board preset names with RS485_TX/RS485_RX; introduced pinRS485TX, pinRS485RX, pinRS485DE (initialized to UINT8_MAX); extended readPins() to detect RS485 pins; when all three pins present, initialize UART1 for RS485 half‑duplex, install driver, set params, enable RS485 mode, and run an optional Modbus‑style demo exchange with logging. |
Layout exposure src/MoonLight/Modules/ModuleDrivers.h |
Added SE16V2Layout to the layouts exposed via addNodes() and attempted allocation path mirrors SE16Layout allocation (attempt to instantiate SE16V2Layout when appropriate). |
New layout implementation src/MoonLight/Nodes/Layouts/L_SE16.h |
Added SE16V2Layout class (inherits Node) with static accessors, config/state members (mirroredPins, pinsAreColumns, ledsPerPin), setup() to expose controls, addStrip() helper, and layout logic (hasOnLayout()/onLayout()) mirroring SE16 behavior with per‑strip mapping. |
Project manifest platformio.ini |
(No functional change reported; listed in diff manifest.) |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant ModuleIO
participant GPIO_Scan as GPIO Scan
participant UART1 as UART1 Driver
participant RS485_Device as RS485 Device
ModuleIO->>GPIO_Scan: readPins() -> detect RS485_TX/RX/DE
GPIO_Scan-->>ModuleIO: return pinRS485TX / pinRS485RX / pinRS485DE
alt All RS485 pins present
ModuleIO->>UART1: install & configure UART1 (baud/parity)
ModuleIO->>UART1: set TX/RX pins and DE control (half‑duplex)
UART1-->>ModuleIO: UART1 ready
opt Demo test enabled
ModuleIO->>UART1: send Modbus‑like request frame
UART1->>RS485_Device: transmit (driven by DE)
RS485_Device-->>UART1: response frame
UART1-->>ModuleIO: deliver response for parsing/log
end
else Missing one or more pins
ModuleIO->>ModuleIO: skip RS485 init
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Board presets p4 #74 — modifies
ModuleIO.hboard presets and pin-mapping logic (overlaps RS485/serial naming changes). - Move from main to module IO #90 — touches
ModuleIO.hpin discovery/setup and runtime pin initialization logic. - SE16V2 / Lightcrafter16 board support #86 — updates
ModuleIO.haround RS485 pin names and board initialization; overlaps RS485-related changes.
Poem
🐰 I hop on copper, nibble code at night,
Three pins now whisper, RS485 lights,
UART1 twinkles, half‑duplex tune,
Strips and layouts mirror the moon,
A rabbit hums — firmware feels right!
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| 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. |
|
| Title check | ❓ Inconclusive | The title mentions 'handling low level rs485' which directly relates to the RS485 initialization and communication logic added in ModuleIO.h, but omits the significant SE16V2Layout addition and doesn't clearly convey the main scope of changes. | Clarify whether the RS485 changes or the SE16V2Layout addition is the primary focus, and consider a more comprehensive title that reflects all major changes. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
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: |
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.