jgromes / RadioLib

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

'virtual float PhysicalLayer::getRSSI()' was hidden ESP-IDF #1064

Closed KjartanOli closed 3 months ago

KjartanOli commented 3 months ago

I'm trying to compile a simple test that just receives data in a loop using ESP-IDF (code shown below).

The build fails with the following error:

In file included from /home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/modules/CC1101/CC1101.h:7,
                 from /home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/RadioLib.h:78,
                 from /home/kjartan/Documents/kjartan/radiolib-receiver/main/radiolib-receiver.cpp:7:
/home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/modules/CC1101/../../protocols/PhysicalLayer/PhysicalLayer.h:305:19: error: 'virtual float PhysicalLayer::getRSSI()' was hidden [-Werror=overloaded-virtual=]
  305 |     virtual float getRSSI();
      |                   ^~~~~~~
In file included from /home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/RadioLib.h:93:
/home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/modules/SX127x/SX1272.h:241:11: note:   by 'float SX1272::getRSSI(bool, bool)'
  241 |     float getRSSI(bool packet = true, bool skipReceive = false);
      |           ^~~~~~~
/home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/modules/CC1101/../../protocols/PhysicalLayer/PhysicalLayer.h:305:19: error: 'virtual float PhysicalLayer::getRSSI()' was hidden [-Werror=overloaded-virtual=]
  305 |     virtual float getRSSI();
      |                   ^~~~~~~
In file included from /home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/modules/SX127x/SX1276.h:8,
                 from /home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/RadioLib.h:95:
/home/kjartan/Documents/kjartan/radiolib-receiver/managed_components/jgromes__radiolib/src/modules/SX127x/SX1278.h:252:11: note:   by 'float SX1278::getRSSI(bool, bool)'
  252 |     float getRSSI(bool packet = true, bool skipReceive = false);
      |           ^~~~~~~
cc1plus: some warnings being treated as errors
ninja: build stopped: subcommand failed.

To Reproduce

#include <cstdio>
#include <array>
#include <chrono>
#include <string_view>
#include <thread>

#include <RadioLib.h>
#include "EspHal.h"

using namespace std::chrono_literals;
using namespace std::literals::string_view_literals;
constexpr auto TAG = "MAIN"sv;

extern "C" void app_main(void)
{
    auto hal = new EspHal(5, 19, 27);
    SX1276 radio = new Module(hal, 18, 26, 14, 33);

    ESP_LOGI(TAG.data(), "[SX1276] Initializing ... ");
  int state = radio.begin();
  if (state != RADIOLIB_ERR_NONE) {
    ESP_LOGI(TAG.data(), "failed, code %d\n", state);
    while(true) {
      hal->delay(1000);
    }
  }
  ESP_LOGI(TAG.data(), "success!\n");

    while (true) {
        auto buffer = std::array<std::byte, 50>{};
        auto state = radio.receive((uint8_t*) buffer.data(), buffer.size());
        if (state == RADIOLIB_ERR_NONE) {
            std::printf("%s\n", (char*) buffer.data());
        } else {
      ESP_LOGI(TAG.data(), "failed, code %d\n", state);
    }
        std::this_thread::sleep_for(10s);
    }
}

EspHal.h is taken directly from the ESP-IDF example. Radiolib is installed from the ESP-IDF component registry.

Expected behavior The code compiles

Additional info (please complete):

jgromes commented 3 months ago

Seems like the CI does not have any issues compiling with ESP-IDF: https://github.com/jgromes/RadioLib/actions/runs/8621766528/job/23631387349

Can you try with the unedited ESP-IDF example? Also, how are you building it? The CI just calls idf.py build, are you doing anything else?

KjartanOli commented 3 months ago

Yes I'm just runningidf.py build. The example compiles without error.

jgromes commented 3 months ago

The example compiles without error.

That would suggest an issue in your code rather than in the library itself.

The only two significant differences I see in your code is that your HAL instance is auto-typed (why? it can never be anything other than EspHal) and that both your HAL and radio instances are local to app_main, so I'm wondering if the C linkage declared by extern "C" is causing any issues. Can you try to move both of these into the global scope?

KjartanOli commented 3 months ago

I'll try, but this:

#include <cstdio>
#include <string_view>

#include <RadioLib.h>
#include "EspHal.h"

using namespace std::literals::string_view_literals;
constexpr auto TAG = "MAIN"sv;

extern "C" void app_main(void)
{
    auto hal = new EspHal(5, 19, 27);
    SX1276 radio = new Module(hal, 18, 26, 14, 33);

    ESP_LOGI(TAG.data(), "[SX1276] Initializing ... ");
  int state = radio.begin();
  if (state != RADIOLIB_ERR_NONE) {
    ESP_LOGI(TAG.data(), "failed, code %d\n", state);
    while(true) {
      hal->delay(1000);
    }
  }
  ESP_LOGI(TAG.data(), "success!\n");

    // loop forever
  for(;;) {
    // send a packet
    ESP_LOGI(TAG.data(), "[SX1276] Transmitting packet ... ");
    state = radio.transmit("Hello World!");
    if(state == RADIOLIB_ERR_NONE) {
      // the packet was successfully transmitted
      ESP_LOGI(TAG.data(), "success!");

    } else {
      ESP_LOGI(TAG.data(), "failed, code %d\n", state);

    }

    // wait for a second before transmitting again
    hal->delay(1000);

  }
}

also compiles, so I find it unlikely this is the issue.

KjartanOli commented 3 months ago

Just tried

#include <cstdio>
#include <array>
#include <chrono>
#include <string_view>
#include <thread>

#include <RadioLib.h>
#include "EspHal.h"

using namespace std::chrono_literals;
using namespace std::literals::string_view_literals;
constexpr auto TAG = "MAIN"sv;

EspHal* hal = new EspHal(5, 19, 27);
SX1276 radio = new Module(hal, 18, 26, 14, 33);

extern "C" void app_main(void)
{

    ESP_LOGI(TAG.data(), "[SX1276] Initializing ... ");
  int state = radio.begin();
  if (state != RADIOLIB_ERR_NONE) {
    ESP_LOGI(TAG.data(), "failed, code %d\n", state);
    while(true) {
      hal->delay(1000);
    }
  }
  ESP_LOGI(TAG.data(), "success!\n");

    while (true) {
        auto buffer = std::array<std::byte, 50>{};
        auto state = radio.receive((uint8_t*) buffer.data(), buffer.size());
        if (state == RADIOLIB_ERR_NONE) {
            std::printf("%s\n", (char*) buffer.data());
        } else {
      ESP_LOGI(TAG.data(), "failed, code %d\n", state);
    }
        std::this_thread::sleep_for(10s);
    }
}

Same error.

jgromes commented 3 months ago

Ah, I missed the most crucial difference, which is that receive is being called. I actually had to diff the two files you showed to find out what was the difference among all those strange indentations ...

With that I am able to replicate the issue, but only in the latest ESP-IDF (5.2.1). The problem is that the virtual getRSSI has no arguments in PhysicalLayer, but the only implementation available in SX1272 and SX1278 classes has arguments with default values, which I guess is a warning in new versions of GCC/G++ that ESP-IDF automatically throws an error over due to its -Werror flag.

Fixed in the above-linked commit, thanks for reporting!

illysky commented 2 months ago

Hi Thank you for posting this., I am also using ESP-IDF. I think the same overload error is also occurring in the sleep function.

./../protocols/PhysicalLayer/PhysicalLayer.h:117:21: error: 'virtual int16_t PhysicalLayer::sleep()' was hidden [-Werror=overloaded-virtual=]
  117 |     virtual int16_t sleep();

I applied similar fixes to what you did in the commit and it fixes it, I've forked it for now,

I love RadioLib, thank you - you've saved me so much time.

jgromes commented 2 months ago

@illysky it would be great if you could submit a PR for that ;)

Though what confuses me the most is that the ESP-IDF CI is not able to catch these errors. I'm guessing it's because the methods are not being called from the example that's being compiled ... but then again the entire library is being compiled, so it'S a bit weird.

illysky commented 2 months ago

Of course. I will try to PR after Ive done my EspHal.

So I guess an overload is a warning? And I am getting errors because of -Werror flag is set, which is good. But perhaps their CI doesn't have -Werror enabled and warnings get through in CI. Maybe

jgromes commented 2 months ago

@illysky thank you! The CI is in this repository, here's is an example of the latest run: https://github.com/jgromes/RadioLib/actions/runs/8960526520/job/24607133566

It has default configuration and produces (as far as I can tell) no warnings. Furthermore RPi builds have -Wall and -Wextra and likewise produce no warnings.