ethercrab-rs / ethercrab

EtherCAT master written in pure Rust
258 stars 25 forks source link

Why `try_split` can only be called once? #182

Closed Dirreke closed 8 months ago

Dirreke commented 8 months ago

I use ethercrab as a part of my program. And I have to reset the program in some reason. However, for ethercrab, I get an error when call try_split. Then, I tried to make PduRx, PduTx.... be a static variable and I'm failed, either

I'm not familiar with the underlying logic. However, I want to know if we can use once to make sure we only call compare_exchange once and then every time I call try_split, it will return (rx, tx, pduloop). Alternatively, I want to know if we can split the try_split into two parts, one for calling compare_exchange and the other for returning the (rx, tx, pdu_loop). In this way, I can use the former method to get the (rx, tx, pdu_loop) and run the task.

Thanks!

jamwaffles commented 8 months ago

try_split is designed in such a way to uphold the invariant that only one PduTx and PduRx may be held at a time, otherwise there's the potential for race conditions and other UB if more than one of either of these instances is held concurrently.

Are you able to put the part of your program that needs to be reset in a loop? That way you can reset as much as you like on failure but only need to call try_split once.

It would be good if you could describe what you mean by "reset the program" in a bit more detail. Are you encountering NIC issues, program bugs, etc? Does your program crash a thread as opposed to returning a Result::Err(...), etc?

Dirreke commented 8 months ago

Thank you for your prompt reply.

Actually, I have a really heavy industrial software with its own business function and multiple communication protocols, which include Ethercat. Its startup logic is "A. initialize -> B. load config and database -> C. start communication function(and also other business functions) and serve an web". In some situation, such as modifying key configurations, it will be reset and do step B and C again. Which communication protocol to enable depends on configuration and database loaded by step B.

It seems that the only way to solve this issue is add try_split in the step A. However, I have to pass the parameters layer by layer through many unrelated functions. Worse still, the software is designed for various platform(different architecture, different system, different abi) and it makes it very difficult to disable the feature of ethercrab during compilation.

I have tried many method for several weeks. I would greatly appreciate it If you have any other good suggestions.

jamwaffles commented 8 months ago

If this suits your application, you could use thread::scope which allows PduStorage to be non-'static. Here's a demo that uses smol::block_on which executes futures on two threads; one the TX/RX, the other the actual task thread:

use env_logger::Env;
use ethercrab::{
    error::Error,
    std::{tx_rx_task},
    Client, ClientConfig, PduStorage, Timeouts,
};
use futures_lite::StreamExt;
use std::{thread, time::Duration};

/// Maximum number of slaves that can be stored. This must be a power of 2 greater than 1.
const MAX_SLAVES: usize = 16;
/// Maximum PDU data payload size - set this to the max PDI size or higher.
const MAX_PDU_DATA: usize = 1100;
/// Maximum number of EtherCAT frames that can be in flight at any one time.
const MAX_FRAMES: usize = 16;
/// Maximum total PDI length.
const PDI_LEN: usize = 64;

fn main() -> Result<(), Error> {
    env_logger::Builder::from_env(Env::default().default_filter_or("info")).init();

    let interface = std::env::args()
        .nth(1)
        .expect("Provide network interface as first argument.");

    let pdu_storage: PduStorage<MAX_FRAMES, MAX_PDU_DATA> = PduStorage::new();

    let (tx, rx, pdu_loop) = pdu_storage.try_split().expect("can only split once");

    let client = Client::new(pdu_loop, Timeouts::default(), ClientConfig::default());

    thread::scope(|s| {
        // TODO: Handle panics!
        let _tx_rx_task =
            s.spawn(|| smol::block_on(tx_rx_task(&interface, tx, rx).expect("spawn TX/RX task")));

        let app_task = s.spawn(|| {
            smol::block_on(async {
                let mut group = client
                    .init_single_group::<MAX_SLAVES, PDI_LEN>()
                    .await
                    .expect("Init")
                    .into_op(&client)
                    .await
                    .expect("PRE-OP -> OP");

                let mut tick_interval = smol::Timer::interval(Duration::from_millis(5));

                loop {
                    group.tx_rx(&client).await.expect("TX/RX");

                    // Cyclic data sent here
                    for mut slave in group.iter(&client) {
                        let (_i, o) = slave.io_raw_mut();

                        for byte in o.iter_mut() {
                            *byte = byte.wrapping_add(1);
                        }
                    }

                    tick_interval.next().await;
                }
            })
        });

        app_task.join().unwrap()
    })
}

If you don't want to use threads, this should also work on an executor that doesn't require 'static bounds on futures passed to it, for example smol::LocalExecutor.

There might be other options but I'd need to see your code to tell for sure. I'm available for contracting hours if you have appetite for that.

Dirreke commented 8 months ago

Thanks, I will try it. But run_tx_rx still require 'static in windows( windows.rs ). Could I change it to 'sto

jamwaffles commented 8 months ago

Ah, right, sorry about that! I fixed it a while ago for Linux/UNIX but I don't test on Windows any more so that slipped past.

I've opened #183 to fix. I'm going to test it now but if you want to, try changing your Cargo.toml to:

ethercrab = { git = "https://github.com/ethercrab-rs/ethercrab.git", branch = "non-static-windows" }
Dirreke commented 8 months ago

If this suits your application, you could use thread::scope which allows PduStorage to be non-'static. Here's a demo that uses smol::block_on which executes futures on two threads; one the TX/RX, the other the actual task thread:

I don't think this suits my application. My application is using tokio for the whole program.

However, I have a new idea which may make things easier.

Could we add Copy method to PduStorageRef, PduRx, PduTx and PduLoop? Then we can call try_split in lazy_static and use (tx, rx, pdu_loop) in my program.

If it's good, I can write and commit the PR.

Dirreke commented 8 months ago

Thanks, it works.