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

SNS - Low Pressure Sensor #73

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

Conversation

siny-over-cosy
Copy link
Contributor

No description provided.

Copy link

linear bot commented Feb 5, 2025

@davidbeechey davidbeechey marked this pull request as draft February 6, 2025 09:28
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, a few wee things + you need to add hyped_adc as a dependency for hyped_sensors. We're still figuring out whether or not we'll have tests for ADC sensors, so feel free to leave that for now

@siny-over-cosy
Copy link
Contributor Author

removed no_std and no_main to try and remove the "no #[panic_handler]" error

@davidbeechey davidbeechey marked this pull request as ready for review February 9, 2025 17:09
@davidbeechey davidbeechey added the needs reviews Needs reviews, otherwise finished label Feb 9, 2025
@siny-over-cosy
Copy link
Contributor Author

telemetry jumpscare

doesn't look like this is something i can fix?
"pnpm: command not found"

@davidbeechey davidbeechey added the needs testing Needs testing in the lab before merging label Feb 14, 2025
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.

Looks good, main thing is adding sensor bounds and checking with Samuel if you haven't already that you're converting the ADC reading to pressure correctly

Comment on lines +34 to +37
let adc_val = self.adc.read_value() as f32;

// convert to bar unit
let bar_pressure_val: f32 = adc_val * GRADIENT_LOW;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Confirm with Samuel that this conversion is correct

/// 4096 is the maximum ADC reading value.
/// Since LPS has a minimum pressure of 0 bar, c0 is 0 and was did not need to be included in
/// the source code.
pub fn read_pressure(&mut self) -> f32 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

read_pressure should return an f32 wrapped in a SensorValueRange - we want to know whether a value is too high or too low. Have a look at temperature sensor for an example of sensor bounds :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

you'll also need to define a default calculate_bounds function for this

Comment on lines +24 to +27
loop {
info!("{}", low_pressure_sensor.read_pressure());
Timer::after(Duration::from_millis(100)).await;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're loop here will need to match on the SensorValueRange to determine whether it's a critical, warning, or safe reading once that's implemented

#[embassy_executor::main]
async fn main(_spawner: Spawner) -> ! {
let p = embassy_stm32::init(Default::default());
info!("Low pressure sensor");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
info!("Low pressure sensor");

not really necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs reviews Needs reviews, otherwise finished needs testing Needs testing in the lab before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants