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

ESP-NOW strategy support for esp8266 #361

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Strix-CZ
Copy link

@Strix-CZ Strix-CZ commented Aug 9, 2020

Hi!
This aims to close issue #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 :)

@Strix-CZ
Copy link
Author

Strix-CZ commented Aug 9, 2020

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.

@gioblu
Copy link
Owner

gioblu commented Aug 9, 2020

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.

@gioblu
Copy link
Owner

gioblu commented Aug 9, 2020

@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 :)

packet[len + 2] = _magic_header[2] ^ len;
packet[len + 3] = _magic_header[3] ^ len;
packet[len + 2] = ESP_NOW_MAGIC_HEADER[2] ^ len;
packet[len + 3] = ESP_NOW_MAGIC_HEADER[3] ^ len;

ATOMIC()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using interrupts and noInterrupts instead of the ATOMIC dependency, ie

noInterrupts();
Serial.println("Inside critical section.");
interrupts();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Fixed :) The dependency is removed.

@Strix-CZ
Copy link
Author

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.

@gioblu
Copy link
Owner

gioblu commented Aug 11, 2020

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.

@gioblu
Copy link
Owner

gioblu commented Dec 12, 2020

Ciao @Strix-CZ thank you very much for your support and bringing this forward.
I recently have accepted the contribution of @porkyneal f4e492d 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.

@Strix-CZ
Copy link
Author

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.

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

Successfully merging this pull request may close these issues.

2 participants