-
Notifications
You must be signed in to change notification settings - Fork 0
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
afs firmware with semi-tested apogee detection #2
base: main
Are you sure you want to change the base?
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.
Haven't read it all, but left a few comments on the stuff I have read through so far.
|
||
/* Magnetometer data */ | ||
magData = magnetometer.Read(); | ||
afsData.magneticFieldX = magData.magneticFieldY; |
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.
Why is this reversed?
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.
Oh, it's because the orientation of the magnetometer is different then Axis we decided for afs. I'll write a comment for that
if(altData.altitude < prevAltitude) | ||
{ | ||
if (afsData.altitude != -1 && altData.altitude != prevAltitude) { | ||
if (altData.altitude < prevAltitude) { | ||
tick++; | ||
// 100 is found based on HZ of loop being around 50 HZ ish |
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 comment is inaccurate now that you changed it to 5, might be confusing to anyone who has to read this later.
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.
Mb, thanks for the catch
tick++; | ||
// 100 is found based on HZ of loop being around 50 HZ ish | ||
if(tick > 100) | ||
{ | ||
if (tick > 5) { |
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.
How fast/slow do we poll the altimeter for us to be confident the metric "five decreasing readings in a row" will never happen because of random noise?
{ | ||
if ((afsData.altitude != -1) && (altData.altitude != prevAltitude)) { | ||
// change state after 5 ticks of AFS under 200 ft or 61 meters | ||
if ((altData.altitude < startAltitude + 61)) { | ||
tick++; |
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.
Are you sure you want to reuse the same variable you used for the drogue chute? I feel like it would be easy to introduce a bug like this.
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 reset the variable tho before moving this state. Do you think we need another variable?
@@ -521,95 +464,97 @@ int main(void) { | |||
case (0x8): |
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.
Stopped reading here, will come back to it eventually.
Also @cadguyen @DanielHeEGG I recommend you download this extension to look at the file diff for this PR since 80% of it is formatting changes. https://mergease.com/ |
No description provided.