Skip to content

Fix ESC-Sensor RPM #13286

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 13, 2024
Merged

Fix ESC-Sensor RPM #13286

merged 4 commits into from
Jan 13, 2024

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Jan 10, 2024 •
edited
Loading

erpmToHz was not intialized for ESC_SENSOR.

Just a simple fix as we need to refactor this later on as ESC_SENSOR should not be dependent on DSHOT but seems it has been that for 5 years already

IDK if ONESHOT or MULTISHOT provides ESC_SENSOR telemetry in the first place.

@haslinghuis haslinghuis added this to the 4.5 milestone Jan 10, 2024
@haslinghuis haslinghuis self-assigned this Jan 10, 2024
@github-actions GitHub Actions
Copy link

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

  • Simply put #13286 (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!

@haslinghuis
Copy link
Member Author

@sniperxnl please test

@haslinghuis haslinghuis linked an issue Jan 10, 2024 that may be closed by this pull request
ESC telemetry RPM lost in 4.5 firmware #13283
Closed
KarateBrot
KarateBrot reviewed Jan 10, 2024
View reviewed changes
@sniperxnl
Copy link

THanks @haslinghuis

It works

image

sniperxnl
sniperxnl approved these changes Jan 10, 2024
View reviewed changes
Copy link

@sniperxnl sniperxnl left a comment

Choose a reason for hiding this comment

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

Test done on the esc and it works
image

@KarateBrot
Copy link
Member

KarateBrot commented Jan 10, 2024 •
edited
Loading

You can do this instead to fix the issue for 4.5 without opening a whole new can of worms. Take a look at my example commit:
7259293

But looks like we need to do some refactoring for 4.6 to split ESC SENSOR and DSHOT code. I was not aware that ESC sensor code is dependent on Dshot specific code.

@haslinghuis
Copy link
Member Author

Yes agree this is not the time for a refactoring. Only bug fixes now until after release.

@blckmn
Copy link
Member

blckmn commented Jan 10, 2024 •
edited
Loading

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> PASS
  • 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 -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> PASS

nerdCopter
nerdCopter reviewed Jan 10, 2024
View reviewed changes
@nerdCopter
Copy link
Member

  • 6cfaa4430 logic flows as
 dShotTelem + !ESC-Sensor --> init
 dShotTelem +  ESC-Sensor --> init
!dShotTelem +  ESC-Sensor --> init
!dShotTelem + !ESC-Sensor --> do-not-init / return-if-called-from-elsewhere

  • unsure about the pt1 init. will leave for others to review/approve.

  • @sniperxnl , please retest latest commit.

@sniperxnl
Copy link

sniperxnl commented Jan 11, 2024 •
edited by nerdCopter
Loading

Do i need to reflash FC with #13286 again?

@sniperxnl
Copy link

#13286

Flashed and still works, yesterday we flown with the first version and no problems at all. All works great

@ctzsnooze ctzsnooze requested a review from nerdCopter January 13, 2024 05:58
@haslinghuis haslinghuis merged commit 90ab9f3 into betaflight:master Jan 13, 2024
@haslinghuis haslinghuis deleted the fix-esc-serial-rpm branch January 13, 2024 12:42
lida2003 pushed a commit to Aocoda-RC/betaflight that referenced this pull request Jan 15, 2024
@haslinghuis @lida2003
Fix ESC-Sensor RPM (betaflight#13286)
b47b4e1 
* Fix ESC-Sensor RPM

* Fixes per review from KarateBrot

* Fixes per review from NerdCopter

* Forgot to add init.c
freasy pushed a commit to freasy/betaflight that referenced this pull request Jan 22, 2024
@haslinghuis
Fix ESC-Sensor RPM (betaflight#13286)
57b8461 
* Fix ESC-Sensor RPM

* Fixes per review from KarateBrot

* Fixes per review from NerdCopter

* Forgot to add init.c
davidbitton pushed a commit to davidbitton/betaflight that referenced this pull request Feb 5, 2024
@haslinghuis @davidbitton
Fix ESC-Sensor RPM (betaflight#13286)
263f419 
* Fix ESC-Sensor RPM

* Fixes per review from KarateBrot

* Fixes per review from NerdCopter

* Forgot to add init.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ctzsnooze ctzsnooze ctzsnooze approved these changes

@sniperxnl sniperxnl sniperxnl approved these changes

@KarateBrot KarateBrot KarateBrot approved these changes

@sugaarK sugaarK sugaarK approved these changes

@blckmn blckmn Awaiting requested review from blckmn

@SteveCEvans SteveCEvans Awaiting requested review from SteveCEvans

@ledvinap ledvinap Awaiting requested review from ledvinap

@McGiverGim McGiverGim Awaiting requested review from McGiverGim

@MJ666 MJ666 Awaiting requested review from MJ666

@klutvott123 klutvott123 Awaiting requested review from klutvott123

@nerdCopter nerdCopter Awaiting requested review from nerdCopter

Assignees

@haslinghuis haslinghuis

Labels
RN: BUGFIX Tested
Projects
Finalizing Firmware 4.5 Release
Status: COMPLETED
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

ESC telemetry RPM lost in 4.5 firmware
7 participants
@haslinghuis @sniperxnl @KarateBrot @blckmn @nerdCopter @ctzsnooze @sugaarK