gioblu / PJON

PJON (Padded Jittering Operative Network) is an experimental, arduino-compatible, multi-master, multi-media network protocol.
Other
2.73k stars 239 forks source link

bug: only #include "ESP32/ESPNOWHelper.h" if PJON_INCLUDE_EN is defined #256

Closed xlfe closed 6 years ago

xlfe commented 6 years ago

Otherwise I'm getting a compilation error for PJON :(

PJON_Interfaces.h :-


#pragma once

#include "ARDUINO/PJON_ARDUINO_Interface.h"
#include "RPI/PJON_RPI_Interface.h"
#include "WINX86/PJON_WINX86_Interface.h"
#include "LINUX/PJON_LINUX_Interface.h"
+  #if defined(PJON_INCLUDE_EN)
    #include "ESP32/ESPNOWHelper.h"
+  #endif
gioblu commented 6 years ago

Ciao @xlfe is there any motivation why you did not include the helper in the ARDUINO interface dir (as the tcp or udp helpers of fred) considering that the helper depends on a custom arduino core?

xlfe commented 6 years ago

No that makes sense too actually. I guess the only reason not to is that it doesn't run on generic Arduino - but if you want to move it there I have no objections

gioblu commented 6 years ago

Another thing @xlfe why you include twice the helper both in PJON_Interfaces and in the Strategy itself? Would not have sense to just remove the inclusion from the PJON_Interfaces file?

gioblu commented 6 years ago

@xlfe about custom core, the "norm" we have developed is just to fit in ARDUINO interface all arduino compatible cores see: https://github.com/gioblu/PJON/blob/master/src/interfaces/ARDUINO/PJON_ARDUINO_Interface.h#L30

xlfe commented 6 years ago

Sounds sensible - no objections from me at all

gioblu commented 6 years ago

Consider that when compiling an ESPNOW sketch on ESP32, the interface used is ARDUINO, from where PJON takes all the system calls in the first place.

xlfe commented 6 years ago

true

gioblu commented 6 years ago

Thank you @xlfe for your clarifications, I will make the necessary changes.

xlfe commented 6 years ago

Thank you @xlfe for your clarifications, I will make the necessary changes.

Thanks @gioblu - sorry for the added work!

gioblu commented 6 years ago

@xlfe it is a pleasure no probs, I am still reviewing your contribution, one thing I have noticed is that you include here: https://github.com/gioblu/PJON/blob/master/src/interfaces/ARDUINO/ESPNOWHelper.h#L3 and here: https://github.com/gioblu/PJON/blob/master/src/strategies/ESPNOW/ESPNOW.h#L24 PJON and PJONDefines files? Shouldnt they be already available for you there?

xlfe commented 6 years ago

@xlfe it is a pleasure no probs, I am still reviewing your contribution, one thing I have noticed is that you include here: https://github.com/gioblu/PJON/blob/master/src/interfaces/ARDUINO/ESPNOWHelper.h#L3 and here: https://github.com/gioblu/PJON/blob/master/src/strategies/ESPNOW/ESPNOW.h#L24 PJON and PJONDefines files? Shouldnt they be already available for you there?

Yes - removing the include from ESPNOW.h doesn't cause any problems

gioblu commented 6 years ago

Good @xlfe thank you again for testing it, I will remove those :)