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

AP-STA mode #299

Merged
merged 16 commits into from
Nov 6, 2023
Merged

AP-STA mode #299

merged 16 commits into from
Nov 6, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 20, 2023

Built on top of #298

Open questions:

  • How do we want to return WifiDevice from new_with_mode? The AP-STA mode needs two devices (and consequently, two Stacks due to different link state, rx queues, etc.). We can return an enum (user needs to match), we can return a struct (ugh), or we can have different constructors (which removes the need for embedded_svc::wifi::Configuration which I consider a + because I don't like its None option and our need to handle it).

    I'm happy with the current solution of 2 constructors. Please let me know if we should instead offer a different API.
    That is, the constructors I propose:

    • new_with_mode
    • new_with_config
    • new_ap_sta
    • new_ap_sta_with_config
  • Are we fine with the bunch of runtime branching on the WifiDeviceMode
    • or should we prefer a bigger WifiDevice that stores references to the actual resources?
    • Option c is type-state in WifiDevice, which would allow keeping it zero-sized but with a bit of user-facing complexity bump.

    I'm leaning towards type-state. Let me know if I shouldn't!

  • WifiController design isn't very well-suited for AP-STA mode: it is not currently possible e.g. to observe AP connection/disconnection events while also scanning. This isn't easy to solve as currently only one caller may wait for events (2 would race and would need 2 events to get unblocked for example).

TODO:

  • resolve open questions
  • set_configuration shouldn't change the type of configuration
  • examples
  • add to examples.md
  • 🍝
  • update readme
  • conversion between modes (maybe later?) (AP or STA -> AP-STA -> AP or STA)
    • Down-conversion (AP-STA -> AP or STA) could happen on Device::drop), up-conversion would need to return a new Device

Closes #190

@bugadani bugadani changed the title [WIP] AP-STA mode AP-STA mode Oct 21, 2023
@bugadani
Copy link
Contributor Author

On the technical side, this seems to be working - I managed to set up AP-STA mode to serve a website while occasionally scanning for networks.

Due to the controller APIs requiring mutable access, it's not very easy to use, but since the AP and STA modes aren't much affected by it, maybe it's a decent first stab at the problem.

@bugadani bugadani force-pushed the apsta branch 24 times, most recently from a0f7961 to 2c94292 Compare October 26, 2023 17:35
@bugadani
Copy link
Contributor Author

bugadani commented Oct 26, 2023

I'm definitely not touching the examples right now.

@bjoernQ / @MabezDev I'd love to hear your opinions on the open questions and the last commit that implements the type-state device stuff. This got a bit hairy just to avoid some runtime branching, though an added benefit is that now users won't be able to mix up STA and AP devices/interfaces. I should probably hide a bunch of methods in WifiDeviceMode, that'll come later if this is a good approach.

@bugadani bugadani marked this pull request as ready for review November 3, 2023 08:25
@bugadani
Copy link
Contributor Author

bugadani commented Nov 3, 2023

I'll be frank, the examples are slapped together by merging dhcp and access_point examples, but they seem to be working.

I'll put this up for review, though I'm happy to make any changes if you have ideas where this could be improved.

@bugadani bugadani force-pushed the apsta branch 3 times, most recently from 9b4aaa7 to fada531 Compare November 3, 2023 12:14
@bugadani bugadani force-pushed the apsta branch 2 times, most recently from af868c9 to c196024 Compare November 6, 2023 07:26
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 6, 2023

Code looks good and the new examples work fine for me with the exception of ESP32-C2.

The embassy_access_point_with_sta example crashes there - shrinking the buffers to the same size used in acess_point_with_sta makes it work

@bugadani
Copy link
Contributor Author

bugadani commented Nov 6, 2023

Thanks - amended and shrunk the other embassy AP example, too

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 - thanks!

@bjoernQ bjoernQ merged commit 5166089 into esp-rs:main Nov 6, 2023
7 checks passed
@bugadani bugadani deleted the apsta branch November 6, 2023 09:30
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.

Support combined AP/STA mode
2 participants