Skip to content

Fix gps rescue throttle pid gains #572

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
Closed
wants to merge 4 commits into from
Closed

Fix gps rescue throttle pid gains #572

wants to merge 4 commits into from

Conversation

@ctzsnooze
Copy link
Member

@ctzsnooze ctzsnooze commented Apr 29, 2022 •
edited
Loading

This PR is a simple one to:

  • update the GPS Rescue Altitude control Debug traces (PID and altitude),
  • add GPS Rescue Velocity control Debug traces (PID and velocity), and
  • update the RTH graphs and labels.

It is intended to properly display the new GPS Rescue debug traces from 11579

@ctzsnooze ctzsnooze requested a review from KarateBrot April 29, 2022 01:32
@ctzsnooze ctzsnooze self-assigned this Apr 29, 2022
@ctzsnooze ctzsnooze force-pushed the Fix-gps-rescue-throttle-pid-graphics-and-names branch from ea4507b to 82929a0 Compare April 29, 2022 11:59
@blckmn
Copy link
Member

blckmn commented Apr 30, 2022 •
edited
Loading

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> FAIL
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

blckmn
blckmn previously approved these changes May 2, 2022
View reviewed changes
haslinghuis
haslinghuis reviewed May 2, 2022
View reviewed changes
haslinghuis
haslinghuis reviewed May 2, 2022
View reviewed changes
haslinghuis
haslinghuis reviewed May 2, 2022
View reviewed changes
haslinghuis
haslinghuis reviewed May 2, 2022
View reviewed changes
haslinghuis
haslinghuis reviewed May 2, 2022
View reviewed changes
haslinghuis
haslinghuis requested changes May 2, 2022
View reviewed changes
Copy link
Member

@haslinghuis haslinghuis left a comment

Choose a reason for hiding this comment

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

Please remove duplicate blocks (to remove the code duplication errors).

@ctzsnooze ctzsnooze force-pushed the Fix-gps-rescue-throttle-pid-graphics-and-names branch from ba4cb6f to 245e7f4 Compare May 3, 2022 00:49
McGiverGim
McGiverGim requested changes May 4, 2022
View reviewed changes
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

My comments go in the same line than @haslinghuis. Try to group case elements, in this way is clear that various elements have exactly the same behaviour. For the rest the PR is ok.

@ctzsnooze ctzsnooze force-pushed the Fix-gps-rescue-throttle-pid-graphics-and-names branch 2 times, most recently from 363a643 to eb1c7c9 Compare May 14, 2022 13:57
@ctzsnooze
Copy link
Member Author

ctzsnooze commented May 15, 2022

Not sure if I addressed all the issues above, if not please let me know.

Added another debug for betaflight/betaflight#11579, tested.

@McGiverGim
Copy link
Member

McGiverGim commented May 18, 2022

Not sure if I addressed all the issues above, if not please let me know.

I think my review has not been fixed, I don't see any change in the code about it.

@ctzsnooze ctzsnooze force-pushed the Fix-gps-rescue-throttle-pid-graphics-and-names branch from eb1c7c9 to cfc6659 Compare May 20, 2022 08:47
McGiverGim
McGiverGim requested changes May 20, 2022
View reviewed changes
@ctzsnooze ctzsnooze force-pushed the Fix-gps-rescue-throttle-pid-graphics-and-names branch from cfc6659 to 713e64c Compare May 20, 2022 12:11
McGiverGim
McGiverGim requested changes May 20, 2022
View reviewed changes
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Apart from my other review, Sonar is complaining about some things, I've marked the basic. The others, I think we can ignore them.

@ctzsnooze
Fix gps rescue throttle pid gains
8fd2fd7 
Add heading, velocity and tracking and fix rth
@ctzsnooze ctzsnooze force-pushed the Fix-gps-rescue-throttle-pid-graphics-and-names branch from 713e64c to 8fd2fd7 Compare May 21, 2022 08:45
@ctzsnooze ctzsnooze force-pushed the Fix-gps-rescue-throttle-pid-graphics-and-names branch from e291e23 to 5133443 Compare May 21, 2022 13:15
McGiverGim
McGiverGim requested changes May 22, 2022
View reviewed changes
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

The latest one, please ;)

McGiverGim
McGiverGim reviewed Jun 1, 2022
View reviewed changes
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

I think is ok now. A missing trailing space, but for the rest is ok to me. If you squash the commits, please remove the trailing space too ;)

js/graph_config.js Outdated
}
case 'GPS_RESCUE_HEADING':
switch (fieldName) {
case 'debug[0]': // Rescue yaw rate deg/s * 10 up to +/- 90
Copy link
Member

@McGiverGim McGiverGim Jun 1, 2022

Choose a reason for hiding this comment

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

Sonar issue: trailing space at the end

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 1, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

No Coverage information No Coverage information
7.9% 7.9% Duplication

@haslinghuis haslinghuis added this to the 3.7.0 milestone Jun 23, 2022
@ctzsnooze
Copy link
Member Author

ctzsnooze commented Aug 29, 2022

Closing, replaced with #593

@ctzsnooze ctzsnooze closed this Aug 29, 2022
@ctzsnooze ctzsnooze mentioned this pull request Aug 29, 2022
Merged
@ctzsnooze ctzsnooze deleted the Fix-gps-rescue-throttle-pid-graphics-and-names branch September 27, 2022 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@McGiverGim McGiverGim McGiverGim approved these changes

@haslinghuis haslinghuis haslinghuis approved these changes

@blckmn blckmn blckmn left review comments

@KarateBrot KarateBrot Awaiting requested review from KarateBrot

Assignees

@ctzsnooze ctzsnooze

Labels

RN: IMPROVEMENT

Projects

None yet

Milestone

3.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@ctzsnooze @blckmn @McGiverGim @haslinghuis