Json schema for COMP_METADATA_TYPE_VERSION, COMP_METADATA_TYPE_PARAMETER

[DonLakeFlyer]

• I've run these through https://www.jsonschemavalidator.net and the schema is clean/correctly formatted
• I also tested the schema against the test metadata I am using in QGC and it validates the metadata to be correct against the schema
• Related to Using COMPONENT_INFORMATION to obtain vehicle metadata #1346

[hamishwillee]

@DonLakeFlyer The output I'm generating for parameters in now matches this - so you could test against https://gist.github.com/hamishwillee/3779fcfb6bd368b1cd9fd3f84cbcb5b6

1. For consistency it would be consistent to change these value names:
   • defaultValue > default
   • units > unit
2. For smaller payload it might also make be worth changing:

• shortDescription > shortDesc OR short_desc
• longDescription > longDesc OR long_desc

3. The schema appears correct, but unfortunately the validation doesn't work properly with respect to required": [ "name", "type" ], and "additionalProperties": false. I.e . you can remove the name field, or have any name you like for another field in the array. This is a problem not just in the online validator, but has been reported in some others.

[DonLakeFlyer]

1. For consistency it would be consistent to change these value names:

What is this trying to consistent with?

2. For smaller payload it might also make be worth changing:

That would save something like 10 bytes from a compressed file.

I'd rather keep the naming as is since it matches naming QGC has been using for years now. Changing it means changing all the internal json files in QGC. Certainly doable, but needs to be a decent enough reason. Since it will trash localizations on json files which have already been done.

[hamishwillee]

@DonLakeFlyer

Consistency with your other field names - ie min, max, increment are not minValue, maxValue, incrementValue because it is redundant.

Those are very good reasons to keep it the same (in particular the localisations). You're right that the amounts of memory we're talking are small, but Firmware people worry about bytes.

[DonLakeFlyer]

Consistency with your other field names

Ah, got it. Let me look at changing those.

[hamishwillee]

Cool. I think it is just default and units that make sense to change. The others would be nice to have shorter, but completely defensible that you wouldn't want to.

[DonLakeFlyer]

I did "shortDesc", "longDesc" and "default". I left "units" since "unit" just doesn't make sense to me as english.

[hamishwillee]

@DonLakeFlyer Should we have a list of valid values here so we can validate against it?

[DonLakeFlyer]

I look at the schema spec and it is not possible to define in schema a set of possible values, which can also include those play anything else you want. Only the "known" units can translate back/forth in a gcs ui. But units can be anything. You can have "foo" units if you want. It's just a textual label.

[hamishwillee]

Thanks. I'll try for consistency in PX4 at least. The problem I see with the concept of known unit is that this is just saying what QGC does. It isn't clear whether the spec is mandating that a GCS does support translation between units here. I suspect not. If we are guiding/mandating I'd also almost prefer that we reduce the set of known units that we allow in order to get that consistency.

it might be worth changing the comment to something like:

Any unit may be used. A GCS may convert "known units" for locale-sensitive display (e.g. using metres and feet as needed). Commonly supported "known units" are: m/meter/meter, 'vertical m' (vertical distance), cm, px, m/s, C (celcius), m^2, g (grams), centi-degrees, radians, norm."

[hamishwillee]

@DonLakeFlyer Should we recommend the set from PX4?: '%', 'Hz', 'mAh',
'rad', '%/rad', 'rad/s',
'bit/s', 'B/s',
'deg', 'deg*1e7', 'deg/s',
'celcius', 'gauss', 'gauss/s', 'mgauss', 'mgauss^2',
'hPa', 'kg', 'kg/m^2',
'mm', 'm', 'm/s', 'm/s^2', 'm/s^3', 'm/s^2/sqrt(Hz)', 'm/s/rad',
'Ohm', 'V',
'us', 'ms', 's'

[DonLakeFlyer]

For me (QGC specifically) the only ones that really matter are the ones that QGC can do units translation on https://github.com/mavlink/qgroundcontrol/blob/master/src/FactSystem/FactMetaData.cc#L48. I don't know how that differs from say MP. But I assume it would be similar. Since they are a superset of PX4/ArduPilot parameter metadata translatable units. The rest are mostly for consitency I guess, but I"m not sure how critical that is with respect to getting units consistency across any mavlink system.

[hamishwillee]

Thanks @DonLakeFlyer . I've regenerated the param.json so you can look at it here: https://gist.github.com/hamishwillee/3779fcfb6bd368b1cd9fd3f84cbcb5b6

Some of the PX4 units look inconsistent - e.g. Hz, Hertz. I'll talk to Beat about that.

I'm pretty sure this is good but I propose we leave it unmerged until I manage to get PX4 exporting this properly and you able to test it.

[DonLakeFlyer]

Hz, Hertz

Examples of non-translated units. That said there should still be "consistency".

[DonLakeFlyer]

I propose we leave it unmerged until I manage to get PX4 exporting this properly and you able to test it.

You can merge it whenever you feel ready.

[auturgy]

The mavlink known units are maintained in the mavlink schema. If we’re after consistency, use those.

…

On 6 Aug 2020, at 3:22 pm, Hamish Willee ***@***.***> wrote:  @hamishwillee commented on this pull request. In component_information/parameter.schema.json: > + }, + "shortDesc": { + "description": "Short user facing description/name for parameter. Used in UI intead of internal parameter name.", + "type": "string", + "default": "" + }, + "longDesc": { + "description": "Long user facing documentation of how the parameters works.", + "type": "string", + "default": "" + }, + "units": { + "description": "Units for parameter value.", + "type": "string", + "default": "", + "comment": "A 'Known Unit' allows a GCS to convert between units like meters to feet as needed. Known Units are: 'm/meter/meter', 'vertical m' - vertical distance, 'cm/px', 'm/s', 'C' - celcius, 'm^2', 'g' - grams, 'centi-degrees', 'radians', 'norm'." @DonLakeFlyer Should we recommend the set from PX4?: '%', 'Hz', 'mAh', 'rad', '%/rad', 'rad/s', 'bit/s', 'B/s', 'deg', 'deg*1e7', 'deg/s', 'celcius', 'gauss', 'gauss/s', 'mgauss', 'mgauss^2', 'hPa', 'kg', 'kg/m^2', 'mm', 'm', 'm/s', 'm/s^2', 'm/s^3', 'm/s^2/sqrt(Hz)', 'm/s/rad', 'Ohm', 'V', 'us', 'ms', 's' — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[hamishwillee]

@auturgy Good point, though FWIW these are recommendations only because these very much depend on what the (flight stack) specific parameters are. I just looked at what PX4 use, and there are some "interesting" options.

[DonLakeFlyer]

I'm going to merge this instead of letting it sit here. It has no affect on the build. And I have new updates to make to it I want to discuss in a separate pull.

[hamishwillee]

@DonLakeFlyer Cool - thanks. FYI, current status on PX4 is that we generate the parameter metadata. I was going to do most of the rest last week but I saw that Lorenz is looking at this, and it doesn't make sense for me to do anything in parallel.

[DonLakeFlyer]

I saw that Lorenz is looking at this

Yup. Lorenz, Beat and I will be talking about it soon.