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

Add missing debugs #671

Merged
merged 2 commits into from
Dec 3, 2023
Merged

Conversation

haslinghuis
Copy link
Member

No description provided.

@blckmn
Copy link
Member

blckmn commented Dec 2, 2023

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 -> PASS
  • 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

@ctzsnooze
Copy link
Member

If the debug is 'new', it only needs to be added to the DEBUG_FRIENDLY_FIELD_NAMES_INITIAL list.

We do not make new debugs conditional on a semver check. A server check is only needed if the original field names in DEBUG_FRIENDLY_FIELD_NAMES_INITIAL have been updated in a later firmware version to a new set of field names.

These debugs are all completely new:

    "FAILSAFE",
    "GYRO_CALIBRATION",
    "ANGLE_MODE",
    "ANGLE_TARGET",
    "CURRENT_ANGLE",
    "DSHOT_TELEMETRY_COUNTS",
    "RPM_LIMIT",
    "RC_STATS",
    "MAG_CALIB",
    "MAG_TASK_RATE",

Therefore, we should enter their field names in the DEBUG_FRIENDLY_FIELD_NAMES_INITIAL list, appending them them after GPS_DOP.

If new debugs are appended same order that they appear in the firmware code, ie the above order, they match the debug order in the firmware, making it easier to follow.

case 'CURRENT_ANGLE':
return value.toFixed(0);
case 'DSHOT_TELEMETRY_COUNTS':
return value.toFixed(0);
Copy link
Member

@ctzsnooze ctzsnooze Dec 2, 2023

Choose a reason for hiding this comment

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

Later, perhaps in a different PR, we can set the display value and units for values in these debugs.

@ctzsnooze
Copy link
Member

Could I suggest adding these graph settings into graph_config.js, for Mag_Calib and Mag_Task_rate:

                   case 'MAG_CALIB':
                        switch (fieldName) {
                            case 'debug[0]': // X
                            case 'debug[1]': // Y
                            case 'debug[2]': // Z
                            case 'debug[3]': // Field
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 2000,
                                    outputRange: 1.0,
                                };
                            case 'debug[4]': // X Cal
                            case 'debug[5]': // Y Cal
                            case 'debug[6]': // Z Cal
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 500,
                                    outputRange: 1.0,
                                };
                            case 'debug[7]': // Lambda
                                return {
                                    offset: -2000,
                                    power: 1.0,
                                    inputRange: 2000,
                                    outputRange: 1.0,
                                };
                            default:
                                return getCurveForMinMaxFields(fieldName);
                            }
                    case 'MAG_TASK_RATE':
                        switch (fieldName) {
                            case 'debug[0]': // Task Rate
                            case 'debug[1]': // Data Rate
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 1000,
                                    outputRange: 1.0,
                                };
                            case 'debug[2]': // Data Interval
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 10000,
                                    outputRange: 1.0,
                                };
                            case 'debug[3]': // Execute Time
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 20,
                                    outputRange: 1.0,
                                };
                            case 'debug[4]': // Bus Busy Check
                            case 'debug[5]': // Read State Check
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 2,
                                    outputRange: 1.0,
                                };
                            case 'debug[6]': // Time since previous task uS
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 10000,
                                    outputRange: 1.0,
                                };
                            default:
                                return getCurveForMinMaxFields(fieldName);
                            }

@ctzsnooze
Copy link
Member

Also in graph_config for GPS_DOP this gives consistent scaling of the DOP values:

                    case 'GPS_DOP':
                        switch (fieldName) {
                            case 'debug[0]': // Number of Satellites (now this is in normal GPS data, maybe gpsTrust?)
                            case 'debug[1]': // pDOP
                            case 'debug[2]': // hDOP
                            case 'debug[3]': // vDOP
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 200,
                                    outputRange: 1.0,
                                };
                            default:
                                return getCurveForMinMaxFields(fieldName);
                        }

@haslinghuis haslinghuis added this to the 3.7.0 milestone Dec 2, 2023
@haslinghuis haslinghuis force-pushed the add-missing-debugs branch 6 times, most recently from f83c151 to e349b10 Compare December 2, 2023 19:20
ctzsnooze
ctzsnooze previously approved these changes Dec 3, 2023
@ctzsnooze
Copy link
Member

Looks good!

Unfortunately we missed out on the GPS_UNIT_CONNECTION debug, which goes just before ATTITUDE, and is used to debug a GPS module connection.

Details of what is needed is in PR 645 .

PS... I have a feeling that the name of every debug is supposed to be in the big list in DEBUG_MODE_COMPLETE in js/flightlog_fielddefs.js

@ctzsnooze
Copy link
Member

Please also use the angle mode graph scaling factors for ANGLE_MODE from PR 620, which are simply:

              case 'ANGLE_MODE':
                        switch (fieldName) {
                            case 'debug[0]': // angle target
                            case 'debug[3]': // angle achieved
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 1000,
                                    outputRange: 1.0,
                                };
                            case 'debug[1]': // angle error correction
                            case 'debug[2]':  // angle feedforward
                                return {
                                    offset: 0,
                                    power: 1.0,
                                    inputRange: 5000,
                                    outputRange: 1.0,
                                };
                            default:
                                return getCurveForMinMaxFields(fieldName);
                            }
     ```
     

'debug[0]': 'Mag X',
'debug[1]': 'Mag Y',
'debug[2]': 'Mag Z',
'debug[3]': 'Norm / Length of magADC',
Copy link
Member

Choose a reason for hiding this comment

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

This can be named 'Field Strength` I think.

'debug[4]': 'Estimated Mag Bias X',
'debug[5]': 'Estimated Mag Bias Y',
'debug[6]': 'Estimated Mag Bias Z',
'debug[7]': 'Mag Bias Estimator',
Copy link
Member

Choose a reason for hiding this comment

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

This can be called 'Lambda', it represents an estimate of the precision of the estimate of the bias values.

Copy link

sonarcloud bot commented Dec 3, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

No Coverage information No Coverage information
43.9% 43.9% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@ctzsnooze
Copy link
Member

Looks really, really good now. Fantastic!
Thank you very much @haslinghuis

@haslinghuis haslinghuis merged commit 31e6234 into betaflight:master Dec 3, 2023
5 of 6 checks passed
@haslinghuis haslinghuis deleted the add-missing-debugs branch December 3, 2023 12:10
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.

3 participants