esp-rs / esp-idf-svc

Type-Safe Rust Wrappers for various ESP-IDF services (WiFi, Network, Httpd, Logging, etc.)
https://docs.esp-rs.org/esp-idf-svc/
Apache License 2.0
323 stars 179 forks source link

Enabling `esp_idf_svc::ws::client`, documentation needed #337

Closed coder0xff closed 9 months ago

coder0xff commented 10 months ago

I want to use esp_idf_svc::ws::client. Based on the source code that pulls in the client module

#[cfg(all(
    feature = "alloc",
    esp_idf_comp_tcp_transport_enabled,
    esp_idf_comp_esp_tls_enabled,
    any(
        all(esp_idf_version_major = "4", esp_idf_ws_transport),
        esp_idf_comp_espressif__esp_websocket_client_enabled
    )
))]
pub mod client;

and guessing that I'm meant to remove the esp_idf prefix, capitalize, prepend CONFIG, and add it to sdkconfig.defaults, I added the following to my sdkconfig.defaults:

CONFIG_COMP_TCP_TRANSPORT_ENABLED=y
CONFIG_COMP_ESP_TLS_ENABLED=y
CONFIG_COMP_ESPRESSIF__ESP_WEBSOCKET_CLIENT_ENABLED=y

However, cargo doc --open still doesn't show the client module in esp_idf_svc::ws. I found an (exhaustive?) list of variables in ./target/xtensa-esp32s3-espidf/debug/build/esp-idf-sys-3a66d5767bd3b7f0/out/sdkconfig but the above variables don't appear even after changing my sdkconfig.defaults.

Much of esp_idf_svc contains no documentation at all. As a newcomer to the ESP32 platform, getting a footing is difficult. Adding good documentation is a lot of work of course. I think outlining how to enable or disable each (forgive the abuse of terminology) feature is low-hanging fruit that would have a lot of value.

coder0xff commented 10 months ago

Just an additional note as a testament for the need of more guidance, before I found #265 I had tried adding

    println!("cargo:rustc-cfg=esp_idf_comp_tcp_transport_enabled");
    println!("cargo:rustc-cfg=esp_idf_comp_esp_tls_enabled");
    println!("cargo:rustc-cfg=esp_idf_comp_espressif__esp_websocket_client_enabled");

to my build.rs to no avail.

ivmarkov commented 10 months ago

This effort is based on voluntary, unpaid work. If you would like to make a difference, the best way to make a difference is to open a PR with the documentation changes you would like to see. In the meantime, or if you don't feel like doing this, you can use the splendid ESP IDF documentation, which is very thorough, even if for the C API.

Specifically for the WS client component, you have difficulties which will be soon addressed with https://github.com/esp-rs/esp-idf-svc/pull/336

With that said, the built-in WS client is a weird thing, and I would rather recommend instead using the AsyncEspTls wrapper + edge-net's WS client combo. That is, if you feel comfortable with async.

coder0xff commented 10 months ago

This effort is based on voluntary, unpaid work. If you would like to make a difference, the best way to make a difference is to open a PR with the documentation changes you would like to see.

I understand completely. I appreciate the work done here, and it's not my intention to offend. I know contributing to open-source is often thankless work, so thank you. I'll make an effort to contribute once I have the requisite comprehension. (I'm going to be spending a lot of time with the esp32-s3.)

Thanks for the link to #336. I'm sure it will be helpful. I'm glad you mentioned the C API. I tried a couple of times to correlate the Rust API to the C API, but I didn't have much success (time to read the sources). Adding cross-references to the docs might also add value.

I am trying to do everything async. The impression I got from the signature of esp_idf_svc::ws::client::EspWebSocketClient::new was that it would continue to send events via callback, which I had planned to abstract to async. Will AsyncEspTls + edge-net still be the preferred API after #336 is landed? My gut tells me to use EspWebSocketClient if able.

ivmarkov commented 10 months ago

I am trying to do everything async. The impression I got from the signature of esp_idf_svc::ws::client::EspWebSocketClient::new was that it would continue to send events via callback, which I had planned to abstract to async.

Yes and no. To elaborate: you have the correct intuition that "whenever there is a callback, it can probably be turned into async". Yet, the native EspWebSocketClient API has at least two issues w.r.t. async:

Also not to be underestimated, that while I have written async adapters for WS server, MQTT client, and a few others, I think I did not do it for the ESP IDF WS client (the client was a contribution and I think the contributor - at the time - was not interested in async). So here you would be on your own, dealing with Condvars and stuff s as to turn the callback-with-backpressure into a Future.

Will AsyncEspTls + edge-net still be the preferred API after #336 is landed? My gut tells me to use EspWebSocketClient if able.

I would bet your gut is wrong this time. I can see your line of thought - this is ESP IDF, tried and tested and so on. Yet, ESP IDF can only do so much for async, as it was not designed for async - rather - either for blocking stuff, or (at best) for callback-ing, which relies on internal threads as I mentioned and is often only half-way solution compared to true async. All of WS server/WS client/MQTT/event bus are like that.

The edge-net in the meantime is pure Rust and was designed with async-first in mind. No hidden threads, absolutely minimal.

And btw it is not me trying to advertise my stuff as somebody once accused me :) (after all, I'm also contributing a lot to the esp-idf-* crates so this is to some extent "my stuff" too). It is just that I suffered (quite) a bit when trying to use ESP IDF app networking components (I think for the rest async-ifying ESP IDF kinda works, but these were painful), hence I came to the conclusion, that we (as in the Rust community) should rather push our own minimal embedded stuff on top of plain UDP/TCP sockets stack. Hence edge-net was born.

coder0xff commented 9 months ago

Thank you. On further consideration, I'm not going to be using web sockets. I still have questions though, so I'll open another issue #350.