-
Notifications
You must be signed in to change notification settings - Fork 321
common.xml: specify that TUNE_FORMAT.tune is specifically MML #453
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
base: master
Are you sure you want to change the base?
Conversation
Only MAVProxy and ArduPilot seem to support this. And possibly something on SkyViper.
|
@hamishwillee we agreed that we are quite happy with this change in ArduPilot-land. Merging this would make QGC non-compliant with the spec as QBasic is assumed. |
|
@peterbarker I'll need to think on this. Likely end of next month. We don't make any flight stack non-compliant without some discussion, and the same courtesy should be given to QGC.
It would certainly be easier if there was one standard format. |
Sorry, this should be PX4 Firmware, not QGC. There's no client I've found except MAVProxy which sends
On reflection... I still don't think it's a good idea to force autopilots to understand every type of "tune format" you might want to support. |
Of course not. A good design, IMO is to do what the web standards do. Define a mandatory "thing" to support; allow others to be supported as well, and provide mechanisms to discover what is supported. That way you end up with everyone using the same thing, and if a real need exists for a new format, it can be supported. Given where we are, there is no standard, so the "right" think to do is to provide a mechanism to know when the tune failed or to discover the supported tune formats. I pushed for that with PLAY_TUNE_V2 but there was no interest. FMI what do you see as the use case for this message? I've been thinking it is for a MAVLink-based system to provide custom sounds to reflect its own errors - such as a remote id that is failing. |
|
To summarise
The main problems I see:
One question. QBASIC is a subset of MML. Does Ardupilot actually support the full MML - multiple channels, fade in/out, etc? Or is it actually the case that ArduPilot practically supports QBASIC? In other words, is it possible that the real supported format is actually QBASIC in both cases? I'll add this to the MAVLink call, but perhaps worth thinking about a realistic convergence path. |
This is likely. But it's also used just to test tunes out. @muramura wanted to be able to send play_tune messages at the GCS in scripting IIRC, 'though :-)
Only difference I've spotted is that we support "V" for changing the volume. And it looks like that's the only thing that PX4 doesn't do. The code is... rather similar. https://en.wikipedia.org/wiki/Music_Macro_Language seems to say that AP is doing all that's required to be "MML" - but it seems to be even less of a standard than MAVLink :-)
Shifting PX4 to support (or just ignore) volume would be a simple convergence path. PLAY_TUNE_V2 could simply be removed at that point. |
|
EDITED: It certainly makes sense for PX4 to support or ignore V if that allows us to align the PLAY_TUNE. I'd still prefer we moved to V2 though - I know you disagree, but I think it is a better message. Not my call though - will take to the dev call. OK, so created place in PX4 for discussion PX4/PX4-Autopilot#26224
I'm going to push for that change, but I would prefer we
|
Only MAVProxy and ArduPilot seem to support this. And possibly something on SkyViper.
This is a breaking change for PX4-AutoPilot, and we must not merge this without getting the OK from them. PX4-AutoPilot is expecting qbasic tune strings in this field. Given that there doesn't appear to be any clients which will send QBASIC in
PLAY_TUNE, and that clients could easily move toPLAY_TUNE_V2, they may be willing to accept the change. @hamishwilleeI've checked QGC, paparazzi, PX4-AutoPilot, MAVProxy, ardupilot, APMPlanner2.
It was a mistake to not specify the format for
PLAY_TUNE.tunestring in a standards document.PLAY_TUNE_V2came out of that mistake, and was an even bigger mistake (resources are available on the ground to convert to/from a standardised input format)