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 dshot_telemetry_start_margin setting #12912

Merged
merged 1 commit into from Jul 10, 2023

Conversation

SteveCEvans
Copy link
Member

#12612 introduced an optimisation where the number of cycles of preamble preceding the DSHOT telemetry was counted and recorded, and then on subsequent cycles the code could skip forwards that count, less a margin, in the input buffer to reduct CPU load. This virtually eliminated dropped DSHOT frames.

Whilst this works with BLHeli32 with a margin of 10 cycles it has been noted by @limon and in a conversation at https://discord.com/channels/868013470023548938/1120926144682791064 that AM32 suffers from high error rates at high RPM. It has been observed that the time from the DSHOT command being sent to the start of telemetry reception varies randomly from approx 30us down to 23us at higher RPM.

The margin therefore needs to be increased on AM32.

The dshot_telemetry_start_margin has a default value of 10 to match current behaviour and a valid range of 0 to 100. A value less than 4 causes errors to rise with BLHeli32.

@limon please test with AM32 and increase this value to see if it will fix the errors.

If you debug with DSHOT_TELEMETRY_COUNTS and look at debug 3 on the sensors tab you'll see the max valid value beyond which no preamble skipping is done. For BLHeli32 and DSHOT300 this is 29, and for DSHOT150 it's 16.

@github-actions
Copy link

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #12912 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@limonspb
Copy link
Member

limonspb commented Jun 22, 2023

AM32 Skystar AK55A ESC:
This is debug[3] for the dshot_telemetry_start_margin 10:
image

when set dshot_telemetry_start_margin = 30:
image

@limonspb
Copy link
Member

Values of 60-70 shows 0.3% errors.
Value of 100 still gives 0-0.3% errors

@blckmn
Copy link
Member

blckmn commented Jun 22, 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 -> FAIL
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@ASDosjani
Copy link
Contributor

I tested it with NEUTRONRCF435MINI aio and it did solve the problem that I could only arm once but no matter what dshot_telemetry_start_margin I set (I tried 10,30,50,100) it will be always some dshot error. 0-0.2% as I saw.

@ASDosjani
Copy link
Contributor

By the way, atbetaflight couldn't solve this either.
image

@ASDosjani
Copy link
Contributor

ASDosjani commented Jun 29, 2023

Update: I flashed AM32 1.99 (development version), according to Alka there are some dshot timing improvements in it and dshot_telemetry_start_margin = 20 works with 0% error now.

@haslinghuis
Copy link
Member

@ASDosjani We need to check on Bluejay, BLHeli32 and AM32.

@ASDosjani
Copy link
Contributor

@ASDosjani We need to check on Bluejay, BLHeli32 and AM32.

I only have this one AT32 based FC for now so I can only test AM32.

@SteveCEvans
Copy link
Member Author

@haslinghuis So, should we merge this as a DSHOT debug aid, or just leave it here?

@haslinghuis haslinghuis added Development Instrumentation To be removed before a release and removed RN: BUGFIX labels Jul 2, 2023
@haslinghuis
Copy link
Member

Agree this would help with support - being able to find right margin. We can always remove if there is no need or other solution in place.

@SteveCEvans
Copy link
Member Author

Just flashed a MAMBAF722 stack with BLHeli 32.7 and was seeing ~1% errors, but with 32.9 there are no errors.


// Attempt to skip the preamble ahead of the telemetry to save CPU
if (startMargin > motorConfig()->dev.telemetryStartMargin) {
preambleSkip = startMargin - motorConfig()->dev.telemetryStartMargin;
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should create a static for motorConfig()->dev.telemetryStartMargin for optimization. Not sure how the compiler handles this.

@ctzsnooze
Copy link
Member

@SteveCEvans not directly related to this PR, but there were / are issues with turtle mode that are fixed by adding some kind of delay. PR 12276 adds a delay that helps solve the problem There was a comment that suggested a better approach could be for the DShot command system to identify when it is 'ready' to reverse its operating mode. It's a bit over my head, and just a simple delay may be sufficient, but I wonder if it's related to this PR in any way?

@blckmn blckmn merged commit be96529 into betaflight:master Jul 10, 2023
21 checks passed
@limonspb
Copy link
Member

@SteveCEvans @haslinghuis does this need to be also in 4.4-maintenance?

@hydra
Copy link
Contributor

hydra commented Jul 27, 2023

Regardless of the ESC firmware used, the BF could should always be able to get a TLM response sent within the allowed timeframe.

If ESC software is not sending the response at the correct time that the ESC software should be updated so that all flight control software, BF included, doesn't have to work around sub-optimal implementations.

For ESCape32, the shortest TLM response period I could capture with my scope is as follows:

DS1Z_QuickPrint27
26us.

The longest TLM response period I could capture was this:
DS1Z_QuickPrint28
at 26.7us.

The difference between the shortest and longest is probably due to the 1 or 0 as the last bit of the dshot command and the resulting difference in signal.

The DSHOT bi-directional telemetry protocol places very strict timing constraints on FC and they /MUST/ be observed by ESC software.

The above scope traces were captured from M1, while M4 was at non-zero throttle:

image

@hydra
Copy link
Contributor

hydra commented Jul 27, 2023

The same timings and errors occur given this situation:

image

@MJ666
Copy link
Contributor

MJ666 commented Jul 31, 2023

Have a new build with Seedybee F7 V3 stack and BlHeli32 32.9. This exposed DShot telemetry errors mainly on motor 2 (0.3-0.6% with all motors off) with 4.4.2. This going up to high as 30% during another motor was running but went away when motor 2 was also running. I only see errors for the motor 2 not for any of the others. After updating to 4.4.3-zulu and applying 30 to dshot_telemetry_start_margin the problem went away,

I have a similar build with an OmnibusF4 and BlueJay 0.20 and there is no dshot telemery problem shown with 4.4.2. This was falling out of the sky with 4.4.2 and BLHeli_S 16.7 (no bidirectional dshot) a few times before. The symptoms look like one motor failed in the air and the copter was start looping uncontrolled. With this build i only had 10-15 flight before and it was running on 4.3.x and 4.4.0 for some of this flights. The problem started after updating to 4.4.2 and i could hear motor twitching after this update. After the rebuild and changed the configuration (BlueJay and bidirektional dshot) motors are running smooth again and i had already two good flights.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development Instrumentation To be removed before a release
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

None yet

8 participants