init script tune_control/tone_alarm review/fix/cleanup

[dagar]

@mcsauder @davids5 @simonegu

[dagar]

Idea - create command line utility for logging error messages. http://man7.org/linux/man-pages/man1/logger.1.html

[dagar]

Obsolete PR for reference. https://github.com/PX4/Firmware/pull/8851

[davids5]

Things that need looking at:

1. The tune control is blocking.

2. In rcS Some of the code is calling tone alarm some is calling tune_control.

   • Why?
   • Do we need a boot strapping or is this just an artifact of the commits.

3. Some of the sequences do not emit tones

   • On Kinetis: tune_control play -m MNBG

We need branch testing on all HW (ie. FMUv4 would have showed the missing TUNE_ERR in the case where there is no px4io)

[dagar]

Magic numbers for different tunes - https://github.com/PX4/Firmware/pull/10170

[mcsauder]

So, there are 3 tone_alarm.cpp files in the code base, and while they are all extremely similar and are mostly copy/paste, they are not exactly the same.

Firmware/src/drivers/kinetis/tone_alarm/tone_alarm.cpp
Firmware/src/drivers/samv7/tone_alarm/tone_alarm.cpp
Firmware/src/platforms/posix/drivers/tonealrmsim/tone_alarm.cpp

It seems like refactoring the tone_alarm files and condensing their instances to one primary file might be the smartest thing to accomplish first... Once that is accomplished, (or in conjunction), default_tunes.cpp and tune_definition.h should be combined. This should condense the issue greatly and allow better focused work to take place.

[davids5]

@mcsauder - the division is by architecture, there is an open issue for breaking out the common code on tone_alarm and hrt. It has been a low priority. The common code should be very extractable but that is not the root issue here. It is when tune_control came and the effect it had on rcS. The split to common code and arch specific is welcome, but maybe more worthwhile after NXPhlite (kinetis) come in (once all the nuttx code is upstreamed and back ported). Alternately it could be done off the branch. to get the kinetis one in the mix.

[mcsauder]

Thanks @davids5 , Correct. Refactoring applies to this issue, but I understand that for the immediate you are still seeking a focused fix for tune_control. To fix the blocking call to tune_control, it can be launched as a background task in the startup scripts, (however this can lead to two instances of tune_control getting called and playing notes on top of each other).

Here's the remainder of the discussion, in addition to the 3 tone_alarm.cpp files, tunes.cpp is another copy/paste implementation, e.g.:

So, back to refactoring, it seems like collapsing all of these files into one solid base class instance has the best chance of success in the long term.

[potaito]

What about having the systemcommand tune_control start a task on the work queue while a tune is being played back? That way we can keep track of tunes currently being played back while the entire thing is no longer blocking.

Although it could probably lead to a stuttering of the startup tune. Reason being that during the boot sequence the work queue might not be polled regularly just yet, causing delays in the execution of the playback task and therefore delays in the playback of the next tone. That could be resolved by moving the startup tune to the very end of the boot sequence, if that's not already the case...

[dagar]

What's the longest tune?

[mcsauder]

Hi everyone, I've put some time into this over the past few days and the solution is a little more tangled than I had hoped... In a nutshell (no pun intended, but still funny in this context :) ), There are 5 files that share ~85% commonality, but not commonality at the method level, only at the LOC level... which means sorting through blended methods and cherry picking the right implementations. I'm working on the issue, but it hasn't been a quick job. Anyone who can beat me to it is always welcomed! :)

Progress updates to follow.

-Mark

[davids5]

@mcsauder - I ended up have to deal with this for an issue with the FMUv5 mini. I will have a PR shortly

[davids5]

Except for the Junk uOrb publication (see https://github.com/PX4/Firmware/issues/10455) the Tune control and tone_alarm issues were resolved in #10257