Add smoltcp TCP transport #2213

conectado · 2021-11-16T15:08:53Z

Implementing feature discussed in #1804

Description

This PR includes:

An implementation of the tcp transport protocol using smoltcp.
Abstract common behavior between the smoltcp and std implementation into ockam_transport_core.

This PR does NOT include:

A portal and inlet/outlet implementation for smoltcp.
A no_std implementation of the ockam_transport_smoltcp traits needed to run an example in a no_std device. (Although it does compile in no_std).

Implementation notes

ockam_transport_core::tcp includes the extracted behavior from ockam_transport_tcp.
- worker are the same workers as they were in the old ockam_transport_tcp crate but using the new traits to work in no_std with smoltcp.
- router includes the old definition of router and handle as in the old ockam_transport_tcp, save for the methods used by portals and inlets/outlets, those were extended by a newtype pattern in the ockam_transport_tcp crate.
- traits Includes the definition of the behavior needed from tokio's socket to implement the workers(and endpoint_resolver to resolve endpoints with the router). Additionally:
  - io Includes a defintion of the async io traits, copied from futures, that is compatible with no_std(plus the necessary extensions), this is needed because neither tokio nor futures have a no_std compatible AsyncRead and AsyncWrite due to their use of std::io::Error.
ockam_transport_smoltcp includes the implementation of the protocol using smoltcp. The surface structure is basically the same as ockam_transport_tcp using a SmolTcpTransport instead of TcpTransport with some differences needed by additional configuration for smoltcp and the new method get_stack to obtain the stack for polling manually.
- port_provider is needed to abstract the behavior of obtaining a port that can be pretty different in each implementation(For now I decided to make the PortProvider trait static to be able to hide it inside the smoltcp's EndpointResolver and then an implementation can use an static global if it needs to keep track of some state, this way the PortProvider doesn't exist for the normal tcp implementation).
- net here is most of the interesting things for this crate, providing a wrapper for smoltcp related things. (A lot of the implementation was based on embassy)
  - stack provides a tcp stack.
    - StackFacade provides type safety to make sure the stack is initialized before being used. (Note: I only managed to panic to prevent double initialization any idea to give type safety to prevent this is welcomed!)
  - tcp Provides an interface similar to tcp's sockets.
  - timer provide a monotonic clock trait that is needed by the stack(More discussion about this in this PR comments!).
  - device Provides a Device wrapper around smoltcp's Device to handle explicitly the wakers for async. (This module also includes the tuntap implementation)
I tried not to use unsafe unless absolutely needed(only for the tuntap interface), even if it meant having mutexes wrapping other mutexes, so that those can be added in a separate PR.
I tried to prevent allocations to make it easier to stop depending on alloc in the future except when the implementation would be much harder or would require unsafe.(I figured it'd be okay requiring alloc since the whole project requires alloc anyways) I tried to leave a todo comment over each allocation.
I didn't include an integration test like in the tcp implementation because that would either require creating programatically a tuntap interface or mocking the Device. Leaving this for a future PR.

Test the PR

To test that ockam_transport_smoltcp compiles in no_std, even if there is no example doing this:
cd implementaitons/rust/ockam/ockam_transport_smoltcp && cargo +nightly check --target thumbv7em-none-eabihf --no-default-features --features="no_std, alloc, pool-32"
To test the PR functioning, I added an example with a README on how to run it.

Next steps

Add at least one no_std implementation of:
- clock
- Device. Leaving this for a future PR.
- PortProvider
Use embedded_nal as suggested here, this probably would let us replace some of the traits in ockam_tcp_core.
Extract portal and inlet/outlet behavior into ockam_tcp_core and add an implementation for smoltcp.
Add the integration test mentioned before.
Add unsafe sections to prevent wrapping mutexes within mutexes and allocations.

Checks

All commits in this Pull Request are signed.
All commits in this Pull Request follow the Ockam commit message convention.
I accept the Ockam Community Code of Conduct.
I have accepted the Ockam Contributor Licence Agreement by adding my Git/Github details in a row at the end of the CONTRIBUTORS.csv file in a separate pull request to the ockam-network/contributors repository.

thomcc · 2021-11-17T16:32:49Z

This looks great so far, please don't hesitate to ask if you have any questions!

mrinalwadhwa · 2021-12-06T02:45:12Z

@conectado thank you for continuing to work on this.
Let us know if you need help or have any questions along the way.

conectado · 2021-12-06T04:24:26Z

@conectado thank you for continuing to work on this. Let us know if you need help or have any questions along the way.

Thanks 😄 For now things are going quite smoothly, I will start working on making it work with no-std. Will let you know if I've any difficulties.

conectado · 2021-12-13T02:14:36Z

@mrinalwadhwa @thomcc I'm in the middle of changing the implementation to support no-std (I'm starting with a version that needs alloc) and when polling the ethernet interface for new packages I need to pass the current Instant which is the number of milliseconds that passed since startup. However, the method Instant::now provided by smoltcp only works in std, I was wondering if there's something already in the repo to deal with time in no-std enviroment.

If there's not I think there are a couple of options on how to advance with this:

We could use embassy-rs on which I based most of the async implementation for smoltcp and it provides a timer implementation that we could use. The disadvantages of using embassy are:
- It doesn't have a release in crates.io yet so we have to depend on an specific commit
- It only works on nightly (But it seems that the no_std examples in ockam only works on nightly too, so that might not be a problem)
We could leverage the types and traits provided by embedded-time this could be beneficial also because there seems to be an on-going effort to include it into the embedded-hal crate(see Solution for dealing with clocks and time in embedded systems rust-embedded/embedded-hal#211)
We could create our own traits and types as simple as possible for this case.

For now I'll try going with the third option(at least to understand the problem better and decide later) but any kind of tip or insight is appreciated as I don't have any experience with this topic 🙏

thomcc · 2021-12-13T02:18:20Z

@antoinevg might have some insight onto the 2nd option, but in general the third option sounds best for now, if possible, since I believe we'd like not to only be able to use this in embedded (even if that is the primary use case).

conectado · 2021-12-13T02:21:50Z

@antoinevg might have some insight onto the 2nd option, but in general the third option sounds best for now, if possible, since I believe we'd like not to only be able to use this in embedded (even if that is the primary use case).

I think we could use smoltcp's Instant::now when std is on and use one of the other options when that's not the case.

thomcc · 2021-12-13T02:24:28Z

Ah, that would work.

antoinevg · 2021-12-13T06:32:14Z

when polling the ethernet interface for new packages I need to pass the current Instant which is the number of milliseconds that passed since startup. However, the method Instant::now provided by smoltcp only works in std

Everyone will tend to have their own way of handling system time for their particular project so I'd probably try to steer away from making that decision for them.

For myself, I usually handle this by defining an atomic integer and then setting a SysTick exception or similar to occur every 1ms to increment it.

See, for example:

https://github.com/antoinevg/nucleo-h7xx/blob/1e3aa9bae284da845c5f7a76c2ce192cf91d91ef/examples/ethernet_hal.rs#L195-L222

I'd be perfectly happy if you just passed smoltcp's timestamp requirement on to the developer.

i.e. requiring developers to pass it in via your Stack poll method as poll_iface(self: &Arc<Self>, cx: &mut Context, timestamp: Instant) or somesuch.

conectado · 2021-12-13T08:46:46Z

when polling the ethernet interface for new packages I need to pass the current Instant which is the number of milliseconds that passed since startup. However, the method Instant::now provided by smoltcp only works in std

I'd be perfectly happy if you just passed smoltcp's timestamp requirement on to the developer.

i.e. requiring developers to pass it in via your Stack poll method as poll_iface(self: &Arc<Self>, cx: &mut Context, timestamp: Instant) or somesuch.

Since I spawn the task to poll the interface myself I could ask for the developer a struct when starting the Stack (Stack::run(&self, clock: impl Clock)) implementing a simple trait:

type MIlliseconds = i64;

trait Clock {
  type Instant: Into<Milliseconds>
  fn now(&self) -> Self::Instant;
}

Or do you think it is better if I let starting the task for polling to the developer too?

antoinevg · 2021-12-13T09:09:34Z

Or do you think it is better if I let starting the task for polling to the developer too?

It may also be useful to give the developer the option to do even the polling themselves.

e.g. On embedded it's best to poll smoltcp in the [ethernet] interrupt handler and that can be set up very differently depending on how the app is structured.

In the longer run maybe we should even be looking at something like:

https://crates.io/crates/embedded-nal

All that said, there are so many open questions around async/embedded that I think almost any approach that results in running code is valuable right now because it helps us all understand the space better!

conectado · 2021-12-13T11:01:52Z

Or do you think it is better if I let starting the task for polling to the developer too?

All that said, there are so many open questions around async/embedded that I think almost any approach that results in running code is valuable right now because it helps us all understand the space better!

Alright! Then, I will advance with my current approach, I think adding the possibility for the developer to poll the interface themselves is easy so I will add that too for now and later on revisit to use embedded-nal if possible.

* Changes to support using smoltcp as transport protocol in crate `ockam_transport_smoltcp`. * Includes async interface. * Ability to manually poll. * Necessary implementation of interfaces to run in std using tun/tap interface. * Extract common behavior from smoltcp and std tcp into `ockam_transport_core` in `tcp` module. * Necessary unix socket behavior extracted into traits. * Worker and Route behavior extracted into common modules. * No support for portals and inlet yet.

mrinalwadhwa · 2022-02-21T04:44:23Z

@conectado awesome work! Thank you ❤️
Looking forward to merging this after we get some reviews.

mrinalwadhwa · 2022-02-21T04:46:52Z

Cargo.lock

@@ -204,9 +244,9 @@ dependencies = [

 [[package]]
 name = "autocfg"
-version = "1.1.0"
+version = "1.0.1"


We may need to update the Cargo.lock before merging since this diff bumps several dependency versions down from what is in develop.

Fixed here!

spacekookie

Hey!

Thank you for all the effort you already put into this PR. I left you some comments with minor things I'd like to see changed in the API.

I haven't read all the code yet (it's quite a lot 😅) but this looks very good so far 🙂

spacekookie · 2022-02-22T14:56:17Z

...entations/rust/ockam/ockam_examples/example_projects/smoltcp/examples/network_echo_server.rs

+    let tcp = SmolTcpTransport::<ThreadLocalPortProvider>::create(&ctx, configuration, Some(StdClock))
+        .await?;


Is there any way we can hide that generic parameter here? It's good to have the option to use a different port provider mechanism but most users are not going to care and be confused by what exactly this is.

I think we could either create a type alias:

type TcpTransport = SmolTcpTransport<ThreadLocalPortProvider>

So most users would use TcpTransport and those who care about the port provider use SmolTcpTransport<P>.

Or we could add a default generic parameter:

struct SmolTcpTransport<P = ThreadLocalPortProvider> { ... }

But that has the problem that users that wanted to use the default would need to do <SmolTcpTransport>::create(...).

However, I think that most users that will use this crate would do it in a no_std context, meaning they will need to provide the port provider. I think that using this in a std context would be mostly done for testing and examples.

spacekookie · 2022-02-22T15:01:28Z

implementations/rust/ockam/ockam_transport_core/src/lib.rs

+/// TCP address type constant
+pub const TCP: u8 = 1;
+
+pub(crate) const CLUSTER_NAME: &str = "_internals.transport.tcp";


Minor note: maybe we want to rename this cluster to smoltcp so that shutdowns between the two tcp stacks (if running in parallel) are independent.

spacekookie · 2022-02-22T15:06:26Z

implementations/rust/ockam/ockam_transport_core/src/tcp/workers/receiver.rs

    peer_addr: Address,
+    tcp_stream: R,
+    cluster_name: &'static str,


Is there a reason to store the cluster_name again here?

It was a vestigial change when I took the cluster_name as a parameter from outside the crate and I needed to store it to use it in the worker's initialize. But since I changed it back to taking the cluster_name from outside I need to keep it.

thomcc · 2022-02-22T17:26:24Z

I'll finish reviewing later today, but the length of the lines that have comments is a bit much. I think we try to keep that to around 100 characters.

If you use vscode, I use https://marketplace.visualstudio.com/items?itemName=stkb.rewrap to help with this.

antoinevg

Thank you. This is a stunning PR, really good work.

I can't wait to try this out on embedded!

antoinevg · 2022-02-23T12:08:43Z

implementations/rust/ockam/ockam_transport_core/README.md

 [license-image]: https://img.shields.io/badge/License-Apache%202.0-green.svg
 [license-link]: https://github.com/ockam-network/ockam/blob/HEAD/LICENSE
-
 [discuss-image]: https://img.shields.io/badge/Discuss-Github%20Discussions-ff70b4.svg
 [discuss-link]: https://github.com/ockam-network/ockam/discussions


Nit: Please restore the formatting here to keep it consistent with the other crates.

antoinevg · 2022-02-23T12:47:29Z

implementations/rust/ockam/ockam_transport_core/Cargo.toml

+std = [
+    "ockam_core/std",
+    "ockam_node/std",
+    "tokio",
+    "tokio/io-util",
+    "tokio/net",
+]


I agree 100% with you that we should try to reduce code duplication as much as possible but, in this particular case, I'd argue against moving the TCP router/workers etc. into ockam_transport_core.

My reasoning is basically this:

We've now created a dependency inockam_transport_core on ockam_node and tokio. [1] This could be problematic as we already have an alternate async executor implementation for no_std targets (ockam_executor) and we are also likely to have alternate ockam_node implementations in the near future.

There's likely to be other weirdnesses that come up when we start using ockam_transport_smoltcp in anger on bare-metal embedded devices that would require some changes to router/worker that are exclusive to ockam_transport_smoltcp

It's not a lot of code duplication and it's nice to have it living close to the code that will be using it. We now have multiple transports and I don't think we've yet had a case where we had to make big changes to one of their router/worker implementations that needed to be propagated to the others.

[1] Aside: Check out the (admittedly hackish) way we currently wrap tokio in ockam_node for no_std:

https://github.com/ockam-network/ockam/blob/b50149e4b23a5247922d6622b8ba9815a47c970e/implementations/rust/ockam/ockam_node/src/lib.rs#L23-L27

And:

https://github.com/ockam-network/ockam/blob/b50149e4b23a5247922d6622b8ba9815a47c970e/implementations/rust/ockam/ockam_executor/src/lib.rs#L34-L47

I think this is not a problem since tokio is an optional dependency that's only included with std enabled. I only use tokio here for the implementations of the AsyncRead and AsyncWrite traits on std. I could completely remove the dependency by using the newtype pattern to do the implementation in ockam_transport_tcp if that's preferable.

I figured that we can start extracting the behavior as the weirdness appears, that way we maximize the shared behavior.

Although not particularly a big change while working on this the heartbeat was added and by solving conflicts it was automatically enjoyed by the smoltcp crate.

All in all I thought it was a good idea as the behavior they share don't seem coincidental and maybe in the future more protocols could use this worker/router implementation.

Anyways, I understand the points and also the generic implementation along with all the trait bounds make the code much harder to parse so if you still think is better to have each crate with their own worker/router implementation I will go back to that!

Valid points all but I do feel like the end result is a bigger increase in overall code complexity than what we're gaining through de-duplication here.

Thoughts @thomcc @SanjoDeundiak @spacekookie ?

antoinevg · 2022-02-23T12:48:38Z

implementations/rust/ockam/ockam_transport_core/src/tcp/router/handle.rs

+// We need alloc here for `async_trait` it's very easy to prevent this by making this clone
+// directly inside of `bind` but since `processors` already needs to be an async_trait and
+// ockam_core still needs alloc I'll leave this here.
+use ockam_core::compat::boxed::Box;


antoinevg · 2022-02-23T12:51:35Z

implementations/rust/ockam/ockam_transport_core/src/tcp/traits/io.rs

@@ -0,0 +1,222 @@
+//! Traits based in the `futures::io` crate to support `no_std` compilation.


Thank you for these!

I don't think we're likely to get official ones any time soon: rust-lang/wg-async#23

I'm wondering if these shouldn't live in ockam_core::compat? @SanjoDeundiak @thomcc @spacekookie ?

See nrc/portable-interoperable#5 (and https://www.ncameron.org/blog/async-read-and-write-traits/) for discussion on "official" equivalents.

But in the mean time, these should be in ockam_core::compat, yeah.

antoinevg · 2022-02-23T12:55:48Z

implementations/rust/ockam/ockam_transport_smoltcp/README

+[main-ockam-crate-link]: https://crates.io/crates/ockam
+[crate-image]: https://img.shields.io/crates/v/ockam_transport_tcp.svg
+[crate-link]: https://crates.io/crates/ockam_transport_tcp
+[docs-image]: https://docs.rs/ockam_transport_tcp/badge.svg
+[docs-link]: https://docs.rs/ockam_transport_tcp
+[license-image]: https://img.shields.io/badge/License-Apache%202.0-green.svg
+[license-link]: https://github.com/ockam-network/ockam/blob/HEAD/LICENSE
+[discuss-image]: https://img.shields.io/badge/Discuss-Github%20Discussions-ff70b4.svg
+[discuss-link]: https://github.com/ockam-network/ockam/discussions


Nit: See previous nit on consistency between crates :-)

antoinevg · 2022-02-23T12:57:19Z

implementations/rust/ockam/ockam_transport_smoltcp/src/net/device/packet_pool.rs

@@ -0,0 +1,77 @@
+// This is copied verbatim(Some stuff we don't need removed) from [embassy](https://github.com/embassy-rs/embassy/blob/7561fa19348530ce85e2645e0be8801b9b2bbe13/embassy-net/src/packet_pool.rs)
+// One advantage of using atomic_pool instead of heapless it doesn't seem to have the same kind of ABA problem.


Do you think we should consider using atomic_pool to replace heapless in ockam_core::compat where we can?

The main reason to use atomic_pool was because embassy was doing that.

I'm not sure the advantages of heapless's Treiber stack over atomic_pool's method of keeping track of available pool using a bitset.

While the advantage of atomic_pool's method is clear, it is Sync for all architectures, I'm not sure if it's because atomic_pool didn't take the same considerations and edge cases into account or the implementation is sound.

Also, atomic_pool doesn't mention the ABA soundness in x64 problem and by superficially reviewing it I came to the conclusion that it doesn't seem to have that problem since that seems to arise from the Treiber stack using a linked list but I can't confirm it %100.

I will read more on this and get back to you! (Atomics are really complicated 😵‍💫)

etorreborre · 2023-02-21T14:44:40Z

Hi @conectado, do you plan to resume your work at some stage on this PR? Otherwise, since it has derived quite a bit from the current develop I propose to close it and hopefully someone else will be able to use it to restart the work at some stage.

etorreborre · 2023-02-22T15:38:24Z

Since this branch has diverged we are going to close this PR for now. If anyone wants to resume the work on the smoltcp transport, the branch is available in the ockam repository at conectado/tcp-transport-using-smoltcp.

mrinalwadhwa · 2023-02-22T15:50:59Z

@conectado Thank you for all the time you put into this 🙏
Hopefully someone else will come along and take your great work further from this branch
https://github.com/build-trust/ockam/tree/conectado/tcp-transport-using-smoltcp

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch from 757e886 to a717755 Compare November 19, 2021 21:13

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch 2 times, most recently from 80d574a to 123f0a9 Compare December 14, 2021 08:38

conectado mentioned this pull request Dec 14, 2021

fix(rust): fix no_std compilation due to ockam::node attribute #2355

Closed

4 tasks

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch 2 times, most recently from d41504c to a7afe28 Compare December 17, 2021 08:11

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch from d35610c to b449ea2 Compare December 29, 2021 08:58

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch 2 times, most recently from c6b1cd9 to f2d28c6 Compare January 16, 2022 05:16

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch from 1942f29 to 6e99517 Compare January 23, 2022 21:17

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch 2 times, most recently from b36a3ec to a7804f1 Compare February 12, 2022 18:42

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch 3 times, most recently from f36d0f0 to 6d17559 Compare February 20, 2022 01:56

conectado marked this pull request as ready for review February 20, 2022 03:02

conectado requested a review from mrinalwadhwa as a code owner February 20, 2022 03:02

conectado changed the title ~~(WIP) Add smoltcp TCP transport~~ Add smoltcp TCP transport Feb 20, 2022

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch from 7948d14 to 8517611 Compare February 20, 2022 23:42

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch from 8517611 to 4d51a34 Compare February 20, 2022 23:50

conectado force-pushed the conectado/tcp-transport-using-smoltcp branch from 4d51a34 to 4f089cd Compare February 21, 2022 00:05

mrinalwadhwa requested review from antoinevg, thomcc, SanjoDeundiak and spacekookie February 21, 2022 04:39

mrinalwadhwa assigned thomcc Feb 21, 2022

mrinalwadhwa reviewed Feb 21, 2022

View reviewed changes

chore(rust): fix cargo.lock downgrades

9cd88d2

spacekookie reviewed Feb 22, 2022

View reviewed changes

feat(rust): separate smoltcp and tcp cluster names

87cb060

thomcc mentioned this pull request Feb 22, 2022

Create a checklist of things to verify when new crates are introduced. #2526

Closed

antoinevg suggested changes Feb 23, 2022

View reviewed changes

adrianbenavides mentioned this pull request Mar 24, 2022

Revisit ockam_transport_websocket crate #2588

Closed

19 tasks

etorreborre closed this Feb 22, 2023

		let tcp = SmolTcpTransport::<ThreadLocalPortProvider>::create(&ctx, configuration, Some(StdClock))
		.await?;

		@@ -0,0 +1,222 @@
		//! Traits based in the `futures::io` crate to support `no_std` compilation.

		@@ -0,0 +1,77 @@
		// This is copied verbatim(Some stuff we don't need removed) from [embassy](https://github.com/embassy-rs/embassy/blob/7561fa19348530ce85e2645e0be8801b9b2bbe13/embassy-net/src/packet_pool.rs)
		// One advantage of using atomic_pool instead of heapless it doesn't seem to have the same kind of ABA problem.

Add smoltcp TCP transport #2213

Add smoltcp TCP transport #2213

Conversation

conectado commented Nov 16, 2021 • edited Loading

Implementing feature discussed in #1804

Description

Implementation notes

Test the PR

Next steps

Checks

thomcc commented Nov 17, 2021

mrinalwadhwa commented Dec 6, 2021

conectado commented Dec 6, 2021

conectado commented Dec 13, 2021

thomcc commented Dec 13, 2021

conectado commented Dec 13, 2021

thomcc commented Dec 13, 2021

antoinevg commented Dec 13, 2021

conectado commented Dec 13, 2021

antoinevg commented Dec 13, 2021

conectado commented Dec 13, 2021

mrinalwadhwa commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacekookie left a 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

Choose a reason for hiding this comment

thomcc commented Feb 22, 2022

antoinevg left a 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

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

etorreborre commented Feb 21, 2023

etorreborre commented Feb 22, 2023

mrinalwadhwa commented Feb 22, 2023

conectado commented Nov 16, 2021 •

edited

Loading