Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framebuffer update #60

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Framebuffer update #60

wants to merge 4 commits into from

Conversation

cooperq
Copy link
Collaborator

@cooperq cooperq commented Aug 22, 2024

Fixes #57
adds quality of life improvements to framebuffer UI
white: not running
green: running
red: heuristic triggered

Also changes web ui to reflect same design language.

TODO: Write migration for qmdl metadata file, this could be in the same pull request or separate pull request.
DO NOT MERGE THIS PULL REQUEST WITHOUT COMMENTING OUT THE TEST ANALYZER IT IS VERY LOUD

@@ -43,11 +43,13 @@ async fn run_server(
config: &config::Config,
qmdl_store_lock: Arc<RwLock<RecordingStore>>,
server_shutdown_rx: oneshot::Receiver<()>,
ui_update_tx: Sender<framebuffer::Color565>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this channel should use descriptive enums of display state, rather than colors directly. something like:

enum DisplayState {
    Recording,
    Paused,
    WarningDetected,
}

and then the framebuffer code assigns those colors as an implementation detail

@@ -47,6 +49,7 @@ impl AnalysisWriter {
writer: BufWriter::new(file),
harness: Harness::new_with_all_analyzers(),
bytes_written: 0,
has_warning: false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i'm not sure why the writer should keep this state around?

Comment on lines +156 to +157
qmdl_store.update_entry_has_warning(index, analysis_writer.has_warning).await
.expect("failed to update analysis file has warning");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo we shouldn't be storing whether a warning was detected in the QMDL store, but instead should return from analysis_writer.analyze() an indication of whether a warning was detected and then update the UI from here.

changing analyze()'s return type to be Result<(usize, bool), std::io::Error> would be easy enough

Comment on lines +124 to +128
if current_entry.clone().unwrap().has_warning {
info!("a heuristic triggered on this run!");
state.ui_update_sender.send(framebuffer::Color565::Red).await
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, format!("couldn't send ui update message: {}", e)))?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per above, this should happen in the diag thread

}

fn get_description(&self) -> Cow<str> {
Cow::from("Always returns true, if you are seeing this you are either a developer or you are about to have problems.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lmao

Comment on lines +8 to +10
pub struct ExampleAnalyzer{
pub count: i32,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo this kinda thing should be defined in a test file. also this is pedantic i guess but i'd describe it less as an ExampleAnalyzer and more like a DebugAnalyzer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

framebuffer UI: more expressive statusbar
2 participants