Add WiFi Easy Connect (DPP) wrappers and associated example #228

jasta · 2023-02-15T01:33:38Z

No description provided.

jasta · 2023-02-15T01:49:58Z

Having troubles getting the example to build for unknown reasons. I have things working in my own local repository, but for some reason sdkconfig.defaults isn't being picked up like I expect for the examples. I can ofc just remove it, but that seems like a shame since this really helped me figure out a good path for provisioning without hardcoding stuff.

jasta · 2023-02-15T01:51:38Z

Also looks like I'm missing some conventions about using alloc vs heapless, std::fmt::Write, etc. Fairly new to the conventions in esp-idf-svc but I'll do my best to try to fix things up. Pointers welcome :)

jasta · 2023-02-15T23:35:49Z

Cargo.toml

@@ -30,8 +30,8 @@ log = { version = "0.4", default-features = false }
 uncased = "0.9.7"
 anyhow = { version = "1", default-features = false, optional = true } # Only used by the deprecated httpd module
 embedded-svc = { version = "0.24", default-features = false }
-esp-idf-sys = { version = "0.32.1", default-features = false, features = ["native"] }


Oops, will remove :)

jasta · 2023-02-15T23:39:16Z

src/wifi_dpp.rs

+}
+
+pub struct EspDppBootstrapped<'d, T> {
+    events_rx: &'d Receiver<DppEvent>,


Hmm, this should probably be &mut so that it guarantees the bootstrapper binding cannot be used while the bootstrapped binding is in scope.

ivmarkov · 2023-02-17T15:59:02Z

Sorry for the delay here. Will look at it over the weekend.

…ow one :)

jasta · 2023-02-19T19:33:02Z

I think I might want to rework this a bit so that there's only one struct, EspDppBootstrapped, and that the borrow of EspWifi only occurs during the constructor. This way you could have a separate thread call EspWifi::stop while EspDppBootstrapped is "stuck" in listen_forever/listen_once() and have it be interrupted. I believe this would occur because esp_supp_dpp_* internally is implemented to listen for wifi events and bail out on error.

This would also make the code a bit more ergonomic to use in practice as all the lifetime stuff would disappear from the struct definition.

I will also remove the example unless ya'll have a good way to keep it and have it actually compile correctly given that it will need sdkconfig.defaults to be provided and I'm not at all sure how that is supposed to work in practice (how can each example have its own config???)

jasta · 2023-02-19T19:56:59Z

Actually I just realized that if I do this so that EspWifi isn't borrowed for the lifetime of the object that handles esp_supp_dpp_init/deinit then we could end up unsafely accessing the singleton (e.g. calling esp_supp_dpp_init twice). The only solution I can think of there is to add a new peripheral. That looks like a more invasive change for sure, and would like to get feedback before I proceed.

ivmarkov · 2023-02-20T13:25:59Z

Actually I just realized that if I do this so that EspWifi isn't borrowed for the lifetime of the object that handles esp_supp_dpp_init/deinit then we could end up unsafely accessing the singleton (e.g. calling esp_supp_dpp_init twice). The only solution I can think of there is to add a new peripheral. That looks like a more invasive change for sure, and would like to get feedback before I proceed.

What about merging the DPP support inside WifiDriver itself, or inside EspWifi?

By the way: your current PR relies on STD being around, including using the std::mpmc queue. The current policy is that esp-idf-svc should ideally expose functionality whithout requiring STD and - where possible - without alloc.

jasta · 2023-02-20T16:53:38Z

Ackd, I think the right compromise here is to have a member of EspWifi that you can borrow to access esp_supp_dpp_*. I'll still keep the code in wifi_dpp.rs so it's neatly separated. That said, I'm still waffling on whether it's better to mutable borrow EspWifi while esp_supp_dpp is in use. There definitely seems to be a few APIs that wouldn't be safe like connect while listening. It does at least seem safe to use EspWifi while you're not actively listening tho (generating the QR code doesn't seem to even utilize the wifi driver, just some generic code in wpa_supplicant).

Re std, yes I'm aware that I need to drop that, I'm just not 100% sure what the proper way to do it is. I'll dig deeper into the rest of the code to see if I can't find prior examples of similar callback relaying going on. If you've got any pointers tho that'd be appreciated to save me some time :)

jasta · 2023-02-20T16:55:00Z

Oh and I'd like some advice on the example I originally had. Can't get it to compile because it needs sdkconfig values and some other project weirdness. Is it preferable to put the example in some other repo?

ivmarkov · 2023-02-20T17:01:05Z

Ackd, I think the right compromise here is to have a member of EspWifi that you can borrow to access esp_supp_dpp_*. I'll still keep the code in wifi_dpp.rs so it's neatly separated. That said, I'm still waffling on whether it's better to mutable borrow EspWifi while esp_supp_dpp is in use. There definitely seems to be a few APIs that wouldn't be safe like connect while listening. It does at least seem safe to use EspWifi while you're not actively listening tho (generating the QR code doesn't seem to even utilize the wifi driver, just some generic code in wpa_supplicant).

Borrowing mutably EspWifi would still work as long as the DPP thing is a "one-off" thing at the beginning, and once it is setup, you can get rid of it. Honestly, I need to read on the DPP to understand what its lifecycle is.

Re std, yes I'm aware that I need to drop that, I'm just not 100% sure what the proper way to do it is. I'll dig deeper into the rest of the code to see if I can't find prior examples of similar callback relaying going on. If you've got any pointers tho that'd be appreciated to save me some time :)

Well, a lot of the stuff in STD is either type aliases to stuff which is actually in core (and is thus perfectly fine to use), or to stuff which is actually in alloc (which is kinda OK to use as long as you don't over-use it as it is heap).

If you really really need to reach out for other STD stuff (as in networking, FS, multithreading, or complex stuff with mutexes and condvars, you probably need to rethink all of it). If you really really need mutexes and condvars, we have these in esp-idf-hal for use while implementing a driver/API.

jasta · 2023-02-20T22:13:16Z

Thanks, I'll iterate a bit on my end and update the PR when it's ready for you to look again.

FWIW, the DPP lifecycle is pretty mysterious from the documentation and even implementation. It's almost like it can coexist simultaneously with the rest of the Wi-Fi functionality but there's glaring conflicts at the edges. What I wasn't sure about was whether this project prefers to be conservative (the Rust wrappers can't reasonably break in unsafe ways, e.g. segfault) or liberal (the Rust wrappers use convenient idioms but are roughly as dangerous/powerful as the C APIs). I'm currently assuming the former.

ivmarkov · 2023-02-21T07:06:58Z

Thanks, I'll iterate a bit on my end and update the PR when it's ready for you to look again.

FWIW, the DPP lifecycle is pretty mysterious from the documentation and even implementation. It's almost like it can coexist simultaneously with the rest of the Wi-Fi functionality but there's glaring conflicts at the edges. What I wasn't sure about was whether this project prefers to be conservative (the Rust wrappers can't reasonably break in unsafe ways, e.g. segfault) or liberal (the Rust wrappers use convenient idioms but are roughly as dangerous/powerful as the C APIs). I'm currently assuming the former.

Absolutely the former. If a Rust API is marked as "safe", then it should not break, period. :)

jasta · 2023-03-02T03:42:49Z

@ivmarkov, updated as an RFC with a few caveats:

Turns out the feature is very flaky upstream and requires esp_dpp: Fix retry with esp_supp_dpp_start_listen after failure (IDFGH-9508) espressif/esp-idf#10865 to be reliable at a minimum. The code "works" without this fix, but I recommend that we save our users the frustration of having to retry like a thousand times in certain edge case scenarios. I recommend then we wait for this to land and conditionalize the feature on the upstream version that has the support.
Still don't have a clear path for publishing an example, but I really think we should provide one as the API is a little bit counter intuitive (it is not great upstream either, IMO). I recommend that we upgrade the examples/ dir a bit and use separate cargo repositories that can have their own sdkconfig's and all that to provide functional examples like esp-idf upstream does. I can give this a shot but don't want to start hacking until I have buy-in.
I don't think the esp_idf_version_major/minor thing is enough today to be able to reliably select for the correct version since we really need to be able to say esp_idf_version >= 5.1 or something. Maybe what we're looking for is to introduce new cfg features that represent inequalities we need, like esp_idf_version_at_least_5_1? Is that even possible? EDIT: I think what I'm after is a runtime check of ESP_IDF_VERSION
The commits are a bit of a mess, I'll squash and clean things up once we get through the review.

jasta · 2023-03-02T06:54:12Z

src/wifi_dpp.rs

+    }
+
+    // TODO: Sure would be nice to be able to write esp_idf_version >= 5.1...
+    #[cfg(any(


Note: I could do this at runtime for much less code, but I'm not sure how to access the version from esp-idf-sys? Is ESP_IDF_VERSION/_VAL exposed?

jasta · 2023-03-02T06:57:28Z

src/wifi_dpp.rs

+    /// Blocking wait for credentials or a possibly retryable error.  Note that user error
+    /// such as scanning the wrong QR code can trigger this error case.  Retries are highly
+    /// recommended, and especially via [Self::attempt_retry].
+    pub fn wait_for_credentials(&self) -> Result<ClientConfiguration, EspError> {


I hesitated on this being &mut self or &self. It will break stuff if you call this in parallel (should hang forever), but if it's &mut self we can't expose stop_listen on Self. Would it be preferred to split the listener into two structs, one that can be used to cancel and one that can be used to wait with some shared lock between them?

I'm fine with the current little cheat for now. We can fix/complicate in a subsequent version.

ivmarkov · 2023-03-04T05:24:08Z

This is my understanding as well. Rust is unaware of this because we don't have targets for the new architecture (which ofc aren't really new but for our purposes I think that's how we should treat it). This is sort of like what would happen if all the sudden we got an x86 128 bit arch, you'd have to add this new arch in dozens of places before you can use it.

I think the comparison you do above might be slightly misleading. The way I understand it, is that Rust's current riscv32imc- targets are in fact riscv32imc_zicsr_zifencei- targets according to the new RISCV spec. Because what is named in the old RISCV spec riscv32imc- is in fact equal to riscv32imc_zicsr_zifencei- in the new RISCV spec.

What Rust doesn't have is the new "pure" riscv32imc- targets, which do not implement the zicsr and zifencei extensions, but since the esp32cXX chips in fact DO implement these extensions, we can just continue to use the existing Rust targets on those.

Just add the new arch alongside the old one (to rustc, cc, our examples, etc). User has to work this out and if they want to use 5.1+, they'll need to change the --target they use in .cargo/config.toml.

Waiting for Rust to do - whatever - just to allow folks to use ESP IDF 5.1 seems like the wrong choice for me.

It is absolutely not so simple to add the new targets.

First of all, to add those to Rust, LLVM itself has to have these new targets (and this is a target which is the current one MINUS zicsr and minus zifencei).
Further, we have the problem of backwards compatibility, because - again - the existing riscv32imc- target is in fact the target that you need, except wrongly named (and that's a wrong name only according to the NEW spec).
Last but not least - you actually need the existing targets (putting aside how these are named).

Pass compatibility option in cc-rs (--isa-spec=2.2 iirc) that makes gcc 12 behave like gcc 11. Not recommended by RISC-V folks it seems but I don't know why exactly.

Why not? Given the situation, it seems this is the cleanest option, until everyone had upgraded?

Swap the order cc-rs adds -march vs CFLAGS such that the CMAKE_C_FLAGS we pick up from esp-idf ends up turning the gcc command line into -march=rv32imc ... -march=rv32imc_zicsr_ziferencei. See my log output above which shows we actually are passing both the correct and incorrect -march, the order is such that the wrong arch is last. This probably works by sheer dumb luck but I suspect is fragile with lots of corner cases.
I feel like (3) isn't really an option and will blow up in our face quickly. (1) and (2) are pretty significant changes that commit us down a heavy maintenance path and likely need to include a few more stakeholders, but I'm not sure who to contact.

If we can't do (2) from above, we should be doing (3) IMO. So my order in terms of feasibility would be:
(2)
(3)
(1)

jasta · 2023-03-04T19:52:57Z

@ivmarkov i think your above comment was from a different issue I filed. On my mobile now and can't find the link easily but it relates to building esp idf 5.1. we can consider this PR about DPP separate. I'll wait to respond in full on the other task for a clearer paper trail (but i do agree with the gist of what you said).

Fwiw they were related in the sense that I had to patch esp-idf which meant I had to build and test master which ofc broke :)

ivmarkov · 2023-03-08T15:41:10Z

@jasta Yes, sorry for the noise. I've moved my comment to the issue here: esp-rs/esp-idf-sys#176

ivmarkov · 2023-03-17T19:20:49Z

src/wifi_dpp.rs

+/// esp_supp_dpp_init.
+static DPP_INITIALIZED: Mutex<Option<Weak<DppInitialized>>> = Mutex::wrap(RawMutex::new(), None);
+
+impl DppInitialized {


I guess I'm missing a detail here, but I'm a bit lost as to why we need DppInitialized in the first place? Here's my thinking:

EspWifi is a singleton; you cannot have two instances of it, alive at any time

Ergo, there can be only one instance, ever of EspWifiDpp alive at any time, as by design, creating such an instance needs a mutable borrow of EspWifi

You initialize DPP when constructing EspWifiDpp

You de-initialize DPP when dropping EspWifiDpp

What am I missing?

I can combine them, but the main reason I kept them separate was so that it was very clear the global singleton (DPP_INITIALIZED) was literally tied to the esp_supp_dpp_init/deinit lifecycle, whereas EspWifiDpp was tied to the bootstrapped lifecycle after you call esp_supp_dpp_bootstrap_gen. In theory, you could bootstrap multiple times without deinit which would require fewer changes with the way it's currently designed than it would if DppInitialized/EspWifiDpp were combined. I'll leave it up to you if you prefer to reduce the total amount of code further in exchange for a maybe heavier refactoring needed later if we want to add features (which I suspect will happen since Espressif's support for DPP is pretty lacking at the moment).

ivmarkov · 2023-03-17T19:21:43Z

src/wifi_dpp.rs

+    /// ESP-IDF is being used.
+    /// * `associated_data` - (Optional) Arbitrary extra information to include with the QR
+    /// code that may be relevant to the configurator.
+    pub fn generate_qrcode(


Why not naming it simply... new?

In the future I expect there to be other ways to bootstrap DPP, as DPP itself supports QR code, proof of knowledge, NFC URI, and even Bluetooth in later revisions. Espressif's API even supports this idea through an awkward enum, though I'd prefer the Rust API expose this with new fn's like fn using_pkex(...), fn using_ble(...), etc.

ivmarkov · 2023-03-17T19:23:25Z

src/wifi_dpp.rs

+
+#[cfg(not(esp_idf_version_major = "4"))]
+/// ESP-IDF 5.x requires the caller put the key into the PEM format
+fn frame_key(unframed: &[u8; 32]) -> Vec<u8> {


Why calling into alloc just for this? You can either return a heapless::Vec (as the size of the returned thing is actually known), or even an array, as the final array size is also known?

I was struggling with this one a bit going back and forth between a compile-time approach vs a runtime one (compile-time can do as you say with heapless, runtime would need alloc since you wouldn't know the result size). Since I settled on compile-time, I'll go ahead and slip in heapless::Vec again.

ivmarkov · 2023-03-17T19:25:10Z

src/wifi_dpp.rs

+    /// Blocking wait for credentials or a possibly retryable error.  Note that user error
+    /// such as scanning the wrong QR code can trigger this error case.  Retries are highly
+    /// recommended, and especially via [Self::attempt_retry].
+    pub fn wait_for_credentials(&self) -> Result<ClientConfiguration, EspError> {


I'm fine with the current little cheat for now. We can fix/complicate in a subsequent version.

ivmarkov · 2023-03-17T19:30:17Z

@jasta Very sorry for having you wait for so long for my feedback!

jasta · 2023-03-20T23:08:49Z

No worries, I understand the need to take this slow on new APIs, especially since it's not cut and dry how exactly to port C APIs to Rust idioms.

Also note I'm not in a hurry to land this just yet as Espressif still hasn't accepted or even really acknowledged my patch that actually makes the feature work reliably: espressif/esp-idf#10865. Without the patch, even their own example code doesn't work better than 1/10 times for me.

mzakharocsc · 2024-10-21T00:52:48Z

Also note I'm not in a hurry to land this just yet as Espressif still hasn't accepted or even really acknowledged my patch that actually makes the feature work reliably: espressif/esp-idf#10865. Without the patch, even their own example code doesn't work better than 1/10 times for me.

On the other hand, Espressif's own provisioning works well over BLE, has apps and documentation for Android and iOS, and importantly, supports custom endpoints. It may be a better candidate to implement over DPP.

ivmarkov · 2024-10-21T06:10:29Z

Also note I'm not in a hurry to land this just yet as Espressif still hasn't accepted or even really acknowledged my patch that actually makes the feature work reliably: espressif/esp-idf#10865. Without the patch, even their own example code doesn't work better than 1/10 times for me.

On the other hand, Espressif's own provisioning works well over BLE, has apps and documentation for Android and iOS, and importantly, supports custom endpoints. It may be a better candidate to implement over DPP.

You can always take the time and open a PR for it then!

DPP is at least a standard albeit not widely accepted. This means eventually you might get support for it out of the box in your phone, with a built-in app.

The Espressif provisioning is done with their own, Espressif-branded app, that you have to fork rebrand and maintain.

If you do this, you might be better off just maintaining your own, DPP-compatible app. At least on Android, the DPP APIs are available. Not sure for iPhone.

mzakharocsc · 2024-10-21T12:11:39Z

The Espressif provisioning is done with their own, Espressif-branded app, that you have to fork rebrand and maintain.

If you do not need custom endpoints, for hobby projects, you can download Espressiff-branded app from the Play Store/App store. No forking/rebranding necessary. As for building non-hobby projects, firmware could require custom configuration in some way on first boot, so custom endpoints are a must. DPP lacks in that regard.

ivmarkov · 2024-10-21T12:54:32Z

The Espressif provisioning is done with their own, Espressif-branded app, that you have to fork rebrand and maintain.

If you do not need custom endpoints, for hobby projects, you can download Espressiff-branded app from the Play Store/App store. No forking/rebranding necessary. As for building non-hobby projects, firmware could require custom configuration in some way on first boot, so custom endpoints are a must. DPP lacks in that regard.

I have no idea what endpoints or custom endpoints even mean in the context of wifi provisioning, so I don't follow.

mzakharocsc · 2024-10-21T14:10:18Z

I have no idea what endpoints or custom endpoints even mean in the context of wifi provisioning, so I don't follow.

Custom endpoints allow exchange of extra configuration during provisioning process besides just wifi credentials. One example could be setting MQTT url during provisioning process. I am unclear how DPP-based provisioning would accomplish this.

ivmarkov · 2024-10-21T14:24:18Z

I have no idea what endpoints or custom endpoints even mean in the context of wifi provisioning, so I don't follow.

Custom endpoints allow exchange of extra configuration during provisioning process besides just wifi credentials. One example could be setting MQTT url during provisioning process. I am unclear how DPP-based provisioning would accomplish this.

This might not be necessary at all in certain cases, like most / all B2C use-cases, where the MQTT server endpoint is simply hard-coded in the firmware or flashed in the device NVS storage during factory setup.

Most end-user equipment only needs access to the internet. You can't ask your customer to type-in an MQTT URL, as she might not even know what is an MQTT URL in the first place.

B2B is another story. There, having the option to provision extra info as part of the Wifi provisioning might or might not be useful. If your devices are hitting the cloud directly, surely not useful. If you have large enterprises with their own MQTT or whatever, surely useful yes.

For a hobby setup, I don't think any of that (including the Espressif custom provisioner) is necessary. You could just hard-code all of that or run the good old soft-ap + captive portal + HTTP server combo.

ivmarkov · 2024-10-21T14:58:10Z

But with that said, and to repeat:

You can always take the time and open a PR for it then!

Unlike the bare-metal crates, the development of esp-idf-* is not sponsored by Espressif. Therefore, implementing new wrappers for ESP-IDF functionality is in the hands of external contributors.

mzakharo · 2024-10-25T22:31:31Z

You can always take the time and open a PR for it then!

I have very basic implementation here - please let me know if you think this is worth doing a PR for.

jasta mentioned this pull request Feb 15, 2023

Wi-Fi provisioning support #87

Open

jasta force-pushed the esp_dpp branch from c156ba9 to 8016e66 Compare February 15, 2023 01:47

Add WiFi Easy Connect (DPP) wrappers and associated example

89ad844

jasta force-pushed the esp_dpp branch from 8016e66 to 89ad844 Compare February 15, 2023 01:48

jasta commented Feb 15, 2023

View reviewed changes

Removed local paths added by mistake, will use patch.crates-io from n…

4514611

…ow one :)

jasta added 2 commits February 19, 2023 11:36

Removed wifi_dpp_setup example, can't get it to compile

c4feaf3

Run cargo fix / cargo fmt

c1a0f05

jasta force-pushed the esp_dpp branch from 2f090ff to c1a0f05 Compare February 19, 2023 19:40

Don't forward disconnect/stop errors during init

e647aab

jasta mentioned this pull request Feb 21, 2023

DPP Auth Failed on dpp-enrollee example (IDFGH-9228) espressif/esp-idf#10615

Open

3 tasks

jasta added 2 commits March 1, 2023 19:33

wifi_dpp: Reworked to not use std and generally simplified code

96f80ae

cargo fmt

a6132ea

jasta commented Mar 2, 2023

View reviewed changes

ivmarkov requested changes Mar 17, 2023

View reviewed changes

ivmarkov force-pushed the master branch 3 times, most recently from 8a8adba to 5ccb542 Compare July 5, 2023 19:27

Add WiFi Easy Connect (DPP) wrappers and associated example #228

Are you sure you want to change the base?

Add WiFi Easy Connect (DPP) wrappers and associated example #228

Conversation

jasta commented Feb 15, 2023

jasta commented Feb 15, 2023

jasta commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov commented Feb 17, 2023

jasta commented Feb 19, 2023

jasta commented Feb 19, 2023

ivmarkov commented Feb 20, 2023

jasta commented Feb 20, 2023

jasta commented Feb 20, 2023

ivmarkov commented Feb 20, 2023

jasta commented Feb 20, 2023

ivmarkov commented Feb 21, 2023

jasta commented Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov commented Mar 4, 2023

jasta commented Mar 4, 2023

ivmarkov commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov commented Mar 17, 2023

jasta commented Mar 20, 2023

mzakharocsc commented Oct 21, 2024

ivmarkov commented Oct 21, 2024

mzakharocsc commented Oct 21, 2024

ivmarkov commented Oct 21, 2024

mzakharocsc commented Oct 21, 2024

ivmarkov commented Oct 21, 2024 • edited Loading

ivmarkov commented Oct 21, 2024

mzakharo commented Oct 25, 2024

jasta commented Mar 2, 2023 •

edited

Loading

ivmarkov commented Oct 21, 2024 •

edited

Loading