search

jgromes / RadioLib

Universal wireless communication library for embedded devices
https://jgromes.github.io/RadioLib/
MIT License
1.59k stars 395 forks source link

Only supports ESP32 but this is not specified in the ESP-IDF manifest file #1322

Open IanBurwell opened 1 week ago

IanBurwell commented 1 week ago

In the ESP components registry RadioLib is listed as "supports all targets" but the code only compiles for a basic esp32. The following should be added to the idf_component.yml file:

targets:
  - esp32

Esp component registry screenshot: image

jgromes commented 1 week ago

I'm a bit conflicted here, because the fact that this only compiles for an ESP32 is intentional by that macro check you have pointed out. The reasoning is that the ESP-IDF HAL is pretty much just an example how to implement RadioLib HAL in ESP-IDF environment. It's not really the intention to have a fully compatible HAL for all ESP targets - at that point we are basically re-inventing Arduino.

Bulk of this library is platform-agnostic, all the platform-dependent code is in the HAL. But I do get that this can cause some confusion. There are a few options how to proceed:

  1. Do what you suggest and mark this as only supporting the base ESP32. I don't like that approach because it hides the fact that porting to a new ESP device is as simple as we could make it. You don't have to do any modifications to the library core, you just have to provide basic stuff like SPI reading and writing.
  2. Move the current ESP-IDF HAL back just to examples and make it clear in the readme that porting to something like ESP32-S3 is up to the user.
  3. Make the ESP-IDF HAL compatible with all ESP32 targets. This is by far my least preferred option since my experience with ESP-IDF is limited, and testing this would be challenging to say the very least.
IanBurwell commented 1 week ago

I think the confusion here is that as an esp-idf component it currently only supports the ESP32 out of the box. But I agree that saying it only supports the esp32 does hide the fact that it should be relativity easy to support other variants (and this is just something the user needs to do). Maybe the error/comment should more specifically call out that other targets (etc esp32s3) should be implemented and tested by the user, using the esp32 hal as an example?

Regardless I may take a deeper look myself at the current example HAL and see what changes one would need to make to have it support the ESP32S3 (which I have).