-
Notifications
You must be signed in to change notification settings - Fork 1
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
Navigator WebAssistant First Commit #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to remove merge commits
0ed23e6
to
23e9e37
Compare
ping2 @patrickelectric |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the overall, I have just a few notes, feel free to merge the PR as it is and work on them afterward.
let datalogger_directory = matches | ||
.get_one::<String>("datalogger_directory") | ||
.map(|d| d.to_string()) | ||
.unwrap_or("./".to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure we should add the default values when specifying the argument instead of leaving them as optionals and using these unwrap fallbacks. The benefit is that it will show the default values for each argument in the help, and it will be easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan to fix it changing the clap use
src/hardware_manager.rs
Outdated
pub fn init_monitor(refresh_interval: u64) { | ||
NavigationManager::get_instance().lock().unwrap().monitor = Some( | ||
thread::Builder::new() | ||
.name("Navigator service - monitor".into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name will not be distinguishable in htop
or ps
. Please keep the thread names under 15 characters.
Since they are subprocesses of the Navigator binary, you can name it as just monitor
for example.
src/hardware_manager.rs
Outdated
pub fn init_datalogger(refresh_interval: u64, file_path: PathBuf) { | ||
NavigationManager::get_instance().lock().unwrap().datalogger = Some( | ||
thread::Builder::new() | ||
.name("Navigator service - datalogger".into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. datalogger
src/hardware_manager.rs
Outdated
loop { | ||
thread::sleep(std::time::Duration::from_micros(refresh_interval)); | ||
|
||
let reading = DATA.read().unwrap().state; | ||
|
||
logger.log_data(&reading).expect("Failed to log data"); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: by sleeping first, we are adding a delay to the logged measurements, which is a constant phase between the monitored and the logged measurements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to sleep after
src/logger.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should switch to tracing and add output to files with rotation ASAP.
src/main.rs
Outdated
hardware_manager::init_monitor(monitor_settings.interval); | ||
log::info!("starting monitor..."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardware_manager::init_monitor(monitor_settings.interval); | |
log::info!("starting monitor..."); | |
log::info!("starting monitor..."); | |
hardware_manager::init_monitor(monitor_settings.interval); |
src/main.rs
Outdated
hardware_manager::init_datalogger( | ||
datalogger_settings.interval, | ||
format!("{}/{}", datalogger_settings.directory, datalogger_settings.filename).into() | ||
); | ||
log::info!("starting datalogger..."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardware_manager::init_datalogger( | |
datalogger_settings.interval, | |
format!("{}/{}", datalogger_settings.directory, datalogger_settings.filename).into() | |
); | |
log::info!("starting datalogger..."); | |
log::info!("starting datalogger..."); | |
hardware_manager::init_datalogger( | |
datalogger_settings.interval, | |
format!("{}/{}", datalogger_settings.directory, datalogger_settings.filename).into() | |
); |
} | ||
} | ||
Ok(ws::Message::Binary(bin)) => ctx.binary(bin), | ||
_ => (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we handle the Close frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joaoantoniocardoso Will test it:
Ok(ws::Message::Close(msg)) => ctx.close(msg)
If it works we can add to mavlink2rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joaoantoniocardoso It works, thanks, before this the connection keep holding for some seconds or minutes until it really closes and finish session on websocket client, we need to add this do mavlink2rest.
fix: remove cargo.lock from git
src: server: fix bind route, fix CORs issue src: server: package: minor fix on sensor type src: server: package: add pwm package handlers src: server: package: add pwm_channel from string handler src: server: rest: rework on get_sensors method src: server: rest: add pwm related post services src: server: add pwm related post services src: server: change sensors service name
src:server: Add embedded host index to services and the fronted source folder path src:server:frontend Add a html to embed and host interface
…onstructor src: server: package: use a new method to compose the packets src: server: frontnd: integrations with new sensor readings json and change update interval to use ms src: server: package: remove altitude from sensors readings
src: hardware_manager: Add sensor readings thread service, and start function
…use defaults src: hardware_manager: Add user led functions src: hardware_manager: Add neo pixel function
…ation src: server: Add errors sections src: server: rest: Fix schemathesis tests with defined errors code on generated spec src: server: frontend: Add trailing zeros on left side src: server: frontend: Change refresh event and validate inputs src: server: frontend: Add sensor broadcast received frequency src: server: frontend: Add docs button
src: server: rest: Add derive validate and min/max value for Channel and Frequency src: server: rest: Add validation and retrieve error for channel value route src: server: rest: Add validation and retrieve error for frequency value route
src: server: structures: Move API structures src: server: remove unnecessary code src: server: websocket: Use new API structure, similar to restAPI src: server: packages: Remove deprecated function src: server: frontend: Update websocket structure
src: server: frontend: RGB and User LEDs syncronization through websocket src: server: frontend: RGB and User LEDs functions using websocket and their structures src: server: frontend: Minor change, format src: server: websocket: Fix Userled routes
…ficant overlap src: hardware_manager: Change the wait check src: server: frontend: Add buttons to disable or restart autopilot
README: Fix the adresses
src: server: structures: Minimal req changes src: server: rest: Minimal req changes
src: server: frontend: Simplify pwm frequency parser src: server: frontend: Simplify neopixel parser src: server: frontend: Simplify userled parser
src: server: Add port argument src: main: Add port argument Readme: Add instructions for port argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pro tip: when requesting reviews, please, do not rebase before solving all comments (leave the fixups instead), it makes the reviewer's life a lot harder, it's especially hard to keep track of the changes are up to the request, which is terrible on these gigantic PRs.
Other than that, it seems good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be used on BlueOS locally, please change ardupilot to SITL then add extension: