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

Update the harmonic notch dynamically #12270

Merged
merged 4 commits into from Oct 10, 2019

Conversation

@andyp1per
Copy link
Contributor

commented Sep 11, 2019

This is part II of the harmonic notch change - #11795
This PR takes the throttle output and creates a harmonic notch center frequency scaled using INS_HNTCH_REF on multicopters only. The harmonic notch picks up the requested change automatically.
In addition the requested center frequency is logged.
This PR also adds an autotest for the entire harmonic notch setup

@andyp1per andyp1per force-pushed the andyp1per:pr-dynamic-harmonic-notch branch 2 times, most recently from f10bfc0 to c2d23fc Sep 11, 2019
@andyp1per andyp1per changed the title Update the harmonic notch dyanmically Update the harmonic notch dynamically Sep 11, 2019
@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@rmackay9 and @lthall please review. There are no gyro loop or attitude control functional changes here - all of the complicated work has been done in #11795

@bnsgeyer

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@andyp1per So if reference parameter is set to 0 then it will not update the notch dynamically? I could see some usefulness to being able to do this. It looks like you've left the door open enough that I could make a change later to allow Heli to do this. I have to think on it. It would only be to cover users that work at multiple rotor speeds. If they devised their RSC setpoints properly, then it could be scaled to different rotor speeds. we also have a rotor speed setpoint for the governor that could be used.

BTW, congrats on the merging of the Harmonic Notch PR! very cool!

@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@bnsgeyer - yes, correct and thanks!

@andyp1per andyp1per force-pushed the andyp1per:pr-dynamic-harmonic-notch branch 2 times, most recently from 04954ed to d5f2a3a Sep 11, 2019
@lthall

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Looks good. I will do another flight test of this and then give the thumbs up.

@lthall

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

Working really well so far!!

I am happy with this.

@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

@rmackay9 good to go?

Copy link
Contributor

left a comment

Looks pretty good, just a couple of questions ..

ArduCopter/system.cpp Outdated Show resolved Hide resolved
libraries/AP_Motors/AP_MotorsMatrix.cpp Show resolved Hide resolved
@andyp1per andyp1per force-pushed the andyp1per:pr-dynamic-harmonic-notch branch 2 times, most recently from bfbd30f to 343fc5d Oct 5, 2019
@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

I've squashed and rebased

@bnsgeyer

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

@andyp1per i have made a commit that will use RPM to dynamically update the notch. I haven't flight tested it yet but wanted you to look at it and get comments from you and @rmackay9. If you think it is good and the flight tests turn out fine. Maybe we could pull it into this PR. here is the link to my commit

@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

@andyp1per i have made a commit that will use RPM to dynamically update the notch. I haven't flight tested it yet but wanted you to look at it and get comments from you and @rmackay9. If you think it is good and the flight tests turn out fine. Maybe we could pull it into this PR. here is the link to my commit

@bnsgeyer I've put a couple of comments in the commit

@andyp1per andyp1per force-pushed the andyp1per:pr-dynamic-harmonic-notch branch from e063954 to 9b058d2 Oct 7, 2019
@rmackay9

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Do you guys think this is ready for me to review again or still some more changes coming? No pressure of course.. just wondering if you think it's ready.

@lthall

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

I think so. The only thing to add now is the Heli changes. Maybe you could take a quick look at what Bill has done there too.

@andyp1per andyp1per requested a review from rmackay9 Oct 8, 2019
@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@rmackay9 yeah I think it's ready to go modulo any heli changes.

@bnsgeyer

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@andyp1per and @lthall I need help! I am trying to test the dynamic hntch with my changes for heli and nothing seems to be working. I even removed the dynamic hntch code and tried running the hntch currently in master with no success.
So here is the setup. I have an older heli with a CUAV pixhack V3 with an rpm sensor. Got that up and running and everything works fine with the sensor and pixhack. can you please review the logs and my settings (in the logs) to be sure I am not overlooking a setting or missed something. I am prepared to move over to my newer heli with the pixhack V5 but I'm hesitant to do so with beta software (however I did it before with the first release of hntch). plus it doesn't have RPM set up on it. here is a link to my branch of andy's PR branch which contains my commits for heli. Here is a link to the log files. Any help would be greatly appreciated.
One thought, is it possible that since this is a V3 (like a pixhawk 1) that it is not running the hntch code?

@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@bnsgeyer it looks like its working to me:

image

Your rpm is about 1700 which translates to a frequency of 27Hz. Your frequency is set to 27.5Hz and so the only variation you are seeing is when your code selects 8Hz as the frequency instead of 0. I think your code is wrong in this regard - it should be MAX(ref_freq, rpm/60*ref) that way you will get a nice clean line that never goes below ref_freq.

If you are looking at FFT's you have INS_LOG_BAT_OPT set to 1 which is pre- filter gyro sample rate. You will need to set it to 2 (post-filter) to see an effect!

@bnsgeyer

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@andyp1per thanks for the sanity check. I was hoping it was something I was overlooking. I always get that pre/post filtering opt wrong. As for the 8 hz versus ref_freq, I spoke with Leonard this morning I he convinced me as to why heli would want to use ref_freq.
So I will clean my branch up tonight and push my commits to your PR. I plan on adding some additional wording you your parameter descriptions to explain how they are to be used for heli.

@bnsgeyer

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@andyp1per @lthall and @rmackay9 changes are complete for tradheli and pushed to the PR branch. ready for merge.

@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@bnsgeyer looks good

@tridge
tridge approved these changes Oct 9, 2019
ArduCopter/system.cpp Outdated Show resolved Hide resolved
andyp1per added 2 commits Jul 10, 2019
…ally log the frequency.

tradheli- make RPM sensor input to harmonic notch in hertz
@andyp1per andyp1per force-pushed the andyp1per:pr-dynamic-harmonic-notch branch from 510656f to faf5e17 Oct 10, 2019
Modify parameter descriptions for tradheli
@andyp1per andyp1per force-pushed the andyp1per:pr-dynamic-harmonic-notch branch from faf5e17 to d862c63 Oct 10, 2019
@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

I've squashed and made the changes that @rmackay9 suggested. I have also fixed a couple of typos in the parameter descriptions.

@rmackay9 rmackay9 merged commit ef7f853 into ArduPilot:master Oct 10, 2019
4 checks passed
4 checks passed
ArduPilot.ardupilot Build #20191010.12 succeeded
Details
ArduPilot.ardupilot (Cygwin SITL build) Cygwin SITL build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
@rmackay9

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

Great, merged! @bnsgeyer, can you just check that you're happy with the, "ins.update_harmonic_notch_freq_hz(ref_freq);" running even if tradheli doesn't have an RPM sensor?

@andyp1per

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

Marvelous!

@bnsgeyer

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

@rmackay9 I don't see any issue with that. It allows the user to update the static notch center frequency in-flight which I don't see as being a problem. thanks for checking!

@andyp1per andyp1per deleted the andyp1per:pr-dynamic-harmonic-notch branch Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.