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

ardupilotmega.xml - remove empty parameters #389

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hamishwillee
Copy link

MAVLink docs/xml is unclear on the meaning of empty, reserved, and undefined parameters, and what the default values of each parameter should be. I've created this PR as a point of discussion before rolling out a consistent strategy.

TLDR;

  • There is no safe way to override a param defined in a base XML, whether used or unused. Therefore all unused parameters marked as empty, reserved, or not defined mean the same thing, and should be marked consistently in XML.
  • There are multiple ways of marking the default value of unused params, but these are ignored by consumers (they are set to 0).

Proposal:

  • We delete all of "empty", reserved, etc params - if it isn't present it is assumed to be reserved. This is consistent, clearly means unused, and should reduce the amount of information to digest in XML.
  • Default values are assumed to be 0 for all unused params, matching what the GCS do.

Whatever we end up agreeing I will roll out everywhere. FYI @auturgy @tridge @julianoes @peterbarker


The long version:

Where we are now:

  1. Inconsistent meaning naming and docs for unused parameters:

    • Kudos to ardupilotmega.xml, which only uses <param index="n">Empty</param>. Note though, it is not defined what this means if someone were to override the dialect.
    • common.xml has params that are marked as <param index="n">Empty</param> and <param index="n" reserved="true" />, and there are also some omitted params. It is not clear if these are the same or somehow different.
    • We added reserved as a mechanism in order to be able to do away with text and auto-generate it consistently if needed for docs. I.e. it was intended as a replacement.
  2. Default values are defined to be settable in different ways but are ignored:

    • A default value is the value of the param that means "do nothing". It is important because if you have been sending 0 or NaN for the unimplemented param, you need to keep sending that value to mean "do nothing" - you can't send 0 and then decide later that the param is a temperature setting, and 0 means 0C (you have to say "0.1 means 0, and 0 means ignore).
    • COMMAND_INT and COMMAND_LONG specify that float params are default NaN and int params are default INT32_MAX
      • This can't work, because you don't know for sure which command a message will be sent in.
      • XSD allows you to specify a default value for a parameter such as <param index="n" reserved="true" default="NaN">, overriding the default.
      • Mission Planner and QGC tend to set 0 for all unused params. MAVSDK sets 0 for unused params on ArduPilot and NaN on PX4.

Proposal (duplicated from above):

  • We delete all of "empty", reserved, etc params - if it isn't present it is assumed to be reserved.
  • Default values are assumed to be 0 for all unused params, matching what the GCS do.

Reasoning for proposal above:

  • W.r.t. deleting unused params vs having some standard format I am a little ambivalent. I do think though that if we have a standard format it should use the attribute "reserved" and auto-generate the docs - i.e. here replace <param index="n">Empty</param> with <param index="n" reserved="true" />. Important thing is to be consistent.
  • I would love to specify that default values for unused params are: param1, 2, 3, 4, 7 = NaN, param5,6 to INT32_MAX. That should be easy - but no one is doing it so if we can't get people to buy into this, we should force 0 and have a simple rule
  • I would love to override the default defaults using the default attribute :-) - but again, if no one is doing this, better to have a simple rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant