Please Revert my other PR's i will recreate them again.

[nielsnl68]

Hi Phil, based on the mayhem i created because of the ill formed Repo i used for the last PR's the other day. Maybe it would be better to redo those again with the now better setup repo i use.

What do you think will that restore the correct changes we made?

[pschatzmann]

Where do you see any issues with the current implementation ?

[nielsnl68]

I'm missing parts of the last commits from you and me. So i am worried we lost more then thought.

[pschatzmann]

I simpified quite a few things: Only change it if you encounter any issues

[nielsnl68]

Indeed i found an small issue using the configuration below with setting the channel.

  auto cfg_now = now.defaultConfig();
  cfg_now.use_long_range = true;
  cfg_now.buffer_count = 2;
  cfg_now.channel = 6;
  now.begin(cfg_now);

I got a channel mismatch error running this code. Could you look in to this to fix it properly. I just did set the peer.channel to zero, to use the Wifi channel for now. And that worked fine, but i have not tested it properly if the channel was really set to channel 6 or not.

I did have also some issues setting up eSpeak with the new AAT DEV version, but i cant reproduce the error anymore :( something about missing some or corrupted file. Sorry i did not copied the error message. And now it works fine. :(

Other then that it seems to work properly. Including with choice of the faster speed you made. Nice.

[nielsnl68]

However i have still some suggestions for improvement of the ESPNowStream component:

1. From my understanding you allow to config the WiFi outside the ESPNowStream, when that is the case, i see some possible issues inside bool begin( ... ) { } where the WiFi mode is set and setupMAC() called without checking the wifi state. I would suggest to check the WiFi connect state first and then set the mode and call setupMAC().
2. Atm. you use a char* to store the mac_address, i like to change that into a std::array<uint_8,6> so you can store a proper 6 byte array for the mac_address. I belief this is better to maintain and you do not need to validate the given string for the right content anymore.
3. Add an option to config the destination mac_address inside the config structure so we could easily switch to where new packet's are send to.
4. Move the body code of default_send_cb() {} and default_recv_cb() {} in two new ESPNowStream methods and just call them in the callbacks instead.
5. To extend the ESPNowStream class i would prefer to have the 2 new methods and sendPacket() to be made virtual.
6. the sendPacket() and Write() process is starting to get very complex. I understand what you want to do but it is very complex. Lets try to simplify the process a bit and remove the blocking structure.

When you agree to the point above i would love to create the PR's for that, no problem.

[pschatzmann]

1. I think defining a custom mac after wifi has been started is just not supported by the ESP32. So issuing an error message is the best what we can do. From my point of view no change is needed.
2. I am not storing any mc addresses: so it is just a question of adding another bool addPeer() method which might be a good idea.
3. I don't think this is a good idea since we might need to manage multiple destination addresses. I would rather suggest to add a clearPeers() method, that would be called before the addPeer() to change the destination(s).
4. I not sure I quite understand: do you prefer to overwrite in a subclass instead of setting your own callbacks ? I am not opposed to this.
5. No issue with making sendPacket() virtual
6. If you want to simplify maybe splitting the logic into a sendPaccket() and sendPacketWithConfirmations() might help. The complexity comes with handling confirmations.

[nielsnl68]

• 1. i was not talking only about the setupMAC() call, i mostly worried about the WiFi.mode(cfg.wifi_mode); call.
• 2. Okay, fear enough, i will add the addPeer(); as you mentioned.
• 3. Okay, i will add this to my own class later. I will add the clearPeers(); as you suggested.
• 4. Yes that is the idea why i suggesting this.
• 5. Okay, i will make them virtual.
• 6. I have made some easier to use way's in the past that i could implement here.
• 7. fixing missing Setting the channel.

[pschatzmann]

Ad 1: got it: in this case it might be a good idea to call WiFi.mode() only if WiFi.getMode() returns WIFI_MODE_NULL, so it is the responsibility of this class to manage this only if it was not done yet.

[nielsnl68]

okay, that is a better idea. I will make a PR for that.

[nielsnl68]

Okay most of the PR's are live. Only the rework of SendPacket() has some more work to it and will be in the next couple of days being done.

[nielsnl68]

BTW, i have made a branch where all 4 PR's are combined, when you want me to create a PR from it let me know. and i will create it.

[nielsnl68]

Never mind i see that you already merged them :D Thanks

[pschatzmann]

Dont forget to pull my corrections and addition