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

Async support without blocking #90

Open
tarfu opened this issue Oct 11, 2023 · 9 comments
Open

Async support without blocking #90

tarfu opened this issue Oct 11, 2023 · 9 comments

Comments

@tarfu
Copy link
Contributor

tarfu commented Oct 11, 2023

Right now, the Ingress supports async but the sending, spinning and data service related functions have blocking operations which makes it impossible to use in an environment like embassy (with a single executor).

Copying the client and getting everything to async is not that bad, but copies a lot of code in the easy to do implementation.
The hard part is the data service and socket related stuff.
embedded-nal-async has quite a different API. You only implement a connect function which returns a socket which you can write to and read from via the embeeded-io-async traits, and dropping the socket would need to cause a disconnect.

@MathiasKoch
Copy link
Member

Hi @tarfu,

I am currently on vacation, and just checking in once in a while from the pool 😄
I will be back on Saturday, so I can give you a proper review and reply then.

Long story short is that we are currently working on an async first rewrite of this library along with the ublox wifi crate, that is much more along the same implementation strategy as embassy-net, and fits well with the embedded-nal-async way of doing things.

@tarfu
Copy link
Contributor Author

tarfu commented Oct 12, 2023

Hi,

that sounds great and a nice vacation to you.
Keep in touch with the reimplementation of this library! Very interested in helping.

@MathiasKoch
Copy link
Member

Thank you!
That sounds great! Collaborators are highly welcomed! I will make an update post as soon as i get home, so we can find a consensus on direction 👍

@MathiasKoch
Copy link
Member

MathiasKoch commented Oct 14, 2023

Hi @tarfu

As I said, we are currently experimenting with different architectures and designs on how we want to implement async-first implementations for this crate as well as the Wi-Fi cousin crate at ublox-short-range-rs.

So far we have reached this https://github.com/BlackbirdHQ/ublox-short-range-rs/tree/async/no-channel/src/asynch as our most promising approach.
It is highly inspired by the structure of the various embassy-net crates, and it should be possible to copy most of the skeleton for it to the cellular version.

The two examples at https://github.com/BlackbirdHQ/ublox-short-range-rs/tree/async/no-channel/examples/rpi-pico/src/bin compiles and works, so they should be a nice place to see how it looks in action (embassy based)

TLDR of the implementation. It splits the crate into 3 parts:

  1. A background runner (Runner), intended to be spawned and run for the full duration, responsible for always keeping connection up and handling URC's from the module
  2. A control handle (Control), that has functions for manipulating the connection state (connect, disconnect, operator select (cellular), AP/STA setup (Wi-Fi), etc.)
  3. A data stack (Device), intended to be used for services, e.g. data service, by passing a reference to it to an IP stack implementation. Currently implemented as UbloxStack, that will use the offloaded TCP/IP stack of the ublox module, but it should be possible to also make a different stack implementation that uses smoltcp/embassy-net or similar through PPP mode.
    Not sure if this Device should also eventually be what facilitates the other services (voice, sms, location, etc.), but as we are not using those it is not my primary concern, though I would like to have thought about it, so it is indeed feasible to implement, if anyone want to.

As for the UbloxStack, all data related implementation lives within this, including handling of data ingress, egress and socket connection state.

Others

  • We have yet to decide if we want to keep a blocking/nb client of the two crates, and if so how it should be structured. We are not going to need, nor be using them ourselves.

  • The TCP implementation in the Wi-Fi version currently doesn't support TLS yet, but my idea would be to add a full copy of TcpSocket, TcpClient etc., called TlsSocket, TlsClient etc. that will use the USECMNG family to configure secure sockets somehow, where a TlsSocket is basically a wrapper around a TcpSocket. This would most likely work such that you configure the instance of TlsClient with a set of security parameters (cert, keys, etc.) and every socket it creates by connect will have that security profile enabled, thus making it work fully with the abstraction of embedded-nal-async. This would mean that if you want TLS sockets for one connection, and TCP sockets for another, you would create both a TlsClient and a TcpClient, and from the consumer of those two, there is no difference between them.

  • The Wi-Fi version is also still missing UDP implementations, both client and listener, as well as TcpListener implementations.

Feedback, reviews and implementation help is HIGHLY appreciated! ❤️ Nothing is set in stone, this is merely our most promising approach based on a few experiments and reading a ton of reference implementations.

We have not yet begun the cellular implementation, as we are still playing around in the Wi-Fi realm (less state and usually easier to debug with wireshark etc.), but we think it should be fairly easy to apply the same approach to cellular, even though it will obviously be more complex.

@tarfu
Copy link
Contributor Author

tarfu commented Oct 14, 2023

I had a quick look at the Wi-Fi crate and I thought about extensibility, as cellular chips have a lot of functionality and tend to be on the more feature rich side.

It would be great to have something that gives you a basic init, and then you maybe want to split stuff like PDP/PDS management and stuff like that to be reusable by multiple managers.
You will mostly need sockets but, for example, if you wanted to use FOTA you would also need the PDP/PDS manager.

This is basically what your point 3 is about.
If you bake everything into the Device, it would become quite a huge thing if every other service needs to rely on it.
So maybe it is worth looking for things that perhaps need the same basics, like PDP/PDS. Then you would create a PDP/PDS manager and the single components could just rely on that. And if you like, you can have a PPP provider, but that then sets you up for also having to deal with the multiplexing hell.
If you create a TLS Client, you would need to pass it a Data stack and a cert manager. If you want to use the build in HTTP client with HTTPS, you would pass a PDP/PSD manager and a cert manager.

A probable issue with this design is RAM usage, and it could be a trait bound nightmare.

My main things I would need are sockets, FOTA, location and time and my main module right now is a sara-r5 (SARA-R510M8S to be exact).

@MathiasKoch
Copy link
Member

MathiasKoch commented Oct 15, 2023

I had a quick look at the Wi-Fi crate and I thought about extensibility, as cellular chips have a lot of functionality and tend to be on the more feature rich side.

I completely agree on the part about it usually being a lot more feature rich, hence my point about why we started with the Wi-Fi crate internally.

I think we have a bit of different terminology going on here. I would not implement service specific features on Device (naming TBD?), but would rather do the same as with the Wi-Fi, such that we could have a collection of service structs wrapping an &Device: UbloxStack (sockets), UbloxMqtt, CellLocate, Voice, FOTA etc.

I think it would make sense to have each of these handle their own relevant URC's, such that the crate would need to be able to have N + 1 URC subscribers, where N is the number of services in use (configured by a const generic), and plus 1 because the Runner needs a subscription to handle network registration URC's (In the Wi-Fi crate, the URC subscription currently lies in Device, but could easily be moved to UbloxStack)

Furthermore, I think the PDP/PSD is common for pretty much all the services, and should be configured either through the Control handle, or directly on Device.

A probable issue with this design is RAM usage, and it could be a trait bound nightmare.

I don't see why we would introduce traits into much of this?
I also don't see why the RAM usage would be high?

And if you like, you can have a PPP provider, but that then sets you up for also having to deal with the multiplexing hell.

Yeah, that's a completely different story, and one I think will be mainly managed by modifications to ATAT (BlackbirdHQ/atat#76)

Example

#[embassy_executor::task]
async fn wifi_task(runner: Runner<'static, AtClient, Output<'static, PIN_26>, 8>) -> ! {
    runner.run().await
}

#[embassy_executor::task]
async fn net_task(stack: &'static UbloxStack<AtClient>) -> ! {
    stack.run().await
}

#[embassy_executor::task]
async fn ingress_task(
    mut ingress: atat::Ingress<'static, AtDigester, EdmEvent, RX_BUF_LEN, URC_CAPACITY, 2>,
    mut rx: uart::BufferedUartRx<'static, UART1>,
) -> ! {
    ingress.read_from(&mut rx).await
}

bind_interrupts!(struct Irqs {
    UART1_IRQ => BufferedInterruptHandler<UART1>;
});

#[embassy_executor::main]
async fn main(spawner: Spawner) {
    defmt::info!("Hello World!");

    let p = embassy_rp::init(Default::default());

    let rst = Output::new(p.PIN_26, Level::High);

    let (tx_pin, rx_pin, rts_pin, cts_pin, uart) =
        (p.PIN_24, p.PIN_25, p.PIN_23, p.PIN_22, p.UART1);

    let tx_buf = &mut make_static!([0u8; 64])[..];
    let rx_buf = &mut make_static!([0u8; 64])[..];
    let uart = uart::BufferedUart::new_with_rtscts(
        uart,
        Irqs,
        tx_pin,
        rx_pin,
        rts_pin,
        cts_pin,
        tx_buf,
        rx_buf,
        uart::Config::default(),
    );
    let (rx, tx) = uart.split();

    let buffers = &*make_static!(atat::Buffers::new());
    let (ingress, client) = buffers.split(tx, AtDigester::default(), atat::Config::new());
    defmt::unwrap!(spawner.spawn(ingress_task(ingress, rx)));

    let state = make_static!(State::new(client));
    let (device, mut control, runner) = new(state, &buffers.urc_channel, rst).await;

    defmt::unwrap!(spawner.spawn(wifi_task(runner)));
    
    // Configure PDP/PDS stuff on either `Device` or `Control`, not sure which makes most sense? 
    
    

    // Init network stack with room for 4 sockets
    let stack = &*make_static!(UbloxStack::new(
        device,
        make_static!(StackResources::<4>::new()),
    ));

    defmt::unwrap!(spawner.spawn(net_task(stack)));


    // Not sure how fota is normally used, does it need to be static and have a background runner, or is it a temp thing?
    let fota = &*make_static!(FOTA::new(
        device
    ));
    

    // And now we can use it!
    defmt::info!("Device initialized!");
    
    let addr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(10, 42, 0, 1), 8000));
    
    // `client` here implements `embedded-nal-async`, and issues `TcpSocket` on calls to `connect()`
    let state: TcpClientState<1, 1024, 1024> = TcpClientState::new();
    let client = TcpClient::new(&stack, &state);
        
    info!("connecting...");
    // `socket` implements `embeded-io-async`
    let socket = client.connect(addr).await;

    // Example code. Might look different IRL
    let tls_config = tls_config::new().cert(b"some device cert").root_ca(b"some root CA").private_key(b"Some private key").build();
    let tls_state: TlsClientState<1, 1024, 1024> = TlsClientState::new();
    // `tls_client` here implements `embedded-nal-async`, and issues `TlsSocket` on calls to `connect()`
    let tls_client = TlsClient::new(&stack, &tls_state, tls_config);

    info!("connecting...");
    // `tls_socket` implements `embeded-io-async`
    let socket = tls_client.connect(addr).await.unwrap();
    
    // It is also possible to go without the `embedded-nal-async` abstractions, and create sockets directly like:
    let mut rx = [0u8; 128];
    let mut tx = [0u8; 128];
    // `socket` implements `embeded-io-async`
    let socket = TcpSocket::new(stack, &mut rx, &mut tx);
    socket.connect(addr).await.unwrap();
    
    let mut tls_rx = [0u8; 128];
    let mut tls_tx = [0u8; 128];
    // `tls_socket` implements `embeded-io-async`. 
    let tls_socket = TlsSocket::new(stack, &mut tls_rx, &mut tls_tx);
    tls_socket.connect(addr, tls_config).await.unwrap();

    loop {
        
    }
}

@tarfu
Copy link
Contributor Author

tarfu commented Oct 16, 2023

I think we have a bit of different terminology going on here. I would not implement service specific features on Device (naming TBD?), but would rather do the same as with the Wi-Fi, such that we could have a collection of service structs wrapping an &Device: UbloxStack (sockets), UbloxMqtt, CellLocate, Voice, FOTA etc.

Yes, we had some different terminology going on there.
We are more on the same page as I first thought after looking at it again :)

On the development side, would you want to directly put it in master or dev on an async branch and I PR against this branch?

@MathiasKoch
Copy link
Member

On the development side, would you want to directly put it in master or dev on an async branch and I PR against this branch?

Think I would prefer a branch. Would you like to establish the initial structure, then we can PR to it from there, maybe with a small comment somewhere on what we are working on, so we don't end up with duplicate work 😅

@tarfu
Copy link
Contributor Author

tarfu commented Oct 16, 2023

If we take a branch, it would be a good idea that you create the branch and maybe the structure, so we can PR against that branch here in the same repo and target that branch for the PRs and base the PRs from all within this repo.

I at least haven't seen any branch we could use for that or was already used for something like that here.

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

No branches or pull requests

2 participants