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

Added demag and stall events to edt status frame #12170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

damosvil
Copy link
Contributor

@damosvil damosvil commented Jan 10, 2023

This pr implements esc alert, warning and error events over edt. In the current commit these events are printed by console with the dshot_telemetry_info command and in the osd esc warning. A small rework has also been done to humanize the previous edt code so literals are replaced by macro identifiers.

The following Bluejay pr in the works with a branch that can be used this new functionality: bird-sanctuary/bluejay#64

@damosvil
Copy link
Contributor Author

damosvil commented Jan 10, 2023

Updated EDT spec can be found here: https://github.com/bird-sanctuary/extended-dshot-telemetry/blob/main/README.md

@blckmn
Copy link
Member

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

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@nerdCopter
Copy link
Member

rebase from master and continued testing please. followed back here from checking open Configurator PR's

src/main/drivers/dshot.h Outdated Show resolved Hide resolved
src/main/msp/msp.c Outdated Show resolved Hide resolved
src/main/msp/msp.c Outdated Show resolved Hide resolved
@damosvil damosvil force-pushed the edt_events branch 2 times, most recently from c64fd01 to f07c366 Compare December 5, 2023 18:26
@haslinghuis haslinghuis modified the milestones: 4.5, 4.6 Dec 5, 2023
// Update debug buffer
if (motorIndex < motorCount && motorIndex < DEBUG16_VALUE_COUNT) {
// Update debug buffer (motorIndex < motorCount guaranteed by caller)
if (motorIndex < DEBUG16_VALUE_COUNT) {
Copy link
Member

Choose a reason for hiding this comment

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

Just grepping some code here.

#define DEBUG16_VALUE_COUNT 8

🤔 should we only update for motorCount as some code seems dubious in how it uses the current amount of assigned motors:

for (int motorIndex = 0; motorIndex < MAX_SUPPORTED_MOTORS && motorIndex < motorCount; motorIndex++) {

Not letting motorCount exceed MAX_SUPPORTED_MOTORS would take out the first condition in several loops. This loop is used 11 times at minimum (in master)

Like to understand this statement as the if condition would not be needed:

// Update debug buffer (motorIndex < motorCount guaranteed by caller)
if (motorIndex < DEBUG16_VALUE_COUNT) {

Copy link
Contributor Author

@damosvil damosvil Dec 11, 2023

Choose a reason for hiding this comment

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

That code (inside dshot_decode_telemetry_value) is called from updateDshotTelemetry function, in a loop that that checks all existing motors.
The only place in the whole project where dshot_decode_telemetry_value is called is in this line:
imagen

@ctzsnooze
Copy link
Member

Daniel I think its best for all of us not to add this code to 4.5. It is very new, and there is a risk that we introduce issues.
We will keep working together on it for 4.6.
As soon as 4.5 is final, users can test/use this with 4.6zulu just by including the PR number in the build.

@ctzsnooze ctzsnooze modified the milestones: 4.5, 4.6 Dec 8, 2023
@damosvil
Copy link
Contributor Author

damosvil commented Dec 11, 2023

Daniel I think its best for all of us not to add this code to 4.5. It is very new, and there is a risk that we introduce issues. We will keep working together on it for 4.6. As soon as 4.5 is final, users can test/use this with 4.6zulu just by including the PR number in the build.

I understand your worries, but I am not sure I will be available in the following months to keep the code, so I would prefer to go for a revision process and add this feature during this version. After all the pr has been there for many months.

@damosvil damosvil closed this Dec 11, 2023
@damosvil
Copy link
Contributor Author

bad button

@damosvil damosvil reopened this Dec 11, 2023
Fixed demag osd_warning

Added fix for incorrect RPM debug values writtings in debug buffer

Updated dshot status frame

Added debug capabilities to dshot edt frames. DBG3 edt frame becomes demag metric frame

DSHOT debug data packs 5 signals in a value. Added command to check debug data

Added retries to activate EDT, because sometimes EDT is not successfully activated

Added debug modes to be in sync between betaflight, betaflight-configurator and blackbox-explorer. Some of others will be deleted on future merge

Added dshot_edt to MSP_MOTOR_CONFIG frame.
Added dshot_edt setting to MSP_SET_MOTOR_CONFIG frame

Removed debugging function added to cli.
Fixed automated unit tests

Added max demag metric warning to osd warnings

Improved wording to make EDT firmware/hardware agnostic

Merged to master

Updated debug.c that was left behind during rebasing

Fixed compilation bug.
Reduce EDT enable commands to 1.
Replaced literal by identifier to simplify edt enable response understanding

Updated esc osd warnings to only notify errors. Warnings and alerts are stored in blackbox.

Fixed dshot bitbang module for AT32 targets

Fixed review findings
Copy link
Contributor

@ledvinap ledvinap left a comment

Choose a reason for hiding this comment

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

A lot of code duplication in this PR. It needs some cleanup

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

Successfully merging this pull request may close these issues.

None yet

7 participants