Add Espressif RMT driver#101448
Conversation
| * - ESP_ERR_NO_MEM: Create RMT bytes encoder failed because out of memory | ||
| * - ESP_FAIL: Create RMT bytes encoder failed because of other error | ||
| */ | ||
| esp_err_t rmt_new_bytes_encoder(const rmt_bytes_encoder_config_t *config, rmt_encoder_handle_t *ret_encoder); |
There was a problem hiding this comment.
You're writing a zephyr driver, please return zephyr error codes :)
Also use @retval for documenting the discrete error values
| # Copyright (c) 2025 Joel Guittet (joelguittet@gmail.com) | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| description: ESP32 RMT |
There was a problem hiding this comment.
Add a title, spell out the acronym according to the guidelines, and consider adding an actual description :) (bonus point for including an example of a dts snippet as well)
| int channel_id = chan->channel_id; | ||
| uint32_t periph_src_clk_hz = 0; | ||
| bool clock_selection_conflict = false; | ||
| // check if we need to update the group clock source, group clock source is shared by all channels |
drivers/misc/espressif_rmt/Kconfig
Outdated
| if ESPRESSIF_RMT | ||
|
|
||
| config ESPRESSIF_RMT_INIT_PRIORITY | ||
| int "Espressir RMT init priority" |
|
The support file should probably be in the esp hal, unless you want to rewrite them to comply with the upstream coding style, apis etc, then the upstream code should be a driver implementing the led_strip apis. |
|
@kartben thanks for the first comments. I basically agree with all of them and will update the code, but I should have explained a bit more: it's actually ported from esp-idf as is to permit Espressif and particularly @sylvioalves to provide the first feedbacks, we may consider as a draft PR probably for the moment. Support of RMT is also something asked several from community here so at least there is a first version people can cherry-pick. @fabiobaltieri There will be an additional stuff for support of DMA and this need to use DMA APIs from Zephyr of course, so my guess is that it is probably better to have it in Zephyr. It's not a problem for me to update so that it follow Zephyr style. Basically the driver is based on esp-idf v5.1.6, which is already different from the latest esp-idf v6, so keeping in Zephyr is good I think (I have no interest to keep esp-idf style for the driver - and only the driver of course, hal is hal, driver is in zephyr !) |
|
Sure it's fair to have it in misc if you are willing to put to work down to fix it up, problem though is that if it gets updated in idf it'll be impossible to track the changes. Anyway, your call on that. For the rest, yeah prob fair to have it in misc to be used without a specific behavior but then I don't think we should merge a sample that makes it do a functionality that we have a subsystem for if it does not use the subsystem apis. But at that point it should be pretty straightforward to implement a led_strip driver on top of it. |
|
@joelguittet my 2 cents:
|
drivers/misc/espressif_rmt/Kconfig
Outdated
| menuconfig ESPRESSIF_RMT | ||
| bool "Espressif RMT driver" | ||
| help | ||
| Enable config options for Espressif RMT driver. |
There was a problem hiding this comment.
My sugestion:
config RMT_ESP32
bool "ESP32 RMT driver"
default y
depends on DT_HAS_ESPRESSIF_ESP32_RMT_ENABLED
select DMA if DT_HAS_ESPRESSIF_ESP32_GDMA_ENABLED
help
Enables the ESP32 Remote Control Transceiver driver.There was a problem hiding this comment.
My comment on this proposition:
- Renaming the name of the config: ok for me.
- Why defaulting to y ? We may want to just not include it in general. => EDIT: with default y then we just need to activate rmt in the dts of course :-) I add this one.
- select DMA should be added later when I will integrate DMA API for sure. However, this is only for ESP32S3 .so an additional condition should be added I guess.
There was a problem hiding this comment.
@wmrsouza For renaming the config, I cannot. The CI is checking for proper order and consistency of drivers in misc directory. I should be espressif_rmt top have vendor first, and then ESPRESSIF_RMT for the config.
|
@fabiobaltieri I see there is a led_strip sample. I can imagine to not commit the sample I have done in this branch, and instead open another pull-request to add a rmt based led-strip driver. So that the current led_strip sample can be reused to demonstrate rmt usage. What do you think? @wmrsouza your inputs are much appreciated! But I'm not sure to understand your 2 first bullet points. Today I have just followed the esp-idf APIs. RMT is specific to Espressif in any case so I don't see any interest to define a general driver API for that, isn't it ? |
b1249e5 to
de36e05
Compare
|
@joelguittet I think you should follow Zephyr's driver model:
in general way you should start with something like this: #define ESP32_RMT_INIT(idx) \
\
PINCTRL_DT_INST_DEFINE(idx); \
\
static const DRAM_ATTR struct rmt_esp32_config rmt_esp32_cfg_##idx = { \
.hal = { .dev = (rmt_dev_t *)DT_INST_REG_ADDR(idx),}, \
.pcfg = PINCTRL_DT_INST_DEV_CONFIG_GET(idx), \
.clock_subsys = (clock_control_subsys_t)DT_INST_CLOCKS_CELL(idx, offset), \
.irq_source = DT_INST_IRQ_BY_IDX(idx, 0, irq), \
.irq_priority = DT_INST_IRQ_BY_IDX(idx, 0, priority), \
.irq_flags = DT_INST_IRQ_BY_IDX(idx, 0, flags), \
ESP_RMT_DMA_INIT(idx), \
.property_0 = DT_INST_PROP(idx, property_0), \
.property_1 = DT_INST_ENUM_IDX(idx, property_1), \
... \
.property_n = DT_INST_ENUM_IDX(idx, property_n), \
}; \
\
static struct rmt_esp32_data rmt_esp32_data_##idx = { \
.rx_config = {0}, \
.tx_config = {0}, \
... \
}, \
}; \
\
DEVICE_DT_INST_DEFINE(idx, rmt_esp32_init, NULL, &rmt_esp32_data_##idx, \
&rmt_esp32_cfg_##idx, POST_KERNEL, \
CONFIG_RMT_INIT_PRIORITY, &rmt_esp32_api);
DT_INST_FOREACH_STATUS_OKAY(ESP32_RMT_INIT); |
|
@sylvioalves @wmrsouza for your information I created zephyrproject-rtos/hal_espressif#516 and #104097 because I will be blocked here with the Doxygen Coverage Delta Check pipeline and the comments on the new pinctrl definitions. If these PR get merged then I will be able to update here to fix the pipeline. |
|
@sylvioalves after rebasing I have again a twister issue here regarding to esp32c6 ulp core and function ulp_lp_core_sw_intr_trigger. I don't think it's currently cover by one of your opened PR so bring this to your attention. |
Please rebase. |
Re-rebased, passing. Thanks. |
drivers/misc/espressif_rmt/rmt.c
Outdated
| /* Retrieve DMA device if defined, else default to NULL */ | ||
| #define ESPRESSIF_RMT_DT_INST_DMA_CTLR(idx) \ | ||
| COND_CODE_1(DT_INST_NODE_HAS_PROP(idx, dmas), \ | ||
| (DEVICE_DT_GET(DT_INST_DMAS_CTLR(0))), \ |
There was a problem hiding this comment.
DMa is hardcoded with 0. Use idx instead:
(DEVICE_DT_GET(DT_INST_DMAS_CTLR(idx))),
drivers/misc/espressif_rmt/rmt.c
Outdated
| (ESPRESSIF_RMT_DMA_CHANNEL_UNDEFINED)) | ||
|
|
||
| #define ESPRESSIF_RMT_INIT(idx) \ | ||
| PINCTRL_DT_DEFINE(DT_NODELABEL(rmt)); \ |
There was a problem hiding this comment.
+ PINCTRL_DT_INST_DEFINE(idx); \
+ static const struct espressif_rmt_config espressif_rmt_cfg_##idx = { \
+ .pcfg = PINCTRL_DT_INST_DEV_CONFIG_GET(idx), \
.dma_dev = ESPRESSIF_RMT_DT_INST_DMA_CTLR(idx), \
- .tx_dma_channel = ESPRESSIF_RMT_DT_INST_DMA_CELL(0, tx, channel), \
- .rx_dma_channel = ESPRESSIF_RMT_DT_INST_DMA_CELL(0, rx, channel), \
+ .tx_dma_channel = ESPRESSIF_RMT_DT_INST_DMA_CELL(idx, tx, channel), \
+ .rx_dma_channel = ESPRESSIF_RMT_DT_INST_DMA_CELL(idx, rx, channel), \
drivers/misc/espressif_rmt/rmt.c
Outdated
| }, \ | ||
| }; \ | ||
| DEVICE_DT_INST_DEFINE(idx, rmt_init, NULL, &espressif_rmt_data_##idx, \ | ||
| &espressif_rmt_cfg_##idx, PRE_KERNEL_1, CONFIG_ESPRESSIF_RMT_INIT_PRIORITY, \ |
There was a problem hiding this comment.
I guess it should be POST_KERNEL?
There was a problem hiding this comment.
I copied on other ESP32 drivers, but okay to change to POST_KERNEL. Done.
drivers/misc/espressif_rmt/Kconfig
Outdated
|
|
||
| config ESPRESSIF_RMT_INIT_PRIORITY | ||
| int "ESP32 RMT driver init priority" | ||
| default 90 |
There was a problem hiding this comment.
Probably better with config KERNEL_INIT_PRIORITY_DEVICE (which default to 50).
| { | ||
| const struct espressif_rmt_config *config = dev->config; | ||
| int rc; | ||
|
|
There was a problem hiding this comment.
Missing common driver init checks:
if (!device_is_ready(config->clock_dev)) {
LOG_ERR("Clock device not ready");
return -ENODEV;
}
#if SOC_RMT_SUPPORT_DMA
if (config->dma_dev && !device_is_ready(config->dma_dev)) {
LOG_ERR("DMA device not ready");
return -ENODEV;
}
#endif
| /** Carrier wave frequency, in Hz, 0 means disabling the carrier */ | ||
| uint32_t frequency_hz; | ||
| /** Carrier wave duty cycle (0~100%) */ | ||
| float duty_cycle; |
There was a problem hiding this comment.
I think having this as float isn't helpful due to FPU and SoC dependency. I suggest modifying to:
uint16_t duty_cycle;
uint32_t high_ticks = total_ticks * config->duty_cycle / 1000;
or if you follow Zephyr's PWM API:
+ uint32_t period_ticks;
+ uint32_t pulse_ticks;
There was a problem hiding this comment.
Done for the first proposal. Comment describing the duty_cycle variable updated to indicate the unit is 0.1%
|
| pinctrl is used to configure RMT channels and GPIO to be used. | ||
| When using ESP32 or ESP32S2 series, each RMT channel should be used once (eg. RMT_OUT0_GPIOxx | ||
| and RMT_IN0_GPIOxx can't be used at the same time, etc.). An error is displayed when trying to | ||
| create channels whith such configuration. |
There was a problem hiding this comment.
create channels with such configuration
There was a problem hiding this comment.
Good catch! Fixed.
drivers/misc/espressif_rmt/Kconfig
Outdated
|
|
||
| config ESPRESSIF_RMT_ISR_IRAM_SAFE | ||
| bool "RMT ISR IRAM-Safe" | ||
| default n |
There was a problem hiding this comment.
default n can go as it is default.
There was a problem hiding this comment.
Correct, removed.
drivers/misc/espressif_rmt/Kconfig
Outdated
|
|
||
| config ESPRESSIF_RMT_PM | ||
| bool "Support for power management" | ||
| default n |
| #include <zephyr/drivers/dma/dma_esp32.h> | ||
| #include <zephyr/drivers/pinctrl.h> | ||
| #include <zephyr/drivers/pinctrl/pinctrl_esp32_common.h> | ||
| #include <zephyr/logging/log.h> |
There was a problem hiding this comment.
would you move the LOG module not to be between #includes? same on rmt_tx.c file.
| #error "DMA peripheral is not enabled!" | ||
| #endif | ||
|
|
||
| LOG_MODULE_REGISTER(espressif_rmt, CONFIG_ESPRESSIF_RMT_LOG_LEVEL); |
There was a problem hiding this comment.
This is correct as it is registering the RMT logging.
But in all other .c files, it should only DECLARE it, not register again. So please update other .c files to use LOG_MODULE_DECLARE(espressif_rmt, CONFIG_ESPRESSIF_RMT_LOG_LEVEL); instead.
drivers/misc/espressif_rmt/rmt_rx.c
Outdated
| * APB clock no matter what the clock source is used by the RMT channel as the "core" | ||
| * clock | ||
| */ | ||
| #if CONFIG_IDF_TARGET_ESP32 || CONFIG_IDF_TARGET_ESP32S2 |
There was a problem hiding this comment.
#if defined(CONFIG_SOC_SERIES_ESP32) || defined(CONFIG_SOC_SERIES_ESP32S2)
drivers/misc/espressif_rmt/rmt_tx.c
Outdated
| return; | ||
| } | ||
| /* delay a while, wait for DMA data going to RMT memory block */ | ||
| esp_rom_delay_us(1); |
There was a problem hiding this comment.
Use Zephyr API instead: k_busy_wait(1);.
There was a problem hiding this comment.
Fixed. Note: you have a few of them in i2c and sdhc driver + hw_init. Let me know if you would like a PR for them (if relevant, which I'm not sure, but maybe preferable to change using k_sleep with K_USEC).
drivers/misc/espressif_rmt/rmt_tx.c
Outdated
| if (t->loop_count != 0) { | ||
| if (unlikely(encoded_symbols > tx_chan->base.mem_block_num | ||
| * SOC_RMT_MEM_WORDS_PER_CHANNEL)) { | ||
| ESP_DRAM_LOGE("rmt", |
drivers/misc/espressif_rmt/rmt_rx.c
Outdated
| * data and reset the pointer | ||
| */ | ||
| rmt_ll_clear_interrupt_status(hal->regs, RMT_LL_EVENT_RX_ERROR(channel_id)); | ||
| ESP_DRAM_LOGE("rmt", "hw buffer too small, received symbols truncated"); |
There was a problem hiding this comment.
LOG_ERR. Check all files please.
Examples in |
The Espressif RMT driver is created from the ESP-IDF version. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
The RMT peripheral is added to the existing relevant devices. A DTS bindings is created to support this peripheral. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
Adding RMT pinctrl for all relevant ESP32 devices. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
Indicate rmt support for all relevant Espressif boards. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
Add definition for default rmt pinctrl on all relevant boards. GPIO12 and GPIO13 are used by default. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
Adding this application to test espressif_rmt driver using bytes encoder. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
Adding this application to test espressif_rmt driver using simple encoder. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
Update hal to integrate RMT support. Signed-off-by: Joel Guittet <joelguittet@gmail.com>
|



Add initial version of the Espressif RMT driver.
A sample application is created to demonstrate the usage.
This pull-request depends on an update of hal_espressif: zephyrproject-rtos/hal_espressif#510
Fixes: #72546