[feature request] support ESP-IDF platform

Hi @TMRh20 & @2bndy5
We are using the library you maintain in https://github.com/tbnobody/OpenDTU

As we need/want to adress mulitple devices via the same SPI a user has created a fork for of your library which introduce a HAL (Hardware Abstraction Layer) to use the same SPI configuration for both NRF24L01+ as well as an CMT2300A module on the same SPI. This way we can make use of the comm modules on the same SPI and switch between them using the CE/CSN and IRQ lines.

Would you kindly review the changes/commits in the following repo to give us a hint how we could best get this HAL abstraction into upstream RF24, ie changing the public interfaces to accept the hal object ?

master...LennartF22:RF24:master

https://github.com/LennartF22/RF24
https://github.com/LennartF22/OpenDTU

@LennartF22 how do you imagine to propagate your great contributions needed by the OpenDTU Fusion PoE board (by @markusdd) forward into OpenDTU and RF24 masters ?

Using multiple SPI slave devices on the same bus shouldn't be a problem if

each device uses a different CS pin
multi-processing doesn't try using more than 1 device on the bus at a time

I looked at LennartF22/RF24@nRF24:RF24:master...master and found some breaking changes that wouldn't be welcome here, specifically c'tor and begin() changes. Most importantly, I don't see where all the removed code was moved to (functionality just invokes virtual functions of a new RF24_hal class with no implementations).

Our HAL is primarily done through utility drivers that try to use a platform's native implementations wrapped in an Arduino-like API (see utility folder and RF24_config.h). Some platforms need special invocations (in RF24.cpp) to behave as expected, thus all the ifdef soup.

I just saw that @stefan123t created this issue. Here's some input from my side:

We have a little bit of a special situation with the "OpenDTU Fusion" board mentioned above. On that board, the nRF24 and the CMT2300 are routed to two different sets of GPIO pins of the ESP32-S3, mainly because the CMT2300 uses 3-wire SPI, while the nRF24 uses standard SPI. Recently, an Ethernet shield with PoE has been designed for the "OpenDTU Fusion" board, but due to the ESP32-S3 not having an integrated Ethernet RMII controller anymore, we had to use an external SPI Ethernet controller. This is problematic, because we are already using both available SPI controllers of the ESP32, and the externally available pins of the "OpenDTU Fusion" board do not expose the SPI buses of the two RF ICs.

Basically, we have 3 SPI slaves that are connected to 3 different sets of pins on the ESP32. I solved this by re-routing the internal signals to the SPI peripheral inside the ESP32 on demand in software (i.e. when a register of an SPI slave needs to be accessed), which works quite well.

To get this to work with your RF24 library, I created a virtual HAL class with methods for doing SPI transfers and stuff like toggling the CE pin. A user of the library could just implement the virtual class to match their hardware, and pass an instance of that class to the RF24 "begin" method.

In my fork (https://github.com/LennartF22/RF24) I just implemented a quick and dirty proof of concept, which obviously does not maintain compatibility with the existing interface, so for it to be merged, I would make this feature optional (for example by using a define). Our main question is, whether something like this would be merged (if implemented properly), or whether you have a different idea/approach that could help us.

I looked at LennartF22/RF24@nRF24:RF24:master...master and found some breaking changes that wouldn't be welcome here, specifically c'tor and begin() changes.

@2bndy5 That's correct, my fork is just a quick proof of concept. If @stefan123t had informed me that he was about to create this issue/question (maybe he did and I overlooked it), I would have told him to mention this upfront 😅

CMT2300 uses 3-wire SPI

The way I see it, this is your main problem. Is there a pin compatible alternative that uses a CS signal? I see there's a CMT2300A chip, but I'm not familiar with this OpenDTU Fusion PCB.

Our main question is, whether something like this would be merged (if implemented properly), or whether you have a different idea/approach that could help us.

TL;DR: We are not looking to overhaul our current HAL approach.

With implementation aside, the HAL concept proposed is not likely to be merged because we already have a bunch of ifdef soup to satisfy our current HALs. Instead, I would suggest you use our current HAL approach and write a OpenDTU_fusion driver in RF24/utility and just implement the needed changes (see our portability docs). Using our current HAL approach would be more acceptable for merging here.

On a kind of related note, I've been considering namespacing our utility drivers for various reasons.

I looked at LennartF22/RF24@nRF24:RF24:master...master and found some breaking changes that wouldn't be welcome here, specifically c'tor and begin() changes.

@2bndy5 That's correct, my fork is just a quick proof of concept. If @stefan123t had informed me that he was about to create this issue/question (maybe he did and I overlooked it), I would have told him to mention this upfront 😅

@LennartF22 well I thought it is better to ask this as a question first as Brendan and @TMRh20 are quite responsive anyway, before you are going through the troubles of creating a PR forth-and-back again.
Thanks again to them, maybe we can create something like

your_custom_file.h Provides access to custom drivers for spi, gpio, etc

e.g.

virtual_fusion_hal.h	Provides access to custom drivers for spi, gpio, etc to OpenDTU Fusion boards

@2bndy5 what do you have in mind considering namespacing ?
Could you give us an example, maybe we can already consider that when implementing the utility driver for the fusion board.

what do you have in mind considering namespacing ?

I did some experimenting with this in my CirquePinnacle library when porting it to Linux and PicoSDK (similarly to how HAL is done here but with I2C support). My main motivation with namespacing is that our HAL drivers shouldn't be used by other Arduino libraries on Linux because the Arduino-wrapping API can be quite library-specific (and there's always a concern for naming conflicts of structures like class SPI). Each call to a namespaced HAL function would need the namespace scoped to the applicable functions' body.

EDIT: see #926 for a more practical proposal.

Could you give us an example, maybe we can already consider that when implementing the utility driver for the fusion board.

There is a template folder that is supposed be used as a starting off point for new HAL drivers... The idea is that the compiler would respect the following steps:

The OpenDTU project's platform.ini file should include a build_flags to globally define a macro specific to the build (for the OpenDTU Fusion PCB):
```
build_flags =
  -DOPEN_DTU_Fusion
```

In RF24_config.h, the proper driver would be included.

// this should probably be placed within the `#if defined(ARDUINO)` block
#ifdef OPEN_DTU_Fusion
    #include "utility/virtual_fusion_hal.h"
// ...

If this doesn't suite your project's needs, then it might be more feasible to

maintain your own fork of RF24. Merging in updates from upstream (this repo) might be cumbersome though (not ideal).
find a sub-GHz RF module that makes use of the CS signal. This seems more ideal to me, but it might drive up costs to source the adequate parts.

I've already voiced my hesitance to revising our current HAL implementation, but I'm not the only maintainer here. It would be nice to have only radio-specific code in RF24.cpp, but I'd rather not fix what isn't broken.

@LennartF22 I would be more enthusiastic about a utility driver that is meant for ESP-IDF-only projects. But I'm not sure if such a driver would conflict with the Arduino framework on ESP boards.

I already had a look at how you realize hardware abstraction yesterday (but didn't have time to answer), and we should be able to make that approach work for us. I must admit that I had not looked much into that (back when I modified the RF24 library for our needs), mainly because I just wanted to quickly get our 3 SPI devices to work, and had no intention of getting it merged here in that form.

Regarding our somewhat special hardware situation: Although there might be a comparable RF IC with normal SPI, we cannot really change that anymore as well above 100 boards have already been made and distributed, so unfortunately software remains the only solution.

My plan right now is to implement a driver that directly uses the ESP-IDF (instead of the Arduino layer), with the additional feature of being able two specify a pair of functions that is called before/after SPI transfers. Ideally, the transfernb would be used over the transfer function for performance reasons, but maybe we could abuse the RF24_LINUX flag to accomplish this.

I don't know when I'll have time for that though, as I'm quite busy right now (and I'm also still waiting for feedback from the OpenDTU maintainer).

Hi, HW dev of the board here.

regarding the RF-IC: No, there is unfortunately no alternative. The CMT2300A for 868MHz with the needed functionality is the only IC out there we can use. Unfortunately they use this stupid 3-wire SPI. That is also the reason why we ran them on seperate ESP pins as doing otherwise and just switching slave select surely could cause electrical trouble.

Thanks for this fruitful discussion here and considering this, but I admit as a hardware guy who has just some firmware knowledge this architectural discussion for the driver is a bit above my head^^

My plan right now is to implement a driver that directly uses the ESP-IDF (instead of the Arduino layer), with the additional feature of being able two specify a pair of functions that is called before/after SPI transfers

Yeah! 🎉 These additional functions sound like Arduino's begin/endTransaction() functions (which are used in RF24 if SPI_HAS_TRANSACTION is defined).

Ideally, the transfernb would be used over the transfer function for performance reasons, but maybe we could abuse the RF24_LINUX flag to accomplish this.

The RF24_LINUX flag is meant for Linux drivers specifically; there are other special uses that flag enables besides transfernb(). @TMRh20 I wouldn't mind using a generic flag like RF24_SPI_BUFFER_TRANSFERS to indicate a driver that uses buffered transfers, but it might make the code less obvious. I used such a flag in my CirquePinnacle lib because multiple drivers use the transfernb() approach.

regarding the RF-IC: No, there is unfortunately no alternative

Thanks @markusdd for answering my query there. I wish Texas Instruments had an easier to use RF development kit... Then, you'd surely be able to market your own sub-GHz RF module or embed it into the Fusion PCB.

@2bndy5 thanks for the suggestions using a generic flag, RF24_SPI_BUF / RF24_SPI_BUF_TX may be a bit shorter and wieldy, but in the end it does not really matter ;) I'll have to look into the distinction between transfernb() vs. transfer() to follow your discussion with LennartF22 ...

Regarding the RF-IC we have used the Nordic Semiconductor NRF24L01+ simply because the other side of the DTU is a common Solar Micro-Inverter here in EU, which has a communication module built-in (enclosed in housing/thermal jelly mass) that sepcifically relies on 250kBps Enhance ShockBurst (ESB) protocol in their communication. In order to open source this we already have reverse engineered the majority of the Gen2 & Gen3 protocol's on top of that, but emulating Nordic's ESB protocol on some other RF-IC seems overkill for our purpose.

Have in mind that the vendor switched to Sub-1GHz communication using a CMT2300A for the models which came out last year and further modified the latest model range to include a WiFi connection since this year. We are attempting to serve all three communication methods using the so called OpenDTU Fusion board sic, hence the name.

I was wondering about the various forms of wireless used... That historical insight seems like a good tidbit for the readme section on compatible inverters. Their design decisions may have something to do with wireless regulations imposed by the UN's ITU.

For my own reference, this guide outlines how to integrate third-party libs (called "components") into an ESP-IDF based project. Following this design when implementing RF24 HAL wrappers for the ESP-IDF should make it easier for devs to integrate RF24* libs into their ESP-IDF project(s).

Another hint toward extending support for ESP-IDF: https://github.com/LennartF22/OpenDTU/blob/spi-rework/lib/Hoymiles/src/nrf_hal.cpp

Well, I started a branch (named esp-idf) to begin supporting the ESP-IDF as a separate platform (independent of the Arduino platform). I haven't written any examples or performed any test builds, but I did fill out the HAL API to follow our current conventions...

It's going to take me a while to understand how to use the ESP-IDF to build apps. I think I'll just use PlatformIO in CI since it already has support for the espidf framework.

~~I believe I designed the HAL abstraction as a pure CMake ESP-IDF component.~~ My intention is to allow users to specify nRF24/RF24 as a required component (& submit it to the ESP-IDF component registry) in their ESP-IDF projects.

@LennartF22 does the branch Brendan started yesterday already qualify for integration into OpenDTU ?
It contains the expected ifdef-soup to select code for EPS-IDF builds using RF24_ESP_IDF label on ESP_PLATFORM.

I just cross checked with the ESP-IDF component registry and could only spot the LICENSE & README files missing for conformity.
@2bndy5 you probably need some 😋 🍒 compote component upload --namespace nRF24 --name RF24 to publish your new component before we can consume it in any project.

Thanks, I can use whatever help I can get. I am a completely new to ESP-IDF.

Currently, the code compiles but it doesn't work. One of the calls to ESP_CHECK_ERROR() is causing an abort() (which resets the ESP32). I've narrowed it down to SPIClass.begin(), but I don't have a debug port available on my QtPy ESP32-S2. So I get no helpful output on the USB CDC serial port.

I pushed my platformIO project for inspection (located in examples_esp/getting_started/).

@2bndy5 you probably need some 😋 🍒 compote component upload --namespace nRF24 --name RF24 to publish your new component before we can consume it in any project.

Thanks for the tip. I'm planning to do that when the branch is ready to merge into master. The next release will likely be v1.5.0. For now, you can simply use git specification in the idf_component.yml.

dependencies:
  nRF24/RF24:
    version: esp-idf
    git: https://github.com/nRF24/RF24.git

The example project(s) will instead use:

RF24/examples_esp/getting_started/src/idf_component.yml

Lines 5 to 9 in d048b2d

    
           nRF24/RF24: 
        
             # for external projects (not included in the RF24 repo), instead use 
        
             # nRF24/RF24: "^1.5" 
        
             # for this project, we use the current repo changes (for testing purposes) 
        
             path: ../../../

so we are running the examples_esp/ projects with the current repo changes.

@2bndy5 where on this planet do you live, maybe we can get an OpenDTU Fusion PCB (including an ESP-S3) shipped to you if it is within the EU ? Outside we would probably need to check export/ import duties. You can drop into our Discord in case you want to leave a private note.

In the US (California). This is public info as far as I'm concerned. I do have solar panels on top of my house, but there's no battery or monitoring capability enabled.

I think the new tankless water heater uses a ESP32 to control the recirculating pump, but I haven't tinkered with it because it isn't broken.

@2bndy5 i asked the OpenDTU Fusion board resellers and they would be able to send you one to CA/USA. However @markusdd may tell us if the latest board rev would allow you to access the Serial Debug Port on the OpenDTU Fusion ?

@LennartF22 has added a new PR to make use of the SPIManager as an abstraction between the user code and other libraries used, ie nRF24L01+, CMT2300A, W5500. This is possible by passing an spi_device_handle_t to the library. Could you @2bndy5 take a look at this PR from Lennart ?

tbnobody/OpenDTU#2306

@stefan123t I already got a slightly modified version of @2bndy5's RF24 driver for the ESP-IDF running. The main issue probably was that queue_size was set to 0 instead of 1, but I'm going to write more later.

The PR LGTM, but it still uses Arduino API. I'm not sire how it is relevant to this feature request.

If you want more eyes reviewing your code/contributions, might I suggest coderabbitai? It can summarize and review PRs for you (and you can set it to output in German 😉 ).

@LennartF22 Thank you for taking the initiative! 🚀 Would you mind if I cherry-pick (and squash) commits from your fork's esp-idf branch? I see there are merge conflict that would make a PR to upstream difficult.

Yeah, the OpenDTU PR currently does not contain any interesting changes regarding the usage of the RF24 library, so there we do not use a shared SPI bus for the nRF yet.

There is another commit that‘s not included in the PR that changes this: LennartF22/OpenDTU@1b9f181

I'm still not particularly happy with how I solved the issue that we are using the Arduino Framework in OpenDTU, but still need to use a „downgraded“ version of the RF24 ESP-IDF that does not redefine the functions already defined by the Arduino framework.

Apart from the specific, aforementioned hacks (like the renaming of the SPIClass, the locking, and the #ifdef ARDUINO stuff) you can use some of my changes of course.

I've already thought about a better/simpler approach for our situation, but more on that later.

There is another commit that‘s not included in the PR that changes this: LennartF22/OpenDTU@1b9f181

This commit explains the RF24_FORCE_ESP_IDF macro I saw in your branch.

I'm working to merge the useful parts now. I was unable to save the commit author info after I squashed the numerous commits you pushed to your fork. I'll tag you in the PR that will merge into our esp-idf branch (not ready for master yet), and I'll add you as a co-author in the merge commit's trailer. First I'll have to test it... 🤞🏼

Yikes, that git history is a mess. I think that there were a lot of LF -> CRLF changes (undesirable). It looks like you git merged upstream's branch into your origin's branch. This means that commits from upstream (which are older) got pushed on top of your changes (which are newer). A rebase onto upstream would have made cherry-picking commits a lot easier.

I think I'm going to try to just take some improvements from your branch manually. I especially like the use of semaphore mutex in SPIClass::begin/endTransaction().

The main issue probably was that queue_size was set to 0 instead of 1

I also don't see this change in your remote. Maybe it hasn't been pushed from your local?

@2bndy5 i asked the OpenDTU Fusion board resellers and they would be able to send you one to CA/USA. However @markusdd may tell us if the latest board rev would allow you to access the Serial Debug Port on the OpenDTU Fusion ?

@stefan123t Sounds good. Although, if there's no exposed DAP, then I'm afraid it would be pointless to send me one. I have several ESP32 boards lying around, but none of them have a DAP exposed. I have a few debug probes sitting around: STLink V2 and a Segger J-link (for non-commercial purposes). Having no DAP is what halted my progress months ago.

My esp-idf Branch is a version of your esp-idf that I simply rebased to the current master. My changes are on top of that. As far as I see, there are no line ending changes.

I would expect cherry picking to work as usual, although there might be so merge conflicts at locations where there have been changes in the master that are not in your esp-idf branch of course.

In general though, I think it makes more sense for you to just look through my commits, and just apply relevant changes to your branch, anyways, since some commits contain „desired“ and „undesired“ changes at once.

I guess having the mutex does not hurt, but ideally, the caller would have to make sure that they are not calling RF24 functions from different tasks/threads in parallel, and I‘m not sure whether the rest of the library is thread safe already. I had to (at least temporarily) add it, because in OpenDTU, the RF24 library is used from different tasks in parallel (although I think it's wrong to do so and that it must be fixed in OpenDTU in the future).

I did not change the queue_size in the RF24 library, as it was not necessary for a quick proof of concept (we are passing a ready-to-use spi_device_handle_t where queue_size was set correctly), but I noticed you were setting queue_size = 0, which I‘m pretty sure is wrong.

I noticed you were setting queue_size = 0, which I‘m pretty sure is wrong.

The ESP-IDF docs were somewhat confusing about that. My limited understanding is that field was specific to Interrupt or DMA usage. Maybe I'm using DMA without knowing it. Would a higher value like 3 be better (because there's 3 slots in the radio's FIFOs)?

Sounds good. Although, if there's no exposed DAP, then I'm afraid it would be pointless to send me one.

The ESP32-S3 on the OpenDTU Fusion board has an integrated USB serial/JTAG controller, which can be accessed via USB-C. Regular JTAG pins are also exposed via 2.54mm headers.

I've only got ESP32-S2 or older. I tend to spend money on boards that I can also test with CircuitPython.

I noticed you were setting queue_size = 0, which I‘m pretty sure is wrong.

The ESP-IDF docs were somewhat confusing about that. My limited understanding is that field was specific to Interrupt or DMA usage. Maybe I'm using DMA without knowing it. Would a higher value like 3 be better (because there's 3 slots in the radio's FIFOs)?

Since we are not using queued transactions, setting the queue_size to anything larger than 1 does not have any effect.

spi_device_polling_transmit(…) aquires the bus (if not already acquired), puts the transaction into the queue, and waits for it to complete. After completion, the SPI bus is released again (if it was not acquired externally), so that the next polling_transmit can take place. Therefore, the queue will never have more than one entry.

@2bndy5 i asked the OpenDTU Fusion board resellers and they would be able to send you one to CA/USA. However @markusdd may tell us if the latest board rev would allow you to access the Serial Debug Port on the OpenDTU Fusion ?

@stefan123t Sounds good. Although, if there's no exposed DAP, then I'm afraid it would be pointless to send me one. I have several ESP32 boards lying around, but none of them have a DAP exposed. I have a few debug probes sitting around: STLink V2 and a Segger J-link (for non-commercial purposes). Having no DAP is what halted my progress months ago.

As far as I could determine the ESP32-S3 can be debugged via the USB-C Port using the D-/D+ USB Data lines connected to GPIO19 / 20 respectively, connect GND and VCC (if not already powered via the USB Serial !).

See here: Configure ESP32-S3 Built-in JTAG Interface and here: JTAG Debugging

As hardware you may use a whole range of JTAG devices like STLink V2, Segger J-Link, OpenOCD, an Arduino, etc.

The ESP32-S2 may also be used with JTAG, though this comes under several CAVEATs, see here JTAG Debugging - Configure and Connect JTAG Interface, here for the Espressif DevBoard Configure ESP32-S2-Kaluga-1 JTAG Interface and here to Configure Other JTAG Interfaces.
Main exclusion I learned seems to be any type of SWD access is not possible as far as I looked into it.

Tim from Alliance App's will contact you on your mail to ask for the address details.

	nRF24/RF24:
	# for external projects (not included in the RF24 repo), instead use
	# nRF24/RF24: "^1.5"
	# for this project, we use the current repo changes (for testing purposes)
	path: ../../../