PJON
PJON copied to clipboard
ESP-NOW strategy support for esp8266
Hi! This aims to close issue https://github.com/gioblu/PJON/issues/277#
I have ported ESP-NOW strategy to support esp8266. I am a newbie in microcontroller development and it's a long time since I wrote C++. Could you please go over the change carefully and tell me if you spot any mistakes or imperfections? Also, I did some changes to ESP32 code but I have not tested it works for ESP32. Could you please double check it? I have tried out only a very basic scenario for ESP8266, which seems to work.
Please ignore individual commits and focus on the net sum. I have reaaallly struggled with this :D
I have removed the dependency on FreeRTOS and replaced it with one volatile bool variable and my own class PacketQueue. I have introduced a new dependency on SimplyAtomic https://github.com/wizard97/SimplyAtomic Is this OK? How to change the project to include this dependency? The got a feeling that the dependency might not be really necessary, could you have a look at how I used ATOMIC() macro in the code and advice if this is possible and how to do it?
Also, I have removed a bunch of error logging function calls, which seems to be missing in ESP8266 (is this really true?). Is this an OK thing to do?
Thank you for your help and for developing PJON! It's a great work :)
I've just noticed I got still a big TODO to resolve - I am not setting PMK key so the traffic would not be encrypted.
Ciao @Strix-CZ thank you very much for investing your time to solve this issue and add the support for ESP8266. I have now just took a glance to your changes. The only thing that caught my attention is the use of malloc and free. I have tried to always avoid dynamic memory allocation in the strategies I have developed and in the overlaying network layer for obvious reasons. Do you think would be complex to switch to static allocation?
I will review and test your contribution as soon as possible.
@xlfe what do you think about this?
Thank you again for your support to the project.
@Strix-CZ instead of atomic why don't you use the interrupts - noInterrupts methods? Aren't they available on ESP32 and ESP8266?
Thank you for your compliments :)
Thank you for the suggestions!
I've replaced dynamic allocation with static one, which also respects PJON constants for max packet length and buffer size. I also removed dependency on SimplyAtomic as suggested.
There are at least two things left to do:
- Consider removing ESP_NOW_MAGIC_HEADER. I don't know what was the intention of the original author. It seems to me that it can be removed.
- Fix encryption - currently it's commented out (and it was in the original code). Possibly also disable automatic registration with encryption enabled. This should encourage the users of the library to set different encryption keys for different nodes.
I will not be able to finish it this week. Hopefully, I will get to it next week. I would appreciate any help in finishing it.
Ciao @Strix-CZ thank you very much for your support and for patching the issues I have underlined. I am sorry, I would have supported you more directly but I am in the only 2 weeks of vacation this year, and I do not have hardware with me (my girlfriend would have killed me). I will be back the next week.
The magic header was implemented to let the strategy identify PJON traffic and to act as a "frame initialiser", just to avoid decoding data that is not in PJON format.
I totally agree with you, encryption should be enabled for security reasons and to force good practice.
Ciao @Strix-CZ thank you very much for your support and bringing this forward. I recently have accepted the contribution of @porkyneal https://github.com/gioblu/PJON/commit/f4e492d19659abf8eb96efff69a3069aa3cb49ec which, although arises a conflict in ESPNOW.h, enhances quite a bit how the strategy works making use of the new optional MAC address field provided by PJON.
What do you think about it? It would be cool to reach a point in which we can merge the ESP8266 support along with what we have now in master.
I'm sorry for not replying for such a long time.
Originally, I needed this for my project, but then I changed the approach in my project and this was no longer a priority for me. Realistically, I will not finish this. Sorry. It's a good news that it is possible to make ESP-NOW work on esp8266. If anyone needs it in the future, this pull request can be used as a rough guide for a new implementation.
I suggest to abandon this pull request. Sorry.