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

PaPaYoU - Introducing LUA and OSD Updates to utilize the new Filter parameters #51

Closed
wants to merge 8 commits into from

Conversation

papayou66
Copy link

Lua stuff
add imuf

papayou66 added 2 commits March 10, 2018 18:09
test version 66
Replace kd_filter variable name
@kidBrazil kidBrazil changed the title Lua PaPaYoU - Introducing LUA Updates to utilize the new Filter parameters Mar 11, 2018
@kidBrazil
Copy link
Member

hey @papayou66 what is the status of your testing with this so far?

@papayou66 papayou66 changed the title PaPaYoU - Introducing LUA Updates to utilize the new Filter parameters PaPaYoU - Introducing LUA and OSD Updates to utilize the new Filter parameters Mar 17, 2018
@kidBrazil
Copy link
Member

@papayou66 can you please take a look at Travis - seems there is an issue https://travis-ci.org/ButterFlight/butterflight/jobs/354760664

@papayou66
Copy link
Author

Travis is my friend today

Copy link
Member

@kidBrazil kidBrazil left a comment

Choose a reason for hiding this comment

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

Hey Papa, amazing job man. I love this and I know a lot of the community will go bananas for this stuff.

Just have a few minor modifications to request - feel free to pushback if you dont agree we can discuss.

Cheers

@@ -70,9 +70,9 @@ typedef enum {
} filterType_e;

typedef enum {
KD_FILTER_CLASSIC = 0,
Copy link
Member

Choose a reason for hiding this comment

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

Hate to do this to ya PaPaYou but I think we should revert this change and keep them as KD_FILTER_XXXX - just so the code clarity is improved.

SP / CLASSIC / NOSP are fine for people familiar with the genesis of these features but for anyone else that comes in on this code it might be confusing. The KD_FILTER add some clarity by making it immeditaly obvious that its dealing with D term filters.

Copy link
Author

Choose a reason for hiding this comment

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

Yes sure no problem

@@ -108,7 +108,7 @@ void resetPidProfile(pidProfile_t *pidProfile)
.dterm_notch_hz = 260,
.dterm_notch_cutoff = 160,
.dterm_filter_type = FILTER_PT1,
.dterm_filter_style = KD_FILTER_CLASSIC,
.dterm_filter_style = CLASSIC,
Copy link
Member

Choose a reason for hiding this comment

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

Revert to KD_FILTER_CLASSIC

float delta = 0.0f;
float iDT = 1.0f/deltaT; //divide once

switch (pidProfile->dterm_filter_style) {
case KD_FILTER_CLASSIC:
case CLASSIC:
Copy link
Member

Choose a reason for hiding this comment

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

Revert cases to KD_FILTER_XXXX variant

sbufWriteU16(dst, gyroConfig()->gyro_filter_q);
sbufWriteU16(dst, gyroConfig()->gyro_filter_r);
sbufWriteU16(dst, gyroConfig()->gyro_filter_p);
sbufWriteU8(dst, gyroConfig()->gyro_stage2_filter_type);
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need gyro_stage2_filter_type anymore?

break;
#endif

#ifdef USE_GYRO_IMUF9001
Copy link
Member

Choose a reason for hiding this comment

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

Something whack going on with the TABS on this area - could we please get that harmonized to line up with the rest?

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -250,7 +250,7 @@ static const char * const lookupTableLowpassType[] = {
};

static const char * const lookupTableKdStyle[] = {
"KD_FILTER_CLASSIC", "KD_FILTER_SP", "KD_FILTER_NOSP"
Copy link
Member

Choose a reason for hiding this comment

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

Please revert for clarity =D

@@ -80,12 +80,11 @@ void targetConfiguration(void) {
pidProfile->itermAcceleratorGain = 5000;
// should't need to set these since they don't get init in gyro.c with USE_GYRO_IMUF
Copy link
Member

Choose a reason for hiding this comment

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

If these are really no longer needed we should just remove them entirely instead of leaving comments IMO

papayou66 added 2 commits March 23, 2018 16:45
change typo about KD_FILTER CLASSIC SP and NOSP
@papayou66
Copy link
Author

Switch to a new branch to avoid merge conflict

@papayou66 papayou66 closed this Mar 23, 2018
@papayou66 papayou66 deleted the lua branch March 23, 2018 17:22
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.

None yet

2 participants