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

Resolved issue of wrong throttle values at spectrum chart when not all log parameters are selected in blackbox settings #713

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

demvlad
Copy link
Contributor

@demvlad demvlad commented Mar 5, 2024

The issues description:
The wrong throttle values at spectrum chart.

This issue appears if to disable some blackbox fields in list at Blackbox page of Betaflight Configurator 4.5 version.
For example - if enabled the gyro, gyro unfiltered and SetPoints only.
There is log file to test this.
test.TXT

The reason of this issue - the difference of logical conditions what are permissed creating of additional computing fields and filling data into it.
(the functions buildFieldNames() and injectComputedFields() in flightlog.js file).
There are the incongruity fields indexes in log algoritmes in this case. As result - We are seeing the scaled yaw setpoint values instead of throttle in my example.
This PR resolved this issue.

This comment has been minimized.

@demvlad demvlad changed the title The conditions for permissions of calculated fields are synchronized by adding the fields names and by set their values Resolved issue of wrong throttle values at spectrum chart when not all parameters are selected in blackbox settings Mar 5, 2024
@demvlad demvlad changed the title Resolved issue of wrong throttle values at spectrum chart when not all parameters are selected in blackbox settings Resolved issue of wrong throttle values at spectrum chart when not all log parameters are selected in blackbox settings Mar 5, 2024
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.

Some minor changes. Otherwise LGTM.

js/flightlog.js Outdated Show resolved Hide resolved
js/flightlog.js Outdated Show resolved Hide resolved
js/flightlog.js Outdated Show resolved Hide resolved
js/flightlog.js Outdated Show resolved Hide resolved
demvlad and others added 4 commits March 5, 2024 19:15
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
Code style improvement

Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
haslinghuis
haslinghuis previously approved these changes Mar 5, 2024

This comment has been minimized.

@demvlad demvlad requested a review from haslinghuis March 5, 2024 16:24
@demvlad
Copy link
Contributor Author

demvlad commented Mar 5, 2024

Some minor changes. Otherwise LGTM.

Ok. Thank's.

@nerdCopter
Copy link
Member

do you prefer !( || ) or ( && ) for readability here? (two instances)

@haslinghuis
Copy link
Member

Morgan's law :)

@demvlad
Copy link
Contributor Author

demvlad commented Mar 5, 2024

do you prefer !( || ) or ( && ) for readability here? (two instances)

I prefer !a &&. !b
One moment, will change..

nerdCopter
nerdCopter previously approved these changes Mar 5, 2024
Copy link
Member

@nerdCopter nerdCopter left a comment

Choose a reason for hiding this comment

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

  • approving, but only tested few logs for not-crashing and expected traces.
  • not tested with logs missing parameters except the provided test.TXT.

@demvlad demvlad dismissed stale reviews from nerdCopter and haslinghuis via e6e8bdc March 5, 2024 17:16
haslinghuis
haslinghuis previously approved these changes Mar 5, 2024
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.

Easier to read now :)

@nerdCopter
Copy link
Member

nerdCopter commented Mar 5, 2024

I prefer !a &&. !b One moment, will change..

so the proper gate is not-disabled-gyro && not-disabled-pid ? i think such does make sense.
is this for line 684 as well, i see the commit did not include such.

@demvlad
Copy link
Contributor Author

demvlad commented Mar 5, 2024

Easier to read now :)

morgan law is too clever:)

This comment has been minimized.

Copy link

sonarcloud bot commented Mar 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@demvlad
Copy link
Contributor Author

demvlad commented Mar 5, 2024

  • approving, but only tested few logs for not-crashing and expected traces.
  • not tested with logs missing parameters except the provided test.TXT.

Yes, this is a thin moment.
The index definition in one place and index using in other with some conditions. I've tryed to do identity conditions. But it is need to test this with different blackbox parameters set combination.

Copy link

github-actions bot commented Mar 5, 2024

Do you want to test this code? Here you have an automated build:
Betaflight-Blackbox-Explorer-Linux
Betaflight-Blackbox-Explorer-macOS
Betaflight-Blackbox-Explorer-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@haslinghuis haslinghuis merged commit cb53f08 into betaflight:master Mar 5, 2024
5 checks passed
@demvlad demvlad deleted the log_computed_data_issues branch March 5, 2024 17:46
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.

None yet

3 participants