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

Allow for GPS Data in Table/Graph/Exports #606

Merged
merged 21 commits into from
Nov 23, 2022

Conversation

LinusThorsell
Copy link
Contributor

@LinusThorsell LinusThorsell commented Nov 21, 2022

PR for Issue #602

Opening PR to discuss changes required for this feature to be completed.

@LinusThorsell
Copy link
Contributor Author

LinusThorsell commented Nov 21, 2022

[OLD] Status:
Display data is being logged to lastGPS array.
Data is being displayed in the Table and on the Graph, BUT out of sync from the other data.

If you want to help developing this feature please post here.

@McGiverGim
Copy link
Member

@LinusThorsell how do you know the data is out of sync of the other data? Any easy way to see that?

@github-actions

This comment has been minimized.

@LinusThorsell
Copy link
Contributor Author

LinusThorsell commented Nov 21, 2022

@LinusThorsell how do you know the data is out of sync of the other data? Any easy way to see that?

@McGiverGim
Mainly assuming that it is, since the currently logged GPS Altitude data is out of sync with the new GPS Altitude data from the log I currently grab from the G frame.

image

If you have a better log file then feel free to test it and confirm or deny my assumptions!

(there is also another issue that the GPS heading value is not being properly parsed right now I think, but that is not priority 1 at the moment.)

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 didn't touch the Blackbox since a long time ago, and I don't understand totally your code but I thing you are doing one thing wrong:

  • The blackbox have two kind of data. One is directly read from the log. Let's call it direct data. The other is "calculated" based on the direct data because the firmware does not write it.
  • You are storing the data of the GPS, and later you are calculating it. I think this complicates the process, is better to store the data in the "standard" chunk and do nothing with the calculated fields.

I think your problem comes from here, you are not taking into account the size of the chunk in all the places, but better we can start cleaning the code with my suggestions and continue from here.

js/flightlog.js Outdated Show resolved Hide resolved
js/flightlog.js Outdated Show resolved Hide resolved
js/flightlog.js Outdated Show resolved Hide resolved
@LinusThorsell
Copy link
Contributor Author

There we go, implemented all the suggestions you had @McGiverGim

I think that the changes i pushed should make it work in theory, and it is almost working.

What is currently working:
GPS-Data is being merged into the frame in the same way as the slow frames.
GPS-Data is being input into the fieldName array.

What is left to figure out:
The table/graph gets the wrong values still. Look at this image:
bild
The entries in the frame that are interesting are frames 39 -> 45 as per this image:
bild
Which come from the G frame like this:
bild

But the current issue is that the values that the application reads from the frame is incorrect, for example the gps_numSat is reading the gps_ground_course frame data.

Anyone got any ideas for how this could happen and what the fix is?

@McGiverGim
Copy link
Member

I'm out of the computer. I will try to give another try tomorrow.

@github-actions

This comment has been minimized.

@LinusThorsell
Copy link
Contributor Author

LinusThorsell commented Nov 21, 2022

Update: Functionality is completed.
At this point the feature is working as intended.

Only things that I require assistance with before I am happy to merge:

  • At the moment there are 2 GPS Altitude fields in the output, one is seemingly from a 'debug' field? Do we remove the debug one? and if so where is it stored?
  • Do we want to store GPS home location in the table?

Things I am going to finish tomorrow:

  • Remove the duplicate time dialog.
  • Handle the H frame by itself.
  • Document the code.

However, the code in its current state should be fully functional, so feel free to test it out.

Will update the PR once I am happy to merge!

Pleasure working with you guys!

@haslinghuis
Copy link
Member

Please rebase the PR

js/flightlog.js Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@ctzsnooze
Copy link
Member

@LinusThorsell this is AMAZING!

Congratulations!

I can see that the GPS altitude being reported here is absolute altitude. Whereas the flight code, after some recent changes, shows 'relative to home point' altitude in the OSD, and logs that value in the debugs, after the home point is set.

So now we have to decide whether we are interested in absolute, or relative, altitude.

In most cases I think it is best to see 'relative to takeoff point' altitude. It's a lot easier to interpret and compare to our debugs.

What do you guys think? If we know the home point, we can transmit absolute altitude to the log, and do the subtraction of the home altitude in the log explorer. Or, in firmware, we can report absolute altitude until armed, then relative altitude.

@ctzsnooze
Copy link
Member

Here is a graph showing the GPS Altitude from this PR in cyan, and the smoothed GPS altitude and Baro altitude from the debugs. Data is from the file above.

The offset is just because we zero the value to home point in the debug.

Altitude values from this PR are PERFECT !!!!

NB: Having the GPS values in the log itself will make it much easier for us to set up position hold things since we can see the raw data.

Screen Shot 2022-11-22 at 09 09 53

@ctzsnooze
Copy link
Member

ctzsnooze commented Nov 22, 2022

After line 355, in graph_config.js, adding this will scale GPS_ground_course so that 0 degrees is at the bottom, and 360 degrees at the top, as we did for the other heading displays.

            } else if (fieldName == "GPS_ground_course") {
                return {
                    offset: -1800,
                    power: 1.0,
                    inputRange: 1800,
                    outputRange: 1.0
                };

and likewise this would scale GPS_numSat so that zero is at the bottom, and 20 in the middle:

            } else if (fieldName == "GPS_numSat") {
                return {
                    offset: -20,
                    power: 1.0,
                    inputRange: 20,
                    outputRange: 1.0
                };

and, finally, GPS_speed, if set like this, will give a range from 0 to 10m/s before clipping at the top.

            } else if (fieldName == "GPS_speed") {
                return {
                    offset: 0,
                    power: 1.0,
                    inputRange: 1000,
                    outputRange: 1.0
                };

Altitude seems OK.
If the user wants Altitude or Speed to be zoomed in or out, they can adjust he zoom percentage, e.g. setting 50% for speed will then not clip until the speed exceeds 20m/s.

Changed GPS Ground Course to GPS Heading
Changed GPS numSat to GPS Sat Count
Changed gps heading to return from 180 to -180 degrees.
js/flightlog.js Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@LinusThorsell
Copy link
Contributor Author

@McGiverGim @haslinghuis @ctzsnooze @blckmn

I am happy with the feature as it is now.

The only issue I know of is that there are still two GPS Altitude fields, I just changed one of them to reported altitude until we have a better solution. What do you guys think of that?

Feel free to look over the changes and test the code.

I'm giving it my personal LGTM!

@github-actions

This comment has been minimized.

@ctzsnooze
Copy link
Member

ctzsnooze commented Nov 22, 2022

@LinusThorsell Re: "The only issue I know of is that there are still two GPS Altitude fields"....

My first thought was to leave the name of the 'stock' GPS Altitude field - the one every GPS user will see 'normally' in their logs as "GPS Altitude".

However, it may be worth re-naming, since the value displayed is currently a 'raw', non-zeroed, absolute or "Above Seal Level" (ASL) GPS value, updated at whatever frequency the GPS unit sends data.

And while the Altitude debug also reports a value with the name "GPS Altitude", that value is the resampled GPS value from position.c. It is 'zeroed' when the GPS system sets its HOME position, and is directly comparable to Baro Altitude in that debug. For that debug, it is nice to just see "GPS Altitude" and "Baro Altitude".

It would be good if, in a separate PR, we would parse the GPS 'H' fields, even if we don't graph them, since they would provide the offset value required for zeroing the ASL value.

Hence perhaps it would be best if we could re-name the 'stock' or everyday GPS Altitude value to GPS Altitude ASL, reflecting its absolute or 'above sea level' meaning.

Once we have GPS 'H' field parsing, the log viewer could, possibly, provide a derived field called GPS Altitude Zeroed, or something like that, as an alternative display element.

@sonarcloud
Copy link

sonarcloud bot commented Nov 23, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@LinusThorsell
Copy link
Contributor Author

@LinusThorsell Re: "The only issue I know of is that there are still two GPS Altitude fields"....

My first thought was to leave the name of the 'stock' GPS Altitude field - the one every GPS user will see 'normally' in their logs as "GPS Altitude".

However, it may be worth re-naming, since the value displayed is currently a 'raw', non-zeroed, absolute or "Above Seal Level" (ASL) GPS value, updated at whatever frequency the GPS unit sends data.

And while the Altitude debug also reports a value with the name "GPS Altitude", that value is the resampled GPS value from position.c. It is 'zeroed' when the GPS system sets its HOME position, and is directly comparable to Baro Altitude in that debug. For that debug, it is nice to just see "GPS Altitude" and "Baro Altitude".

It would be good if, in a separate PR, we would parse the GPS 'H' fields, even if we don't graph them, since they would provide the offset value required for zeroing the ASL value.

Hence perhaps it would be best if we could re-name the 'stock' or everyday GPS Altitude value to GPS Altitude ASL, reflecting its absolute or 'above sea level' meaning.

Once we have GPS 'H' field parsing, the log viewer could, possibly, provide a derived field called GPS Altitude Zeroed, or something like that, as an alternative display element.

Fixed the scaling in the most recent commit!

Copy link
Member

@ctzsnooze ctzsnooze left a comment

Choose a reason for hiding this comment

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

Great work! I think it's ready.
Please squash the commits.

@haslinghuis
Copy link
Member

@ctzsnooze commits will be squashed upon merging.

@github-actions
Copy link

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 added this to the 3.7.0 milestone Nov 23, 2022
@haslinghuis haslinghuis merged commit 4b20639 into betaflight:master Nov 23, 2022
Finalizing Firmware 4.4 Release automation moved this from In progress to Done Nov 23, 2022
@McGiverGim
Copy link
Member

Great work for your first PR @LinusThorsell, the next step is to draw a map with the gps path :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants