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

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

Closed
IanBurwell opened this issue Nov 18, 2024 · 4 comments
Closed
Assignees

Comments

@IanBurwell
Copy link

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
Copy link
Owner

jgromes commented Nov 18, 2024

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
Copy link
Author

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

@jgromes jgromes self-assigned this Nov 27, 2024
@jgromes
Copy link
Owner

jgromes commented Nov 30, 2024

For the time being, I went with option 2 - that is to move the HAL back to examples. I have also slightly updated the errro message to hoefully make it clearer that this is just an example, and that support other targets requires some porting from the user.

@jgromes jgromes closed this as completed Nov 30, 2024
@IanBurwell
Copy link
Author

I think that is a good way of handling it for now. I got things running on the ESP32S3 by changing the SPI functions you defined in that example HAL to use the ESP-IDF SPI master driver. I could honestly see Option 3 being implemented easily with the ESP-IDF's HAL drivers, but they are not technically stable yet so it would have to pin the supported ESP-IDF version. I will continue with my modified HAL for now as I am pinning my own ESP-IDF version.

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

No branches or pull requests

2 participants