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

Component information: add actuators #1729

Merged
merged 3 commits into from Nov 11, 2021

Conversation

bkueng
Copy link
Member

@bkueng bkueng commented Oct 27, 2021

This adds a new metadata file for actuator and geometry/mixer configuration and actuator testing.
The PR contains an example metadata file.

It also adds a new actuator test command. I'm aware of the existing MAV_CMD_DO_MOTOR_TEST message. The one I'm adding here is different in that it operates on an output function level. It allows to specify 'Motor 1' independent from the output where Motor 1 is physically connected.
There is a set of predefined output functions, however the UI in QGC will use the custom range >=1000, as it learns the actually available functions dynamically from the metadata.

QGC implementation: mavlink/qgroundcontrol#9952

This adds to the development.xml, but QGC is using ardupilotmega.xml by default. @DonLakeFlyer @hamishwillee how do you want to proceed with this?

TODO:

  • add a schema
  • add documentation

<entry value="310" name="MAV_CMD_ACTUATOR_TEST" hasLocation="false" isDestination="false">
<description>Actuator testing command. This is similar to MAV_CMD_DO_MOTOR_TEST but operates on the level of output functions, i.e. it is possible to test Motor1 independent from which output it is configured on. Autopilots typically refuse this command while armed.</description>
<param index="1" label="Value" minValue="-1" maxValue="1">Output value: 1 means maximum positive output, 0 to center servos or minimum motor thrust (expected to spin), -1 for maximum negative (if not supported by the motors, i.e. motor is not reversible, smaller than 0 maps to NaN). And NaN maps to disarmed (stop the motors).</param>
<param index="2" units="s" minValue="0" maxValue="3" label="Timeout">Timeout after which the test command expires and the output is restored to the previous value. A timeout has to be set for safety reasons. A timeout of 0 means to restore the previous value immediately.</param>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we order the attributes index, label, first for both this param and param 5? It doesn't matter technically, but I find it easier to scan the purpose if I see the label first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@hamishwillee
Copy link
Collaborator

hamishwillee commented Oct 27, 2021

@auturgy For context, this is stuff to support a new UI in QGroundControl (and PX4) to dynamically configure the airframe geometry and then perform motor testing. There is a preview of this here: https://www.youtube.com/watch?v=oKru4EsF1aM

IMO the mavlink messages are definitely a step forward and should be of interest to ArduPilot.

I think that the definition of the component metadata for the motor mapping is something that ArduPilot can and would want to use because ArduPilot already supports parameter defined motor mappings. However I am out of touch with the ArduPilot architecture so that needs more careful review. Further, to me this isn't enough information to understand what is standard and what is not.

Can you please review this, in particular the enums/commands (perhaps we might split those into their own PR).

@bkueng FYI, James is on leave for at least the next week and might not be able to get to this quickly.

@@ -89,6 +89,60 @@
<param index="1" label="Action" enum="FENCE_ACTION">Fence action on breach.</param>
</entry>
</enum>
<enum name="ACTUATOR_ACTION">
<description>Actuator actions. Component information metadata can be used to know which outputs support which commands.</description>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and of course the MAV_CMD should fail, reject commands that aren't supported in any case.

@@ -89,6 +89,60 @@
<param index="1" label="Action" enum="FENCE_ACTION">Fence action on breach.</param>
</entry>
</enum>
<enum name="ACTUATOR_ACTION">
<description>Actuator actions. Component information metadata can be used to know which outputs support which commands.</description>
<entry value="0" name="ACTUATOR_ACTION_BEEP">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a "do nothing" value? It doesn't matter for the current version of the command but if you add more fields later, will you always want this to beep?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add that.

<param index="6">Reserved (set to 0)</param>
<param index="7">Reserved (set to 0)</param>
</entry>
<entry value="311" name="MAV_CMD_ACTUATOR_ACTION" hasLocation="false" isDestination="false">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about this name and description. We have quite a few actuator commands - e.g. MAV_CMD_DO_SET_ACTUATOR and it would be good to name with "clear differentiation".

I "think" this is essentially a configuration command, that happens to have a beep action as well. Even though the action to beep is not about config, perhaps we might be better with MAV_CMD_CONFIGURE_ACTUATOR? Probably with associated change to the enum name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me.

@@ -0,0 +1,584 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment/from this it isn't clear to me from this what is generic and what is PX4 specific. That makes it hard for me to understand what is the actual standard.

I think we probably need the schema file, and some understanding of how this might differ for another flight stack that, for example, supported setting the function to motor mapping, but not the geometry.

For example, the approach here might be a flight stack agnostic UI definition that maps to the particular flight stacks names for functions and parameters - like the approach used for camera definition files. Or it might be that some of this is hard coded.

Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll do the schema then. I was hoping the example would already provide a good starting point.
It is similar to the camera definition, but goes further than that, and is targeted for the specific use-case while trying to be flight-stack agnostic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkueng Thanks very much. I think we may end up needing docs for the detail too, but just hearing those words above ^^^ makes the example far less confusing.

So schema defines

  • output groups (arbitrary, mapping to busses): MAIN, AUX, UAVCAN,
    • parameters common to all things in the group
    • subgroups of outputs: e.g. AUX 1-4 with:
      • actions - ie config stuff like spin direction (we might want to change that name in schema to match message change
      • parameters that affect all outputs in the subgroup - e.g, an override param for default value of all outpus=ts
      • "per-channel-parameters" that every output has, specified as a templated value.
        • Can a channel have a specific param as well - ie a specific output with a particular non-templated parameter?
      • channels, which I believe are actually the outputs.
        • These have a label and a param index. What is the index into?
  • list of functions (functions_v1)
    • 0 to indicate disabled, and list of each motor, servo, camera capture.
    • presumably each one can only be used once - so if you had two cameras you would have to declare camera capture 2
  • mixer_v1
    • actuator types - motors, servos, and the functions that apply to each as a "range". Which presumably implies that they functions for servos and motors must always be grouped.
    • config appears to define the geometry information. I think it needs more explanation and examples - I somewhat get it

I can see how it can generically define a UI to display any set of outputs and functions and map them to flight-stack agnostic set of parameters. I don't understand the config section yet, but I'm prepared to believe you can define any geometry in this way.

Just trying to get my head around how these would be used/what/when would need to be changed for each flight controller or airframe. Would you have just one file that defined all the possible config (airframe) values. Presumably you might have a different file if a flight controller didn't have an I/O board?

Does QGC actually render the image of the geometry from the information provided? I.e if I provide this for a 15 engine blimp, what would I see in the UI?

Do we need a separate actuator type for actuators (not servos)?

Sorry if this is rambly. I think that it looks pretty good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a channel have a specific param as well - ie a specific output with a particular non-templated parameter?

Yes as a bitfield. Otherwise as well, but it would not make much sense.

These have a label and a param index. What is the index into?

The index is for the templated param. param-index: 3 + PWM_MAIN_MAX${i} -> PWM_MAIN_MAX3 (with optional offset).

presumably each one can only be used once - so if you had two cameras you would have to declare camera capture 2

Yes but you can also assign the same function to multiple outputs (e.g. redundant motors)

Which presumably implies that they functions for servos and motors must always be grouped.

Correct.

I don't understand the config section yet, but I'm prepared to believe you can define any geometry in this way.

Yes I think 3 mechanims are relevant to do so:

  • you can assign the meaning (e.g. 'position x') to any parameter or hardcoded fixed value.
  • you know the actuator type and how many there are
  • you know the vehicle type

Would you have just one file that defined all the possible config (airframe) values. Presumably you might have a different file if a flight controller didn't have an I/O board?

You have one file per board (per hardware configuration: yes this can be IO or not, or but might also be a custom CAN output driver), which contains all software-configurable parts (so all geometries/mixers, which is the select condition in the mixer).

I.e if I provide this for a 15 engine blimp, what would I see in the UI?

atm there's no UI for it, it would show like the vtol example: https://user-images.githubusercontent.com/281593/139041205-4b2f5634-8be0-4a86-9305-1233cb3dbfa1.png.

@hamishwillee
Copy link
Collaborator

@bkueng "My two bits".

  1. The question about this being in development.xml and QGC using ardupilotmega.xml is one for @DonLakeFlyer. A few thoughts:

    • QGC might want to consider taking all.xml to get ardupilot, development and common.xml in one. Note this will require testing because it was not the original intent of all.xml. I think it will work provided QGC builds the library from source.
    • The messages might be able to go into common.xml fairly quickly - at least to me they seem sane.
  2. I much prefer your design of MAV_CMD_ACTUATOR_TEST over MAV_CMD_DO_MOTOR_TEST.

    • Mapping to motor functions rather than a particular output means that it is predictable and works the same irrespective of how/where the motors are connected. All that confusion about which bus something is on ends up being abstracted for the GCS and ultimately for the user. Much better.
    • It is atomic/triggers just one actuator or motor at a time. Much better to have the GCS define test orders appropriate for the frame using a primitive than try embed that in a message.
    • Outputs are scaled, so GCS doesn't need to know what bus they are on.
  3. The actuator action command might be better named to reflect it is a configuration command - as discussed here: https://github.com/mavlink/mavlink/pull/1729/files#r737903801

  4. Component information - the example is interesting, but raises a few questions from the "what is standardized" point of view. I have tried to expand what I mean in https://github.com/mavlink/mavlink/pull/1729/files#r737904381

Hope that all makes sense.

@auturgy
Copy link
Collaborator

auturgy commented Oct 27, 2021

@hamishwillee as you've noted, the json needs to be looked at to ensure it is not flight stack specific.
For information ArduPilot already supports dynamic mixers / in-flight control re-allocation via Lua scripting (https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Scripting/examples/Motor_mixer_dynamic_setup.lua). As you might imagine there are some sophisticated implementations of that out in the wild for complex geometries and fault tolerance.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Oct 28, 2021

@auturgy Absolutely.

A lot of it depends on whether the intent is like camera definition files - to define an abstract UI that the GCS does not need to understand, or it is more like our parameter metadata which provides a layer of additional information but does not generate a UI.

I think the messages are good either way. Of course if this was a generic UI you could embed the motor test stuff as a flight-stack specific UI, but I think the current motor test approach is flawed, and these messages provide a much better / flight stack agnostic approach.

@bkueng bkueng force-pushed the component_information_actuators branch from e9f0b4d to 07f155d Compare October 28, 2021 17:07
@bkueng
Copy link
Member Author

bkueng commented Oct 28, 2021

I added the schema file (almost finished).

"per-channel-parameters": [
{
"label": "Function",
"name": "UAVCAN_EC_FUNC${i}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: UAVCAN_ESC_FUNC$ ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just a lack of characters: the parser in PX4 ensures the index can go to 2 digits, hence this is limited to 14.

@bkueng bkueng force-pushed the component_information_actuators branch from 07f155d to 376f734 Compare October 29, 2021 08:47
@hamishwillee
Copy link
Collaborator

@bkueng I've had a bit of a look at ArduPilot control allocation. They essentially have two approaches. The first/original model essentially hard-codes what PX4 would call the geometry but does allow mapping of outputs to functions using parameters.

The second way uses lua scripting to script the geometry and mixing. I think this also abstracts the outputs. It is very powerful in that the script can effectively dynamically modify geometry on the fly - supporting complex geometries like this which you just can't do with a static definition. It can also set motor test orders etc.

The stuff to map motors and servos to outputs might therefore be generically useful to ArduPilot for the "original" case. It would be less useful for the lua scripts because these can be added and removed from the frame at will. I don't think the geometry stuff would be particular useful to them, though I guess I could be wrong.

I'm not sure what that means for the component stuff because we don't have explicit "rules" on that.

Can you split the messages out into their own PR? I think we can merge that pretty easily and then discuss the component definition separately.

@bkueng bkueng force-pushed the component_information_actuators branch from 376f734 to 7023e56 Compare November 3, 2021 12:54
@bkueng bkueng changed the title [WIP]: Component information: add actuators Component information: add actuators Nov 3, 2021
@bkueng bkueng force-pushed the component_information_actuators branch from 7023e56 to 284a430 Compare November 3, 2021 13:00
@hamishwillee
Copy link
Collaborator

Thanks for making the change. I've requested review of the other PR and sent you some further offline discussion of this one.

@bkueng bkueng force-pushed the component_information_actuators branch from 284a430 to 0357eb5 Compare November 9, 2021 09:21
@hamishwillee
Copy link
Collaborator

Just to summarize the discussion of the dev call this is "broadly approved" as consistent with the existing schema and design of component information.
It is waiting on minor changes from Beat and then I can merge (once pinged).

It should be useful for any flight stack that can assign functions and output mappings using parameters. It can also be useful for any flight stack that can define a geometry in terms of parameters, and that also applies to both PX4 and ArduPilot.
ArduPilot allows geometry/mixing outputs to be dynamically defined using lua scripts. Provided the lua scripts use parameters to define the geometry this approach still works but the information file could not be hard coded and there might need to be a discussion on how to get the CRC to work properly.

@hamishwillee
Copy link
Collaborator

@bkueng With respect to the image used for the geometry settings, I know there is a fallback to "no image" and that is "enough".

However the general concept we try for is that a GCS can work with any flight stack for standard kinds of things without having to have hard coded. So it would be cool if we can provide some mechanisms to allow configuration of standard frames at least - e.g. quad-X, etc.

Do you think it is possible/reasonable we provide a standard set of UIs that different flight stacks might use - e..g. a key to say this geometry is for a Quad-X with motors numbered like this so that if the GCS knows what that is, it can use the image type?

@bkueng
Copy link
Member Author

bkueng commented Nov 11, 2021

Do you think it is possible/reasonable we provide a standard set of UIs that different flight stacks might use - e..g. a key to say this geometry is for a Quad-X with motors numbered like this so that if the GCS knows what that is, it can use the image type?

Yes I intended to add that once it's needed (it's a pure addition, non-breaking change), but I don't want to specify this w/o having at least one image in a GCS for this.
Different flight stacks typically have different actuator numberings.
And for the Quad-X example, this is already possible with the generic rendering: the metadata geometry would just contain the fixed motor positions (something I tested already), which allows any motor ordering to correctly display.

@bkueng bkueng force-pushed the component_information_actuators branch from 0357eb5 to 50f8090 Compare November 11, 2021 06:40
@hamishwillee
Copy link
Collaborator

Thank you! Waiting on you to ping me or Julian to merge, because you need it in.

@hamishwillee hamishwillee merged commit a840149 into master Nov 11, 2021
@hamishwillee hamishwillee deleted the component_information_actuators branch November 11, 2021 08:27
@hamishwillee
Copy link
Collaborator

Merging, as agreed in dev call.

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.

None yet

3 participants