PX4-Autopilot icon indicating copy to clipboard operation
PX4-Autopilot copied to clipboard
verified publisher icon PX4

Refactor tune control & support broken single note playing functionality

Open junwoo091400 opened this issue 3 years ago • 4 comments

Describe problem solved by this pull request

  1. tune_control play -f 1500 -d 500000 (Command to play a single note) was broken, so you couldn't actually use this single note play feature of tune_control
  2. Whenever tune_control uORB message was getting sent with the tune id of TUNE_ID_STOP, it was actually making the tune library enter CUSTOM tune mode, which should only be used to play a custom frequency / duration.
    • The only reason nobody noticed this so far was because the CUSTOM tune has an "" (empty string) defined in the tune_definitions.desc.
  3. Rename and add comments to ambiguous parts of the code to improve readability

Describe your solution

  1. Support single note playing command in tune_control module appropriately
  2. Added the STOP tune ID separately as a custom command / tune-id, and decouple it from original implementation where it mixed it up with CUSTOM tune id.
  3. Rename and re-structure tune_descriptions.h and .cpp to make the file / code more clear to the developers.
  4. Remove libtest command from tune_control, as it doesn't seem to serve much purpose :shrug:

Test data / coverage Tested on PixhawkV5X hardware (the following commands): image

  • tune_control play -t 12
  • tune_control play note -f 1500 -d 500000
  • tune_control play error
  • tune_control stop

Additional context

  • [ ] I have removed the header inclusion from files like src/drivers/drv_tone_alarm.h, is this ok? It seems to compile fine but wanted to make sure this isn't done accidentally.

junwoo091400 avatar May 13 '22 13:05 junwoo091400

Rebased on main

junwoo091400 avatar Jul 08 '22 13:07 junwoo091400

Still needed?

dagar avatar Sep 16 '22 00:09 dagar

Well this PR fixes the broken feature, so I would say it's needed :thinking:

junwoo091400 avatar Sep 16 '22 13:09 junwoo091400

Resolved conflict and rebased on main

junwoo091400 avatar Sep 16 '22 13:09 junwoo091400

FYI @tstastny this is the PR that I mentioned that has been sitting for a while

junwoo091400 avatar Oct 08 '22 21:10 junwoo091400

Remove the sitl_gazebo submodule change?

dagar avatar Oct 09 '22 14:10 dagar

Any implications for the other tune control subscribers (UAVCAN and tap_esc)?

dagar avatar Oct 09 '22 15:10 dagar

Any implications for the other tune control subscribers (UAVCAN and tap_esc)?

I have tested on tap_esc (Mantis Edu), but not on UAVCAN 🤔

junwoo091400 avatar Nov 09 '22 22:11 junwoo091400

Adding motor resonance based tune control can be a fun addition: DSHOT, etc.

Also, Circuit Breaker for buzzer should be removed.

Also, silence mode for people who don't want tunes to play can be added (Daniel and Jay wants this!)

junwoo091400 avatar Nov 16 '22 16:11 junwoo091400