esp-rs / esp-mbedtls

mbedtls for ESP32 bare-metal
Apache License 2.0
17 stars 7 forks source link

Allow user to provide buffers to asynch::Session #37

Closed GnomedDev closed 1 month ago

GnomedDev commented 1 month ago

Closes #36

GnomedDev commented 1 month ago

Since #35 has been merged, this is now ready for review.

bjoernQ commented 1 month ago

I wonder if we could get rid of the const-generics but just an idea

bjoernQ commented 1 month ago

@AnthonyGrondin want to take a look before it gets merged?

AnthonyGrondin commented 1 month ago

My knowledge of how async Rust uses memory, and static memory footprint on embedded system is still relatively, rudimentary, so I can't figure out how this saves on memory, versus using generics.

Why not use two generics, one for RX and one for TX ?

And for API building and user interface, is it more developer friendly to mutably borrow buffers or own them?

GnomedDev commented 1 month ago

This saves stack memory by allowing users to provide static memory buffers I just kept the same generics, a followup PR can change that if you want. Owning the buffer would defeat the whole purpose, as you would no longer be able to pass static memory buffers in (eg: through make_static!)

bjoernQ commented 1 month ago

I think the code generated by today's compilers is still a bit sub-optimal by sometimes copying things on the stack and using references should mitigate that

AnthonyGrondin commented 1 month ago

LGTM.

I've tested before and after the change. I didn't notice a different in the HEAP usage, provided by esp_wifi, but listing symbol size with xtensa-esp32s3-elf-nm, I get the following diff:

Before:

// xtensa-esp32s3-elf-nm -S --size-sort ./target/xtensa-esp32s3-none-elf/release/examples/async_client | rustfilt
...
420683d0 00000bad T mbedtls_ssl_tls13_handshake_client_step
42098190 00000c5d T mbedtls_aes_self_test
420635fc 00000df7 T mbedtls_ssl_handshake_client_step
4202daec 00000e87 T hostap_recv_mgmt
3fcb6ef8 00000f38 B g_cnxMgr
4209d1f0 00000fd0 T mbedtls_camellia_self_test
42092c00 00000fec T mbedtls_internal_sha256_process
4201b88c 00000ff5 T smoltcp::socket::tcp::Socket::process
4201cc9c 0000103c T smoltcp::iface::interface::ethernet::<impl smoltcp::iface::interface::InterfaceInner>::process_ethernet
42054878 000011ab T SHA1Transform
42015150 000013c0 T mbedtls_mpi_exp_mod
420176ec 0000152a T <esp_backtrace::arch::Context as core::fmt::Debug>::fmt
4209441c 000015e7 T mbedtls_internal_sha512_process
420648b8 0000160c T mbedtls_ssl_handshake_server_step
420901dc 000018d4 T mbedtls_internal_sha1_process
4208b818 00001f8a T mbedtls_internal_ripemd160_process
42002920 00001fac T embassy_executor::raw::TaskStorage<F>::poll
3fcb1180 00004000 B esp_wifi::preempt::TASK_STACK
3fc8f110 00009520 b async_client::__embassy_main::POOL
3fc99d47 00017318 b esp_wifi::HEAP_DATA

After:

// xtensa-esp32s3-elf-nm -S --size-sort ./target/xtensa-esp32s3-none-elf/release/examples/async_client | rustfilt
...
42068460 00000bad T mbedtls_ssl_tls13_handshake_client_step
42098220 00000c5d T mbedtls_aes_self_test
4206368c 00000df7 T mbedtls_ssl_handshake_client_step
4202db7c 00000e87 T hostap_recv_mgmt
3fcb2f10 00000f38 B g_cnxMgr
4209d280 00000fd0 T mbedtls_camellia_self_test
42092c90 00000fec T mbedtls_internal_sha256_process
4201b91c 00000ff5 T smoltcp::socket::tcp::Socket::process
3fc92f30 00001001 B async_client::____embassy_main_task::{{closure}}::STATIC_CELL    <--- These are added
3fc93f31 00001001 B async_client::____embassy_main_task::{{closure}}::STATIC_CELL    <--- These are added
4201cd2c 0000103c T smoltcp::iface::interface::ethernet::<impl smoltcp::iface::interface::InterfaceInner>::process_ethernet
42054908 000011ab T SHA1Transform
420151e0 000013c0 T mbedtls_mpi_exp_mod
4201777c 0000152a T <esp_backtrace::arch::Context as core::fmt::Debug>::fmt
420944ac 000015e7 T mbedtls_internal_sha512_process
42064948 0000160c T mbedtls_ssl_handshake_server_step
4209026c 000018d4 T mbedtls_internal_sha1_process
4208b8a8 00001f8a T mbedtls_internal_ripemd160_process
4200289c 0000214f T embassy_executor::raw::TaskStorage<F>::poll
3fc8f110 00003538 B async_client::__embassy_main::POOL    <--- This symbol takes less space
3fcad198 00004000 B esp_wifi::preempt::TASK_STACK
3fc95d5f 00017318 b esp_wifi::HEAP_DATA

async_client::__embassy_main::POOL is 5982 bytes smaller, with the trade-off of having two static cells (async_client::____embassy_main_task::{{closure}}::STATIC_CELL) with a size of 1001 bytes

I'd like to also check with cargo-call-stack but I can't get it to compile for my machine.

GnomedDev commented 1 month ago

As I said, reduces stack size (and therefore future size) (what you are measuring with the embassy pool) by replacing with statically allocated memory.