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

feat: nodes scan wifi channels during discovery #1

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

jamienz
Copy link
Contributor

@jamienz jamienz commented Jan 30, 2024

When a node is performing the discovery process, it loops through an array of wifi channels and sends its broadcast on each channel. Its possible that the node sends a broadcast on one channel, and the host receives it despite being on another (due to the fact that wifi channels overlap?), so the host includes its channel in the response. The node then saves this channel in NVS.

@Johboh
Copy link
Owner

Johboh commented Jan 30, 2024

Thanks for you contribution @jamienz! I didn't expect anyone to use this over engineered library other than my self, given the sparse documentation :).

I'm going to cherry pick this and try it out, and I will leave some inline comments as well.

Do you know, how does the WiFi channel relate to the ESP-NOW channel? As we are setting here and here. E.g. if we should set this instead/also.

For my "statistics", do you use the ESP-IDF or Arduino library variant?

@jamienz
Copy link
Contributor Author

jamienz commented Jan 30, 2024

Thanks for creating the EspNowNetwork library @Johboh!

When you do your testing of this PR, make sure that the WiFi network that the host is connected to is NOT on channel 1.

When the host is connected to a WiFi network, it will send/receive ESP-NOW messages on the same channel as that network (it can't send on a different channel). The node isn't connected to a WiFi network, so by default it will be on channel 1, based on my testing by adding the following code to the node before it sends it's discovery broadcasts:

uint8_t primary = 0;
wifi_second_chan_t second;
ESP_ERROR_CHECK(esp_wifi_get_channel(&primary, &second));
log("channel=" + std::to_string(primary), ESP_LOG_INFO);

The peer_info.channel is set to zero which means ESP-NOW messages will be sent on whatever channel is the current WiFi channel - for the host it will be the WiFi network channel, and for the node it will be 1. With the code currently in the main branch, everything will work well if the host is connected to a WiFi network on channel 1. this PR aims to support the host being on a channel other than channel 1.

I'm using PlatformIO/Arduino.

@Johboh
Copy link
Owner

Johboh commented Jan 31, 2024

Right, I was just thinking of what the point was with the channel field in the peer info. If I understand the documentation correctly, if set to 0 it will use same channel "which station or softap is on", otherwise if not zero it must be set to be "the channel that station or softap is on.". So if I'm not missing something, the channel is quite useless and could have been omitted in the ESP-now API?

src/EspNowPreferences.h Outdated Show resolved Hide resolved
src/Preferences.h Outdated Show resolved Hide resolved
src/impl/EspNowNode.cpp Outdated Show resolved Hide resolved
src/impl/EspNowNode.cpp Outdated Show resolved Hide resolved
src/impl/EspNowNode.cpp Outdated Show resolved Hide resolved
src/impl/EspNowPreferences.cpp Outdated Show resolved Hide resolved
src/impl/EspNowPreferences.cpp Outdated Show resolved Hide resolved
src/impl/EspNowPreferences.cpp Outdated Show resolved Hide resolved
src/EspNowPreferences.h Outdated Show resolved Hide resolved
src/Preferences.h Outdated Show resolved Hide resolved
@Johboh
Copy link
Owner

Johboh commented Jan 31, 2024

Added some comments! Sorry for being nit-picky, but I review PRs for a living :)

@Johboh
Copy link
Owner

Johboh commented Jan 31, 2024

(I moved some files around, but no change to the files themself, so a git pull --rebase should resolve the merge issue automatically)

@jamienz
Copy link
Contributor Author

jamienz commented Jan 31, 2024

Thanks for the feedback. I'll make those changes tonight.

@Johboh
Copy link
Owner

Johboh commented Feb 1, 2024

Some minor comments left, and a failing clang-format (whitespace). If those are fixed, we are good to merge.

@Johboh Johboh force-pushed the main branch 7 times, most recently from 8175f39 to c8fcf24 Compare February 4, 2024 08:36
@jamienz
Copy link
Contributor Author

jamienz commented Feb 9, 2024

Right, I was just thinking of what the point was with the channel field in the peer info. If I understand the documentation correctly, if set to 0 it will use same channel "which station or softap is on", otherwise if not zero it must be set to be "the channel that station or softap is on.". So if I'm not missing something, the channel is quite useless and could have been omitted in the ESP-now API?

Thats a good question. Its definitely not useless, its doing something, but I'll admit that I don't understand exactly what its doing - the documentation is incomplete in my opinion.

If peer_info.channel is assigned zero, and the Node wifi channel isn't explicitly set, and the Node doesn't connect to a wifi network, the wifi channel seems to be 1.

My WiFi access point is on channel 6 right now, and since the Host connects to that access point, it is on channel 6 also. So when the Node broadcasts on channel 1, the Host doesn't hear it. Setting the peer_info.channel on the Node to 6 doesn't work:
E (280) ESPNOW: Peer channel is not equal to the home channel, send fail!
The Node needs to be on Wifi channel 6 also.

See: espressif/esp-idf#9592. The important final comment from the Espressif guy is "ESP-NOW's channel should obey the STA/AP's channel". Which I interpret to mean:

  • the Node's peer_info.channel needs to be 0, so that it uses the Node's WiFi channel (which needs to match the Host's WiFi channel)
  • or the Node's peer_info.channel needs to be explicitly set to the WiFi channel

Leaving peer_info.channel=0 is the least-effort option.

@Johboh
Copy link
Owner

Johboh commented Feb 10, 2024

@jamienz There are three more inline comments that it looks like you are missing:
#1 (comment)
#1 (comment)
#1 (comment)

@Johboh
Copy link
Owner

Johboh commented Feb 14, 2024

I will go a-head and merge this one as I need myself (I will address outstanding comments and fix clang format). There is also an unrecoverable crash loop that can happen in the node that I will fix (and that I ended up with when testing). Will share the fix commit here.

@Johboh Johboh merged commit e0e0296 into Johboh:main Feb 14, 2024
8 of 9 checks passed
@Johboh
Copy link
Owner

Johboh commented Feb 14, 2024

@jamienz FYI: 368aa09

@jamienz
Copy link
Contributor Author

jamienz commented Feb 15, 2024

That's great @Johboh. I really appreciate your time in reviewing this, and for finishing it. Apologies for not getting back to this one sooner to finish off the final tasks.

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

Successfully merging this pull request may close these issues.

2 participants