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

Split wifi state for AP and STA #288

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Split wifi state for AP and STA #288

merged 3 commits into from
Oct 24, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 16, 2023

Closes #210

I wonder what to do with the combined get_wifi_state() function in the long run. If esp-wifi ever gets AP-STA support, picking one state will be difficult. Maybe the current "none of these modes? then invalid" logic will be good enough, or maybe that function can be removed altogether. I'm not sure.

I've removed a bunch of possibilities from the set of states, based on what I found here. If it weren't a breaking change, I have also renamed the event-like ApStop, etc. enum variants to a more grammatically proper form (e.g. ApStopped).

@bugadani bugadani force-pushed the state branch 3 times, most recently from ddc902c to f76edc2 Compare October 16, 2023 12:05
@bugadani bugadani marked this pull request as ready for review October 16, 2023 12:14
@bugadani bugadani mentioned this pull request Oct 16, 2023
@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 17, 2023

From a brief look this looks good to me. I think I'm fine with renaming the enum variants at this point of time. (t.b.h given the upcoming breaking changes in the next esp-hal release this is a really minor break)

@bugadani bugadani force-pushed the state branch 2 times, most recently from a0d43e9 to 4b2aa1c Compare October 18, 2023 09:22
@bugadani bugadani marked this pull request as draft October 18, 2023 09:37
@bugadani bugadani force-pushed the state branch 7 times, most recently from 1a75ced to 9527bef Compare October 18, 2023 11:44
@bugadani bugadani marked this pull request as ready for review October 18, 2023 11:44
@bugadani bugadani changed the title Split wifi state for AP and STA Split wifi state for AP and STA, remove atomic-polyfill Oct 18, 2023
@bugadani bugadani changed the title Split wifi state for AP and STA, remove atomic-polyfill Split wifi state for AP and STA Oct 18, 2023
@bugadani bugadani mentioned this pull request Oct 19, 2023
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM, shame about portable atomic but its something we can resolve in the future :)

@bugadani
Copy link
Contributor Author

Sooo... do you think we can move forward with this? 🙃

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 24, 2023

There two are new

warning: unnecessary `unsafe` block
  --> esp-wifi\src\wifi\os_adapter.rs:45:5
   |
45 |     unsafe { get_sta_state() == WifiState::StaConnected }
   |     ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default



warning: unnecessary `unsafe` block
    --> esp-wifi\src\wifi\mod.rs:1462:13
     |
1462 |             unsafe {
     |             ^^^^^^ unnecessary `unsafe` block

Then I guess it's fine

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

@bjoernQ bjoernQ merged commit 5578266 into esp-rs:main Oct 24, 2023
7 checks passed
@bugadani bugadani deleted the state branch October 24, 2023 10:20
TuEmb pushed a commit to TuEmb/esp-wifi that referenced this pull request Oct 27, 2023
* Split wifi state for AP and STA

* Use atomic enum to store state

* Clean up unnecessary unsafe blocks
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.

Adjust WIFI_STATE handling
3 participants