TcpClient trait for creating shared TCP/IP stack

[lulf]

New set of traits using embedded-io that allows creating shared TCP stack implementations using async.

[lulf]

fyi @dirbaio @ivmarkov

[MathiasKoch]

As a follow-up to my comment on ConnectOpts, i would really love it if we were able to come up with an actual TLS config abstraction.. Something along the lines of https://github.com/sfackler/rust-native-tls/ should be possible? Actual structs for configuring TLS options, owned by embedded-nal such that it can be generalized.

Along the lines of this old attempt, that we never got any further: https://github.com/BlackbirdHQ/embedded-nal/blob/feature/tls-options/src/tls.rs

[lulf]

Yeah, for TLS (which I assume is out of scope for this PR), another example is https://github.com/drogue-iot/embedded-tls/blob/main/src/config.rs#L68 where you have additional stuff like an RNG needing to be passed through for the TLS open phase.

[lulf]

Example implementations:

Smoltcp: https://github.com/drogue-iot/drogue-device/blob/main/device/src/drivers/tcp/smoltcp.rs Std: https://github.com/drogue-iot/drogue-device/blob/main/device/src/drivers/tcp/std.rs Esp8266: https://github.com/drogue-iot/drogue-device/blob/main/device/src/drivers/wifi/esp8266/mod.rs#L580 EsWifi: https://github.com/drogue-iot/drogue-device/blob/main/device/src/drivers/wifi/eswifi/mod.rs#L723

[ivmarkov]

I'll try to contribute a STD implementation based on the minimalistic smol async_io reactor. The existing STD example does not depend on any reactor impl, but is obviously not really async.

smol does work on top of ESP-IDF's STD implementation and is thus a good fit for production use on the ESP32 chip (non-bare-metal, with ESP-IDF). For bare-metal we are using smoltcp, so that's covered already.

[MathiasKoch]

Seems like this is okay to me, but my knowledge on Async is too limited to review this properly, perhaps someone else could pitch in? :)

@Dirbaio perhaps?

[lulf]

@ivmarkov Would you mind having a look if these traits look OK to you?

[ivmarkov]

@ivmarkov Would you mind having a look if these traits look OK to you?

@lulf Np, but give me time till the end of the week. Also: the links to the sample implementatios from above return 404 errors. Would you mind updating these?

[lulf]

@ivmarkov Much appreciated, thanks! I've updated the links.

[ivmarkov]

@lulf:

I've implemented the TcpClient trait from this PR for STD with Smol's async-io (that's the async executor that does work on top of ESP-IDF's STD support). The implementation is as simple as it gets, so that's a good sign. You can check out the (extremely simplistic example currently) by cloning the above repo and then cargo run --example simple.

Next steps: I'll layer your async HTTP & MQTT clients on top of TcpClient and its Smol-based implementation from above, possibly implementing these and these traits from embedded-svc, thus creating two additional examples.

If all goes well, I guess we could call it a day and accept the PR!

P.S. I have a small caveat with embedded-nal not having a (non-default) "std" feature that enables the corresponding "std" feature from no-std-nal, thus causing me to use this ugliness, but that's orthogonal to this PR and we can follow up on that later.

[ivmarkov]

@MathiasKoch @lulf - Latest design looks good! +1 for merging it.

[Dirbaio]

I think the new design is not better than the previous one.

1. It is now possible to do reads/writes on a TcpClientSocket before calling connect(), this was enforced impossible at compile-time before.
2. the "statefulness" makes impls uglier. For example, take an impl on std. Now the TcpClientSocket needs an Option<TcpStream>, and read/write needs to unwrap or error-check that enum.
3. The TcpConnection wrapper struct means impls can no longer expose extra API on a connected TCP socket. All you have is Read/Write. Before, impls could have extra API surface on the TcpConnection associated type (either inherent methods, or more trait impls).
4. There's now two ways of doing things. Directly using the TcpClientSocket, or using TcpConnection.

It's true the lifetime problem is an issue though... (having a client that auto-reconnects if the connection). The new design doesn't fully solve that though, you still have the problem with TcpConnection, to do it you have to fall back to using the raw TcpClientSocket.

[ivmarkov]

Any brilliant ideas how to preserve the statefulness of the current design (TcpClientSocket) while preserving the rigor of the previous one?

If that's not clear, the statefulness of the design is its major selling point IMO. If I don't have it, I can just as well fall down to only relying on Read+Write, as having the ability to connect without the ability to cache and reconnect is not worth a trait.

[ivmarkov]

Also I thought it is obvious but let me state this explicitly: the TcpConnection thing in the new design is supposed to be used in the rare case where you don't need caching. Otherwise the raw TcpClientSocket would've not been there in the first place.

[lulf]

I think the latest trait maps more natural to the no_std drivers, whether it's smoltcp, esp8266 or eswifi. (Smoltcp example here @Dirbaio https://github.com/lulf/embassy/blob/embassy-net-async-nal/embassy-net/src/tcp.rs#L297) . Yes, std, tokio etc. requires the Option, but I think we shouldn't optimize for those.

I agree it is unfortunate that the API can be "misused", but I also think it's easier to use for higher level clients that needs to 'reconnect' without needing the Option at that level.

The TcpConnection type was an attempt at a compromise where you can still get the 'disconnect on drop' semantics if you want that, and I think this is OK.

[Dirbaio]

Now that you mention it embassy-net's API has the same "misuse" issue. Oops. My intention was embassy-net sockets are throw-away, they aren't supposed to be reused like that. I should fix that! D:

Also, the fact that smoltcp sockets are reusable is an artifact on how it manages memory. I don't consider that a good feature. Attempting to "preserve" that in the traits is leaking implementation details.

I'm using a TcpConnect trait similar to the previous design in my company's firmware to abstract between embassy-net and simcom modems. It looks like this:

pub trait TcpConnect {
    type Conn<'a>: Read + Write + Split
    where
        Self: 'a;

    type ConnectFuture<'a>: Future<Output = Result<Self::Conn<'a>, TcpConnectError>>
    where
        Self: 'a;

    fn connect<'a>(&'a mut self, addr: Address<'a>, port: u16) -> Self::ConnectFuture<'a>;
}

// impl for embassy-net 

pub struct TcpClient {
    stack: &'static Stack<NetDevice>,
    rx_buffer: [u8; BUFFER_SIZE],
    tx_buffer: [u8; BUFFER_SIZE],
}

impl TcpConnect for TcpClient {
    type Conn<'a> = TcpSocket<'a>
    where
        Self: 'a;

    type ConnectFuture<'a> = impl Future<Output = Result<Self::Conn<'a>, TcpConnectError>>
    where
        Self: 'a;

    fn connect<'a>(&'a mut self, addr: Address<'a>, port: u16) -> Self::ConnectFuture<'a> {
        async move {
            let ip = ..;

            let mut socket = embassy_net::tcp::TcpSocket::new(&self.stack, &mut self.rx_buffer, &mut self.tx_buffer);
            info!("connecting...");
            let r = socket.connect((ip, port)).await;
            if let Err(e) = r {
                info!("connect error: {:?}", e);
                return Err(TcpConnectError::ConnectionFailed);
            }
            info!("connected!");
            Ok(TcpSocket { inner: socket })
        }
    }
}

The idea is the TcpConnect trait (TcpClient in this PR's previous design) is not a socket itself, it's just the "necessary resources to create one". The socket itself is not reusable.

About caching long-lived connections like MQTT: the previous design works well with a dedicated task. Very rough pseudocode of what I'm doing (it's not MQTT, but it's also a long-lived connection that can do requests in both directions: device->cloud or cloud->device)

async fn net_task(mut client: TcpClient) {
   loop {
      match client.connect(addr, port).await {
          Ok(conn) => match handle_conn(conn).await {
             Ok(_) => unreachable!(); // only returns on fail
             Err(e) => warn!("Handle conn failed: {:?}, reconnecting", e),
          },
          Err(e) => warn!("connect failed: {:?}, retrying", e).
      }
      // some sleep to backoff maybe
   } 
}
async fn handle_conn(conn: TcpSocket<'_>) -> Result<(), Error> {
   // read/write to the conn. Return if conn breaks.
   // communicate with other tasks with channels.
}

Having a dedicated task has more advantages, besides solving the "lifetime issue".

• Say you're sending a request every 1 minute. If the conn breaks, the dedicated task will notice and reestablish it right away, while with the "store the conn in a struct", it won't notice until you try to send the next request up to 1 minute later, so that request will be much much slower because it has to eat the latency of reconnecting.
• You can send requests from multiple tasks concurrently.
• You can handle pub+sub on the same connection, no need to establish 2. Half the memory usage.

[ivmarkov]

@Dirbaio Your design works well for stuff similar to MQTT, because it is the MQTT client who controls and encapsulates the "loop" and thus having two nested loops is not the end of the world as this is encapsulated inside the MQTT client itself.

How would that work for an HTTP client, which by necessity needs to expose something like a Request (+ Response) to the user, so that the user can drive the request (and the following response). Yet, the (potentially TLS) socket between multiple requests and their responses should be preserved, cached and kept open. You don't own the "loop" so to say, it is the user logic which spins the multiple requests and it is the user who controls the loop, so you can't hide the nested loop ugliness from the user, as well as the error propagation from the inner loop which just spins off a new iteration over the outer loop, which does a new connect. This is - with the previous design - plain and simple shifted back to the shoulders of the user, which - for all practical purposes - makes the TcpClient/TcpConnection abstraction useless for that particular case IMO.

This if you wish.

Don't get me wrong - if I've missed the elephant in the room - please point it to me. I would gladly take the previous design then. As it has obvious other advantages.

[Dirbaio]

Ah, the http case... :(

What if connect takes &self instead of &mut self?

Then an HTTP client could look like this. The borrow checker will be happy because it's not self-referential.

struct HttpClient<'a, T: TcpClient> {
   client: &'a T,
   conn: Option<T::Conn<'a>>,
}

&self has the implication that it's now possible to try to create multiple conns with the same TcpClient. Perhaps we should simply accept it? On std/alloc it's fine. On no-alloc the TcpClient would have a "buffer pool", and connect() would return a OutOfMemory error if all buffers in the pool are in use...

[ivmarkov]

Yes that would likely work and it is a variation of the design I proposed initially here, except yours is with a bit different lifetimes which are more likely to work. Sorry - all the code in my original comment is now gone except in github history.

These proposals turn TcpClient into essentially a synchronized sockets' factory. The Stack from embassy-net, if you wish.

But as you correctly say, we'll now have an issue with tx/rx buffer management becoming more complicated (pools and such),

I am not sure what is the best path forward. The original TcpClient design is very clean but restricted for sure. Amongst the current TcpClientStack and your latest suggestion which is my older one I'm not sure, honestly. @lulf ?

[lulf]

I understand more the non-mut connect option now, but would like to experiment a bit with some of the drivers to get a feel for it, so let's hold off merging anything for now.

[MathiasKoch]

Would it be possible, while making sense, to add a similar API to the blocking side, to maintain a somewhat 1:1 capability?

[ivmarkov]

Would it be possible, while making sense, to add a similar API to the blocking side, to maintain a somewhat 1:1 capability?

I suggest we wait until @lulf returns and then continue on the async API. Once we have this one, we can probably think about the non-async ones.

By the way, by blocking" I assume you mean the old "nb" one? Because - technically speaking - it does not block either. For me it is also a question of whether we should even look for further developing the "nb" ones. "nb" is a weird spot to be - neither a true blocking one, nor a true async one.... and difficult to compose.

BTW isn't embedded-hal removing anything "nb" related?

[MathiasKoch]

I absolutely agree we should wait until you reach a concensus on the current design :+1:

By the way, by blocking" I assume you mean the old "nb" one?

Yeah, that was what i meant, though you are correct. I think we will just tag-along on the way embedded-hal shapes up?

The one thing i would like to see on this abstraction is a more rust like feeling along #53

[ivmarkov]

I absolutely agree we should wait until you reach a concensus on the current design 👍

By the way, by blocking" I assume you mean the old "nb" one?

Yeah, that was what i meant, though you are correct. I think we will just tag-along on the way embedded-hal shapes up?

Yep, IMO that's what we should do.

The one thing i would like to see on this abstraction is a more rust like feeling along #53

Heh. The latest design for the client socket is mostly what is suggested in #53 anyway, and it is still a bit controversial. I.e. see the comments from @Dirbaio earlier in the thread. Compare: Current design:

pub trait TcpClientSocket:
    embedded_io::Io + embedded_io::asynch::Read + embedded_io::asynch::Write
{
    type ConnectFuture<'m>: Future<Output = Result<(), Self::Error>> + 'm
    where
        Self: 'm;

    fn connect<'m>(&'m mut self, remote: SocketAddr) -> Self::ConnectFuture<'m>;

    type IsConnectedFuture<'m>: Future<Output = Result<bool, Self::Error>> + 'm
    where
        Self: 'm;

    fn is_connected<'m>(&'m mut self) -> Self::IsConnectedFuture<'m>;

    fn disconnect(&mut self) -> Result<(), Self::Error>;
}

(read() / write() not listed above ^^^ as they come from the asynch::Read and asynch::Write traits.)

Proposal from #53:

pub trait TcpSocket {
    type Error;

    fn connect(&mut self, remote: SocketAddr) -> Result<(), Error>;
    fn send(&mut self, data: &[u8]) -> Result<usize, Error>;
    fn receive(&mut self, data: &mut [u8]) -> Result<usize, Error>;
    fn close(self);
}

Modulo blocking and modulo naming differences (close vs disconnect, send vs write and receive vs read) the two designs are the same thing, no?

[MathiasKoch]

Modulo blocking and modulo naming differences (close vs disconnect, send vs write and receive vs read) the two designs are the same thing, no?

Yup, very much so. :+1: Which is why i am a bit excited about it!

[ivmarkov]

Modulo blocking and modulo naming differences (close vs disconnect, send vs write and receive vs read) the two designs are the same thing, no?

Yup, very much so. 👍 Which is why i am a bit excited about it!

Unfortunately, the same design cannot be used for server sockets, (as in accept + bind).

I.e. see here for my current thinking how a server tcp socket might look like.

[ivmarkov]

Hmmm now that I read it further, I don't think #53 has ended up with a conclusion on the future design, isn't it? There's also this one discussed further in that other thread:

trait AsyncUdpClient {
    type Socket: AsyncWrite + AsyncRead; // Dropping a socket closes it.
    type ConnectFuture<'a>: Future<Output = Result<Self::Socket, Self::Error>> + 'a;
    type Error;

    fn connect<'a>(&'a mut self, remote: SocketAddr) -> Self::ConnectFuture<'a>;
}

... which is essentially what @lulf originally suggested here and which has its own hurdles (as in it can only return ONE active connection; there is no easy way to implement "reconnect" functionality, that you need for e.g. HTTP/1.1 clients).

I think whatever we can come up with here, can be used for blocking as well (modulo removing the GAT types), but what that would be is still open.

[lulf]

@ivmarkov @Dirbaio I've given the non-mut trait a try. Pushed the changes here, and with an example implementation here: https://github.com/lulf/embassy/blob/embassy-net-async-nal/embassy-net/src/tcp.rs#L316-L479

As expected, it's a bit more complicated, but doable. I think the main annoyances from implementing is:

• Need some internal atomic buffer pool management (copied from atomic-pool in the above embassy-net impl)
• Ends up with lots of generics for the impl types, though I guess I could try to mitigate that by passing an array of mut slices instead of the &Pool, I'll try that option as well. Alternatively define a trait for the Pool and use dynamic dispatch to simplify the TcpConnection type signature.
• To free the buffers, I've had to wrap the embassy-net TcpSocket in another type, which is only slightly annoying.

From a API usability perspective I think it's a lot easier to work with in higher layer clients. Conceptually it feels more natural now that a client can create multiple connections, which was one of the confusions initially. I'm fairly confident this works nicely for es-wifi and esp8266 at command drivers as well.

[ivmarkov]

From a API usability perspective I think it's a lot easier to work with in higher layer clients. Conceptually it feels more natural now that a client can create multiple connections, which was one of the confusions initially. I'm fairly confident this works nicely for es-wifi and esp8266 at command drivers as well.

Depends on your higher layer client:

• If it is an MQTT client (which spins the socket connect/reconnect loop and the handling of the connection completely internally to the user) it makes no difference compared to the previous design.
• If it is a stateful HTTP client, that needs to reconnect if it had received a Connection: Close from the server, or because the server had messed up the response (due to - say - a 500 error in itself), this API is harder to consume compared to the "Python-like" API which had connect and disconnect directly on the Read + Write connection. This is due to the extra-lifetime 'm that the TcpConnection GAT has, which means you cannot store a TcpConnection instance as well as its parent TcpConnector in a single struct without some sort of unsafe. In other words, this is difficult to implement now.

But with that said, sure - the latest implementation is somewhat more intuitive compared to all previous, and it has a nice symmetry with potential future server socket (accept/bind) traits so let me give it a try...

[Dirbaio]

you cannot store a TcpConnection instance as well as its parent TcpConnector in a single struct without some sort of unsafe

You can do it, as long as you borrow the TcpConnector: store &'w TcpConnector and TcpConnection<'w>

[ivmarkov]

you cannot store a TcpConnection instance as well as its parent TcpConnector in a single struct without some sort of unsafe

You can do it, as long as you borrow the TcpConnector: store &'w TcpConnector and TcpConnection<'w>

Using borrowed TcpConnector typechecks just fine, thank you!

Final set of comments. With the general approach I'm now completely fine!

• TcpConnector is very generic as a name. Do we want to name it TcpClientSocket or TcpClientConnector?
• The GAT is named TcpConnector::TcpConnection. Tcp repeats itself. Perhaps just TcpConnector::Connection?
• I think the connection GAT should use the error type of the TcpConnector, as in:

  type Connection<'m>: embedded_io::asynch::Read<Error = Self::Error>
      + embedded_io::asynch::Write<Error = Self::Error>
  where
      Self: 'm;

[ivmarkov]

Oh and one more: Do we want

impl<T> TcpConnector for &T
where
    T: TcpConnector
...

@Dirbaio wuld appreciate feedback here ^^^. By the way, the above is currently unimplementable, because Io currently is only implemented for &mut T where T: Io but not for &T where T: Io

[lulf]

• TcpConnector is very generic as a name. Do we want to name it TcpClientSocket or TcpClientConnector?

I disagree with that. I've seen this term used in similar constructs, and I find TcpConnector a sufficiently short term that I think fits well with it's purpose. In the same way I think that Acceptor or TcpAcceptor is a good name for a server side version that accepts connections.

• The GAT is named TcpConnector::TcpConnection. Tcp repeats itself. Perhaps just TcpConnector::Connection?

Agreed, I'll rename it Connection

• I think the connection GAT should use the error type of the TcpConnector, as in:

  type Connection<'m>: embedded_io::asynch::Read<Error = Self::Error>
     + embedded_io::asynch::Write<Error = Self::Error>
  where
     Self: 'm;

It does simplify things, so yes lets do that.

[lulf]

I think the connection GAT should use the error type of the TcpConnector, as in:

Actually, perhaps not? In my smoltcp implementation, I have an error type that can be 'OutOfMemory' when no more connections can be created. It feels wrong to require that connection read/write errors should be the same as the connect errors.

[ivmarkov]

• TcpConnector is very generic as a name. Do we want to name it TcpClientSocket or TcpClientConnector?

I disagree with that. I've seen this term used in similar constructs, and I find TcpConnector a sufficiently short term that I think fits well with it's purpose. In the same way I think that Acceptor or TcpAcceptor is a good name for a server side version that accepts connections.

OK, that would also work, as long as the server "socket" that does bind is named - say - TcpBinder. So:

• TcpConnector - client side, hm, factory returning a connection
• TcpBinder - server side factory that has the bind method fn bind(&self, remote: SocketAddr) -> Self::BindFuture<'_>; and returns a TcpAcceptor instance
• TcpAcceptor - the server side factory that returns connections just like client-side TcpConnector

[ivmarkov]

I think the connection GAT should use the error type of the TcpConnector, as in:

Actually, perhaps not? In my smoltcp implementation, I have an error type that can be 'OutOfMemory' when no more connections can be created. It feels wrong to require that connection read/write errors should be the same as the connect errors.

Let me try to adjust my HTTP client if TcpConnector returns a different error from the Connection GAT. But so far it does not look pretty. Will follow up tonight.

[MathiasKoch]

I think usually TcpBinder would be what is called TcpListener if i am not mistaken? eg in std::net

[Dirbaio]

Io currently is only implemented for &mut T where T: Io but not for &T where T: Io

We can add that. There's only the &mut blanket impl because that's what Read/Write needed.

That being said, do we want TcpConnector: embedded_io::Io? It means if you impl TcpConnector and TcpListener on the same struct, they'll be forced to use the same error type. I think the possible errors for them are quite different, so this might be a bad thing.

---

about naming:

• the convention in Rust is to use the method name without -er / -or suffix, for example Write instead of Writer. Therefore I propose TcpConnect :)
• bind in the Berkeley socket API does lots of things (for example it can be used to set the source addr/port of an outgoing (client) TCP connection), so I don't think TcpBinder is a good name. TcpListen, TcpAccept maybe?
• What was wrong with TcpClient, TcpServer? I think these were quite clear too.

[lulf]

the convention in Rust is to use the method name without -er / -or suffix, for example Write instead of Writer. Therefore I propose TcpConnect :)

I'd prefer TcpConnect in that case - it is more descriptive to the trait and what it does than TcpClient IMHO.

That being said, do we want TcpConnector: embedded_io::Io? It means if you impl TcpConnector and TcpListener on the same struct, they'll be forced to use the same error type. I think the possible errors for them are quite different, so this might be a bad thing.

Agreed, I've removed relying on the embedded_io::Io trait.

[ivmarkov]

I think the connection GAT should use the error type of the TcpConnector, as in:

Actually, perhaps not? In my smoltcp implementation, I have an error type that can be 'OutOfMemory' when no more connections can be created. It feels wrong to require that connection read/write errors should be the same as the connect errors.

Let me try to adjust my HTTP client if TcpConnector returns a different error from the Connection GAT. But so far it does not look pretty. Will follow up tonight.

Haven't finished and will continue tonight, but I'm not thrilled by the complexity introduced as a result from TcpConnect's error type being different from the error type of TcpConnect::Connection:

For example, if I want to wrap both in an enum error, here's what I got:

#[derive(Debug)]
pub enum IoError<C, E>
{
    Connect(C),
    Io(E),
}

impl<C, E> Error for IoError<C, E>
where
    C: Error,
    E: Error,
{
    fn kind(&self) -> embedded_io::ErrorKind {
        match self {
            Self::Connect(e) => e.kind(),
            Self::Io(e) => e.kind(),
        }
    }
}

So far so good, but then the signatures of the Http Client methods become unreadable. E.g.

Old:

    pub async fn initiate_response(&mut self) -> Result<(), HttpError<T::Error>> {

New:

    pub async fn initiate_response(&mut self) -> Result<(), HttpError<IoError<T::Error, <<T as TcpConnector>::Connection<'b> as Io>::Error>>> {

So in the new signature you have it all: lifetimes, trait casts (multiple of them). Joy!

What am I missing? And if nothing, is the purity of differentiating between the error of TcpConnect and the error of TcpConnect::Connection<'m> worth it?

[lulf]

What am I missing? And if nothing, is the purity of differentiating between the error of TcpConnect and the error of TcpConnect::Connection<'m> worth it?

I agree its a mouthful, but on the other hand I think you can enforce the constraint in your http client:

#[derive(Debug)]
pub enum IoError<E> {
    Connect(E),
    Io(E),
}

async fn do_connect_and_write<
    'm,
    E,
    C: Io<Error = E> + Write + Read,
    T: TcpConnect<Error = E, Connection<'m> = C>,
>(
    client: &'m T,
) -> Result<(), IoError<E>> {
    let addr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::localhost(), 1234));
    let mut conn = client
        .connect(addr)
        .await
        .map_err(|e| IoError::Connect(e))?;

    let w = conn
        .write(&[0, 1, 2, 3])
        .await
        .map_err(|e| IoError::Io(e))?;
    Ok(())
}

Yes, it's more typing involved, but this way you enforce the error type to be the same as you want. I'm not sure what's better longer term. You might be able to encode the above in a 'super trait' as well?

The problems of using an error type covering lots of errors that can't happen is that users have to deal with errors that can never happen, and stop caring about handling specific errors, because now they can't rely on the type system to tell them which errors could actually happen.

[ivmarkov]

What am I missing? And if nothing, is the purity of differentiating between the error of TcpConnect and the error of TcpConnect::Connection<'m> worth it?

I agree its a mouthful, but on the other hand I think you can enforce the constraint in your http client:

#[derive(Debug)]
pub enum IoError<E> {
  Connect(E),
  Io(E),
}

async fn do_connect_and_write<
  'm,
  E,
  C: Io<Error = E> + Write + Read,
  T: TcpConnect<Error = E, Connection<'m> = C>,
>(
  client: &'m T,
) -> Result<(), IoError<E>> {
  let addr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::localhost(), 1234));
  let mut conn = client
      .connect(addr)
      .await
      .map_err(|e| IoError::Connect(e))?;

  let w = conn
      .write(&[0, 1, 2, 3])
      .await
      .map_err(|e| IoError::Io(e))?;
  Ok(())
}

Yes, it's more typing involved, but this way you enforce the error type to be the same as you want. I'm not sure what's better longer term. You might be able to encode the above in a 'super trait' as well?

The problems of using an error type covering lots of errors that can't happen is that users have to deal with errors that can never happen, and stop caring about handling specific errors, because now they can't rely on the type system to tell them which errors could actually happen.

>   'm,
>   E,
>   C: Io<Error = E> + Write + Read,
>   T: TcpConnect<Error = E, Connection<'m> = C>,

I think the type constraints you are suggesting will constrain my HTTP client to accept only connectors which happen to implement a single error type for both TcpConnect as well as for its Connection GAT. So my HTTP client will work with STD (because it uses std::io::Error for everything) but will not work with your smoltcp impl that does have separate error types. Which is not what we want - we want to constrain the TCP socket implementors so that clients have an easier time, not the users/clients of the traits?

In general: I agree that the unhappy path should also get a decent level of attention, but the reality is - I think - people tend to be more relaxed about typing in it (errors) than the happy path one and I think might have a lower tolerance for complex types in the unhappy path. We might be going a bit too far with separate error types for TcpConnect and its GAT. I mean, the GAT is an "associated" type, right?

@Dirbaio any thoughts? Is this really how stuff is modelled in embedded-hal now that it got error types with kind? I thought there one driver shares a single error type? As in e.g. the SPI traits have an SpiError type and that's it, regardless whether the thing is a bus, or a device, or whatever?

[lulf]

I agree, it does become a bit much, so maybe let's do this:

pub trait TcpConnect {
    /// Error type returned on connect failure.
    type Error: core::fmt::Debug;

    /// Type holding state of a TCP connection.
    type Connection<'m>: embedded_io::asynch::Read<Error = Self::Error>
        + embedded_io::asynch::Write<Error = Self::Error>
}

This way, TcpConnect + Connection can use the same error as you proposed, while still having a different set of errors from TcpListen? (Difference from before is that TcpConnect does not depend on Io, which would not enforce TcpListen and TcpConnect to use the same error should they be implemented by the same type)

[ivmarkov]

Maybe let's do this:

pub trait TcpConnect {
  /// Error type returned on connect failure.
  type Error: core::fmt::Debug;

  /// Type holding state of a TCP connection.
  type Connection<'m>: embedded_io::asynch::Read<Error = Self::Error>
      + embedded_io::asynch::Write<Error = Self::Error>
}

This way, TcpConnect + Connection can use the same error, while still having a different set of errors from TcpListen? (Difference from before is that TcpConnect does not depend on Io, which would not enforce TcpListen and TcpConnect to use the same error should they be implemented by the same type)

I agree.

[lulf]

@ivmarkov @Dirbaio are we in agreement? I'd like to squash + merge + release a new version with these changes soon.

[Dirbaio]

Sounds good to me. I do think having separate error types would be more "pure" but I agree it's not practical (also std doesn't have separate errors, everything's the kitchen sink io::Error, so meh).

embedded_io::Io requires Error impls the Error trait. I don't understand why TcpConnect compiles without it, shouldn't Read<Error = Self::Error> fail to compile? GAT bug? :joy: :thinking:

[ivmarkov]

@lulf I'm fine with the changes. Maybe one last thing - let's have the Error associated type of TcpConnect constrained to embedded_io::Error instead of just to Debug (basically the topic of @Dirbaio from above). I'm also not sure how this thing currently compiles without that constraint, so let's not risk it with a newer nightly that bails on it.

[Dirbaio]

Okay, what happens is the trait declaration compiles, but it's impossible to implement it unless Self::Error: embedded_io::Error. So I think we should require it as well, to avoid possible confusion to implementers.

(I don't think it's a rust bug, behavior is the same without GATs)

[Dirbaio]

What do we do with the previous traits? Remove them?

[lulf]

What do we do with the previous traits? Remove them?

I think we should sooner rather than later, there are no uses of them that I know of that this point. (And it's not a 1.0 yet so lets do it).

Maybe one last thing - let's have the Error associated type of TcpConnect constrained to embedded_io::Error instead of just to Debug (basically the topic of @Dirbaio from above). I'm also not sure how this thing currently compiles without that constraint, so let's not risk it with a newer nightly that bails on it.

Agreed.

I'll get the changes sorted out and squashed tomorrow, thanks!