-
Notifications
You must be signed in to change notification settings - Fork 13
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
Host (and ESP-IDF) build; less memory usage; edge_nal::TcpConnect
impl
#59
base: main
Are you sure you want to change the base?
Conversation
…not build on non-baremetal
Awesome and thanks for working on this! Looks all good and sane to me but I leave the real review to @AnthonyGrondin (I haven't done much here besides the first few commits .... time to remove me from the |
Maybe the README.md would need some love (not necessarily in this PR) |
Agreed. |
Removing |
Summary of Changes
===================================================
esp-mbedtls-sys
Host build (changes to
esp-mbedtls-sys
)In order to be able to build and run the library on the host machine, the C build has to happen on the machine, with a
build.rs
script:builder::MbedtlsBuilder
- which does the C library build and codegen and which is used by both the existingxtask
s as well as from thebuild.rs
ofesp-mbedtls-sys
mbedtls
GIT submodule is moved toesp-mbedtls-sys
. Otherwise, we cannot publishesp-mbedtls-sys
tocrates.io
libs
folder (the.a
stuff): it needs to be inesp-mbedtls-sys
or else we can't publish itmbedtls
C code is copied aroundESP-IDF build (changes to
esp-mbedtls-sys
)This is also added. It was very easy because for the
-espidf
targetsesp-mbedtls-sys
does nothing and delegates the binding generation, the library build, linking etc. etc. toesp-idf-sys
.Support for future baremetal targets with pre-generated bindings and
.a
libsCurrently not on the table, but adding a new SOC to the
xtask
s should be even easier nowesp-mbedtls
Removal of async memory buffers (a change to
esp-mbedtls
)This is probably the biggest change.
Please look at the new
Session
async
instance, and specifically - atPollCtx
. Especially the latter is very extensively documented in terms of what is the idea and how it does it.While at it, I also implemented a public
connect/read/write
API on theSession
types themselves, so that users don't have to import the Read/Write traits for simple use cases.Tls
andTlsReference
(a change toesp-mbedtls
).Tls
is a singleton that basically represents the Mbedtls lib itself.new
and optionally - withwith_hardware_RSA
the RSA perihTls::new
does not have parameters.TlsReference
is just like&Tls
but allows us to erase the invariant peripheral'd
lifetime of the SHA and RSA periphs that would otherwise contaminate theSession
types too.Note that now,
Tls
is the only type which does have platform-specific aspects!Everything else is cross-platform just like the mbedtls C lib itself, as it does not assume
esp-hal
or anything.TlsConnector
and simplerTlsAcceptor
for edge-nal compat (a change toesp-mbedtls
)Now that we don't have or need TLS buffers, the signature of
TlsAcceptor
is much simpler and has no generics. Note also that I've removed the "bind" logic fromTlsAcceptor
. Not sure it belongs there.I also implemented
TlsConnector
to have a symmetry withTlsAcceptor
and because I actually do need it for e.g. the edge-http HTTPS client.Finally, the above ^^^ types are no longer hard-coded to
edge-nal-embassy
and can be used with anyedge-nal
impl (I use them withedge-nal-std
for example).Session::close
(a change toesp-mbedtls
)(Only for async sessions, maybe we need it for blocking too?)
The idea of this method is to send asynchronously a notif to the other peer that the TLS session is being closed. I just use the mbedtls facilities for that.
Not sure it works 100%, but if not, we need to fix it, as I think such a method is good to have. A bit like the raw TCP "close" we introduced recently in
edge-nal
and which is wrapped bySession::close
.Session::close
also implements theTcpClose
trait now.Next steps
(ASAP) Fix the alignment issues with xtensa
Just locate
aligned_calloc
and uncomment the commented out code at the top of the function.The fact that these alignment issues are not hitting us is a pure coincidence and only a matter of time until we start seeing these (I do see these with the mbedtls-3.6-idf branch, but they are also there in the currently used mbedtls-3.4-idf branch).
How to fix:
Enable in MbedTLS the
malloc/free
"custom hooks" as described here. This way MbedTLS would call us for every malloc/free call it doesImplement these hooks ourselves, by using
GlobalAlloc::alloc
formalloc
and GlobalAlloc::dealloc forfree
. This way we decouple ourselves fromesp-wifi
and just use the Rust global allocator (which is anyway defined on all platforms, including ESP-baremetal)In the hooks, for riscv align the requested memory to 4 bytes and for xtensa - to 8 bytes. The latter would solve the xtensa alignment issues even if we are unsure what object MbedTLS allocates, as I doubt MbedTLS uses u128 anywhere - only up to u64 (the u64s, i.e. their C equivalents are causing us the headaches, as we currently return for structs containing them memory aligned to 4 bytes instead of 8, which is required on xtensa, but not for riscv)
(Optional) in
Certificates
, cluster most or all raw MbedTLS struct pointers into a custom mega-struct which is allocated with a single, safeBox::new()
call. This way this code would become shorter and much safer.(Later) Implement session splitting (full-duplex read and write)
(For web-sockets etc.)
I have an idea, but need to experiment with it a bit. If it works out, the change would be trivial - just one (or two) async mutexes, thanks to
PollCtx::poll_mut
being actually non-blocking and non-awaiting (even if it is marked asasync
).(Later) Get rid of
Session::connect
Forgot to mention that already with these changes,
Session::connect
(a) no longer takesself
but&mut self
and (b) calling it is completely optional, asread
andwrite
call it too.However: it seems MbedTLS anyway calls
mbedtls_ssl_connect
frommbedtls_ssl_read
andmbedtls_ssl_write
so we might not even need all theSession::connect
machinery in the first place...