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

File Format Standardization #989

Closed
hamishwillee opened this issue Sep 20, 2018 · 34 comments
Closed

File Format Standardization #989

hamishwillee opened this issue Sep 20, 2018 · 34 comments
Labels

Comments

@hamishwillee
Copy link
Collaborator

hamishwillee commented Sep 20, 2018

MAVLink systems commonly need to be able to store and restore MAVLink information- e.g. GSC store/restore missions that the users wants to repeat, mission logs can be stored and used by analysis tools, GCS used to plan missions for later execution/upload by developer API.

This issue proposes that we define standard file formats for sharing common MAVLink information: missions (plans), geofence, rally points, parameters, and any other useful data. These can then be adopted by all mavlink systems, easing the above kinds of tasks.

The main suggestion is that we use JSON based on the current QGC format. The new form of JSON will be similar, but should be parsable and usable by systems that don't understand complex types.

In practice this proposal means that:

  • Complex mission items like surveys should also include the same information as a series of simple mission items (which should be easy for any system to extract and play the mission items)
  • This "rendered" form of the mission should be clearly identified by a consistent marker such that it can easily be extracted by a system that does not understand the complex format.

Discussion on this and other requirements welcome.

@hamishwillee
Copy link
Collaborator Author

@LorenzMeier Can you please discuss implications of this with @DonLakeFlyer and @WickedShell ?

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Sep 25, 2018

  • Complex mission items like surveys should also include the same information as a series of simple mission items (which should be easy for any system to extract and play the mission items)

Survey and Corridor Scan already support this. The simple mission items can be found in TransectStyleComplexItem.Items.

Structure Scan does not yet support this. I can add a work item for that.

@hamishwillee
Copy link
Collaborator Author

@DonLakeFlyer Thanks

Does it make sense to have a consistent mechanism for an arbitrary system to be able to work out what field in any complex item contains the simple mission item representation? That could be as simple as being "a first level nested field named XXXX", where XXXX is a name that is reserved for the purpose. I see you have used Items for the TransectStyleComplexItem - that is OK but perhaps something that captures this in a human readable way (simple_form, primitive_format ?)

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Sep 25, 2018

Does it make sense to have a consistent mechanism for an arbitrary system to be able to work out what field in any complex item contains the simple mission item representation?

Hindsight would say yes. Current code would say no. :)

Not sure it's worth the code to support back compat versioning problems to change now. Given the way the code works, it would be difficult to move it out of TransectStyleComplexItem.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Sep 25, 2018

Not sure it's worth the code to support back compat versioning problems to change now.

Fair enough. Can you please at least name the field Items in the new type - so a parser can search on something called items in first level of complex items than only contains simpleitem objects.

Maybe rename next time you do a significant iteration of the format.

@DonLakeFlyer
Copy link
Contributor

Either way the parser will need to walk the hierarchy to different levels to find the simple items. The "Items" will be at different levels of the hierarchy since Structure Scan does not have a common TransectStyleComplexItem section.

@WickedShell
Copy link
Collaborator

It needs to be in a predefined common field, and pretty much every other field on a complex item needs to be considered optional. It's up to a given GCS implementation to decide if it is using it/requires it or not. If it's all stuck off in a different field per complex item then it's not a usable generic format, and not something other GCS's will have a lot of reason to support.

@DonLakeFlyer
Copy link
Contributor

If it's all stuck off in a different field per complex item then it's not a usable generic format

I know. I'm just trying to avoid work for something that seems to me will get little usage. Is anyone really signed up to implement reading these? The concept of say any arduopilot stuff writing these out seems unlikely as well.

Also making this mavlink spec means a whole other level of hell to go through whenever QGC adds/changes feature set. If QGC adds/changes a new/existing complex item type does that mean that json format for its values needs to go through mavlink committee approval? Inner values of complex items change frequently as new features are added. Or is mavlink spec just the outer layer of the .plan json file format (including a way to get simple items from complex items)? And not the inner details of a QGC written complex item?

For example, does mavlink spec only define this for complex items:

            {
                "complexItemType": "...",
                "type": "ComplexItem",
                "items": [ ... ]
                "version": #
            }

Even with the above, the current json file format for complex item will not quite work. The reason being that the version number is for entire complex item including the detailed values which are essentially gcs specific values. There would need to be an additional level of hiearchy where the mavlink spec part of a complex item is versioned, and the the detailed values are in a sub-object which can also versioned.

Or does mavlink spec fully define this (using Structure Scan as an example):

            {
                "Altitude": 10,
                "CameraCalc": {
                    "AdjustedFootprintFrontal": 225.00336000000001,
                    "AdjustedFootprintSide": 60.96,
                    "CameraName": "Manual (no camera specs)",
                    "DistanceToSurface": 30.48,
                    "DistanceToSurfaceRelative": true,
                    "version": 1
                },
                "Layers": 1,
                "StructureHeight": 225.00336000000001,
                "altitudeRelative": true,
                "complexItemType": "StructureScan",
                "polygon": [
                    [
                        47.39899935665066,
                        8.545324019938546
                    ],
                    [
                        47.39899935665066,
                        8.546652617606426
                    ],
                    [
                        47.39810003606109,
                        8.546652617606426
                    ],
                    [
                        47.39810003606109,
                        8.545324019938546
                    ]
                ],
                "type": "ComplexItem",
                "items": [ ... ]
                "version": 2
            }

What exactly is mavlink spec in a .plan file and what is not?

@WickedShell
Copy link
Collaborator

I know. I'm just trying to avoid work for something that seems to me will get little usage. Is anyone really signed up to implement reading these? The concept of say any arduopilot stuff writing these out seems unlikely as well.

Considering I was talking to you in July 2017 about the limitations in the format you adopted, and had discucssed a general json version of the plan with you in December 2016 the anwser is a resounding yes I was trying to. Then you went off and just did your own thing while dropping most of the cross GCS stuff we had discussed. You said you ran into some problem with supporting it but then just closed the issue you had open about it without ever coming back with what the underlying issue was. Given that was the attitude from the QGC side and I needed a workable one and could not get any cooperation on the format I went off and did my own format since I couldn't get any interoperability with you on it.

Inner values of complex items change frequently as new features are added.

This is a huge problem with the current file format that is floating on the site because it means that an old mission's data is entirely lost and can't be assessed to see how it was flown on that date without finding the same version GCS. The fallback items were intended to address this and let you recover.

Or is mavlink spec just the outer layer of the .plan json file format (including a way to get simple items from complex items)? And not the inner details of a QGC written complex item?

That was the intent. A list of simple items, and known ways to extract the simple items from any complex items. If every GCS can agree on those, then the handling and extra metadata needed can be tied down to a specific GCS as needed.

To be clear, I have implemented what I'm asking for, and it does work quite well. There are 2 version numbers associated with the overall file, one for the actual spec of the json (IE what fields are mandatory/must be found), and one for GCS writing version. I have code that loads the fallback items if needed, and otherwise loads complex items from it easily.

@DonLakeFlyer
Copy link
Contributor

Then you went off and just did your own thing while dropping most of the cross GCS stuff we had discussed.

Not sure what has changed with respect to that concept. In reality I have not worked on that other than what was done in the original July 2017 issue. The "Items" field in TransectStyleComplexItem were added to solve an issue with the new Terrain support feature. They weren't added to support cross-gcs reading of complex items. They just happen to get half way to solving the simple/complex item problem so I figured why not use it. I probably should have explained that in my original post. Like I said, if I had thought ahead I would have done it differently in such a way that could have worked for all complex items from a spec standpoint. My bad. I was just trying to solve a Terrain problem without thinking far enough ahead. That is why you only get that with Survey/Corridor since Structure Scan does not support Terrain. Is there something other than the simple items for complex items thing which has changed/added or is bad? What else did I drop? If so make sure to let me know and I can try to adjust. Other than the internals of complex items, the rest of .plan file format hasn't really changed.

You said you ran into some problem with supporting it but then just closed the issue you had open about it without ever coming back with what the underlying issue was.

The original problem was related to creating a single list of simple items when there are complex items in a plan. Which was an original ask detailed somewhere. That continues to not be possible. What is possible is the mixed simple+complex format which is currently written and then implementing such that complex includes a simple item list (which is somewhat NYI at this point).

Also I had no idea you were actually implementing the format. I missed that somehow. I thought it was just still in dicussion. And then I got sidetracked with other things and never came back to it. I seem to do that a lot. Mostly because my time available for QGC jumps up and down alot with large dropouts.

Also from what you write I think you are saying that the guts of complex items are not mavlink spec just the outer layer which allows cross-gcs reading. That makes a lot of sense to me. And given your interest I can fix things to have simple items within a complex item to be a single spec standard name/location. I think that basically means that if a gcs reads a plan file, and the "groundstation" value is not it's own, then it should only use mavlink spec data to read the file. If the "groundstation" value matches itself then it should be able to safely read the detailed complex item values as well.

If you agree with that conceptually I can detail out at least what I think is the mavlink spec part of a .plan file.

@DonLakeFlyer
Copy link
Contributor

@WickedShell Can't remember what was up with closing whatever issue had file format discussion in it. If you can point me to anything open or closed that has file format discussion in it that would be helpful. Since if this is going to happen and be mavlink spec in the 3.5 QGC release then all of that will need to be taken into account.

@DonLakeFlyer
Copy link
Contributor

I think that basically means that if a gcs reads a plan file, and the "groundstation" value is not it's own, then it should only use mavlink spec data to read the file.

That said, anyone can choose to implement loading the gory details of complex items from another gcs if they want. But they are entering the wild west if they do that and need to keep up to date with whatever is changing on that apps feature set.

@DonLakeFlyer
Copy link
Contributor

Also I think the whole geofence and rally point formats may be new since the old file format discussion. Are you looking through those as well?

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Sep 26, 2018

Wait I think you are talking about #5849 was closed right? Just realized that! At the point in time I closed it, I thought it made sense since I was leaning on the Terrain changes to get simple items into the file format. But in reality it was wrong, since Fixed Wing Landing Pattern at that time didn't implement the simple/complex thing. And then I added Structure Scan complex item feature later which also didn't support. Sorry for rambling, just trying to recreate in my memory how the hell I screwed this up.

@hamishwillee
Copy link
Collaborator Author

@DonLakeFlyer @WickedShell I should probably just keep my mouth shut here, but ...

IMO you're both reasonable people, so the lesson to learn here is that if either of you are being unhelpful it is far more likely some sort of communication issue or mistake than disinterest or other poor intent.

Is it clear what needs to happen now, and how we can progress this?
Does not have to happen overnight and we can co-ordinate any breaks.

@DonLakeFlyer
Copy link
Contributor

I'm hoping by now most people realize that I am just a grumpy old man! It should seem obvious.

I'm hoping @WickedShell can parse my ramblings because I think we are on the same page. But I'm not yet sure if there are other changes he wants as well (beyond simple items in complex items done correctly and generic).

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Sep 26, 2018

So my proposal would be that the top layer of the plan file is mavlink spec. But when you get to a complex item inside mission.items only this is mavlink spec:

            {
                "complexItemType": "...",
                "type": "ComplexItem",
                "simpleItems": [ ... ]
            }

All other fields in there are gcs specific fields and all three fields above are required.

Then I think for simple items at the top level inside mission.items array there needs to be some work around the altitude fields which are external to the main mission command stuff:

            {
                "AMSLAltAboveTerrain": null,
                "Altitude": 10,
                "AltitudeMode": 0,
                "autoContinue": true,
                "command": 16,
                "doJumpId": 2,
                "frame": 3,
                "params": [
                    0,
                    0,
                    0,
                    null,
                    47.39740307,
                    8.54371901,
                    10
                ],
                "type": "SimpleItem"
            },

These three fields: AMSLAltAboveTerrain, Altitude, AltitudeMode are used to implement the QGC specific terrain follow feature/ui. It's possible that only AMSLAltAboveTerrain (this is the terrain height from airmap at the specified lat/lon) is needed. The others could be derived from frame and param7.

@hamishwillee
Copy link
Collaborator Author

So basically everything is "mavlink spec" except objects that have "type": "ComplexItem"?
Sounds pretty sensible to me, but @WickedShell and perhaps @meee1 would be best people to comment.

These three fields: AMSLAltAboveTerrain, Altitude, AltitudeMode are used to implement the QGC specific terrain follow feature/ui. It's possible that only AMSLAltAboveTerrain (this is the terrain height from airmap at the specified lat/lon) is needed. The others could be derived from frame and param7.

FWIW IMO

  • stuff that can be derived from other fields should be.
  • Would the best solution for AMSLAltAboveTerrain be to define a MAVLink frame that uses "above terrain"? We're currently in discussion about what frames make sense.

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Sep 27, 2018

  • Would the best solution for AMSLAltAboveTerrain be to define a MAVLink frame that uses "above terrain"?

Nope. The terrain support in QGC is not a special frame type. It is just an absolute altitude where that altitude is caculated from the terrain height (AMSLAltAboveTerrain) and the user specified alt (Altitude) above terrain. It's a completly gcs side feature. And now that I write that I see that I can't get rid of those three fields in order to save the details for that feature.

@DonLakeFlyer
Copy link
Contributor

Inner values of complex items change frequently as new features are added.

Also @WickedShell My comment here wasn't about the inner simple items associated with a complex items. It was about all the other gcs specific meta data. Arguing to not make that part of mavlink spec as I proposed above.

@hamishwillee
Copy link
Collaborator Author

Nope. The terrain support in QGC is not a special frame type. It is just an absolute altitude where that altitude is caculated from the terrain height (AMSLAltAboveTerrain) and the user specified alt (Altitude) above terrain.

Probably dumb, but while I see that this is a QGC feature, I don't see why it can't be generic.
Imagine you have frame type that defines the z/altitude component as "height above terrain". With param 7 you have an altitude that can be AMSL, relative to terrain or relative to home - it is the frame that decides.

@DonLakeFlyer
Copy link
Contributor

Support for height above terrain from a standpoint where the vehicle knows about terrain heights is already supported in the mavlink spec (and implemented by ArduPilot). It's frame = MAV_FRAME_GLOBAL_TERRAIN_ALT.

The QGC feature works in way that the vehicle does not need to know terrain heights. In order to make the QGC feature store into a waypoint you need two values: terrain height, and height above terrain, plus some new frame flag. Which doesn't really work with current spec of how param7+frame defines an altitude. I don't see that it adds much value to add this to mavlink. Since the real mission command is just and absolute altitude. Basically for terrain QGC turns simple items into simples items+ which are somewhere in between a simple item and a complex item.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Sep 27, 2018

@DonLakeFlyer Ah, what I think you are saying is that you're storing the actual height of the terrain in the simple item. That is kind of cool. Is AMSLAltAboveTerrain the terrain height above AMSL or (terrain high above AMSL + current altitude)?

OK, so that does sound very GCS specific. I don't see how it could be represented in terms of simple items that another system could understand. Options:

  • Make it a complex item with the additional fields separate so at least a parser knows that it isn't something they can parse
  • Maybe represent it as a simpleitem with an absolute alt.

@WickedShell Any ideas on how this might be best handled?

@DonLakeFlyer
Copy link
Contributor

I think what is needed is a consistent way to represent additional metadata in both a simple and complex item which is gcs specific. This mechanism then easily separates the gcs specific info (which can be ignored by other gcs) from the mavlink spec standard info. Give me a bit and I'll propose something.

@hamishwillee
Copy link
Collaborator Author

I think what is needed is a consistent way to represent additional metadata in both a simple and complex item which is gcs specific. This mechanism then easily separates the gcs specific info (which can be ignored by other gcs) from the mavlink spec standard info. Give me a bit and I'll propose something.

Yes, as long as removing this information still allows a GCS to use the item as a simple mission item.

@DonLakeFlyer
Copy link
Contributor

So basically I think what you do is something like this:

            {
                "metaData": {
                     "AMSLAltAboveTerrain": null,
                     "Altitude": 10,
                     "AltitudeMode": 0,
                }
                "autoContinue": true,
                "command": 16,
                "doJumpId": 2,
                "frame": 3,
                "params": [
                    0,
                    0,
                    0,
                    null,
                    47.39740307,
                    8.54371901,
                    10
                ],
                "type": "SimpleItem"
            },

Where the metaData object is defined as gcs specific metadata which is not part of the mavlink spec and only relevent to the writing gcs. Complex items would work exactly the same way.

@hamishwillee
Copy link
Collaborator Author

That seems great to me, but let's see what @WickedShell has to say.

@WickedShell - also, is there anything else required of the format at this point?

@hamishwillee
Copy link
Collaborator Author

@WickedShell Ping.

@hamishwillee
Copy link
Collaborator Author

@WickedShell Don is away for a few weeks, but I'd really value your opinion on his proposal.

@hamishwillee hamishwillee added the Dev Call Issues to be discussed during the Dev Call label Oct 17, 2018
@LorenzMeier
Copy link
Member

@WickedShell Your feedback would be very welcome here. Thanks!

@hamishwillee
Copy link
Collaborator Author

@WickedShell Time to look at this before today's dev call?

@hamishwillee hamishwillee removed the Dev Call Issues to be discussed during the Dev Call label Apr 3, 2019
@stale
Copy link

stale bot commented Apr 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 23, 2020
@stale
Copy link

stale bot commented Jun 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jun 22, 2020
@hamishwillee
Copy link
Collaborator Author

I'm closing. @WickedShell If you ever find time, have a chat to Don on this ;-)

@stale stale bot removed the stale label Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants