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.

Sign up for GitHub

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

Jump to bottom

Update MMA8452.cpp #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update MMA8452.cpp #5

wants to merge 1 commit into from

Conversation

kshepitzki
Copy link
Contributor

Added pitch roll calculation. MMA8452.cpp part.

@kshepitzki
Update MMA8452.cpp
38d1f38 
Added pitch roll calculation. MMA8452.cpp part.
akupila
akupila requested changes May 29, 2018
View reviewed changes
Copy link
Owner

@akupila akupila left a comment

Choose a reason for hiding this comment

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

A couple small questions.

I think this can be simplified a bit; ideally the initPitchRoll would not be needed at all.

Also, could you please put both commits in the same PR so we can merge them as one unit? Basically please do the changes to .h in this PR too and close the other one

MMA8452.cpp
*pitch = (atan2(fXg, sqrt(fYg*fYg + fZg*fZg))*180.0) / M_PI;
}

void MMA8452::initPitchRoll(float a)
Copy link
Owner

Choose a reason for hiding this comment

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

Is this needed at all? Could fXg, fYG and fZg not just need to be local variables in getPitchRoll & alpha be an argument to the function? If you forget to call initPitchRoll, alpha would not be set which would probably not work as expected. Also, it's not immediately clear what the purpose of alpha is; i suppose it's acting as range? It's not clear what value should be passed in (0.5?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alpha indeed could be a parameter.
fXg, fYg and fZg need to be members or passed as reference (in/out) parameters to the function and be saved by the caller.
fXg, fYg and fZg are used as a low pass filter implemented as a weighted average of the current reading and the previous readings. It slows the response time but without it the result will be very jerky. When Alpha == 1 the result is calculated without history (no low pass filter). When Alpha is close to zero (e.g. 0.1) the low pass filter is strong and the result is stable (albeit with a few hundred milliseconds delay). 0.5 is a good value for Alpha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you like this format better?
void MMA8452::getPitchRoll(float *pitch, float *roll, float alpha, float *fXg, float *fYg, float *fZg)
{
float Xg, Yg, Zg;

getAcceleration(&Xg, &Yg, &Zg);

//Low Pass Filter
*fXg = Xg * alpha + (*fXg * (1.0 - alpha));
*fYg = Yg * alpha + (*fYg * (1.0 - alpha));
*fZg = Zg * alpha + (*fZg * (1.0 - alpha));

*roll = (atan2(- *fYg, *fZg)*180.0) / M_PI;
*pitch = (atan2(*fXg, sqrt(*fYg * *fYg + *fZg * *fZg))*180.0) / M_PI;

}

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, i see what you mean now! I've typically implemented lowpass as v += (target - v) * a so i missed this. This makes sense. Could you please add a comment on alpha though:

alpha defines how quickly to react to changes. The value should be 0-1.

I would prefer to keep the signature smaller and not having to have the called keep track of these values. Was thinking though, since you're expected to call init() anyway, why not just set the moving average values (fXg/fYg/fZg) to 0 in there?

MMA8452.cpp
@@ -517,3 +517,25 @@ int8_t MMA8452::convertTo2sComplement(int8_t value)
value = (0xFF && ~value) + 1;
return value;
}

void MMA8452::getPitchRoll(float *pitch, float *roll)
Copy link
Owner

Choose a reason for hiding this comment

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

Could this also support yaw?

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 am not aware of a way to calculate yaw without a magnetometer or gyro.

@akupila akupila mentioned this pull request May 29, 2018
Open
@kshepitzki
Copy link
Contributor Author

I don't know how to commit 2 files in one PR when it is not my account.

@akupila
Copy link
Owner

akupila commented May 31, 2018

Oh, I guess you did the PR on GitHub directly? What ends up happening is that the project is forked to your user and if you push new commits to the branch there, they'll show in this PR.

https://github.com/kshepitzki/Arduino-MMA8452/tree/patch-3

git clone git@github.com:kshepitzki/Arduino-MMA8452.git
git checkout patch-3
# do the changes
git push

The commit should then show up here. Happy to help if you need more info!

@akupila
Copy link
Owner

akupila commented Jun 8, 2018

@kshepitzki I think this is a very useful feature, would be nice to see it merged. Let me know if you need any more help with github etc, i'm happy to help

@kshepitzki
Copy link
Contributor Author

kshepitzki commented Jun 8, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akupila akupila akupila requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kshepitzki @akupila