Espnow Support #663
Espnow Support #663
Conversation
Update main Davepl Fork
|
|
||
| void onReceiveESPNOW(const uint8_t *macAddr, const uint8_t *data, int dataLen) | ||
| { | ||
| memcpy(&message, data, sizeof(message)); |
There was a problem hiding this comment.
Is this copy necessary at all? Why is message a global at all?
If so, should it be after the packet is validated, no?
If all you're doign is peeking at the command member of data, just cast it and use it in place after verifying the cbSize is appropriate.
There was a problem hiding this comment.
Depends how async the code is. Am I guaranteed that a second message won't come in in the meantime and corrupt the first? I assume so, but copying seemed safer, and it's nominally 6 bytes, so...
| @@ -569,7 +583,7 @@ void loop() | |||
| String strOutput; | |||
|
|
|||
| #if ENABLE_WIFI | |||
| strOutput += str_sprintf("WiFi: %s, IP: %s, ", WLtoString(WiFi.status()), WiFi.localIP().toString().c_str()); | |||
| strOutput += str_sprintf("WiFi: %s, MAC: %s, IP: %s ", WLtoString(WiFi.status()), WiFi.macAddress().c_str(), WiFi.localIP().toString().c_str()); | |||
There was a problem hiding this comment.
Cat this be in the stgartup prose instead of in the .5hz loop?
There was a problem hiding this comment.
No, we show WiFi status every 5 seconds, if WiFi is enabled. This is based on the fact that connectivity might drop, addresses can change, etc. This decision was made after multiple sessions of hair-pulling over networking stuff being not working stuff.
|
@davepl I noticed you opened two PRs to add ESPNOW support: #663 (this one) straight from the I assume you meant to open the first to merge As @robertlipe has already left some comments on this PR (663) as well, I'll proceed with reviewing and eventually merging this PR. I'll close #664 as a duplicate. |
| @@ -569,7 +583,7 @@ void loop() | |||
| String strOutput; | |||
|
|
|||
| #if ENABLE_WIFI | |||
| strOutput += str_sprintf("WiFi: %s, IP: %s, ", WLtoString(WiFi.status()), WiFi.localIP().toString().c_str()); | |||
| strOutput += str_sprintf("WiFi: %s, MAC: %s, IP: %s ", WLtoString(WiFi.status()), WiFi.macAddress().c_str(), WiFi.localIP().toString().c_str()); | |||
There was a problem hiding this comment.
No, we show WiFi status every 5 seconds, if WiFi is enabled. This is based on the fact that connectivity might drop, addresses can change, etc. This decision was made after multiple sessions of hair-pulling over networking stuff being not working stuff.
| g_ptrSystem->EffectManager().PreviousEffect(); | ||
| break; | ||
|
|
||
| case ESPNowCommand::ESPNOW_SETEFFECT: |
There was a problem hiding this comment.
I'm not doing any argument validation here because I assume (and have confirmed) that the APIs themselves do the sanity checks, so don't want a separate layer of logic if I can avoid it.
This adds ESPNOW support so that an ESPNOW client device can connect and do things like change the current effect. Note that it seems not to be compatible with WiFi use, so you get one or the other.
mainas the target branch.