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

Refactor beeper #13492

Merged
merged 7 commits into from
May 16, 2024
Merged

Refactor beeper #13492

merged 7 commits into from
May 16, 2024

Conversation

ledvinap
Copy link
Contributor

@ledvinap ledvinap commented Apr 2, 2024

Comments are in individual commits

Petr Ledvina added 5 commits April 2, 2024 17:56
- beeperSequenceAdvance is used to advance beeperPos
- sequences starting with 0 (delay first) are handled correctly
- 'empty' beeper states are supressed
- leds do blink beeper sequence, even when beeper is silenced
- simpler length definition
- helpper function to simplify beep generation
Refuse modes where beeperTableEntry->sequence is NULL
Rest of code is refactored without functional changes
- beeperNextToggleTime==0 when not enabled
- lastDshotBeaconCommandTimeUs is updated during DSHOT_BEACON_GUARD_DELAY_US phase
- DSHOT_BEACON_ALLOWED_MODES instead of explicit tests
- improve comments
- add STATIC_ASSERT
- reformat some code
- move #ifdef to better follow semantic structure
- return unsigned from BEEPER_GET_FLAG
Copy link

github-actions bot commented Apr 2, 2024

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

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

@ledvinap
Copy link
Contributor Author

ledvinap commented Apr 2, 2024

There are some open problems with beeper:

  1. beeper_off_flags & BEEPER_USB
    When set, all beeps are disabled, including OSD, onboard LED, LedStrip and DSHOT. Is that intentional?
    Keeping warning led function seems very reasonable, ledstrip is probably OK.

  2. beeper_off_flags & <FLAG>
    Old code did turn warningLedEnable() on. Visual beeper and ledstrip was off (Changed visual beeper to respect beeper settings. #6947, this PR did break led blinking).
    New code blinks LED only.

  3. BEEPER_ARMED vas IMO almost continuous beep instead of - . Nobody cared for 7 years, so long beep is maybe expected now?

src/main/io/beeper.c Show resolved Hide resolved
src/main/io/beeper.c Show resolved Hide resolved
#ifndef BEEPER_PWM_HZ
#define BEEPER_PWM_HZ 0
#endif
# ifndef BEEPER_PWM_HZ
Copy link
Member

Choose a reason for hiding this comment

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

Personal do not like indentation for macro's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll remove it. But it is confusing with deeper nesting

Copy link
Member

Choose a reason for hiding this comment

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

Agree but could not find formal code guidelines on macro indentation as the code base uses them flat all over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually there are 61 lines with indented macros. But it seems that I had written almost all of them and nobody was adventurous enough to change them back ;-)

@ctzsnooze
Copy link
Member

As far as I can tell, motors only beep for Rx loss and Rx set.
For Mag Cal, adding audio beeper is a big improvement.
However without audio beeper, it is useful to have some other non-visual feedback.
Would it be practical to provide a motor beep option that made a brief single 'motor beep' whenever any audio beep would be generated by the audio beeper?

Copy link
Member

@ctzsnooze ctzsnooze left a comment

Choose a reason for hiding this comment

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

Tested with my existing beeper build, all beeps worked normally.
No coding suggestions to make

@nerdCopter
Copy link
Member

4.5 or 4.6?

@haslinghuis
Copy link
Member

If there is nothing to fix - this would be 4.6

Copy link
Member

@nerdCopter nerdCopter left a comment

Choose a reason for hiding this comment

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

  • approving untested.
  • @ctzsnooze had a suggestion/question regarding potential single-motor-beep to be considered.

@haslinghuis
Copy link
Member

@ledvinap please rebase

@ledvinap
Copy link
Contributor Author

Merged upstream

@haslinghuis haslinghuis merged commit 3094dd6 into betaflight:master May 16, 2024
23 checks passed
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

4 participants