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

afs firmware with semi-tested apogee detection #2

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

Conversation

RobertAWoo714
Copy link
Contributor

No description provided.

Copy link
Member

@EricPedley EricPedley left a 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;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this reversed?

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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) {
Copy link
Member

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++;
Copy link
Member

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.

Copy link
Contributor Author

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):
Copy link
Member

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.

@EricPedley
Copy link
Member

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/

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.

2 participants