Implement sequences#12
Conversation
raffael0
left a comment
raffael0
left a comment
There was a problem hiding this comment.
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
|
|
||
| let (controller_tx, controller_rx) = mpsc::channel(); | ||
| let event_dispatcher = self.event_dispatcher; | ||
|
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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:
|
|
|
||
| final_actions.extend(interpolated_actions); | ||
| } | ||
| last_param_states.insert(param_state.param.clone(), new_param_value); | ||
| final_actions.push(timed_action); | ||
| } |
There was a problem hiding this comment.
The indentation here is a bit weird. At a first glance it looks like like 61/62 are in the if block
| from: TimedValue, | ||
| to: TimedValue, | ||
| interval: Duration, | ||
| ) -> VecDeque<TimedValue> { |
There was a problem hiding this comment.
Maybe an Optional would be nicer here? Instead of constructing an empty queue on error
| { | ||
| let secs = f64::deserialize(deserializer)?; | ||
| if !secs.is_finite() { | ||
| return Err(serde::de::Error::custom("duration cannot infinite or NaN")); |
| if let Err(SequenceRunError::Aborted) = &result { | ||
| // TODO: add logging to the frontend | ||
| eprintln!("Execution of sequence '{seq_name}' was aborted, now running abort sequence '{abort_seq_name}'"); | ||
| let _ = Self::execute_actions(abort_schedule, &controller_rx, event_dispatcher); |
There was a problem hiding this comment.
Would it make sense here to return the result of the abort sequence?
| events::Event::StartSequence { | ||
| seq_name, | ||
| abort_seq_name, | ||
| } => { | ||
| // TODO: replace with loading sequences from the frontend | ||
| let seq_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("sequences"); | ||
| let seq = Sequence::load_from_path(&seq_dir.join(&seq_name)); | ||
| let abort_seq = Sequence::load_from_path(&seq_dir.join(&abort_seq_name)); |
There was a problem hiding this comment.
Can you change the StartSequence Event to take the actual sequence string instead of path? Because that's how it's going to work with the socket later on and it shouldn't really change anything for the tests
|
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 |
3 participants
Additions:
Things to be done (
todo!()in code):