Conversation
YanKE01
force-pushed
the
feature/peripheral_rmt_simple
branch
from
fde874e to
b714409
Compare
WalkthroughAdds two new ESP-IDF RMT example projects (simple TX and WS2812 encoder) targeting ESP32-S3. Updates build_config.yml to include them. Each example includes CMake setup, sdkconfig.defaults, and a main.c implementing transmit flows using RMT with a bytes encoder and a callback-based WS2812 encoder. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as app_main (rmt_simple_tx)
participant RMT as RMT TX Channel
participant Enc as Bytes Encoder
participant GPIO as GPIO4
App->>RMT: rmt_new_tx_channel(cfg: 1 MHz, mem, q=4, gpio=4)
App->>Enc: rmt_new_bytes_encoder(bit0/bit1 timings)
App->>RMT: rmt_enable()
loop periodic
App->>RMT: rmt_transmit(Enc, data=0x05, loop_count=0)
RMT->>GPIO: Drive waveform per encoded bits
App-->App: vTaskDelay(500 ms)
end
sequenceDiagram
autonumber
participant App as app_main (ws2812_simple_encoder)
participant RMT as RMT TX Channel (10 MHz)
participant Enc as Callback Encoder
participant GPIO as GPIO38
App->>RMT: rmt_new_tx_channel(cfg: 10 MHz, mem, q=4, gpio=38)
App->>Enc: rmt_new_simple_encoder(callback)
App->>RMT: rmt_enable()
loop For each GRB frame (R, G, B)
App->>RMT: rmt_transmit(Enc, GRB bytes, loop_count=0)
Enc-->>RMT: encoder_callback emits WS2812 symbols (0/1/reset)
RMT->>GPIO: Output WS2812 waveform
App-->App: vTaskDelay(1 s)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
examples/peripherals/rmt/ws2812_simple_encoder/sdkconfig.defaults (2)
1-4: Optional: keep sdkconfig.defaults minimal unless you need the 240 MHz guarantee.If runtime performance at 240 MHz isn’t strictly required, consider omitting Line 4 to inherit the board’s default CPU frequency. This reduces divergence across boards and CI environments.
1-4: Document why the target is pinned in sdkconfig.defaults.A short README note that the example only targets ESP32‑S3 will preempt confusion when users try other targets via idf.py set-target.
I can add a concise README.md with wiring notes and the rationale for pinning the target—want me to draft it?
examples/peripherals/rmt/rmt_simple_tx/sdkconfig.defaults (1)
1-4: Optional: reduce config surface area.Same note as the other example—if 240 MHz isn’t necessary for this demo, consider dropping Line 4 to keep defaults minimal.
examples/peripherals/rmt/rmt_simple_tx/main/CMakeLists.txt (1)
1-2: Add explicit dependency on the driver component for RMT.RMT lives under the driver component in IDF v5.x. Declaring it avoids accidental link-order surprises and makes dependencies explicit.
Apply this diff:
idf_component_register(SRCS "main.c" - INCLUDE_DIRS ".") + INCLUDE_DIRS "." + PRIV_REQUIRES driver)examples/peripherals/rmt/ws2812_simple_encoder/main/CMakeLists.txt (2)
1-2: Declare dependency on driver for RMT/WS2812 encoder.Mirrors the rmt_simple_tx suggestion; keeps linking predictable and documents intent.
Apply this diff:
idf_component_register(SRCS "main.c" - INCLUDE_DIRS ".") + INCLUDE_DIRS "." + PRIV_REQUIRES driver)
1-2: Consider splitting public/private include dirs if headers are added later.If you introduce a local header that shouldn’t be consumed by other components, prefer PRIV_INCLUDE_DIRS over INCLUDE_DIRS to keep interfaces tight. Fine to keep as-is for a single-file example.
examples/peripherals/rmt/rmt_simple_tx/CMakeLists.txt (1)
3-6: Optional: constrain project language to C for marginally faster configure.These examples are C-only; declaring languages can shave a bit off CMake configure time.
Apply this diff:
-cmake_minimum_required(VERSION 3.16) +cmake_minimum_required(VERSION 3.16) @@ -include($ENV{IDF_PATH}/tools/cmake/project.cmake) -project(rmt_simple_tx) +include($ENV{IDF_PATH}/tools/cmake/project.cmake) +project(rmt_simple_tx LANGUAGES C)examples/peripherals/rmt/rmt_simple_tx/main/main.c (2)
10-16: Optional: Align mem_block_symbols style with the WS2812 example for consistency.Both 64 and SOC_RMT_MEM_WORDS_PER_CHANNEL are valid; choosing one consistently across examples helps readability. Given ws2812_simple_encoder uses 64 explicitly, consider mirroring it here.
- .mem_block_symbols = SOC_RMT_MEM_WORDS_PER_CHANNEL, + .mem_block_symbols = 64,
45-49: Optional: Block until TX completes to avoid queue backpressure in other adaptations.Not strictly necessary at 500 ms intervals, but it’s a safer pattern when examples are copy-pasted into tighter loops.
- ESP_ERROR_CHECK(rmt_transmit(tx_channel, bytes_encoder, (uint8_t[]) { + ESP_ERROR_CHECK(rmt_transmit(tx_channel, bytes_encoder, (uint8_t[]) { 0x05 }, 1, &transmit_config)); // For 0x05, it's actually 8-bit data, corresponding to 0000 0101 + ESP_ERROR_CHECK(rmt_tx_wait_all_done(tx_channel, portMAX_DELAY)); vTaskDelay(500 / portTICK_PERIOD_MS);examples/peripherals/rmt/ws2812_simple_encoder/main/main.c (2)
1-7: Use the proper boolean literal and include stdbool.h explicitly.Prefer true over 1 for bool. Add stdbool.h to avoid relying on transitive includes.
#include <stdio.h> #include <string.h> +#include <stdbool.h> #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "driver/rmt_tx.h" #include "esp_log.h" @@ } else { symbols[0] = ws2812_reset; - *done = 1; + *done = true; return 1; }Also applies to: 55-57
91-101: Nit: Clarify GRB vs variable names in comments.Variables are named red/green/blue, but payloads are GRB. Consider updating the inline comments to avoid confusion for readers new to WS2812.
- // WS2812 encoding is GRB - uint8_t red[3] = {0x00, 0xFF, 0x00}; /*!< R */ + // WS2812 encoding is GRB (payloads below are GRB-ordered) + uint8_t red[3] = {0x00, 0xFF, 0x00}; /*!< red in GRB */ @@ - uint8_t green[3] = {0xFF, 0x00, 0x00}; /*!< G */ + uint8_t green[3] = {0xFF, 0x00, 0x00}; /*!< green in GRB */ @@ - uint8_t blue[3] = {0x00, 0x00, 0xFF}; /*!< B */ + uint8_t blue[3] = {0x00, 0x00, 0xFF}; /*!< blue in GRB */
📜 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 (9)
build_config.yml(1 hunks)examples/peripherals/rmt/rmt_simple_tx/CMakeLists.txt(1 hunks)examples/peripherals/rmt/rmt_simple_tx/main/CMakeLists.txt(1 hunks)examples/peripherals/rmt/rmt_simple_tx/main/main.c(1 hunks)examples/peripherals/rmt/rmt_simple_tx/sdkconfig.defaults(1 hunks)examples/peripherals/rmt/ws2812_simple_encoder/CMakeLists.txt(1 hunks)examples/peripherals/rmt/ws2812_simple_encoder/main/CMakeLists.txt(1 hunks)examples/peripherals/rmt/ws2812_simple_encoder/main/main.c(1 hunks)examples/peripherals/rmt/ws2812_simple_encoder/sdkconfig.defaults(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/peripherals/rmt/rmt_simple_tx/main/main.c (1)
examples/peripherals/rmt/ws2812_simple_encoder/main/main.c (1)
app_main(61-102)
examples/peripherals/rmt/ws2812_simple_encoder/main/main.c (1)
examples/peripherals/rmt/rmt_simple_tx/main/main.c (1)
app_main(7-51)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (6)
examples/peripherals/rmt/ws2812_simple_encoder/sdkconfig.defaults (1)
1-4: LGTM: Target lock and 240 MHz default are appropriate for ESP32‑S3 examples.No issues spotted. This keeps the example self-contained and easy to build.
examples/peripherals/rmt/rmt_simple_tx/sdkconfig.defaults (1)
1-4: LGTM: Consistent ESP32‑S3 target and CPU config.Matches the ws2812 encoder example for consistency. Good.
examples/peripherals/rmt/rmt_simple_tx/CMakeLists.txt (1)
1-6: LGTM: Standard ESP‑IDF boilerplate in correct order.The 3.16 minimum and boilerplate order look good for IDF v5.4.
build_config.yml (2)
102-109: New RMT examples registered in CI matrix — looks good.Both project entries follow the existing schema (path/idf_version/target) and are consistent with the rest of the file.
102-109: ✅ Buildability check passed for new RMT examples
All required directories and files exist, and eachCMakeLists.txtdeclares aproject(). Both
examples/peripherals/rmt/rmt_simple_txexamples/peripherals/rmt/ws2812_simple_encoder
have been verified with the provided script, confirming no missing files or typos. CI breakage due to these entries is unlikely.examples/peripherals/rmt/ws2812_simple_encoder/CMakeLists.txt (1)
1-8: CMake boilerplate is correct and matches ESP-IDF expectations.cmake_minimum_required, include(project.cmake), and project() ordering all look good.
| idf_component_register(SRCS "main.c" | ||
| INCLUDE_DIRS ".") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity-check component wiring across both new examples.
Ensure the same dependency pattern (PRIV_REQUIRES driver) is applied to the WS2812 example’s main component CMake as well, since it also uses RMT.
Run this script to verify component registration and basic RMT usage in sources:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify both examples have main.c and component registration:"
fd -H '^main\.c$' examples/peripherals/rmt | nl -ba
rg -nU -C2 'idf_component_register\(' examples/peripherals/rmt/**/main/CMakeLists.txt
echo
echo "Best-effort check for RMT API/header usage in example mains:"
rg -n -C2 -e '\bRMT\b' -e 'driver/.+rmt' examples/peripherals/rmt/*/main/main.c || true
echo
echo "Check build_config.yml entries (idf_version and target):"
rg -nC1 'examples/peripherals/rmt/(rmt_simple_tx|ws2812_simple_encoder)' build_config.yml
rg -nC1 'idf_version:\s*release-v5\.4' build_config.yml
rg -nC1 'target:\s*esp32s3' build_config.ymlLength of output: 297
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "1) List main.c and CMakeLists.txt in each RMT example:"
find examples/peripherals/rmt -type f \( -name main.c -o -name CMakeLists.txt \)
echo
echo "2) Check idf_component_register invocation in each example CMakeLists:"
rg -n 'idf_component_register' -g "examples/peripherals/rmt/**/main/CMakeLists.txt" || true
echo
echo "3) Check for PRIV_REQUIRES usage in each CMakeLists:"
rg -n 'PRIV_REQUIRES' -g "examples/peripherals/rmt/**/main/CMakeLists.txt" || true
echo
echo "4) Scan example mains for RMT API usage and header includes:"
rg -n -C1 'rmt_' -g "examples/peripherals/rmt/*/main/main.c" || true
rg -n -C1 '#include.*rmt.*' -g "examples/peripherals/rmt/*/main/main.c" || trueLength of output: 8334
Apply RMT driver dependency to the WS2812 example’s main component
The WS2812 example’s main/CMakeLists.txt currently omits the driver dependency even though it includes and uses the RMT API. Update it to match the pattern used for RMT-based examples:
• File: examples/peripherals/rmt/ws2812_simple_encoder/main/CMakeLists.txt
idf_component_register(SRCS "main.c"
INCLUDE_DIRS "."
+ PRIV_REQUIRES driver)For consistency, you may also add the same line to:
• examples/peripherals/rmt/rmt_simple_tx/main/CMakeLists.txt (optional)
📝 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.
| idf_component_register(SRCS "main.c" | |
| INCLUDE_DIRS ".") | |
| idf_component_register(SRCS "main.c" | |
| INCLUDE_DIRS "." | |
| PRIV_REQUIRES driver) |
🤖 Prompt for AI Agents
In examples/peripherals/rmt/rmt_simple_tx/main/CMakeLists.txt around lines 1-2,
the component registration omits the RMT/driver dependency; update the
idf_component_register call to include the driver dependency (e.g., add REQUIRES
driver) so the RMT API is linked correctly, matching the pattern used in
examples/peripherals/rmt/ws2812_simple_encoder/main/CMakeLists.txt; apply the
same change to other RMT-based example main/CMakeLists.txt files if present.
| rmt_channel_handle_t tx_channel = NULL; | ||
| rmt_new_tx_channel(&tx_channel_cfg, &tx_channel); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check and surface errors from RMT API calls.
The example currently ignores return codes. Wrapping API calls with ESP_ERROR_CHECK improves debuggability and prevents silent failures.
Apply this diff:
#include <stdio.h>
#include <string.h>
#include "freertos/FreeRTOS.h"
#include "freertos/task.h"
#include "driver/rmt_tx.h"
+#include "esp_err.h"
void app_main(void)
{
@@
rmt_channel_handle_t tx_channel = NULL;
- rmt_new_tx_channel(&tx_channel_cfg, &tx_channel);
+ ESP_ERROR_CHECK(rmt_new_tx_channel(&tx_channel_cfg, &tx_channel));
@@
- rmt_new_bytes_encoder(&bytes_enc_config, &bytes_encoder);
- rmt_enable(tx_channel);
+ ESP_ERROR_CHECK(rmt_new_bytes_encoder(&bytes_enc_config, &bytes_encoder));
+ ESP_ERROR_CHECK(rmt_enable(tx_channel));
@@
- rmt_transmit(tx_channel, bytes_encoder, (uint8_t[]) {
+ ESP_ERROR_CHECK(rmt_transmit(tx_channel, bytes_encoder, (uint8_t[]) {
0x05
- }, 1, &transmit_config); // For 0x05, it's actually 8-bit data, corresponding to 0000 0101
+ }, 1, &transmit_config)); // For 0x05, it's actually 8-bit data, corresponding to 0000 0101
vTaskDelay(500 / portTICK_PERIOD_MS);Also applies to: 38-40, 45-49, 1-6
🤖 Prompt for AI Agents
In examples/peripherals/rmt/rmt_simple_tx/main/main.c around lines 1-6, 17-19,
38-40 and 45-49, the RMT API return values are ignored; update each RMT and
related ESP-IDF API call to check and surface errors by wrapping calls with
ESP_ERROR_CHECK (or capturing the esp_err_t and handling/logging on non-ESP_OK).
Specifically, replace raw calls like rmt_new_tx_channel(&tx_channel_cfg,
&tx_channel) and other RMT/driver/init calls with ESP_ERROR_CHECK(...) so
failures abort with a clear error, or assign the return to esp_err_t and call
ESP_ERROR_CHECK on it.
| static const rmt_symbol_word_t ws2812_one = { | ||
| .level0 = 1, | ||
| .duration0 = 9, /*!< 0.9us */ | ||
| .level1 = 0, | ||
| .duration1 = 3, /*!< 0.4us */ | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix WS2812 ‘1’ timing and comment mismatch; keep a consistent bit period.
At 10 MHz resolution, duration1 = 3 equals 0.3 µs but the comment says 0.4 µs. Also the resulting ‘1’ period (0.9 + 0.3 = 1.2 µs) differs from the ‘0’ period (0.4 + 0.9 = 1.3 µs). Using 9 + 4 keeps both at ~1.3 µs and matches the comment.
static const rmt_symbol_word_t ws2812_one = {
.level0 = 1,
.duration0 = 9, /*!< 0.9us */
.level1 = 0,
- .duration1 = 3, /*!< 0.4us */
+ .duration1 = 4, /*!< 0.4us */
};📝 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.
| static const rmt_symbol_word_t ws2812_one = { | |
| .level0 = 1, | |
| .duration0 = 9, /*!< 0.9us */ | |
| .level1 = 0, | |
| .duration1 = 3, /*!< 0.4us */ | |
| }; | |
| static const rmt_symbol_word_t ws2812_one = { | |
| .level0 = 1, | |
| .duration0 = 9, /*!< 0.9us */ | |
| .level1 = 0, | |
| .duration1 = 4, /*!< 0.4us */ | |
| }; |
🤖 Prompt for AI Agents
In examples/peripherals/rmt/ws2812_simple_encoder/main/main.c around lines 17 to
22, the ws2812_one symbol uses duration1 = 3 (0.3µs) but the comment says 0.4µs
and the total bit period (0.9+0.3=1.2µs) differs from the ws2812_zero period
(~1.3µs); change duration1 to 4 so the values are 9 + 4 (0.9µs + 0.4µs = ~1.3µs)
and update the comment to 0.4µs to keep both bit periods consistent and match
the comment.
| if (symbols_free < 8) { | ||
| return 0; /*!< Need at least 8 symbols to form one byte */ | ||
| } | ||
| printf("symbols_written:%d,data_size:%d\n", symbols_written, data_size); | ||
|
|
There was a problem hiding this comment.
Format string bug: use %zu for size_t or switch to ESP_LOGx.
Using %d for size_t is undefined behavior on some platforms. Switch to %zu (or cast) and, ideally, use ESP_LOGD to control log verbosity.
- printf("symbols_written:%d,data_size:%d\n", symbols_written, data_size);
+ ESP_LOGD(TAG, "symbols_written:%zu, data_size:%zu", symbols_written, data_size);If you prefer printf, use:
- printf("symbols_written:%d,data_size:%d\n", symbols_written, data_size);
+ printf("symbols_written:%zu,data_size:%zu\n", symbols_written, data_size);🤖 Prompt for AI Agents
In examples/peripherals/rmt/ws2812_simple_encoder/main/main.c around lines 35 to
39, the printf uses %d for size_t variables (symbols_written, data_size) which
is undefined on some platforms; change the format specifier to %zu for size_t
(or cast values to unsigned long and use %lu) or replace the printf with an
ESP_LOGD call (ESP_LOGD(TAG, "symbols_written:%zu,data_size:%zu\n",
symbols_written, data_size)) to fix the format-string bug and control log
verbosity.
| ESP_ERROR_CHECK(rmt_new_tx_channel(&tx_chan_config, &led_chan)); | ||
|
|
||
| ESP_LOGI(TAG, "Create simple callback-based encoder"); | ||
| rmt_encoder_handle_t simple_encoder = NULL; | ||
| const rmt_simple_encoder_config_t simple_encoder_cfg = { | ||
| .callback = encoder_callback | ||
| // Note we don't set min_chunk_size here as the default of 64 is good enough. | ||
| }; | ||
| ESP_ERROR_CHECK(rmt_new_simple_encoder(&simple_encoder_cfg, &simple_encoder)); | ||
|
|
||
| ESP_LOGI(TAG, "Enable RMT TX channel"); | ||
| ESP_ERROR_CHECK(rmt_enable(led_chan)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap RMT calls with ESP_ERROR_CHECK and optionally wait for completion.
Keeps the example robust and makes failures obvious during bring-up.
ESP_LOGI(TAG, "Create simple callback-based encoder");
rmt_encoder_handle_t simple_encoder = NULL;
@@
- ESP_ERROR_CHECK(rmt_new_simple_encoder(&simple_encoder_cfg, &simple_encoder));
+ ESP_ERROR_CHECK(rmt_new_simple_encoder(&simple_encoder_cfg, &simple_encoder));
@@
- rmt_transmit(led_chan, simple_encoder, red, sizeof(red), &tx_config);
+ ESP_ERROR_CHECK(rmt_transmit(led_chan, simple_encoder, red, sizeof(red), &tx_config));
+ ESP_ERROR_CHECK(rmt_tx_wait_all_done(led_chan, portMAX_DELAY));
vTaskDelay(1000 / portTICK_PERIOD_MS);
@@
- rmt_transmit(led_chan, simple_encoder, green, sizeof(green), &tx_config);
+ ESP_ERROR_CHECK(rmt_transmit(led_chan, simple_encoder, green, sizeof(green), &tx_config));
+ ESP_ERROR_CHECK(rmt_tx_wait_all_done(led_chan, portMAX_DELAY));
vTaskDelay(1000 / portTICK_PERIOD_MS);
@@
- rmt_transmit(led_chan, simple_encoder, blue, sizeof(blue), &tx_config);
+ ESP_ERROR_CHECK(rmt_transmit(led_chan, simple_encoder, blue, sizeof(blue), &tx_config));
+ ESP_ERROR_CHECK(rmt_tx_wait_all_done(led_chan, portMAX_DELAY));Also applies to: 91-101
🤖 Prompt for AI Agents
In examples/peripherals/rmt/ws2812_simple_encoder/main/main.c around lines 73-85
(and similarly for lines 91-101), the RMT API calls are not wrapped with
ESP_ERROR_CHECK and transmissions aren't waited on; update the code so every RMT
call (e.g., rmt_new_tx_channel, rmt_new_simple_encoder, rmt_enable and any
rmt_tx_start) is wrapped with ESP_ERROR_CHECK to surface failures, and after
starting a transmission call the appropriate wait function (e.g.,
rmt_tx_wait_done or rmt_wait_tx_done with a sensible timeout) to ensure the TX
completes before proceeding.
1 participant
Summary by CodeRabbit