-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
Extended dshot telemetry data parsing and presentation #625
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
Replaced tabs by spaces in modified files |
This comment has been minimized.
This comment has been minimized.
@damosvil I don't understand too mucho about ESCs but an important question to get to the best solution:
I say that because package it only in debug fields, will "hide" it from the major part of users, except if they configure this debug fields in the firmware and look at it expressly in the blackbox.
Regards! |
@damosvil looking deeply at the code, it seems you are using the bookmarks feature to mark them? Maybe you can publish some screen capture about how they look? |
This comment has been minimized.
This comment has been minimized.
At this time I choose to draw the EDT selected magnitude in the graph, but I am thinking on printing a mark in the temperature graph for desync events and a blue vertical line from top to botom for stall events. Demag events are pretty usual so I would let them in the info to the right, but I would not print them. Max demag metric is interesting to be checked at the end of the flight, so I would not print this number in the graph, it can be simply cheched at the end of the flight. Other option is to print a mark when it is bigger than 9. |
In general all this data is interesting to technical users that want to know how the esc/motors are going during flight. |
Ok, understood. So the major part of values from the ESC are debug values and must be shown when the users logs it. As you say this PR is ok for that. But it seems there are some information that can be important to show (maybe desync/stall events? can you confirm?) It seems to me, by your description, that they must not happen and if it does, is a good idea to inform the user. Am I right? If yes, I think they must be shown as events in the timeline. Is similar to what we do with the flight modes, that we draw a "flag/mark" in the timeline: |
Looking at the code, the |
@damosvil please remove dependency changes as they are not required for your (excellent) work. |
@haslinghuis I removed the changes over package.json and yarn.lock. Please, let me know if I have to change anything else. |
This comment has been minimized.
This comment has been minimized.
/* | ||
* Propagates array data fiels over empty fields | ||
* Debug fields can be arrays with data, so it fills data gaps when no arrays are stored | ||
*/ | ||
function propagateArrayFields(chunks) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too sure about this. I think in the rest of the code we simply store the latest value, and add them when we process other kind of data, to add the values while processing. Maybe is clear with your method, with a little more process time (it needs to look at all the data chunk).
Can you look at the slow frames and gps frames chunks to see if they can be "filled" with this and remove the old implementation? Taking times of this method will be great to see if we loose too much time. If it takes not too much time, I prefer your implementation.
When there are errors in the blackbox (frames lost), this method will fill the data? I think remember that not, that we don't have data in the chunk when the frames are lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you look at the slow frames and gps frames chunks to see if they can be "filled" with this and remove the old implementation? Taking times of this method will be great to see if we loose too much time. If it takes not too much time, I prefer your implementation.
I have been looking for the current implementation. Is it the complete frame function set?
When there are errors in the blackbox (frames lost), this method will fill the data? I think remember that not, that we don't have data in the chunk when the frames are lost.
Still looking into this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this been resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@McGiverGim are you okay with this - for a follow up PR?
1b23437
to
e219afd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
49b771d
to
ea0c470
Compare
This comment has been minimized.
This comment has been minimized.
Found a workaround to plot esc signal 4 Added basic ESC plot functionality updated project files Implemented array fiels propagation over empty fields Fixed bad FLIGHT_LOG_FIELD_ENCODING_PACK_1F_1F_1F_1G_4U_8U decoding Removed tabs on changed files Fixed some more review findings Updated DSHOT_STATUS_N_ERPM_FRACTION_18 graph ranges Reverted changes in package.json and yarn.lock Improved wording and graph ranges Updated concatenations to template literals If a field contain array values always use the last field for graph Added DSHOT motor events to graph Fixed review findings Added demag events to graph Changed speciffic Bluejay wording by general esc one Fixed sonarcloud false positive Updated debug modes to match the ones in bf master Added debug mode to match edt_events Betaflight branch Removed duplicated BARO graph config Fixed review findings Using let instead of var Refactored if to meet sonarcloud advise
|
Do you want to test this code? Here you have an automated build: |
please merge master or rebase master if this is still a desirable PR. if the pre-requisite (firmware) PR's are not to be continued, then please close this PR. |
@nerdCopter is there any way I can take over this PR? I don't think Daniel will be able to work on this any time soon. I could fork his changes and submit a new PR, unless you have a better idea? |
@stylesuxx , give me a short stint of time, i can update it and push. if it is not up to par and needs further coding, then i can hand it off. |
@stylesuxx , okay, i can easily update the branches but it turns out i cannot push to the existing PR's. If you would like to take over all 3 PR's, please be our guest. Do note that rather than MSP semver The idea here is to checkout these branches and either merge-master or rebase-master (your choice) then make new PR's from them. Although i'm certainly able to do the same, i presume there may need further technical changes from reviews that i will not be competent to accomplish. TYVM. |
This work will be continued in a new PR. |
This pr adds EDT parsing and presentation functionality to blackbox explorer.
EDT data is packed in debug fields in the following way:
This pr is related to:
bird-sanctuary/bluejay#64
betaflight/betaflight#12170
betaflight/betaflight-configurator#3262
ESC temperature log files for testing:
BTFL_BLACKBOX_LOG_Meteor65_20230201_183226.zip
BTFL_BLACKBOX_LOG_Meteor65_20230202_183845.zip
ESC demag metric files for testing:
BTFL_BLACKBOX_LOG_Meteor65_20230203_224822.zip
ESC rpm log files for testing:
BTFL_BLACKBOX_LOG_Meteor65_20230203_232019.zip