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

Use gyro data for Turn Tilt #19

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aeraglyx
Copy link
Contributor

@aeraglyx aeraglyx commented Oct 5, 2024

Small refactor that makes Turn Tilt frequency independent by using gyroscope data directly. Functionally, not much should change and it gets rid of some variables. The numbers were chosen so it roughly matches the behavior at the default loop frequency. I only tested it inside at low speeds but the setpoint value seemed normal, so hopefully it's correct (I don't really use Turn Tilt).

One possible regression is that during high roll turns (like berms), yaw rate gets reduced (pitch becomes the new yaw). I suppose it could be combated by calculating the yaw rate as d->gyro[2] - sinf(VESC_IF->imu_get_roll()) * d->gyro[1] or from the VESC_IF->imu_get_gyro_derotated() function. But maybe it's not important and gyro[2] is enough?

For now I used an ugly ... / d->float_conf.hertz, I guess ideally we would have a variable for loop time in seconds that could replace these divisions?

Would something like this make sense?

@lukash
Copy link
Owner

lukash commented Oct 27, 2024

Thanks for the PR and sorry about the delay, I needed to take a good look at it. Though, removing the loop hertz dependencies is a very welcome contribution as it's a high priority for me now.

So, IIUC, please correct me if I'm wrong (FWIW it'd be great to have this sort of summary as part of the commit message):
The yaw aggregate is just being accumulated, that value is not dependent on the frequency. But the limits that are applied to it are dependent on the frequency, because they are not applied to the rate of change but to the change itself. That's why you changed it to the gyro value, which is speed, i.e. rate of change.

The switch to gyro is a significant change though, and IMO it's undesired, due to the high roll turns situation as you mentioned. Several people have been telling me the Turntilt doesn't hold their nose up in berms (my experience is similar). Presumably it'd be even worse with this change (I'm reasonably sure it won't make it better 😁).

The VESC_IF->imu_get_gyro_derotated() function (IIUC it recalculates the gyro data to be in world coordinates) is very expensive and I'm not keen on the simpler calculation either, it's not that clear it'll work well in practice.

Related, though a bit of tangent, any change like this needs to be properly tested to make sure it works within some margins the same (or potentially better), so if the way something is calculated is changed, it needs to be properly tested. First, it's expected the author does their best effort to test before submitting 😇. Then, as a validation it's good to have another person (with an idea and a feel for what's being tested) to validate the change. We don't really have people volunteering for this so most of it falls on me. I've done a ton of testing lately and I'm starting to feel my limits...

What if, to keep the change as non-invasive as we can, we just divided the yaw change by dt to get the rate, limited that, and then multiplied by dt again to get the change? (in theory we could calculate the limits based on dt to avoid the division, but I have plans for fixing the timing and the dt should be a precise runtime value at some point in the future)

@surfdado what do you think about this and would you be able to test the change?

@aeraglyx aeraglyx marked this pull request as draft October 27, 2024 22:07
@aeraglyx
Copy link
Contributor Author

Thanks for the feedback. I probably should have marked this as a draft from the beginning, apologies for that.

Dividing the yaw change by dt to get the rate sounds good (the yaw is already derotated), although I was hoping that we could get rid of the checks for going through ±180° and unchanged IMU values, which is a non-problem with gyro. But I don't have a strong preference, it was just a (dumb) idea 🙂

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