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

Cope with negative ESC rpms in notch filter #28309

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

Conversation

andyp1per
Copy link
Collaborator

No description provided.

@andyp1per andyp1per linked an issue Oct 4, 2024 that may be closed by this pull request
@RickReeser
Copy link
Contributor

RickReeser commented Oct 4, 2024

@andyp1per The test flight that caused me to open the issue was done on Copter 4.5.6. Master has a different behavior: the logged filter frequency is negative for notches that are tracking negative RPM. I haven't read through the code enough yet to know what the filter is actually doing in this case. This is what it looks like on master:
image

With your changes:
image

@andyp1per
Copy link
Collaborator Author

It doesn't look quite right, does it. We have made some effort to properly log what values are actually being used.

@andyp1per
Copy link
Collaborator Author

Actually, maybe it does look right. I'm just a little thrown by the weird symmetry of the ESC values - I wonder if there is something else going on there. Unless this is just split yaw.

@RickReeser
Copy link
Contributor

Yeah I think it is fine. The symmetry is because the drone was yawing at the time (uncommanded; bad tune because this was default tuning params).
image

Here are the logs from the above screenshots.
negative rpm test flights.zip

@andyp1per
Copy link
Collaborator Author

great!

@IamPete1
Copy link
Member

IamPete1 commented Oct 6, 2024

My preference would be to just apply the abs in the ESC RPM method. Just so that -1 for a static notch still results in off rather than 1hz. -1 makes no sense for many of the sources.

@andyp1per
Copy link
Collaborator Author

IDK, this feels safer preventing negative numbers ever reaching the notches

@tridge
Copy link
Contributor

tridge commented Oct 7, 2024

@IamPete1 i'm OK with this change, but will delay merge for you to expand on your comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Support negative RPM for dynamic harmonic notch
6 participants