-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ESP32S2: Allow connecting to specific bssid #3433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small potential bug. Otherwise this looks really good! What is left to do?
@tannewt the only thing I'd like to do is to look at the set of parameters exposed after a connection is established to check/verify the details of the connection and know that you're connected where you think you are - right now there's really just |
@astrobokonon What do you think about doing that in a follow up PR? This looks good as-is. |
@tannewt Yeah, I can do that in a different PR for sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you! I'll let @dhalbert merge.
Closes #3403
I couldn't figure out a way to avoid doing a last minute conditional check in
common-hal/wifi/Radio.c
because thebssid_set
flag needs to be set only if you're specifying a specific MAC. If you're not passing in a bssid, but you happen to still set thebssid_set
flag, the connection will fail and you'll end up returningWIFI_RADIO_ERROR_NO_AP_FOUND
so that's no good (and a bit confusing).I needed a way to check the bssid length as an initial sanity check against someone passing it in as a string (with colons) rather than the actual bytes, so I just dropped in
#define MAC_ADDRESS_LEN
when/where needed. I could have just hardcoded it but I saw that done elsewhere so I just did the same. It raises aValueError
but I wasn't sure if there was a better/more preferred type.I don't see a way to get the vital statistics of the currently-connected AP, so that's the final thing to look at before I'd call this good to go. But it compiled and the initial tests worked as expected, which was frankly a surprise.