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

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

Closed
xlfe opened this issue Sep 26, 2018 · 12 comments
Closed

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

xlfe opened this issue Sep 26, 2018 · 12 comments
Labels

Comments

@xlfe
Copy link
Contributor

xlfe commented Sep 26, 2018

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

gioblu commented Sep 26, 2018

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

xlfe commented Sep 26, 2018

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

gioblu commented Sep 26, 2018

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

gioblu commented Sep 26, 2018

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

xlfe commented Sep 26, 2018

Sounds sensible - no objections from me at all

@gioblu
Copy link
Owner

gioblu commented Sep 26, 2018

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

xlfe commented Sep 26, 2018

true

@gioblu
Copy link
Owner

gioblu commented Sep 26, 2018

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

@xlfe
Copy link
Contributor Author

xlfe commented Sep 26, 2018

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

Thanks @gioblu - sorry for the added work!

gioblu added a commit that referenced this issue Sep 26, 2018
@gioblu
Copy link
Owner

gioblu commented Sep 26, 2018

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

xlfe commented Sep 26, 2018

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

gioblu commented Sep 26, 2018

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

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

No branches or pull requests

2 participants