Add RPM field to blackbox logs

[tbolin]

Adds a separate field for motor RPM from dshot telemetry.
Will only log the existing motors and only if bidirectional dshot is activated, and leaves the debug field free to use for other things.
Can also be disabled just like other black box fields.

Work in progress.
The code is working but there might still be some quirks with both how the rpm values are retrieved and the black box logging itself.

Corresponding PRs
Blackbox Explorer: betaflight/blackbox-log-viewer#633
Configurator: betaflight/betaflight-configurator#3392

Some of the extended Dshot telemetry features from #12170 should probably be included at some point, but not sure if it will be in this PR.

Data

rpm_bb_stress.zip
Log from a F411 with all standard BB fields and 8 debug fields enabled running at 3205 Hz. No dropped frames until the flash ran out of space.

blackbox_rpm_new_format_009_stalled_fixed.zip
blackbox_rpm_new_format_010_almost_stalled.zip
blackbox_rpm_new_format_011_almost_stalled_rpm_telemetry_debug.zip
blackbox_rpm_new_format_012_no_telemetry.zip
blackbox_rpm_new_format_013_rpm_filter_debug.zip

Todo

• Stress test on F411
• Evaluate encoding and consider filtering the data before logging (might make compression more efficient)
• Move RPM field to end of FlightLogFieldSelect_e enumerator
• Fix code formatting issues
• Check that current retrieval method does not interfere with other functionality, works if extended dshot telemetry is enabled, and does not cause unnecessary calculations to be done in the black box process.

[blckmn]

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 -> FAIL
• approver count at least three -> FAIL

[KarateBrot]

We now have 8 debug channels to show DEBUG_DSHOT_RPM_TELEMETRY for all motors :)

[McGiverGim]

The problem of adding fields if that the blackbox must keep the speed while writing all of them, if not, other data is lost or it can affect the pid loop time.

I think is good to add "general" data as fields and not let all the work to the debug field, so the PR is ok to me.

Maybe we must decide wich fields are the most important/habitual and let disabled by default, using the mask, all the others. Adding the mask to the configurator will help the users to decide what enable/disable according his preferences.

[haslinghuis]

@McGiverGim we already using the mask in configurator: betaflight/betaflight-configurator#3363

[McGiverGim]

It didn't appear with the virtual fc, I searched for it 😋

Then it's ok, maybe decide what let as defaults.

[tbolin]

We now have 8 debug channels to show DEBUG_DSHOT_RPM_TELEMETRY for all motors :)

I know, but then you can't have any other debug mode active. This field also support up to 8 motors (it should, haven't tested yet due to a lack of octo copter) but unlike the 8 debug channel version it only logs the motors that the multi rotor has, whereas the debug field will waste time and memory on writing zeros to the remaining fields.
With non-debug fields it is also possible to save some space by being clever with the encoding. I haven't looked to deeply into the BB encoding, but if I understand it correctly the encoding that the RPM field uses should get away with writing 1 byte per frame and motor a lot of the time. The debug field will always write 2. The encoding is still subject of tweaking.

Unfiltered gyros should probably also be given a real field. There we are currently wasting even more resources on zero data (and every tuning guide in existence won't have to describe how to set the black box debug mode to gyro scaled).

The problem of adding fields if that the blackbox must keep the speed while writing all of them, if not, other data is lost or it can affect the pid loop time.

I'm very aware of the possible performance problem. As I mentioned above, the field should be more efficient than using the debug field, and as you said, worst case it can just be disabled by default. The field can already be toggled from the CLI by the same mechanism as other BB fields.
Please let me know if there are any more trickery left on the firmware side to make it possible to toggle from the GUI in Configurator.

[haslinghuis]

@tbolin if it's contained in the mask we just have to add another value in the configurator. DId you check the link above?

        const debugFields = [
            { text: "PID" },
            { text: "RC Commands" },
            { text: "Setpoint" },
            { text: "Battery" },
            { text: "Magnetometer" },
            { text: "Altitude" },
            { text: "RSSI" },
            { text: "Gyro" },
            { text: "Accelerometer" },
            { text: "Debug Log" },
            { text: "Motor" },
            { text: "GPS" },
            { text: "RPM" },
        ];

        let fieldsMask = FC.BLACKBOX.blackboxDisabledMask;

        for (let i = 0; i < debugFields.length; i++) {
            const enabled = (fieldsMask & (1 << i)) === 0;
            debugFieldsSelect.append(new Option(debugFields[i].text, i, false, enabled));
        }

[haslinghuis]

This should go after GPS

[tbolin]

Should be in the correct position now

[tbolin]

if it's contained in the mask we just have to add another value in the configurator. Did you check the link above?

I did now, and I was aware of that menu. I'll move the RPM enum and try it with the configurator PR soon(ish). I was worried there were some arcane configuration step that I was not aware of but I guess I don't have to worry about that now since I'm technically not adding any variables.

[ctzsnooze]

We already get many users with gaps in their logs, and also heavy logging messes with CPU stability.
Perhaps an extra blackbox field set like RPM should be disabled by default, or we will get more people having issues when they make a 'default' log.
In effect what the PR is achieving is the enabling of the RPM telemetry debug data as 'normal' blackbox fields, and that's OK by me, in principle, but in practice we need to be careful about the additional load.

[KarateBrot]

We need to evaluate which blackbox log fields to keep/drop/add anyway. We can probably also add gyro pre-filtered, just like @tbolin recommended, and even drop some legacy blackbox fields. Disabling some bb fields by default is also a good addition, I agree.

[tbolin]

We already get many users with gaps in their logs, and also heavy logging messes with CPU stability.
Perhaps an extra blackbox field set like RPM should be disabled by default, or we will get more people having issues when they make a 'default' log.
In effect what the PR is achieving is the enabling of the RPM telemetry debug data as 'normal' blackbox fields, and that's OK by me, in principle, but in practice we need to be careful about the additional load.

I agree that some fields might have to be off by default, but I think there are better candidates than RPM.
I'll will add stress testing on a F411 to the todo.

Also, the PR for Blackbox Explorer is up now betaflight/blackbox-log-viewer#633
and the OP is updated with links to related PRs and a todo list.

[KarateBrot]

Just now saw that this logged eRPM and not actual RPM

Suggested change

	blackboxCurrent->rpm[i] = getDshotTelemetry(i);
	blackboxCurrent->rpm[i] = erpmToRpm(getDshotTelemetry(i));

[tbolin]

Just now saw that this logged eRPM and not actual RPM

That's intentional (for now at least). Logging eRPM saves a little bit of calculation on the FC and might avoid some loss of precision. The eRPM to RPM calculation is done in Black box explorer.

tl;dr for the stream of thought bellow: the exact value to be logged is still TBD.

I still haven't decided on exactly which value that should be logged. The getDshotTelemetry function raises a few alarm bells when I look at it, especially when extended dhsot telemetry might come into play. There are too much calculations and too many side effects there for my liking right now, and I'm not sure that it won't interfere with the RPM based gyro filters.
It could also be a good idea to filter the RPM signal before logging to help the compression, but then it might as well log the value used by the RPM filters. I'll have to look at how much information there is left on higher frequencies. In the very short log I have now there is just noise.

[tbolin]

I ran a stress test with a F411 logging at 3205 Hz with all fields enabled. Log and details in updated OP.
If anyone have ideas about how to do more stress testing I'm eager to hear.

[tbolin]

The encoding looks like it should fit the purpose. If there turns out to be a more efficient encoding its easy to change later.
Smoothing also seems overkill for now. Bluejay data is much smoother than BLHeli32 for some reason.

[tbolin]

I had a look at getDshotTelemetry, and I don't think using that method will interfere with anything else.
However I'm tempted to expose the raw interval in us and log that instead of the eRPM. With eRPM the step from a change of 1 in the interval is lower than 1 at low RPM, so we lose a bit of information there, and at high RPM it significantly more than one, so the compression will be struggling, especially if the signal is noisy like from BLHeli32.

Not directly related to the PR but the current pattern where the dhsot telemetry is triggered by the getShotTelemetry function should probably be changed and the function divided into a calculation and a get. Especially with extended telemetry on the way.
I'm also a bit worried that extended telemetry will interfere with the working of the RPM filters since there is no check if the retried eRPM is a new value or if it's an old value and another field was updated.

[KarateBrot]

I think it's most user friendly to log RPM directly from the filtered RPM values that are also used by the RPM Filter. If we need eRPM or something else, we still have the debug channels.

The current getDshotTelemetry() function body is an abomination, I agree. I started to refactor it after your comment.

I'm also a bit worried that extended telemetry will interfere with the working of the RPM filters since there is no check if the retried eRPM is a new value or if it's an old value and another field was updated.

That might indeed be a bit worrysome. Should still be working fine if there are some corrupted eRPM samples here and there but we should still try to avoid it, of course. Which line are you talking about specifically?

[tbolin]

@KarateBrot

I think it's most user friendly to log RPM directly from the filtered RPM values that are also used by the RPM Filter. If we need eRPM or something else, we still have the debug channels.

Storing the filtered values would be hard since they have been converted to float. The black box encodings relies quite heavily on the values being integers. Grabbing values from

Pure RPM would have the same problems as the current eRPM/100 values, just less loss of precision at low RPM but more problems with compression rate at high RPM, and right now I'm more concerned with compression than the loss of precision.

I agree that pure RPM would be a bit more friendly to those that like to read raw black box values, but I don't think that is the typical user. Since both the black box data and data rate are constrained, and it's easy to process the interval data into RPM before displaying it, I think it's better to go with the most machine friendly option here.

I'm almost done with a version using the interval, so you might want to hold of on the refactoring for a moment.

[tbolin]

Update using intervals instead of RPM pushed together with corresponding black box explorer commit.
The related fields in the firmware is still refered to as RPM, since that's what they are supposed to be interpreted as.
However the black box fields have been renamed to eInterval, both for clarity and to not interfere with the virtual RPM field created by black box explorer.

A bit more work than I thought it would be, but the compression should be slightly more efficient now, and it's possible to see very low RPMs.

[tbolin]

Pushed an update with a bunch of fixes and added some logs from testing to the first post.

[haslinghuis]

Suggested change

	if(testBlackboxCondition(CONDITION(MOTOR_1_HAS_RPM) + x)) {
	if (testBlackboxCondition(CONDITION(MOTOR_1_HAS_RPM) + x)) {

[tbolin]

fixed

[tbolin]

I'm feeling quite happy with the firmware code now, and the black box explorer works as well.

[KarateBrot]

You might want to rename all instances of "RPM" with "eInterval" where you really mean "eInterval". Also the title of this PR doesn't fit the content anymore.

[ctzsnooze]

I think that we need to look hard at how we can optimise the DShot telemetry code, so that we operate with either RPM or eRPM, but not both, to avoid swapping from one to the other many times.

[tbolin]

I realized I should do some testing and calculations before deciding which value to log.

I recorded 4 logs, 2 with Bluejay and 2 with BLHeli 32. For each ESC firmware I made one log at 1602 Hz and one at 3205 Hz.
I then calculated the number of bytes per (P) frame (per motor) that would be required for logging eIntervals, eRPM, RPM and filtered RPM with the current encoding (variable byte length with the prediction based on the previous value).

	firmware	log rate	interval (Bpf)	erpm (Bpf)	rpm (Bpf)	rpm filtered (Bpf)	erpm vs interval	rpm vs interval	rpm filtered vs interval
0	Bluejay	1602	1.00068	1.00258	1.64125	1.29566	1.00189	1.64012	1.29477
1	BLHeli32	1602	1.0062	1.0111	1.63605	1.25486	1.00488	1.62593	1.24712
2	Bluejay	3205	1.00078	1.00273	1.60655	1.12041	1.00195	1.6053	1.11954
3	BLHeli32	3205	1.00546	1.00929	1.35173	1.08262	1.00382	1.34437	1.07674

Notes

• All values in each row are from the same log. The eInterval was logged and the other values was then calculated based on the eInterval.
• The BPF values are calculated afterwards, not measured based on the size of the log file.
• The values in the table are averages from all four motors.
• The slightly lower values for BLHeli32 are likely due to less aggressive flying (had to be a bit careful with a 3.5 inch indoors).
• eRPM is really eRPM/100 which is the output of getDshotTelemetry

Based on this data I have a hard time motivating the switch from eRPM to eIntervals. The improvement is extremely minor. From some estimates I've done there shouldn't be a significant loss of precision either (IIRC the worst error in an entire log was equivalent to 0.3 Hz, the average was much lower).
Logging RPM directly required around 60% more data in most cases, and even the best case scenario was over 30%. I have a hard time motivating this increase in storage space and bandwidth given that it's very easy to convert eRPM/100 to RPM in post processing.
Filtered RPM have gives better compression than RPM, but still worse than eRPM/100. Logging filtered RPM would also depend on the RPM filter, which feels a bit confusing to me and would require care in the future so that the RPM logging is not accidentally switched off by a changing the RPM filter settings. The RPM filter also outputs rounds per second, so a conversion would be required to get RPM. (and since I'm a bit lazy I have to add that the include guards would also have to be changed).

As for the name of the field: the intention I have with the PR was that a user should be able to choose to log how fast the motors are spinning in Configurator and then have a field with that value appear in blackbox explorer.
I think RPM is a good shorthand for how motors are spinning, it's short and fairly unambiguous in this context. The user selects RPM in Configurator and gets a field named RPM in blackbox explorer, and that field shows a graph with RPM and Hz for each motor. In that sense this PR does add an RPM field to black box logs.
My intention was not necessarily to have a field named RPM containing exactly RPM values if a log was opened in a text editor or similar. It would be nice, but nowhere near the same priority as having good performance and minimizing log size.

I have two suggestions for how to proceeds with this PR:
Keep as is (with some variable renaming)
Pro:

• Least amount of (additional) work for me
• No risk of loss of precision, the logged values is as good as it gets
• Exporting as csv from blackbox explorer actually gives a field with RPM values
  Con:
• Makes changes in dshot.c, which could include bugs and will likely have some conflicts with Added demag and stall events to edt status frame #12170
• eInterval is not a commonly used unit or shorthand
• The RPM field in black box explorer is virtual (not necessarily a bad thing), and the eInterval field still hangs around.

go back to eRPM
Pro:

• Avoids having to stick my fingers into dshot.c
• eRPM is a commonly used term and better conveys the relationship to RPM compared to eInterval.
• Does not require a virtual field in black box explorer. The RPM field would be the only field added.
  Con:
• Requires more additional work and testing in both the FW and blackbox explorer
• Exported CSVs would contain eRPM/100, not RPM
• (very minor loss of precision)

I'm leaning towards going back to eRPM.

[nerdCopter]

recommend setting PR as "draft" until truly ready. same for config/bbe pr's

[tbolin]

@KarateBrot I could really use some input on this. This PR has been in limbo for long enough and I don't want to code up more changes before I've got a go ahead in some direction.

If the field in the black box log includes the unit, like RPM[eRPM/100] it would be clear to anyone looking at it that it's the RPM field, and the actual unit is eRPM/100 (or R/us or whatever). It would also mean that if the unit is changed in the future the field could be left in black box explorer for backwards compatibility, without interfering with the new field.

[github-actions]

Do you want to test this code? Here you have an automated build:
Assets
WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

[tbolin]

I don't want to code up more changes before I've got a go ahead in some direction.

That was apparently a lie. I have made an alternative PR which uses eRPM #12823
That version doesn't mess around in the dshot driver in the firmware and doesn't require the creation of any virtual fields in black box explorer. The variable and field naming should also be a bit more consistent with their content.

[tbolin]

Closing this PR since I prefer #12823