qitech-lib: Refactor out seperate library layer then can not just be used by us, but also by other people

[TheBest6337]

(Moved from @Oshgnacknak's issue: #1018)

In the long run, we want to allow third party to make us of out software stack. Currently they'd have to fork our codebase to do so. If instead, our code serves as a framework to built open, they could implement the machine trait themselves whilst only adding something like qitech-lib into their dependencies. This way, wago for instance, could develop and test there minimal machines on their own.

As an initial pitch, imagine each minimal machine lives in its own crate and only features such a simple main function.

fn main() {
    let control = qitech::lib::control::server::new();

    let devices = qitech::lib::ethercat::probe_ethercat();
    let ethercat_loop = qitech::lib::ethercat::EtherCATLoop::new(mapMyDevicesToMachines(devices));
    control.add_ethercat(ethercat_loop);

    control.init_api(3000).with_websockets();
    control.loop_forever();
}

fn mapMyDevicesToMachines(devices: qitech::lib::ethercat::Devices) {
    myMinimalMachine(devices.get(0))
    // ... and then some
}

[TheBest6337]

(Second message from @Oshgnacknak )

we might actually do this. One idea could be a bus based architecture where different components (ethercat machines, other machines, rest api, or something entirely different), can emit or listen to events to communicate. The bus should not need to know in what context (async/threaded/realtime/etc.) bus components are being executed. All a component would need is a channel to send events and a channel to receive events from. A similar approach is currently already taken by the machines themselves.

[Oshgnacknak]

From #910

By the latest refactor of our machine logic, the API server only needs two channels to send/receive data form/to the machines. Therefore, I propose moving our http API into a separate subcrate that will implizitly depend on tokio. Tokio should be banished from all other crates. It will run on a single thread that only does web server things.

Compile time wise, tokio seems acceptable to me, but should not be mixed with other async runtimes.

Optimally, the api would be a component, that you can include or not, depending on onces use case.

[Oshgnacknak]

From #1036

It might make sense to use enums at

control/machines/src/lib.rs

Line 33 in 1406d6c

pub const VENDOR_QITECH: u16 = 0x0001;

and following lines.

One can make enum constants equal to const values and automatically get an into() method.

#[repr(u16)]
enum MyEnum {
    Qitech = 1,
    Other = 2
}
const x: u16 = MyEnum::Qitech.into();

As part of this, we should also redo the machine ids. Using Enums for things we control. But more importantly, don't use a nested struct - thats just a pain.

Having exactly 6 bytes (2 vendor, 2 product, 2 serial) we could even consider using mac addresses.

[Oshgnacknak]

@just-some-entity and I did some prototyping on how we can store the discovered hardware (by whatever discovery mechanism) and allow a Machine(Factory) take specific hardware from the hardware map. Importantly, we use Any to allow other library dependents to add their own hardware into the mix without changes on our part.

use std::{any::{self, Any, TypeId}, collections::HashMap, fmt::Debug};

trait Hardware: Any + Debug {}

#[derive(Debug)]
struct PullerHardware {}

impl PullerHardware {

    fn print(&self) {
        println!("Pulling se beaches");
    }
}

impl Hardware for PullerHardware {}

fn foo<H: Hardware, P: FnMut(&H) -> bool>(hardware: &mut HashMap<any::TypeId, Vec<Box<dyn Hardware>>>, mut pred: P) -> Option<H> {

    if let Some(vec) = hardware.get_mut(&TypeId::of::<H>()) {
        let index = vec.iter().position(|h| {
            let any = h.as_ref() as &dyn Any;
            let casted = any.downcast_ref().expect("Cast failed despite check!");
            pred(casted)
        });

        if let Some(index) = index {
            let box_any = vec.remove(index) as Box<dyn Any>;
            let casted = box_any.downcast::<H>().expect("Cast failed despite check!");
            return Some(*casted)
        }
    }

    None
}

fn main() {
    let mut hardware: HashMap<any::TypeId, Vec<Box<dyn Hardware>>> = HashMap::default();

    println!("{:?}", hardware);

    hardware.insert(TypeId::of::<PullerHardware>(), vec![Box::new(PullerHardware {})]);

    if let Some(p) = foo(&mut hardware, |_: &PullerHardware| true) {
        p.print();
    }

    println!("{:?}", hardware)
}

[kraemr]

i dont like using predicates, i think this will lead to unreadable code, also i think that the initialization makes sense to happen inside of the machines as machines using the same terminals have different configurations for the terminal, that they need to apply. It doesnt make much sense to do it outside of the machine init logic in my opinion.

[Oshgnacknak]

The use of predicates is a compromise because we cannot give the callee the downcasted vector to select the machine themselves. We can however, give them each downcasted device individually. I would also prefer, to just give them the vector back to search for themselves.
I write down a more detailed comment why

[Oshgnacknak]

@kraemr Sorry for the confusion, I did not write down all our notes due to it being so late. I just saved a code snipped we got to compile, because it was not save in my /tmp - and because it took forever to make rustc happy.

Let me try to write down what we discussed from the bottom up! Our discussion was lead by the following questions:

• What should a machine actually know/do (single responsibility producible)?
• How should we store hardware that has been discovered, but now assigned/adopted by a machine?
• How should we select the hardware that a machine needs to operate?
• How should other components, like the API, read values and pass mutations to the machine?
• Who owns the hardware, at what times?
• How can we implement this in code, without knowing what our library users will write in advance? Enum types for example are mostly out of the question.

The last question's answer will be incorporated into the others.
I will use the winder as an example, as I did with JSE.

Q: What does the winder do?
A: It winds filament. To do so, it needs a spool, a traverse, a puller and a tension arm - called its devices. Periodically, id needs to update its internal state, so the filament is winded evenly.

Q: What should the winder not do?
A: The winder should not detect hardware to construct its devices. The winder should not parse mutation requests from the http endpoint. It should not iterate (or otherwise go other all) devices to see, if it can construct itself. The winder should not send its values to a websocket endpoint, or to any other more sophisticated data channel.

Q: Where would we put the devices we discover, so the winder can pick them up?
A: We want to allow other QiTech-Lib users to make their own discovery service. They should be able to instantiate their devices own their own and just give them to QiTech-Lib. This is where my previous comment comes in. This POC uses std::any - without other crates - to store all devices separated by type.

Q: If the winder does not select its devices, what should?
A: We propose a simple solution, to have a factory for each machine: The WinderFactory could look something like this:

class WinderFactory
  create(hardware: HardwareTree)
      puller = Puller::new(hardware)?
      spool = Spool::new(hardware)?
      traverse = Traverse::new(hardware)?
      tension_arm = TensionArm::new(hardware)?
      ret Winder::new(puller, spool, traverse, tension_arm)

Note that the device constructors can fail, if hardware (like the EL1100) is not found. Alternatively, devices are also be stored in such a DeviceRegistry and have their own factory. The data structure from my previous post was an attempt to realize the HardwareTree in a way, that does not depend at all own the library users implementation of their hardware (driver) at all. Storing Box<dyn MachineFactory> would be even easier. Im personally still not super happy with having Box<dyn Trait> be the best (rustiest) way to achieve some form of polymorphism (in java, we would not have this discussion), but that has to suffice assuming we want to give total control to our dependents.

Q: What mutates the Winder?
A: Note that we already ruled out that the winder handles its own mutations. I especially don't like this, because it forces the winder to understand our API. Why does the winder even know that their is JSON being passed around for instance? To solve this, we thought about using, if for the lack of a better name, the visitor pattern and just have the mutation mutate the winder.

class SetStepperSpeed: Muation<Winder>
  mutate(winder)
    winder.stepper.set_target_rpm(55)

Note here, that we only mutate the target state without locking - or in any other way, understanding how the change passed down to the hardware. We propose this pattern, to keep mutations as simple as possible. It should be the devices (only) responsibility to match make the actual hardware state match the target state as closely as possible. Importantly, the Mutation gets the machine, not the other way around. Here again, we have to pay attention to types. We want to allow someone to send us a Mutation<Winder> and figure out, if and where the winder is, that it wants. JSE already wrote a MachineManager that would get this responsibility.
Assuming machines are managed in a central place - that is our "loop" currently - anything can then pass mutations to that. Mutations can be applied at the appropriate time - perhaps when already updating the machine. If an API request to mutate the winder has been received by the http endpoint, the mutation can be parsed by the API(-extension for the winder WinderApi) and be given to the MachineManager. The MachineManager makes sure the mutation is then processed and mutates the Winder.

Q: How does the winder publish its values?
A: JSE proposed to have the MachineManager store the most recent emitted state/live values. A machine would only need to know, how to read the values from its devices. Components, like the API would not need to wait for a machine to response (asynchronously) to look at the values.
As an alternative, perhaps we want an observer pattern, where one can request the MachineManager to call back with the newly gotten values. I played around with this in the past and it was not super easy to get rustc happy with callbacks. Again, downcasting machines would be the biggest hurdle.
In both cases, the timing should not be handled by all machines individually, instead, the manager just checks, if the latest values are more then 1s/30 in the past. Also, we have to again resort to using std::any for machine values as we want a library users machine to publish arbitrary data that we don't know in advance. We could, if we wanted to, again use the previous' post data structure to store WinderLiveValues as such until someone queried them.
In a good world, there would be no difference between the API reading values from the winder and the winder reading values form the laser.

A: Who owns the Winder?
A: The Machine Manager. But let me explain the live cycle we imagine in more detail: Firstly, I would never again advice to have a shared reference to a machine or a device. That has bitten us in the past and we still feel the wound as of now. In relation with my first post in this discussion:

1. A library user (maybe QiTech control) registers all the factories, discovery services and other components (e.g. the API) to QiTech lib.
2. They tell QiTech lib to start.
3. QiTech lib start discovery and an empty machine manager
4. Discovery gives hardware to the machine manager (maybe a hardware manager, but yeah)
5. The machine manager queries all factories until one can construct hardware
6. The factory takes hardware from the hardware store and constructs devices/machines
7. Machines are given back to the machine manager.
8. The manager updates/interacts with the hardware until the hardware fails, is disconnected or the program exits.

[Oshgnacknak]

@kraemr finished

@just-some-entity can you check/correct/add things that I got wrong/are missing?

[MilanVDB]

@kraemr pls post a version how you would do it

[Oshgnacknak]

What do you guys consider a plausible next step for QiTech Lib? The way I see it, we should push towards making an example for each new feature we add (or already have) into the lib. Importantly, the examples should show us, how easy to use our lib really is. The more strait forward the example, the better. Examples should become stable as soon as possible because in the end, they show others how our API can used. How we implement the examples, we can iterate over and over again.

For cleaning up the EtherCAT a bit, if already started with this approach and I would like to make a few suggestions right of the bat.

• I would rename ethercat_hal to just ethercat or perhaps qitech::ethercat
• Start the ethercat state machine only once per interface and allow everybody to get a reference to that machine (a struct wrapping the channel, that will be cloned for each start_ethercat_thread call)
• Rename EtherCATController to EtherCATStateMachine
• Make a struct (prob. called EtherCAT) for the tuple returned by start_ethercat_thread that is a wrapper to a channel communicating with the EtherCATController. The struct should have useful methods like for going into another state or sending data to the subdevices. The channel mechanism should be hidden from the user at all times. This should allow us to not have the EtherCATController in an Arc. It should only ever exist in the thread its running in.
• Rename start_ethercat_thread to something like start_ethercat or get_ethercat. Nobody should really need to know that this is another thread. But if we want to later control what parameters the thread is running with, we should allow someone to pass their own thread (builder) to the start_ethercat and use that. Perhaps there will be some annoying call backs that basically revolve around calling ethercat_state_machine after setting core affinity and similar things.
• Some internal renames I will not explicitly state here.

As if now, I would like to get the following two example running, assuming the renames:

fn main() {
    let ethercat = qitech::ethercat::get_ethercat("eth0").expect("start");
    ethercat.goto_state(EtherCATState::PreOp).expect("goto preop");
    let subdevices = ethercat.get_subdevices().expect("subdevices");
    println!("{:?}", subdevices);
}

under the sync feature flag and perhaps under the async feature flag

#[tokio::main]
async fn main() {
    let ethercat = qitech::ethercat::get_ethercat("eth0").await.expect("start");
    ethercat.goto_state(EtherCATState::PreOp).await.expect("goto preop");
    let subdevices = ethercat.get_subdevices().await.expect("subdevices");
    println!("{:?}", subdevices);
}