PRE OP -> SAFE OP hook should be a closure

[jimy-byerley]

Hello @jamwaffles

At the moment, the PRE OP -> SAFE OP hook is a function pointer (fn) so it cannot borrow data specifying the mapping to operate nor can return locally computed mapping offsets to the rest of the program.

Do you think the hook could be a closure (maybe a dyn Fn) instead ?

I thing beeing able to do the following would be great:

let config = {/* data defining the mapping to set */}
let mut offsets = {/* data containing the pdo entries offsets deduced from mapping */}
let groups = SlaveGroup::<MAX_SLAVES, PDI_LEN>::new(|slave| {
    Box::pin(async {
        // use config to write the correct SDOS
        something(config, slave)
        // compute the corresponding offsets
        offsets = something(config, slave)
});
// use offsets to extract entries from the process data

[jamwaffles]

Yeah that makes sense. It's currently a fn to work around some ownership/type weirdness due to some limitations in current Rust. It's also there to make steps toward supporting no_std but that's a moot point considering it has to return a Pin<Box<...>>...

I'll look for a solution when I'm done refactoring some other stuff.

[jimy-byerley]

I wonder if a hook function is the best option, even with a closure. :thinking: You can see here what a pain it is having to pass to the closure all the variables to be set, as Arc<Mutex<_>> and clone it each closure creation ...

// everything to be mutated in a mutex
let profile = ...;
let vars = Arc::new(tokio::sync::Mutex::new(None));

// clone the arc so it can be moved
let profile = profile.clone();
let vars = vars.clone();
SlaveGroup::<MAX_SLAVES, PDI_LEN>::new(Box::new(move |slave| {
        // clone again so it can be moved again as often as the current fn is called
    let profile = profile.clone();
    let vars = vars.clone();
    Box::pin(async move {
            // lock (gh ...) and assign
        vars.lock().await = Some(...)

Maybe splitting the init into several methods to call in a sequence could be better ?

[jamwaffles]

A hook function is required for if/when EtherCrab supports slave "hot plug" as well as autorecovery. The hooks needs to run more than once to cater for these scenarios. It also means that the user can't screw up the init sequence by reordering or leaving out method calls as it's all handled internally by EtherCrab. This init complexity is one of my major beefs with SOEM and is something I wanted to avoid for EtherCrab, even if it means trading away some flexibility.

In the code you linked, it's strange to me that you're writing to vars in the hook fn. Shouldn't the slave state be read from the PDI in OP instead? If you don't need to mutate anything, I'm pretty sure the types could be a lot simpler as long as everything you move into the closure is Copy.

To me, this matches the intended semantics of the init hook; It may optionally take some config data in the parent scope and use that to configure a slave device, but is otherwise pure (i.e. no other side effects) unless the programmer really wants them. At this point, all the smells from using Arcs and Mutexes etc can come along for the ride where the longer types and hoop jumping tells the programmer that this isn't the intended purpose of the init hook.

[jimy-byerley]

The code I linked needs to export values to the main task to schematize the mapping step done in my PR for mappings which is doing approximately the same as the original example. In #48 I introduced offsets to values in the process data, to avoid the boilerplate of manually computing the byte position of each value and writing the code to extract them, where as the former example was all hardcoded. Because of this change, the offsets needed to be computed where the mapping is decided

• option 1 is to decide the mapping in the hook function (it makes the mapping tools simpler), but it means
  • returning the offsets from the hook to the main function
• option 2 is to decide the mapping before the creation of the hook function (and pass a readonly buffer to the hook), but it means
  • buffering all SDO values for the mapping (in a heapless vec ?)
  • iterating on slaves before the group is created (if it is possible ?)
  • passing the buffer of instructions to the hook to be applied (assuming offset's won't change on auto-recovery, what I'm not sure of since the number of slave can change ?)

From that perspective, and the fact that there is no auto-recovery in ethercrab at the moment, I thought the first option was the best. I can understand that the hook function was not meant for this use, however it is a good idea to defer the mapping ?

[jimy-byerley]

if/when EtherCrab supports slave "hot plug" as well as autorecovery

About that particular point, under which circumstances can hot-plug or auto-recovery happen in an ethercat setup ? In my understanding of the ethercat protocol, hot-plug means temporarily openning the loop and stopping all communication to insert a device in. Also as the "logical memory" (which is the process data frame itself) concerns the whole bus and not a set of slave, adding slaves, or recovering a slave that has been reset would mean temporarily stop the transmission of process data for all slaves.

So could hot-plug and auto-recovery simply exist in an ethercat segment ? I've no experience in such practices

[Lomsor]

As far as I know, coming from TwinCAT, there, "HotConnect" has to be preconfigured in the init configuration setup. The logical addresses are reserved. There is the 'standard' bus and at the same time there can be multiple HotConnect groups, and during operation, the HotConnect groups can be removed or added to the bus at any free port. The HotConnect groups have to be identifiable through ID. Either a hardware ID select or a slave configured ID. Most EtherCAT devices support HotConnect, due to electrical reasons it is usually done on the coupler level. Beckhoff also offers a line of "Fast HotConnect" that has faster start up times, but can only work with corresponding "Fast HotConnect" ports.

The rest of the bus stays operational while a HotConnect Group is brought online. Depending on the slave, this can take several seconds. Once ready, the master then switches the communication sequence to include the devices in the HotConnect group. This inclusion happens nearly instantaneously. Similar to the connection process, when a HotConnect group is physically disconnected, the master quickly identifies this change through the working counter check. It adjusts the communication sequence to exclude the disconnected HotConnect group. Again, communication with other devices continues seamlessly.

I don't know if hot-plug or auto-recovery refers to "HotConnect". Also see TwinCAT documentation.

My experience here is specific to TwinCAT as a user and mostly from its documentation. I only tried it a few times in a test setup with normal couplers.

[jamwaffles]

Hey @jimy-byerley. I've been playing with this on and off for the last few weeks and the closest I can get is this playground, which allows an FnMut to be used, however requires any captured variables to be 'static which is unfortunate.

I'm of the opinion that any config should tell the slave device how to configure itself (i.e. read only), so to me being static is ok as I wouldn't want to write to the config to update it as you mention in #40 but I appreciate this attempt still doesn't solve your problem.

There are a few Rust features in the long and slow RFC/release pipeline that might help, but the root of the problem currently is that Rust just really doesn't like async closures.

There might be a way to do this init without closures whilst still guaranteeing correct startup order, which would mean giving up the possibility of hotplug but I'd be ok making that sacrifice as I'm not sure it's used that much. I might experiment with that approach at some point.

P.s. thanks @Lomsor for explaining how TwinCAT's HotConnect functions - much appreciated!

[jimy-byerley]

I reached the same conclusion. I am experimenting a different API, without hook function in my new project etherage. I will see if not having a hook is convenient or not ;)

but the root of the problem currently is that Rust just really doesn't like async closures.

I agree ... This is a big hole in the rust ecosystem of async programming, considering how important closures are in the rust paradigms :(

[jamwaffles]

Keeping this open as I want to fix the original issue, but it's blocked on Rust features that don't seem to be coming any time soon :(