Implement sequences

[fweichselbaum]

Additions:

• sequence definitions & schema
• sequence parsing
• sequence executing
• integration with event dispatcher

Things to be done (todo!() in code):

• Implement evaluation of hold conditions
• Implement dispatching CAN messages in sequences
• Loading of sequences from frontend

[raffael0]

Nice work so far. I think this is a solid base to build on. I know there are still some things to implement but I decided to do the review now anyways. Hope thats fine! Also feel free to disagree with any of the points i've raised. I am in no way a rust expert

[raffael0]

Right now if the new thread panics, the rest of the system never is notified that there was an error. You can test this by adding a panic! to the thread => all the tests still pass.

What you can do is join the previous thread before creating the new one and logging(and maybe forwarding?) the error. I don't know if forwarding makes the most sense here, since we'd essentially forward the previous error to the current sequence execution. But I also don't know what the proper thing to do instead would be

[fweichselbaum]

The idea I came up with now is to catch panics within the thread with panic::catch_unwind logging the panic and then exiting the thread. This way, the panic gets logged immediately and not when the next sequence is started. I'm not sure if this is a good solution but better than silently ignoring any panics.

[raffael0]

Yeah it's better than nothing
@miDeb What do you think what we should do here? We have the same problem on the other threads..

[miDeb]

Hmm, generally panics are not a recoverable error. I think we should catch them with catch_unwind, log the error, try to notify the user... Maybe it would make sense to have some sort of "supervisor" thread that monitors the status of the other threads and can decide if it should restart the thread, or stop everything, in case a critical error like a panic happens?

[raffael0]

Hm yeah maybe. I guess the question is how recoverable we want the system to be. Because if the can thread dies it would maybe make more sense to just panic the application.

[fweichselbaum]

I rewrote some parts of the code because I wasn't happy with the previous implementation. Also, I added more tests for the interpolation and some tests for the sequence running.

Still TODOs:

• implement evaluation of hold conditions
• send can messages from sequences
• loading of sequences from frontend

[raffael0]

Looks good.
Just some minor things.

@miDeb added some nodemanager helper functions in #16 which you can then use for the hold conditions and for sending can messages from the sequences

[raffael0]

One more thing: Can you please create a document(maybe just a .md) in the repo explaining how the sequences work, what preconditions a sequence must have (all the checks in the code) and maybe a sample sequence? That would be helpful to show to the others

[fweichselbaum]

I implemented the changes we discussed in the last meeting.

Node manager: get / set fields
The sequence runner now calls methods from node manager, to set and get field values. The method signatures are currently copied from #16 and only implemented with todo!(). Once #16 is merged, this should be fixed.

Sequence tests
The sequence runner tests are currently disabled. This is, because the runner depends on the node manager. In the future, I think it would be a good idea to mock the node manager for the tests, so that sequence running can be tested more easily (for example using the crate mockall).

Catch unwind
I removed the panic::catch_unwind from the sequence run thread, because using a Dashmap reference within a catch_unwind does not work. For now, I replaced it with a panic guard, that logs an error if a panic occurs. I think, in the future it would be a better idea to use some kind of monitoring/supervisor system, because the other threads could also benefit from that.

[raffael0]

Thanks for all the work!