jgromes / RadioLib

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

Cannot load RadioLib.h and can't #include <algorithm> in esp-idf on platformio #748

Closed nicklasb closed 1 year ago

nicklasb commented 1 year ago

I only seem to be able to use RadioLib.h by way absolute paths; like /Users/username/Documents/PlatformIO/Projects/RobArch/.pio/libdeps/TTGO-LoRa32-v1/RadioLib/src/RadioLib.h

However, I then get this error: /Users/username/Documents/PlatformIO/Projects/RobArch/.pio/libdeps/TTGO-LoRa32-v1/RadioLib/src/BuildOpt.h:313:12: fatal error: algorithm: No such file or directory

Which I think that may contribute to the compiler not finding RadioLib.h, as I have tried all tricks to make the component visible to ESP-IDF, like adding CMakelists.txt and similar.

In BuildOpt my builds become "generic non-Arduino platform" builds, and that is when they try to include the algorithms library.

Notably, I run this an a MacOs, but I have tried with all compilers I have found (CLANG 14, GCC 13 etcetera).

Great library, I have almost gotten everything to work now, but I suppose there is always something. :-)

jgromes commented 1 year ago

I can't test this on MacOS, did you try installing the library using cmake? See this wiki page: https://github.com/jgromes/RadioLib/wiki/Porting-to-non-Arduino-Platforms

EDIT: regarding algorithm, I will have to check what exactly needs it, maybe there's a way to get rid of it.

nicklasb commented 1 year ago

Hi, I have done the things on that page, including the HAL. That seems to work now (really needed to understand what is really a part of SPI and what isn't). I'd rather not install it as a global library as that would bypass the dependency management of the development frameworks and make it seem portable while really not being. I would also not have control over what I was actually testing. I tried removing it, but then I got a whole bunch of errors all over. Things like "using" seems to be a part of that.

Strangely, I haven't had any problems with this until I started referring to the library from different directions. Have you tried using it from PlatformIO?

nicklasb commented 1 year ago

I am thinking that a part of the problem may be that I can't refer to it as I can to other packages in PlatformIO. As you say in the wiki, the normal way should be #include <RadioLib.h> or #include <RadioLib/RadioLib.h>, which doesn't work. Perhaps absolute referrals puts contexts...out of context or something?

I did add a CMakelists.txt, not sure that PlatformIO cares about it post-install though.

nicklasb commented 1 year ago

Just to be clear, realized that I had not actually said it, but for indexing: #include <RadioLib.h> does not work after following: https://registry.platformio.org/libraries/jgromes/RadioLib/installation

If I use a relative path (../../../.pio/XX/libdeps and so on), it works, but then the relative path will fail when other parts my system wants to include it. (seems that relative paths aren't all that relative to where they are declared or something).

nicklasb commented 1 year ago

I changed to depend on https://github.com/jgromes/RadioLib.git instead and still it didn't work. However, that had a CMakeLists.txt, which should've helped. Annoyingly, only VSCode resolves it, not PlatformIO (or ESP-IDF).

I also tried with a relative path to it now, for some reason relative paths now work (could it be because of CMakelist?) but still the same result with not finding <algorithm>

nicklasb commented 1 year ago

Ok. Well. This seems all to be about the compiler being confused with C/C++ because of the full path #include. Or at least that is my theory for now.

After putting all the C++ stuff, like the RadioLib classes in one file, so that RadioLib did not have to be imported by anyone else, it started to compile again.

However, and I think that this might be part of the issue; I still cannot include RadioLib normally.

#include <RadioLib.h>
#include <RadioLib/RadioLib.h>
#include <RadioLib/src/RadioLib.h>

doesn't work.

Only thing that works is: #include "/full_path_to_project/RobArch/.pio/libdeps/TTGO-LoRa32-v1/RadioLib/src/RadioLib.h"

Regardless if I am using the latest release or master.

jgromes commented 1 year ago

@nicklasb in the last few commits, the dependency on algorithm should be resolved. As to the rest of your issues, I'll try to replicate them with ESP-IDF. If you could share your HAL implementation and Cmake, that would help me get started.

nicklasb commented 1 year ago

Great with the algorithm dependency!

The HAL implementation is quite complex as it in turn depends on a framework (which is sort of a HAL itself and still too embarrassing to share) I am developing. That should not be a factor, though. I know, famous last words. :-)

But I'll set up a project and try to replicate the issue, and share it with you. This might also be related to PlatformIO, not only ESP-IDF of course.

nicklasb commented 1 year ago

@jgromes Hi, it doesn't seem like the issue was related to the dependency. It doesn't include after that fix either, and I have not yet succeeded in replicating it in a smaller project. I created a small project (https://github.com/nicklasb/RadioLib_PIO_ESPIDF) with an empty HAL and there it compiles. I haven't been able to make that break either.

I think that you might be onto something with that it has something to do with the HAL. I will keep trying tomorrow, if I can make this work it'd be great. Could one overdo extern "C"?

nicklasb commented 1 year ago

Hi, I don't seem to be able to figure this out. I will continue with relative includes for a while and see if I will come to some insights when I continue developing.

It is likely that it has something to do with how I am implementing my HAL, however I see no strange includes in that that could cause issues, it does what ArduinoHal does, basically. It should be noted, though that ArduinoHal is inside the library, and the same folder.

There was a comment in /RadioLib/extras/test/SX126x/CMakeLists.txt about having to use add_subdirectory if one haven't built it as a shared library...but if I do that, it would seem that I'd break the dependency-driven model.

jgromes commented 1 year ago

I don't see your HAL implementation in the repository you linked, so I can't really comment on that. In the meantime, I started writing my own ESP-IDF HAL and everything seems to be working so far - although it is without platformio. Once it's done I will add it to the NonArduino examples as a reference.

nicklasb commented 1 year ago

That sounds like a great idea! My issues where:

  1. SPI - Transactions aren't really a thing, in my implementation BeginTransation and EndTransaction doesn't do anything. Still works.
  2. Pulse - That API doesn't exist on ESP-IDF, did a crappy implementation I haven't properly tested yet.
  3. pinMode - Direction is separate in ESP-IDF, so that becomes something to think about.

My HAL works against a framework that already hides if it is Arduino, ESP-IDF or STM32, so I basically just added the above code. I suppose that logging to ESP_LOGX will be something you'd need to implement too. Another thing I did was to use the XPower library for managing the axp192 on the TTGO T-beam.

nicklasb commented 1 year ago

OK. I have now recreated the problem in the above mentioned repo: https://github.com/nicklasb/RadioLib_PIO_ESPIDF

The problem happens when the code #include:ing RadioLib.h is an ESP-IDF-component.

I suppose that the solution has something to do with being able to refer to RadioLib as such, currently that doesn't seem possible. But actually, it should not be needed.

nicklasb commented 1 year ago

I tried adding an idf_component.yml, to make the component handle its own dependencies:

  # Required IDF version
  idf: ">=4.1"
  RadioLib:
    git: https://github.com/jgromes/RadioLib.git

But then the CMakeLists.txt for the library is configured as a project, so that causes an error. I will ask for guidance at the PlatformIO issues.

nicklasb commented 1 year ago

Posed a question here: https://community.platformio.org/t/how-do-i-refer-to-a-library-in-libdeps-from-an-esp-idf-component-in-components/33867

nicklasb commented 1 year ago

So it would appear that in order to be used by ESP-IDF-components, RadioLib needs to have a CMakeLists.txt that works for that purpose. Not sure how that is done, though, obviously project(radiolib) can't be there, at least like that.

I asked Espressif for a solution: https://github.com/platformio/platform-espressif32/issues/1100

jgromes commented 1 year ago

@nicklasb I added a functional ESP-IDF example - functional as in it builds, flashes and transmits via SX1278. Feel free to take a look at it, maybe you will be able to identify the issue you're seeing?

It's not using platformio, but I don't think that should be the issue - I didn't have to do any changes to the root RadioLib cmake to make it work (as per your previous comment).

nicklasb commented 1 year ago

Hi, The issue I was seeing WRT communication was solved, everything works now. Really well too, RadioLib is a really cool library, you've done great work!

WRT the component situation, the implementation has a relative path to the location of the library, so it doesn't help I am afraid as that is my current and very temporary workaround, and the CMakeList of RadioLib has a "project" definition.

I proposed that one perhaps could use conditionals in the CMakeLists.txt, I know it is one-pass but I think that it should work.

The ESP-IDF HAL is nice. Even though my HAL..eh...works...yours seems way more worked-through, I am not really an embedded dev by trade, so my bag of tricks isn't particularly full, there's a couple of gems there I'll reuse. :-)

jgromes commented 1 year ago

the CMakeList of RadioLib has a "project" definition.

I still don't understand why this is a problem. The project definition in RadioLib is not strictly necessary, so it can be removed, but I don't understand why is this causing trouble.

nicklasb commented 1 year ago

the CMakeList of RadioLib has a "project" definition.

I still don't understand why this is a problem. The project definition in RadioLib is not strictly necessary, so it can be removed, but I don't understand why is this causing trouble.

Ok, I will try some variants. It causes trouble because I am getting an error if such configs are in components. (not scriptable and so on)

jgromes commented 1 year ago

It causes trouble because I am getting an error if such configs are in components. (not scriptable and so on)

Sorry, that's still not descriptive enough for me to understand. A working example would be nice, or at least the structury (e.g. - copy-paste RadioLib source to components/, make some change to the top-level CMake etc.). As far as I can tell right now there is no problem in the library itself, so I'm closing this issue for the moment. There clearly exists a way to build RadioLib for ESP-IDF.

Additionally, I don't think RadioLib CMake should include anything ESP-IDF specific, such as the idf_component command(s). Otherwise we would have to keep adapting the library to different build systems ...

nicklasb commented 1 year ago

Hi, I recreated the problem before, in the https://github.com/nicklasb/RadioLib_PIO_ESPIDF I referenced above. The problem is that an ESP-IDF component can not have a project() definition and other non-scriptable stuff in its CMakeLists.txt, but RadioLib does. If I want add RadioLib as a dependency to a component in an ESP-IDF project, it will be added to a "managed components" library, where it will cause an error.

In your example, RadioLib is not added as a component, but a local component is added to the project and then Radiolib to it. This is similar to my current workaround, where I have a relative path to where RadioLib resides in LibDeps. In both we need to make assumptions and have either absolute paths and know the name of the current PIO environment, or have relative paths but having to either be inside RadioLib or put it next to ones project.

IMO, I consider neither that as a very redistributable workaround, I want my stuff to be just one of many easily added dependencies. But I will fork RadioLib and try out a lot of variants, I'll get back to you with something that hopefully works.

Adapting to different build systems is something that I feel is a kind of natural chore for shared libraries, though?

jgromes commented 1 year ago

The problem is that an ESP-IDF component can not have a project() definition and other non-scriptable stuff in its CMakeLists.txt, but RadioLib does

I tried to approach a bit more to your configuration (RadioLib directly cloned into components/), removed the project() command from RadioLib root CMake, and the only thing that this changes is that not even add_library can be used. For me, having this is a hard requirement, because without it, CMake-based build system won't be able to use RadioLib as a ... well, library.

The ultimate problem is that ESP-IDF uses CMake in script mode and assumes components will be compatible with it. RadioLib obviously isn't, so the only thing that I can do at this level is to guard the CMake commands that cannot be used in script mode with if(NOT(CMAKE_SCRIPT_MODE_FILE)) but that would effectively make the RadioLib root CMake empty, not sure if that helps.

nicklasb commented 1 year ago

Yes, but in a parallel, and quite unrelated, discussion (https://github.com/ttlappalainen/NMEA2000_esp32/pull/12#issuecomment-1555244948) I saw this guy simply do if(ESP_PLATFORM) around the ESP stuff and the rest in the else().

I haven't tried that myself yet, won't have time for that until the weekend, but it seemed promising and worked for him.

nicklasb commented 1 year ago

@jgromes, Hi, I seems like that worked! I forked this library, made a CMakeLists.txt with an if(ESP_PLATFORM) like above it works. Check out the https://github.com/nicklasb/RadioLib_PIO_ESPIDF repo, the master branch builds using this method. Also, It seems like it keeps working doing a normal cmake build, so it doesn't break the library.

Granted, I haven't tried with a huge number of projects yet, but at least this example library and my framework (that uses many more parts of RadioLib due to its HAL), works well like this. Running ESP-IDF, it adds a managed_components folder in the project.

(Thanks @phatpaul for the inspiration!)

nicklasb commented 1 year ago

@jgromes Oh, forgot to add the link to the fork: https://github.com/nicklasb/RadioLib.git The CMakeLists.txt is what is of interest, of course.

jgromes commented 1 year ago

I'm not a huge fan of this solution, I don't think it should be the job of the library to adapt to non-standard build systems (and ESP-IDF does require calling idf_component_register). The sheer popularity of ESP platform somewhat forces my hand here, so I will implement it, however, with two modifications:

  1. Instead of if/else, I will use if/return, same as is done in ArduinoJson ( https://github.com/bblanchon/ArduinoJson/blob/6.x/CMakeLists.txt). That way, if some other modification for some other non-standard build system is needed, it can be added without additional branching.

  2. I will also add conditional error to be raised when the library is being compiled in script mode (after the ESP_PLATFORM condition, of course), since that is primary issue.

nicklasb commented 1 year ago

@jgromes My view is that there are no standard build systems, they are just popular/unpopular. Basically because a build system is a way too complex a thing to standardize. A standard then become a problem because it has to have its own developer community and handle all possible situations, ending up either too political, complicated or a hurdle that stops progress*. Maybe I have been a bit burnt by "standards" in a lot of other languages and platforms, but that is how I see it.

Sure, that is probably better, looks better, too.

*For example, I am pretty sure that Espressif didn't make ESP-IDF just to mess with the community and lock people in, the costs associated would never justify that singular end.

jgromes commented 1 year ago

@nicklasb added into the root CMake and also updated the example - thanks for investigating this!

nicklasb commented 1 year ago

@nicklasb added into the root CMake and also updated the example - thanks for investigating this!

Works great for me! Thanks for implementing!!