xtensa/espressif/rmt: Implement a common RMT (Remote Control) driver for xtensa-based devices.

[tmedicci]

Summary

• drivers/rmt: Implement an upper-half RMT character driver

The RMT (Remote Control) character driver allows to use the RMT peripheral (usually, a one-wire peripheral dedicated to driving IR remote control) as a character driver.

• drivers/leds/ws2812: Fix WS2812 pixel size

Although the LED might be RGB-only, the LED data is packed in a 32-bit long variable and, then, this is the default size of a LED pixel to define the 'WS2812_RW_PIXEL_SIZE' macro. Please note that the lower-half driver will deal with the case of the addressable LED being 3 or 4-pixel sized.

• xtensa/esp/rmt: Add the lower-half implementation of the RMT driver

The lower-half implementation of the RMT character driver based on Espressif HAL enables using the RMT peripheral of ESP32, ESP32-S2 and ESP32-S3 as a common xtensa-based Espressif driver.

• xtensa/esp/ws2812: Add the lower-half WS2812 driver based on RMT

This lower-half WS2812 LED driver uses the RMT peripheral of the Espressif's SoCs to drive the RGB addressable LEDs. Compared to the SPI-based implementation, it is faster!

• esp32s3: Fully integrate Espressif HAL repository to ESP32-S3

By integrating the Espressif`s HAL repository into the current ESP32-S3 implementation on NuttX, it is possible to call functions that make it easier to set up the registers of the ESP32-S3 and enable the usage of common Espressif drivers. Please note that Espressif's HAL repository was already being used for the Wi-Fi driver.

• esp32s3/rmt: Use the Espressif's common RMT driver.

This commit use the new common RMT driver for all Espressif's xtensa-based chips on ESP32-S3.

• esp32s2: Integrate Espressif HAL repository to ESP32-S2

By integrating the Espressif`s HAL repository into the current ESP32-S2 implementation on NuttX, it is possible to call functions that make it easier to setup the registers of the ESP32-S2, enabling the usage of common Espressif drivers.

• esp32s2/rmt: Use the Espressif's common RMT driver.

This commit use the new common RMT driver for all Espressif's xtensa-based chips on ESP32-S2.

Impact

Implement a common RMT driver for xtensa-based devices. ESP32 requires updating the Wi-Fi driver to enable using the HAL (and the common RMT driver)

Testing

Internal CI testing + ESP32-S2-Saola-1 board and ESP32-S3-DevKitC-1 v1.0 board (loopback test + WS2812 LED strip)

[xiaoxiang781216]

but, it's good to use the vendor hal file directly

[tmedicci]

but, it's good to use the vendor hal file directly

I'm sorry, I didn't understand what you meant. Could you please elaborate a little further?

[acassis]

@tmedicci I think there are some models of WS2812 that doesn't have WHITE LED, so instead of using 32-bit it uses 24-bit.

@vbenso could you please confirm/verify?

[tmedicci]

Hi @acassis !

Yes, I know (in fact, the "real" ws2812 doesn't contain 4 LEDs at all, only variants that use the same protocol and sometimes are sold as ws2812 RGBW), but the variable that the LED driver uses is 4-byte long (independent of the LED). The lower-half driver should handle that. I explained in the commit message:

Although the LED might be RGB-only, the LED data is packed in a 32-bit long variable and, then, this is the default size of a LED pixel to define the 'WS2812_RW_PIXEL_SIZE' macro. Please note that the lower-half driver will deal with the case of the addressable LED being 3 or 4-pixel sized.

[acassis]

Ok, understood. So maybe @vbenso was thinking that the model without white LED was 24-bit instead of 32-bit.

[tmedicci]

@acassis , just to make it clearer: the model is 24-bit, but this is handled by the lower-half driver. The way the upper-half driver works on NuttX, it uses 32-bit (a word) for both RGB and RGBW models.

[vbenso]

Hi guys, merry Christmas.
I didn't want to allocate 32 bits unless it was strictly required. That being said, I understand that using packed structures could degrade the performance. One could say that there is plenty of memory and people shouldn't care too much about it. In my case, I was using 700+ leds, so there were some memory concerns.

[tmedicci]

Hi guys, merry Christmas. I didn't want to allocate 32 bits unless it was strictly required. That being said, I understand that using packed structures could degrade the performance. One could say that there is plenty of memory and people shouldn't care too much about it. In my case, I was using 700+ leds, so there were some memory concerns.

Hi, Merry Christmas!

Yes, I understand your concerns about using a 32-bit when only 24-bit is necessary. However, no application that used the WS2812 encodes the LED pixel data in 24-bit space (this would depend on the application itself).
Even considering that the application could encode LED data in 24-bit/pixel, we still had:

#ifdef WS2812_HAS_WHITE
#  define WS2812_RW_PIXEL_SIZE  4
#else 
#  define WS2812_RW_PIXEL_SIZE  3
#endif

with WS2812_HAS_WHITE not being defined by Kconfig (it should be 'CONFIG_WS2812_HAS_WHITE). Also, the [ws2812_write](https://github.com/apache/nuttx/blob/d1326e81bca47617aa253705ca778c7d51f82038/drivers/leds/ws2812.c#L316) (for CONFIG_WS2812_NON_SPI_DRIVER`) is not compliant with raw 24-bit data length (LED pixel data should be encoded in 32-bit space).

[xiaoxiang781216]

but, it's good to use the vendor hal file directly

I'm sorry, I didn't understand what you meant. Could you please elaborate a little further?

All upstream code never calls vendor HAL directly, only download and link the static library.

[tmedicci]

but, it's good to use the vendor hal file directly

I'm sorry, I didn't understand what you meant. Could you please elaborate a little further?

All upstream code never calls vendor HAL directly, only download and link the static library.

Oh, I see... this PR isn't about the "HAL", it's about the implementation of a common driver that is valid for all Espressif's xtensa-based devices. The same "common driver" approach is true for RISC-V-based devices, like ESP32-C3, ESP32-C6, ESP32-H2, for instance.

Espressif is making a huge effort to avoid using closed static libraries: we plan to provide as much as possible the sources. Specifically for NuttX, we are just using header files and building specific auxiliary functions. There is no direct usage of "Espressif's HAL" on NuttX. Drivers' code and HAL-adapters are implemented on NuttX. License is compliant, there is no "HAL" code on NuttX (all code is downloaded or cloned) and called functions are according to NuttX standards. Please note that the same is valid for the Wireless driver of xtensa-based SoCs.

[tmedicci]

but, it's good to use the vendor hal file directly

I'm sorry, I didn't understand what you meant. Could you please elaborate a little further?

All upstream code never calls vendor HAL directly, only download and link the static library.

Oh, I see... this PR isn't about the "HAL", it's about the implementation of a common driver that is valid for all Espressif's xtensa-based devices. The same "common driver" approach is true for RISC-V-based devices, like ESP32-C3, ESP32-C6, ESP32-H2, for instance.

Espressif is making a huge effort to avoid using closed static libraries: we plan to provide as much as possible the sources. Specifically for NuttX, we are just using header files and building specific auxiliary functions. There is no direct usage of "Espressif's HAL" on NuttX. Drivers' code and HAL-adapters are implemented on NuttX. Please note that the same is valid for the Wireless driver of xtensa-based SoCs.

Just to make sure: please note that there is no generic code. The repository being downloaded is dedicated to providing sources being used on NuttX:

Similar approach with other manufacturers' implementation:

• https://github.com/apache/nuttx/blob/master/arch/arm/src/nrf52/CMakeLists.txt
• https://github.com/apache/nuttx/blob/master/arch/arm/src/nrf52/CMakeLists.txt
• https://github.com/apache/nuttx/blob/master/arch/arm/src/nrf91/CMakeLists.txt
• https://github.com/apache/nuttx/blob/0eeca0f3751876fc090108d01156d0999dd02116/arch/risc-v/src/bl602/Make.defs

[xiaoxiang781216]

Thanks for explanation, the change looks good to me.

[raiden00pl]

@tmedicci hi, why is this driver not in drivers/rc ? can't this be implemented with common drivers/rc/lirc_dev.c instead of creating vendor-specific drivers?

[tmedicci]

@tmedicci hi, why is this driver not in drivers/rc ? can't this be implemented with common drivers/rc/lirc_dev.c instead of creating vendor-specific drivers?

Hi @raiden00pl! The RMT peripheral is very versatile (the name doesn't reflect all of its capabilities). To cover more applications we developed this driver in layers. There is the generic RMT upper-half driver that enables using it directly for any kind of application based on timed signals, but the lower-half can be bound to any kind of existing driver with the correct adapter (for instance, we created an intermediate layer to use it with the upper-half WS2812 driver). So, answering your question: yes, we can use it with drivers/rc (probably it'd need a simple adapter), but we decided not to limit its capabilities to remote controlling.

[raiden00pl]

In that case you should use some kind of lower-half architecture API for rmt driver that later can be reused to implement other upper-half drivers. Then based on the lower-half RMT implementation you can build other drivers like rc/lirc_dev.c or WS2812 and then we have a nice modular architecture.

For example, like its done for timer peripheral in stm32: you have one low-level interface for timers (https://github.com/apache/nuttx/blob/master/arch/arm/src/stm32/stm32_tim.h), and that interface is used to implement other features like periodic timer, oneshot or tickless.

Equivalent functions from struct rmt_ops_s should be exposed at the arch level and then reused in the arch-specific WS2812 implementation. This structure should not be used by arch code, it should be implemented by arch code.
Now, there is a strange coupling between the upper-half RMT driver and the architecture specific implementation for WS2182.

[raiden00pl]

here is what I mean:

[tmedicci]

Hi @raiden00pl , I may have understood your concerns wrongly, but the current implementation is more like the GOOD than the BAD. See, let's check nuttx/boards/xtensa/esp32s3/common/src/esp32s3_board_rmt.cs board_rmt_txinitialize. This function initializes both the generic RMT driver and the RMT-based WS2812 driver.

Note that, the RMT peripheral is initialized by esp_rmt_tx_init, which is arch-specific (lower-half). Although not implemented, calling rmtchar_register here to register the generic RMT driver is totally optional. esp_ws2812_setup uses the lower half initialized by esp_rmt_tx_init.

Backing to esp_rmt_tx_init: it calls esp_rmtinitialize which registers priv->ops = &g_rmtops; here. g_rmtops registers the esp_rmt_read and esp_rmt_write to struct rmt_ops_s's read and write. Both esp_rmt_read and esp_rmt_write are implemented by arch code (lower-half): this seems to be the "RMT_COMMON".

esp_ws2812_setup receives a structure of the RMT lower-half driver initialized by esp_rmt_tx_init. The registered write function for struct ws2812_dev_s is esp_write (defined at arch/xtensa/src/common/espressif/esp_ws2812.c, i.e. a lower-half interface). This function end-up calling priv->rmt->ops->write(), which is the esp_rmt_write function of the lower-half driver (not the upper-half). As we used commonly-used names (read/write), this may have caused the confusion here.

Also, please note that the struct rmt_ops_s (at include/nuttx/rmt/rmt.h) isn't part of the upper-half generic RMT driver (nuttx/include/nuttx/rmt/rmtchar.h. I agree that the location of the
rmt.h file could be understood like that (and should eventually be moved to the arch folder, although it is used by more than a arch)

[raiden00pl]

Also, please note that the struct rmt_ops_s (at include/nuttx/rmt/rmt.h) isn't part of the upper-half generic RMT driver (nuttx/include/nuttx/rmt/rmtchar.h. I agree that the location of the
rmt.h file could be understood like that (and should eventually be moved to the arch folder, although it is used by more than a arch)

My reasoning is that if it's in the common driver headers (include/nuttx/rmt), then it's upper-half part. So here is our difference :)

RMT upper-half itself seems to be wrong, as it's architecture specific driver. ESP architecture logic should implement RMT_COMMON and then based on RMT_COMMON there should be lower-half implementation for rc/lirc_dev.c and WS2818_UPPER.

Currently we have 2 drivers implementing IR driver that are incompatible with each other, which means there is no way to have a portable architecture between different vendors. Especially when drivers/rc was already upstream.

[xiaoxiang781216]

Yes, I also have the same feeling, both RMT and PNCT(#15079) don't define the clear contract between the application and driver. The general driver framework should:

1. Define IOCTL and struct, which all driver implementations must follow
2. Define the behavior of open/close/read/write/poll
3. Provide a common upperhalf to simplify the implementation

both definitions should be general enough to support the different hardware. The user can't write a general application to control the hardware come from different provider if these key elements doesn't catch in the design.

[tmedicci]

Hi @raiden00pl and @xiaoxiang781216, let's put into perspective about RMT:

My reasoning is that if it's in the common driver headers (include/nuttx/rmt), then it's upper-half part. So here is our difference :)

We can move it to arch folder. It's used by both xtensa and RISC-V.

RMT upper-half itself seems to be wrong, as it's architecture specific driver.

It isn't, it's used by either RISC-V and xtensa devices with the RMT peripheral.

ESP architecture logic should implement RMT_COMMON and then based on RMT_COMMON there should be lower-half implementation for rc/lirc_dev.c and WS2818_UPPER.

We did this exactly the way you described for WS2812 upper-half. We didn't implement it for RC, but we encourage community to do it (and we can help with testing and validation). Then, I agree that we could even remove the RMT upper-half driver (nuttx/drivers/rmt/rmtchar.c) because it isn't coupled with the lower-half implementation.

The user can't write a general application to control the hardware come from different provider if these key elements doesn't catch in the design.

I agree that this is partially true for the RMT peripheral, but users can move between different devices/architectures (from ESP32 to ESP32-C3, for instance). About WS2812, this isn't true: the same application (https://github.com/apache/nuttx-apps/tree/master/examples/ws2812, for instance) can be used by any chip/arch/vendor that implements the driver.

In general, I agree with your considerations regarding the driver organization, but I'm not convinced that we aren't providing that. Not allowing binding a peripheral to an existing upper-half driver is different from providing an upper-half driver for a peripheral whose capabilities are not covered by the existing upper-half drivers. We implement our drivers thinking that they could be bound to any other existing driver (just like the ws2812 upper-half driver)

[raiden00pl]

We can move it to arch folder. It's used by both xtensa and RISC-V.

The point is that the entire RMT driver belongs to arch and should be moved there.

It isn't, it's used by either RISC-V and xtensa devices with the RMT peripheral.

I can't agree here. It's vendor specific driver, only used by ESP chips, so it should be in arch. A long time ago there was a similar case with HRTIM which is a peripheral specific to STM32. It can be used to implement other drivers like PWM, SMPS and so one. My initial implementation also included an upper-half driver in drivers/hrtim which was eventually rejected by Greg, because it's a stm32-specific driver. It doesn't matter if it is present in stm32f3, stm32g4 or stm32h7, it is still a stm32-specific peripheral and of course Greg's decision was justified and correct.

We did this exactly the way you described for WS2812 upper-half. We didn't implement it for RC, but we encourage community to do it (and we can help with testing and validation). Then, I agree that we could even remove the RMT upper-half driver (nuttx/drivers/rmt/rmtchar.c) because it isn't coupled with the lower-half implementation.

RMT shouldn't be in drivers/ in the first place, it belongs to arch (yes, I know, I'm repeating myself :P) and IR feature should be provided with already present driver that provides good and standardized abstraction: drivers/rc/lirc_dev.c. Otherwise it looks to me like a violation of INVIOLABLES.md:

### No Short Cuts

  - Doing things the easy way instead of the correct way.
  - Reducing effort at the expense of Quality, Portability, or
    Consistency.
  - Focus on the values of the organization, not the values of the Open
    Source project.  Need to support both.
  - It takes work to support the Inviolables.  There are no shortcuts.

I agree that this is partially true for the RMT peripheral, but users can move between different devices/architectures (from ESP32 to ESP32-C3, for instance). About WS2812, this isn't true: the same application (https://github.com/apache/nuttx-apps/tree/master/examples/ws2812, for instance) can be used by any chip/arch/vendor that implements the driver.

I don't see a problem with WS2812, the problem is in RMT.
I wouldn't call the interchangeability of chips in a given vendor portfolio good architecture. Good architecture is when I can replace chip from one vendor to a chip from other vendor. Of course, assuming that the chips have the same capabilities.

In general, I agree with your considerations regarding the driver organization, but I'm not convinced that we aren't providing that. Not allowing binding a peripheral to an existing upper-half driver is different from providing an upper-half driver for a peripheral whose capabilities are not covered by the existing upper-half drivers. We implement our drivers thinking that they could be bound to any other existing driver (just like the ws2812 upper-half driver)

There is no feature that RMT implements that cannot be implemented with existing portable drivers. IR with drivers/rc/lirc_dev.c, and ws2812 as it's implemented now, but with the difference that esp-specific ws2812 implementation use esp-specific API for RMT.

[tmedicci]

The point is that the entire RMT driver belongs to arch and should be moved there.

Let's avoid using RMT generically. I suppose you didn't mean to move drivers/rmt/rmtchar.c to arch folder, right? It's clearly an upper-half driver and totally optional. It's just an interface to the lower-half.

It isn't, it's used by either RISC-V and xtensa devices with the RMT peripheral.

I can't agree here. It's vendor specific driver, only used by ESP chips, so it should be in arch.

You've said earlier:

RMT upper-half itself seems to be wrong, as it's architecture specific driver.

And I just said that it isn't architecture specific. It's vendor-specific (at least for now), not arch-specific.

RMT shouldn't be in drivers/ in the first place, it belongs to arch (yes, I know, I'm repeating myself :P) and IR feature should be provided with already present driver that provides good and standardized abstraction: drivers/rc/lirc_dev.c.

I suppose you are referring to include/nuttx/rmt/rmt.h, right? If so, as I've said earlier: it isn't related to the upper-half driver and it can be moved to the arch. I'm not that convinced that it's in the wrong place because it's totally decoupled with the upper-half RMT driver (drivers/rmt/rmtchar.c) and its header file (include/nuttx/rmt/rmtchar.h), but we can move it to arch folder (duplicating it for xtensa and RISC-V).

I don't see a problem with WS2812, the problem is in RMT.

Now I suppose you are referring to drivers/rmt/rmtchar.c, right? If so, I said earlier that this upper-half driver isn't needed for WS2812.

and ws2812 as it's implemented now, but with the difference that esp-specific ws2812 implementation use esp-specific API for RMT.

I'm not sure if I got your point here. The "common" API it uses is defined at include/nuttx/rmt/rmt.h. If we simply move it to arch-folder without any modification it'd be called "esp-specific API for RMT"? If so, again, I'm not against it...

There is no feature that RMT implements that cannot be implemented with existing portable drivers. IR with drivers/rc/lirc_dev.c

I agree, and there are a bunch of other features that could be implemented with the RMT peripheral (PWM, audio, DACs etc). Our main goal is to provide support for the peripherals. Unfortunately, we have limited resources to create bindings to all of those upper-half driver, so we implemented the RMT lower-half driver and bound it with an existing upper-half driver (ws2812). Then, we provided a generic API for users to deal directly with the RMT peripheral (drivers/rmt/rmtchar.c). As soon as people bind the lower-half driver with the existing upper-half drivers, RMT upper-half wouldn't be necessary anymore and can be removed. There is a difference between the good and the needed.

[raiden00pl]

Let's avoid using RMT generically. I suppose you didn't mean to move drivers/rmt/rmtchar.c to arch folder, right? It's clearly an upper-half driver and totally optional. It's just an interface to the lower-half.

If you want to stick with RMT character driver then yes, I mean to move it to arch/ folder. You can grep arch/ for register_driver and you can see that there is nothing wrong with upper-half arch-specific implementations in arch/.

And I just said that it isn't architecture specific. It's vendor-specific (at least for now), not arch-specific.

We can argue whether it's arch-specific or vendor-specific. This doesn't make sense to me, since both should be placed in arch/. We have here similar case as the HRTIM I mentioned earlier and the solution should be the same.

There is a difference between the good and the needed.

Again, this approach is against INVIOLABLES.md, but I understand that we are all busy and don't have time to do everything we would like to. In that case I propose to create an issue to not forget about this problem and in the future avoid vendor/arch specific drivers in drivers/ if possible.