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

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

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

GnomedDev
Copy link
Contributor

@GnomedDev GnomedDev commented Jul 17, 2024

Closes #36

@GnomedDev GnomedDev marked this pull request as ready for review July 18, 2024 12:22
@GnomedDev
Copy link
Contributor Author

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

Copy link
Collaborator

@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

@bjoernQ
Copy link
Collaborator

bjoernQ commented Jul 19, 2024

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

@bjoernQ
Copy link
Collaborator

bjoernQ commented Jul 19, 2024

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

@AnthonyGrondin
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

bjoernQ commented Jul 19, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@bjoernQ bjoernQ merged commit e8a33f2 into esp-rs:main Jul 23, 2024
1 check passed
@GnomedDev GnomedDev deleted the pass-in-buffer branch July 23, 2024 11:33
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.

Session should have it's buffer passed in from outside
3 participants