Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature request] support ESP-IDF platform #925

Open
stefan123t opened this issue Oct 24, 2023 · 19 comments
Open

[feature request] support ESP-IDF platform #925

stefan123t opened this issue Oct 24, 2023 · 19 comments

Comments

@stefan123t
Copy link

stefan123t commented Oct 24, 2023

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 ?

@2bndy5
Copy link
Member

2bndy5 commented Oct 24, 2023

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

@2bndy5
Copy link
Member

2bndy5 commented Oct 24, 2023

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.

@LennartF22
Copy link

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.

@LennartF22
Copy link

LennartF22 commented Oct 24, 2023

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 😅

@2bndy5
Copy link
Member

2bndy5 commented Oct 24, 2023

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.

@2bndy5
Copy link
Member

2bndy5 commented Oct 24, 2023

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

@stefan123t
Copy link
Author

stefan123t commented Oct 25, 2023

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.

@2bndy5
Copy link
Member

2bndy5 commented Oct 25, 2023

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:

  1. 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
  2. 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.

@2bndy5
Copy link
Member

2bndy5 commented Oct 25, 2023

@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.

@LennartF22
Copy link

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).

@markusdd
Copy link

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^^

@2bndy5
Copy link
Member

2bndy5 commented Oct 25, 2023

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.

@stefan123t
Copy link
Author

@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.

@2bndy5
Copy link
Member

2bndy5 commented Oct 27, 2023

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.

@2bndy5
Copy link
Member

2bndy5 commented Mar 17, 2024

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).

@2bndy5 2bndy5 changed the title [Question] HAL for SPI abstraction on ESP the platform [feature request] support ESP-IDF platform Apr 7, 2024
@2bndy5
Copy link
Member

2bndy5 commented Apr 7, 2024

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

@2bndy5
Copy link
Member

2bndy5 commented Apr 8, 2024

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.

@stefan123t
Copy link
Author

@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.

@2bndy5
Copy link
Member

2bndy5 commented Apr 9, 2024

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:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants