Skip to content

Wifi device refactor #115

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

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Wifi device refactor #115

merged 5 commits into from
Feb 2, 2023

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Feb 2, 2023

Currently, the WifiDevice Is heavily tied to the smoltcp stack. This PR implements embedded_svc::wifi::Wifi on the raw device instead of on the stack to allow usage of other stacks (i.e embassy_net). I have included an implementation that just calls the inner device functions for compatibility but I can remove that if needed. Finally, I added the internal error codes from esp-idf and reworked the error types slightly. I'm sure we could get Bingen to generate this for us, but we only really need it for a few things so I just hand-rolled it for now.

- Move svc impls to the WifiDevice itself
  - This way any stack can be uses once connection is handled
  - Added compat impl for Wifi which forwards to the WifiDevice impls
- Added internal wifi error enum and upgraded the basic wifi error enum
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 2, 2023

The static_ip example needs an update just like the dhcp example

Unfortunately, these changes seem to trigger mis-compilations for me

  • ESP32-S3: BLE example crashes with IllegalInstruction - opt-level 1 fixes that
  • ESP32-S3: DHCP example gets stuck in WifiScan - opt-level 1 fixes that
  • ESP32-S3: DHCP example crashes with IllegalInstruction when awaiting the IP address - with all opt-levels

And one totally irrelevant nit-picky thing: Maybe the commit message of the last commit is a bit mis-leading?

@MabezDev MabezDev force-pushed the wifi-device-refactor branch from 1714cf6 to 6d45675 Compare February 2, 2023 12:14
@MabezDev
Copy link
Member Author

MabezDev commented Feb 2, 2023

Went a bit overboard and overhauled the errors in the wifi module to use a result and fixed the examples. I can't reproduce any of those miscompilations though :(. Maybe try cargo update? That's what fixed my issues earlier.

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 2, 2023

Very nice improvements to the error handling

Unfortunately, BLE doesn't compile anymore

Even cargo update and cargo clean don't make it work for me 😢

On S3 in opt-level 3 the dhcp and static_ip examples just freeze at the wifi scan. Oddly enough the coex example gets the scan done but never connects

On S2 in opt-level 2 the dhcp example runs into an illegal instruction while the static_ip example works.

With opt-level 1 everything seems to work on S3.

But on S2 the dhcp example still crashs.

I think needed opt-level 1 is not a big deal but I am a bit concerned I never can make the dhcp example work on S2

Maybe @jessebraham can also give this a try

@MabezDev MabezDev force-pushed the wifi-device-refactor branch from 6d45675 to 9c31518 Compare February 2, 2023 13:59
@MabezDev
Copy link
Member Author

MabezDev commented Feb 2, 2023

Unfortunately, BLE doesn't compile anymore

Oops fixed.

On S3 in opt-level 3 the dhcp and static_ip examples just freeze at the wifi scan. Oddly enough the coex example gets the scan done but never connects

That was the behaviour I saw yesterday then a cargo update magically fixed it 🤷

I can reproduce the S2 issue, which goes away with lto = 'off' under the release profile. Which by the way, is different to lto = false see here: https://doc.rust-lang.org/cargo/reference/profiles.html#lto

@MabezDev
Copy link
Member Author

MabezDev commented Feb 2, 2023

To be honest, I think we should be setting LTO = false (easy to get confused 🤦 ) LTO = 'off' always for the time being (on esp-hal too), until we know the codegeneration from LLVM is more reliable, there are a few buggy optimizations that LTO may be applying here even when we don't want it to.

@MabezDev
Copy link
Member Author

MabezDev commented Feb 2, 2023

Out of curiosity I also just retested #112 (comment) (S2 static IP opt-level = 3) with lto = 'off' and it now works...

Disabling LTO completely is obviously not the true fix, but until it's fixed in the backend I don't really see any reason to have it enabled.

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 2, 2023

Great - lto = 'off makes it work for me - only BLE example on C2 fails with that but we can just state that in the README like we did before with the opt-levels .... So, if you don't mind to change that this LGTM

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.

Given that #116 is already in this LGTM

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Changes look good, I only tried a few examples (using different chips) but they all worked.

@MabezDev MabezDev merged commit 6359e6d into esp-rs:main Feb 2, 2023
MabezDev added a commit to MabezDev/esp-wifi that referenced this pull request Feb 12, 2023
* WifiDevice refactor

- Move svc impls to the WifiDevice itself
  - This way any stack can be uses once connection is handled
  - Added compat impl for Wifi which forwards to the WifiDevice impls
- Added internal wifi error enum and upgraded the basic wifi error enum

* add wifi event

* Add missing fields in scan

* Rework errors in wifi module to return a result

* Fix examples
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.

3 participants