Non arduino build not working

[Mesteery]

Describe the bug I'am trying to port radiolib to raspberrypi 4. Firstly, to make radiolib compile successfuly, i made these changes: https://github.com/Mesteery/RadioLib/commit/b48d9ae4c4e939e39e05c950005e22a3151ba3d3. (I wanted to test my changes before opening a PR) Plus these few lines in noarduino.h:

enum class byte : uint8_t {};

#define RISING 0
#define FALLING 0 // just trying to compile, the value is not important in this issue
#define DEC 10

#define RADIOLIB_NONVOLATILE
#define RADIOLIB_NONVOLATILE_READ_BYTE(addr) (*(const uint8_t *)(addr))

And enabled GODMODE. On compilation i have the error:

src/main.cpp: In function ‘int main(int, char**)’:
src/main.cpp:7:9: error: ‘class SX1278’ has no member named ‘setCb_digitalWrite’
    7 |   radio.setCb_digitalWrite(::digitalWrite);
      |         ^~~~~~~~~~~~~~~~~~

It's maybe because of the toolchain I use or a misconfiguration because if I remove this line, I have another error:

main.cpp:(.text.startup+0x28): undefined reference to `Module::Module(unsigned char, unsigned char, unsigned char, unsigned char)'
/usr/bin/ld: main.cpp:(.text.startup+0x34): undefined reference to `SX1278::SX1278(Module*)'

I am using xmake with the following config:

add_rules("mode.debug", "mode.release")

package("RadioLib")
    set_homepage("https://github.com/jgromes/RadioLib/")
    set_description("Universal wireless communication library for embedded devices ")
    set_license("MIT")

    add_urls("https://github.com/Mesteery/RadioLib/archive/refs/tags/$(version).tar.gz",
             "https://github.com/Mesteery/RadioLib.git")
    add_versions("5.7.0", "ead59223faaf225087b1db8da852b49d5e5947d9")

    on_install(function (package)
        os.cp(path.join("src", "*"), package:installdir("include"))
    end)
package_end()

add_requires("RadioLib fix-generic-build")

target("radio")
    set_kind("binary")
    add_defines("RADIOLIB_GODMODE")
    add_includedirs("include") -- include/noarduino.h
    add_packages("RadioLib")
    set_languages("c++17")
    add_files("src/*.cpp")

To Reproduce

//main.cpp
#include <cstddef>

#include "RadioLib.h"

int main(int argc, char** argv) {
  SX1278 radio = new Module(1, 2, 3, 4);
  radio.setCb_digitalWrite(::digitalWrite);
  return 0;
}

//noarduino.cpp
#include <iostream>

void digitalWrite(uint32_t pin, uint8_t value) {
  std::cout << "call" << std::endl;
}

and finally noarduino.h with the modification above (+ template in the wiki) and

void digitalWrite(uint8_t pin, uint8_t value);
#define RADIOLIB_CB_ARGS_DIGITAL_WRITE \
  (void, digitalWrite, uint32_t pin, uint8_t value)

Expected behavior Compile successfuly

[jgromes]

The first error is understandable, the Wiki is wrong in this regard. It should be like this:

// some random NSS/IRQ/RST/GPIO pins
Module mod(1, 2, 3, 4);
SX1278 radio(&mod);

(...)
mod.setCb_digitalRead(::wrap_HAL_GPIO_ReadPin);
mod.setCb_digitalWrite(::wrap_HAL_GPIO_WritePin);
// etc. for all the callbacks
(...)

// we can go now!
radio.begin();

Try it and let me know if the second error persists. That one is strange, because the default contructor for Module should be available.

Also, thanks for testing this! The non-arduino build is something of a hack.

[Mesteery]

Thanks you for your response. I tried Module mod(1, 2, 3, 4); alone to begin, and the undefined reference persist. I have the impression that it comes from xmake, I will try to dig a little more.

[Mesteery]

Finally, i succeeded to compile the lib, the problem was coming from my configuration (by the way, the include of noarduino.h is quite strange and was the origin of the errors). I also pushed a new commit to https://github.com/Mesteery/RadioLib/tree/fix-generic-build. I started to port it to pi using pigpio lib, and I discovered that if the name of the HAL function is not the same as the Arduino base function name generate a lot of problems. For example, If I have mod.setCb_digitalRead(::wrap_HAL_GPIO_ReadPin);, the macro RADIOLIB_GENERATE_CALLBACK will generate the class members with the name of the HAL function, and not the Arduino name which is used in the rest of the lib, so setCb_ become setCb_wrap_HAL_GPIO_ReadPin and cb_digitalRead remains undefined. This is "normal" since FUNC refers to the supplied HAL function and not the Arduino function name: https://github.com/jgromes/RadioLib/blob/8587f73bd98ada2f20e5147c3656c7b391cff28f/src/BuildOpt.h#L1082 Maybe, we can add an argument for the Arduino function name like

    RADIOLIB_GENERATE_CALLBACK(RADIOLIB_CB_ARGS_PIN_MODE, "pinMode");
    RADIOLIB_GENERATE_CALLBACK(RADIOLIB_CB_ARGS_DIGITAL_WRITE, "digitalWrite");
    RADIOLIB_GENERATE_CALLBACK(RADIOLIB_CB_ARGS_DIGITAL_READ, "digitalRead");

or event better, rethink the non-arduino/compat mechanism?

[jgromes]

I think this is again a mistake/bad documentation in the Wiki: the Arduino function name should be left in the CB_ARGS macros so that Arduino-like callbacks are generated. Following the example from the Wiki for STM32 HAL:

// noarduino.h
#define RADIOLIB_CB_ARGS_DIGITAL_WRITE              (void, digitalWrite, uint32_t pin, GPIO_PinState value)

or event better, rethink the non-arduino/compat mechanism?

I'm not opposed to it, but I don't know how else to handle this. At the end of the day, the only thing the callbacks and macros do is hide a huge amount of boiler-plate code, and give you some flexibility as to the the signature of your HAL functions.

[Mesteery]

SPI functions does not seem to be called.

[jgromes]

Thanks for pointing this out, there was indeed some weirdness in the SPI callbacks. Could you try with the latest commit?

[Mesteery]

Thanks you for your fast fix. However it does not compile. Verbose error:

error: src/Module.cpp: In member function ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’:
src/Module.cpp:231:9: error: ‘class Module’ has no member named ‘SPIbeginTransaction’; did you mean ‘beginTransaction’?
  231 |   this->SPIbeginTransaction();
      |         ^~~~~~~~~~~~~~~~~~~
      |         beginTransaction
src/Module.cpp:238:32: error: no matching function for call to ‘Module::SPItransfer(int)’
  238 |     this->SPItransfer(reg | cmd);
      |                                ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:240:39: error: no matching function for call to ‘Module::SPItransfer(int)’
  240 |     this->SPItransfer((reg >> 8) | cmd);
      |                                       ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:241:33: error: no matching function for call to ‘Module::SPItransfer(int)’
  241 |     this->SPItransfer(reg & 0xFF);
      |                                 ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:257:37: error: no matching function for call to ‘Module::SPItransfer(uint8_t&)’
  257 |         this->SPItransfer(dataOut[n]);
      |                                     ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:264:43: error: no matching function for call to ‘Module::SPItransfer(int)’
  264 |         dataIn[n] = this->SPItransfer(0x00);
      |                                           ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:275:9: error: ‘class Module’ has no member named ‘SPIendTransaction’; did you mean ‘endTransaction’?
  275 |   this->SPIendTransaction();
      |         ^~~~~~~~~~~~~~~~~
      |         endTransaction
src/Module.cpp: In member function ‘void Module::begin()’:
src/Module.cpp:614:11: error: ‘((Module*)this)->Module::cb_SPIbegin’ cannot be used as a member pointer, since it is of type ‘Module::SPIbegin_cb_t’ {aka ‘void (*)()’}
  614 |   (this->*cb_SPIbegin)();
      |           ^~~~~~~~~~~
src/Module.cpp: In member function ‘void Module::beginTransaction()’:
src/Module.cpp:621:11: error: ‘((Module*)this)->Module::cb_SPIbeginTransaction’ cannot be used as a member pointer, since it is of type ‘Module::SPIbeginTransaction_cb_t’ {aka ‘void (*)()’}
  621 |   (this->*cb_SPIbeginTransaction)();
      |           ^~~~~~~~~~~~~~~~~~~~~~
src/Module.cpp: In member function ‘uint8_t Module::transfer(uint8_t)’:
src/Module.cpp:628:18: error: ‘((Module*)this)->Module::cb_SPItransfer’ cannot be used as a member pointer, since it is of type ‘Module::SPItransfer_cb_t’ {aka ‘unsigned char (*)(unsigned char)’}
  628 |   return((this->*cb_SPItransfer)(b));
      |                  ^~~~~~~~~~~~~~
src/Module.cpp: In member function ‘void Module::endTransaction()’:
src/Module.cpp:635:11: error: ‘((Module*)this)->Module::cb_SPIendTransaction’ cannot be used as a member pointer, since it is of type ‘Module::SPIendTransaction_cb_t’ {aka ‘void (*)()’}
  635 |   (this->*cb_SPIendTransaction)();
      |           ^~~~~~~~~~~~~~~~~~~~
src/Module.cpp: In member function ‘void Module::end()’:
src/Module.cpp:642:11: error: ‘((Module*)this)->Module::cb_SPIend’ cannot be used as a member pointer, since it is of type ‘Module::SPIend_cb_t’ {aka ‘void (*)()’}
  642 |   (this->*cb_SPIend)();
      |           ^~~~~~~~~

(The lines number are different from here because I am on https://github.com/Mesteery/RadioLib/tree/fix-generic-build, but only debugs was changed so it should not affect this)

[jgromes]

I see - I was able to find some more references to Arduino callbacks, everything should be removed, as a test it does compile for me if I undef the RADIOLIB_BUILD_ARDUINO macro in some places. Could you try again with the latest fix?

[Mesteery]

Verbose

error: src/Module.cpp: In member function ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’:
src/Module.cpp:231:9: error: ‘class Module’ has no member named ‘SPIbeginTransaction’; did you mean ‘beginTransaction’?
  231 |   this->SPIbeginTransaction();
      |         ^~~~~~~~~~~~~~~~~~~
      |         beginTransaction
src/Module.cpp:238:32: error: no matching function for call to ‘Module::SPItransfer(int)’
  238 |     this->SPItransfer(reg | cmd);
      |                                ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:240:39: error: no matching function for call to ‘Module::SPItransfer(int)’
  240 |     this->SPItransfer((reg >> 8) | cmd);
      |                                       ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:241:33: error: no matching function for call to ‘Module::SPItransfer(int)’
  241 |     this->SPItransfer(reg & 0xFF);
      |                                 ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:257:37: error: no matching function for call to ‘Module::SPItransfer(uint8_t&)’
  257 |         this->SPItransfer(dataOut[n]);
      |                                     ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:264:43: error: no matching function for call to ‘Module::SPItransfer(int)’
  264 |         dataIn[n] = this->SPItransfer(0x00);
      |                                           ^
src/Module.cpp:229:6: note: candidate: ‘void Module::SPItransfer(uint8_t, uint16_t, uint8_t*, uint8_t*, size_t)’
  229 | void Module::SPItransfer(uint8_t cmd, uint16_t reg, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes) {
      |      ^~~~~~
src/Module.cpp:229:6: note:   candidate expects 5 arguments, 1 provided
src/Module.cpp:275:9: error: ‘class Module’ has no member named ‘SPIendTransaction’; did you mean ‘endTransaction’?
  275 |   this->SPIendTransaction();
      |         ^~~~~~~~~~~~~~~~~
      |         endTransaction
src/Module.cpp: In member function ‘void Module::begin()’:
src/Module.cpp:614:11: error: ‘((Module*)this)->Module::cb_SPIbegin’ cannot be used as a member pointer, since it is of type ‘Module::SPIbegin_cb_t’ {aka ‘void (*)()’}
  614 |   (this->*cb_SPIbegin)();
      |           ^~~~~~~~~~~
src/Module.cpp: In member function ‘void Module::beginTransaction()’:
src/Module.cpp:621:11: error: ‘((Module*)this)->Module::cb_SPIbeginTransaction’ cannot be used as a member pointer, since it is of type ‘Module::SPIbeginTransaction_cb_t’ {aka ‘void (*)()’}
  621 |   (this->*cb_SPIbeginTransaction)();
      |           ^~~~~~~~~~~~~~~~~~~~~~
src/Module.cpp: In member function ‘uint8_t Module::transfer(uint8_t)’:
src/Module.cpp:628:18: error: ‘((Module*)this)->Module::cb_SPItransfer’ cannot be used as a member pointer, since it is of type ‘Module::SPItransfer_cb_t’ {aka ‘unsigned char (*)(unsigned char)’}
  628 |   return((this->*cb_SPItransfer)(b));
      |                  ^~~~~~~~~~~~~~
src/Module.cpp: In member function ‘void Module::endTransaction()’:
src/Module.cpp:635:11: error: ‘((Module*)this)->Module::cb_SPIendTransaction’ cannot be used as a member pointer, since it is of type ‘Module::SPIendTransaction_cb_t’ {aka ‘void (*)()’}
  635 |   (this->*cb_SPIendTransaction)();
      |           ^~~~~~~~~~~~~~~~~~~~
src/Module.cpp: In member function ‘void Module::end()’:
src/Module.cpp:642:11: error: ‘((Module*)this)->Module::cb_SPIend’ cannot be used as a member pointer, since it is of type ‘Module::SPIend_cb_t’ {aka ‘void (*)()’}
  642 |   (this->*cb_SPIend)();
      |           ^~~~~~~~~

on latest commit 390b425 (Maybe working for a fix on a different branch than master would be better in order not to "pollute" the main branch?)

[jgromes]

There's more commits addressing this referenced, if you cherry-picked just the last one it's not enough. You should merge the changes from master back to your branch.

[Mesteery]

Yes I pulled all changes from master and rebased my branch.

[Mesteery]

I just thought of it but is it right? (it should be SPIbegin or begin for example?)

#define RADIOLIB_CB_ARGS_SPI_BEGIN (void, SPIbegin, void)
#define RADIOLIB_CB_ARGS_SPI_BEGIN_TRANSACTION (void, SPIbeginTransaction, void)|
...

(They seems to be correct if I look at the error)

[Mesteery]

I am very sorry, it compile correctly. My bad, xmake didnt re-cloned the repo, so it was out of date when I compiled. I force it to reinstall and it works!

[Mesteery]

Debug/verbose log:

SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (1 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (2 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (3 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (4 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (5 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (6 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (7 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (8 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (9 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
SPIbeginTransaction
SPItransfer
R       42      SPItransfer
0
SPIendTransaction
SX127x not found! (10 of 10 tries) RADIOLIB_SX127X_REG_VERSION == 0x0000, expected 0x0012
No SX127x found!

(SPIxx are currently only logging functions). Everything seems to work, debug log works, I will create the PR. I will finish the HAL and I will try with the CC1101, I will keep you informed but there should be no problem. Thanks!

[Mesteery]

Creating a HAL for SPI is just impossible or very complex with the current abstraction, because can't store data like handle in a beautiful way. So I'm going to rewrite SPI defines/cb so they can work like the Arduino SPI, with a class.

[jgromes]

Creating a HAL for SPI is just impossible or very complex with the current abstraction

Why? Currenty, it's just a bunch of functions, their implementation is completely up to you, whether you have some SPI class in the background or not. If you add a requirement for the SPI interface to be handled by a class, that doesn't seem to be changing much. The only difference is if you wanted to use multiple different SPI instances for different isntances of Module, but even that can be handled by callbacks.

[Mesteery]

Yes, that's true, but wouldn't it be better to have only SPI instance to pass to the Module ctor like for Arduino and get rid of setCb_SPI... and all the #if/#else SPIs? I don't know how it can be done in C++ but it makes me think of interface in TypeScript (accept any class that have same api as in interface definiton) or also trait in Rust.

[jgromes]

wouldn't it be better to have only SPI instance to pass to the Module ctor like for Arduino

This assumes that instance exists, I can easily imagine cases where no such class is present. For example, the HAL can be written in pure C (this is the case for STM32 HAL), hence it would be up to the user to create their own SPI class.

Also, there would have to be some class with only virtual methods, that is implemented either by Arduino SPIClass or your HAL SPI class. I still fail to see the advantages here, definitely not something that would be impossible or very complex. Maybe I'm just missing it, but what exactly is preventing you from calling your HAL SPI class methods from the callbacks?

The point about needing somplace to store the SPI instance/handle is partially valid, you will need some global variable in your noarduino.cpp implementation file. But from the point of view of the architecture, this seems more correct to me, since RadioLib knows nothing about your particular SPI class and any possible configuration of it. So it only requests that you implement callbacks with narrowly defined functionality, and leaves the rest up to you.

[Mesteery]

You are right it's neither impossible nor very complex. I hadn't thought about it, but indeed, it can be done easily, as you say with a global variable, or by creating a class, and then passing in reference the class methods as callback after having instantiated it (I'm not sure if it's possible but I don't see why it could not?). In fact, it ties in with my (previous) idea of rethinking the HAL, and make it more practical. We could have an abstract class HAL for example, then extend it with an default implementation for Arduino, and pass it to the Module constructor. If a user want to support another platform, he would just have to create a class that extend HAL, and pass it to the Module, without having complicated noarduino.h/cpp. For reference, I spent a day getting xmake include the user-defined noarduino.h during the compilation of the lib. Finally, I'm not very familiar with C++ design/good practices, so my opinion isn't necessarily good, but I've discussed this with people who are familiar with, so they can help me get xmake working with this configuration, and they've told me that it's not a very good design to include something from the user's project. -- I like how it is done in the Rust ecosystem, libraries are completly platform agnostic: for example, there are hal impls like https://github.com/golemparts/rppal, which implement traits from https://github.com/rust-embedded/embedded-hal, and hal drivers like https://github.com/dsvensson/cc1101 which also use embedded-hal and so are usable in any platform. The driver's is completely independent of the platform, and the code is only focused on interfacing with the component.

[jgromes]

Like I said previously, I'm not opposed to the idea of redesigning the abstraction layer. The problem I ran into is the surprising inconsistency across different Arduino platforms. Some of them stick to the API (e.g. digitalWrite/Read etc. are functions defined at the global scope), but others break it by e.g. defining the functions as macros. So if your approach works with tehm all, then I'll be more than happy to use it.

[Mesteery]

I discovered C++ concepts introduced in C++20, they are like typescript interface. But I think this is too recent for arduino?

[jgromes]

I think this should be achievable using just virtual methods, I'm not familiar with concepts.

[Mesteery]

I implemented the new hal in https://github.com/Mesteery/RadioLib/tree/new-hal. It's currently very WIP, and I comes to ask you what do you think about it, but also on a specific point: since LOW, HIGH, etc. are macro, I wanted to add equivalent in Hal so the lib use for example Hal::LOW, and so the values could be defined in the class that extend base hal instead of defining them with macros (for generic build). The issue is that I didn't found a design that could do this and I ended up with just enums in the base hal but the problem with this, is that for example RISING does not have the same value on all arduino platforms. I am searching for an idea on how to handle LOW, etc. values in a manner that it's defined in the hal and not globally, or maybe macros are simpler/better for this point? I apologize for the clarity.

[jgromes]

I like it! It's definitely more C++ than the current macro-based C-like stuff. Overall, I think the approach is good, I'm not sure on couple decisions (for example, should the SPI instance, pins etc. be part of Module or (Arduino)Hal class? Intuitively, I would say the former, it's the Module that has the SPI interface or some pins).

Regarding the macros, I was expecting you would run into these problems due to the aforementioned inconsistncy. The Arduino API is really not an API at all, it's based completely on matching names for functions and/or macros. I think this could be solved by having dedicated members, e.g. Hal::GpioLevelLow. Then, for ArduinoHal, this can be set in constructor to LOW (defined by Arduino to be whatever the Arduino platform defines it as), and for any other HAL, it's up to the user to set. The library can then use Hal::GpioLevelLow.

[Mesteery]

for example, should the SPI instance, pins etc. be part of Module or (Arduino)Hal class? Intuitively, I would say the former, it's the Module that has the SPI interface or some pins.

I agree with this, I just designed it in that way because it's for me simpler, and I couldn't see how else to do it, but I am open to any proposal.

I think this could be solved by having dedicated members, e.g. Hal::GpioLevelLow. Then, for ArduinoHal, this can be set in constructor to LOW (defined by Arduino to be whatever the Arduino platform defines it as), and for any other HAL, it's up to the user to set. The library can then use Hal::GpioLevelLow.

My first idea was to have an enum in hal class base, and to redefine it in ArduinoHal, like a virtual function, but unfortunately it's not possible. I tried the same approach with static const and redefine them, still without success.

So If I understand correctly your idea it should look like this:

class Hal {
  public:
  uint8_t GpioLevelLow;
  uint8_t GpioLevelHigh;
  Hal(uint8_t gpioLevelLow, uint8_t gpioLevelHigh, ...): GpioLevelLow(gpioLevelLow), GpioLevelHigh(gpioLevelHigh) {};
};

class ArduinoHal : public Hal {
  public:
  ArduinoHal(...): Hal(LOW, HIGH), ... {};
};

// and used like this
mod->hal->digitalWrite(3, mod->hal->GpioLevelLow);

In that case, I also thought about this approach, but the usage of LOW and HIGH in constexpr : https://github.com/jgromes/RadioLib/blob/d1113a488f3f80810c754a5be7350bf38aa7ff43/src/Module.cpp#L487-L492 prevented me from using this approach and so I gave up waiting for an idea or some fix.

[jgromes]

You can change the RF switch table from constexpr to just const if that helps.

[Mesteery]

Wouldn't that affect the memory or something like that on arduino? And is the example I gave what you were thinking? (because Hal::GpioLevelLow is static, and it's not possible to override it in child class, so I guess that's what you meant.)

[jgromes]

Wouldn't that affect the memory or something like that on arduino?

Maybe, on some platforms. But everything else is going to have much larger impact than this.

is the example I gave what you were thinking

Pretty close. I don't see the need for making the member static if we allow changes in other parts of RadioLib, such as the RF switch mode table. I would also be careful about types (in theory uint8_t may be insufficent to fit HIGH or LOW, though I haven't seen it in practice).

[Mesteery]

I would also be careful about types (in theory uint8_t may be insufficent to fit HIGH or LOW, though I haven't seen it in practice).

Yes, I preferred to align all api to uint8_t even if some platform use uint32_t, and cast them when required.

Also, I temporary removed all RADIOLIB_NC checks in HAL functions, but I think they should remains? The main reason is because I'm not sure if they're useful and I find it strange to check for this in HAL fonctions (it would be strange to add these checks in a user defined hal for instance), and not somewhere else.

[jgromes]

The checks should remain somewhere, to give the user the option to e.g. not use the reset pin. Again, some Arduino platforms implement a "not connected" pin macro, others not.

[Mesteery]

I am closing this issue as it is now fixed (I will PR new-hal branch soon). Thanks you!