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

LOC - Implement Navigator #47

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

LOC - Implement Navigator #47

wants to merge 50 commits into from

Conversation

misha7b
Copy link
Contributor

@misha7b misha7b commented Nov 11, 2024

Transfer navigator.cpp from the 2024 codebase.

  • Think of new name. "Localisator"?

@kshxtij
Copy link
Contributor

kshxtij commented Nov 11, 2024

Localizer?

@misha7b misha7b marked this pull request as draft November 13, 2024 14:41
@misha7b misha7b marked this pull request as ready for review January 23, 2025 15:55
@misha7b misha7b requested a review from davidbeechey January 23, 2025 15:56
Copy link
Collaborator

@davidbeechey davidbeechey left a comment

Choose a reason for hiding this comment

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

Looking good, just a few things to change and comments!

Comment on lines +16 to +17
const DELTA_T: f64 = 0.01;
const STRIPE_WIDTH: f64 = 1.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be good to add a comment with the units for these

//TODOLater: Change state
return;
} else {
//TODOLater: Check unit of keyence data
Copy link
Collaborator

Choose a reason for hiding this comment

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

what unit is this referring to?

.check_keyence_agrees(keyence_data.clone());

if keyence_status == SensorChecks::Unacceptable {
//TODOLater: Change state
Copy link
Collaborator

Choose a reason for hiding this comment

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

rather than handling the logic for changing state in here, the whole preprocessor function could return Result<(), PreprocessorError> or similar in this case so that the caller can deal with it instead

let processed_accelerometer_data =
accelerometer_preprocessor.process_data(accelerometer_data);
if processed_accelerometer_data.is_none() {
// TODOLater: Change state
Copy link
Collaborator

Choose a reason for hiding this comment

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

likewise here

Comment on lines +135 to +139
self.preprocessor(
optical_data.clone(),
keyence_data.clone(),
accelerometer_data.clone(),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you change the return type then iteration will also return a Result type, so just pass the error up

self.keyence_val = keyence_data[0] as f64;
}

let mut accelerometer_preprocessor = AccelerometerPreprocessor::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be better to just have one instance of AccelerometerPreprocessor like with Keyence so it's not being constantly created

Comment on lines +39 to +42
// Assuming frequency of 6400hz for IMU at 120 mu g / sqrt(Hz)
// standard deviation = 120 * sqrt(6400) = 9600 mu g = 0.0096 g
// = 0.0096 * 9.81 = 0.094176 m/s^2
// variance = 0.094176^2 = 0.0089 m/s^2
Copy link
Collaborator

Choose a reason for hiding this comment

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

really good comments here

}
}

impl Localizer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some Rust doc /// comments for Localizer and the preprocessor and iteration functions would be great

Comment on lines +5 to +6
pub fn process_optical_data(raw_optical_data: Vec<f64, 2>) -> f32 {
let mut magnitude: f32 = 0.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to keep this generic to any number of optical flow sensors? Could take the number as a constant NUM_OPTICAL_FLOW_SENSORS like with accelerometers, and just have a Vec of length 1 if there's only one?


let raw_keyence_data: Vec<u32, 2> = Vec::from_slice(&[0, 0]).unwrap();

let raw_accelerometer_data: RawAccelerometerData<K_NUM_ACCELEROMETERS, K_NUM_AXIS> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't spot this earlier, but we don't need K_ prefix for K_NUM_ACCELEROMETERS or K_NUM_AXIS

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.

4 participants