Fix up enum generation

[hamishwillee]

This attempts to fix #538.

The old code worked through each included file creating an "enum map". After parsing a file only the new enums (with respect to the map) were assigned back to the file. What this meant is that duplicated enums get stripped out of any included files. So for example if you build ardupilotmega.xml the generated common.xml does not get the MAV_CMDs - they are stripped out.
This is contrary to the intended design - you should be able to build ardupilot.xml and use the library for common.xml. It also means that the order you build files in matters.

This version does a predictable merge:

• It merges from most deeply included dialects first (so at each level you only get merged entries from the current level and below)
• It only merges if it needs to (i.e. if both of the files to be considered actually have enums in them, and then, only if for the enums that match).
• To merge two enums, it compares the names. If the names match it appends the entries from the nested include to the parent enum.
• The above operation may create duplicates. These are caught by the standard duplicate checker - which raises an error.
• After merging the values are sorted. At this point the values may contain an end marker (but shouldn't).
• After all XML files are updates there is a final enum fixup phase where all enum values are sorted by value, and an end marker is added (this includes both merged and normal enums.

This removes some code that would re number auto-generated enum values (only) if there were clashes. Instead if there are clashes this results in duplicates being discovered. This is better because it is predictable - if autogenerated values can change then the order at which things are included might change - your values for a sub library might differ from a higher library.

This retains the "bug" that you cannot have duplicate names or IDs. I say "bug" because one theory is that a dialect should be able to overwrite values in a sub dialect. I think we want to avoid that.

Testing

Here is a comparison.zip of the libraries built with the current mavlink/mavlink generator and with the new version. Comparison shows:

• The old version is broken - if you build ardupilotmega.xml you strip out MAV_CMDs from common.xml generated library
• The new version produces the same enums in included files irrespective of which parent you build.
• The new version for a dialect "top level" matches the old version - ie building ardupilotmega.xml the enums are the same in both cases. The difference is that the lower level libraries are correct.

@peterbarker @julianoes Can you review. I suspect the code could be better architected. Happy for you to do that, or advise on how. Note, that OllieW's fastmavlink does this similarly, but IMO more cleanly. I'm not a professional programmer though.

@auturgy FYI.

Fixes #538.

[olliw42]

@hamishwillee you may want to look at fastmavlink how i solved it there, i think conceptually more correctly and more systematically, should not be so difficult to translate to pymavlink

[hamishwillee]

@olliw42 I basically just hooked into your expand_includes() then call merge on every include. At the end clean up all the enums to add the end value. I'm fine with the algorithm - maybe should move the location of the merge code to the enum itself as that makes more sense.

I'll have a look at the code thanks. In all likelihood though it is likely to take me more time to work out what you're doing and pulling it into mavgen than I have free for this. Happy to take a PR from you though.

Mostly I just want @peterbarker to look at the PR and confirm that the end result is correct.

[peterbarker]

I'm a bit confused here.

pbarker@bluebottle:~$ grep --after-context=10 '# MAV_CMD$' /tmp/master.py 
# MAV_CMD
enums['MAV_CMD'] = {}
MAV_CMD_NAV_WAYPOINT = 16 # Navigate to waypoint.
enums['MAV_CMD'][16] = EnumEntry('MAV_CMD_NAV_WAYPOINT', '''Navigate to waypoint.''')
enums['MAV_CMD'][16].param[1] = '''Hold time. (ignored by fixed wing, time to stay at waypoint for rotary wing)'''
enums['MAV_CMD'][16].param[2] = '''Acceptance radius (if the sphere with this radius is hit, the waypoint counts as reached)'''
enums['MAV_CMD'][16].param[3] = '''0 to pass through the WP, if > 0 radius to pass by WP. Positive value for clockwise orbit, negative value for counter-clockwise orbit. Allows trajectory control.'''
enums['MAV_CMD'][16].param[4] = '''Desired yaw angle at waypoint (rotary wing). NaN to use the current system yaw heading mode (e.g. yaw towards next waypoint, yaw to home, etc.).'''
enums['MAV_CMD'][16].param[5] = '''Latitude'''
enums['MAV_CMD'][16].param[6] = '''Longitude'''
enums['MAV_CMD'][16].param[7] = '''Altitude'''

pbarker@bluebottle:~$ grep --after-context=10 '# MAV_CMD$' /tmp/hamish.py 
# MAV_CMD
enums['MAV_CMD'] = {}
MAV_CMD_NAV_WAYPOINT = 16 # Navigate to waypoint.
enums['MAV_CMD'][16] = EnumEntry('MAV_CMD_NAV_WAYPOINT', '''Navigate to waypoint.''')
enums['MAV_CMD'][16].param[1] = '''Hold time. (ignored by fixed wing, time to stay at waypoint for rotary wing)'''
enums['MAV_CMD'][16].param[2] = '''Acceptance radius (if the sphere with this radius is hit, the waypoint counts as reached)'''
enums['MAV_CMD'][16].param[3] = '''0 to pass through the WP, if > 0 radius to pass by WP. Positive value for clockwise orbit, negative value for counter-clockwise orbit. Allows trajectory control.'''
enums['MAV_CMD'][16].param[4] = '''Desired yaw angle at waypoint (rotary wing). NaN to use the current system yaw heading mode (e.g. yaw towards next waypoint, yaw to home, etc.).'''
enums['MAV_CMD'][16].param[5] = '''Latitude'''
enums['MAV_CMD'][16].param[6] = '''Longitude'''
enums['MAV_CMD'][16].param[7] = '''Altitude'''
--
# MAV_CMD
enums['MAV_CMD'] = {}
MAV_CMD_NAV_WAYPOINT = 16 # Navigate to waypoint.
enums['MAV_CMD'][16] = EnumEntry('MAV_CMD_NAV_WAYPOINT', '''Navigate to waypoint.''')
enums['MAV_CMD'][16].param[1] = '''Hold time. (ignored by fixed wing, time to stay at waypoint for rotary wing)'''
enums['MAV_CMD'][16].param[2] = '''Acceptance radius (if the sphere with this radius is hit, the waypoint counts as reached)'''
enums['MAV_CMD'][16].param[3] = '''0 to pass through the WP, if > 0 radius to pass by WP. Positive value for clockwise orbit, negative value for counter-clockwise orbit. Allows trajectory control.'''
enums['MAV_CMD'][16].param[4] = '''Desired yaw angle at waypoint (rotary wing). NaN to use the current system yaw heading mode (e.g. yaw towards next waypoint, yaw to home, etc.).'''
enums['MAV_CMD'][16].param[5] = '''Latitude'''
enums['MAV_CMD'][16].param[6] = '''Longitude'''
enums['MAV_CMD'][16].param[7] = '''Altitude'''
pbarker@bluebottle:~$ 

We seem to have the same definitions twice?

[hamishwillee]

You'll have the definitions in every file "above" where the MAV_CMD was defined in the dialect chain. But they should be guarded so that they are imported only once, in the top level file. Is that not what you're seeing?

I.e. is that grep of one file, or showing the definitions across multiple files?

[peterbarker]

On Thu, 17 Jun 2021, Hamish Willee wrote: I.e. is that grep of one file, or showing the definitions across multiple files?

One file

[hamishwillee]

This is Python right? I ask because I tested on C.

Of course Python has to work too and I will look at this.

[hamishwillee]

@peterbarker Fixed for python. Identical-ish output now before and after: py_compared.zip

In summary, the original parser was flawed because it removed any duplicated enums from the nested dialect objects. The merged enums were aggregated in the object for the top level dialect file, which means that including the top level C dialect file works (i.e. ardupilotmega/mavlink.h) but not the nested ones (i.e. common/mavlink.h) and makes build order of libraries relevant. Further, it meant that cases like the original python library could just include the enums from all objects - because the duplicates were already stripped from the dialects and it would just get the single copy from the top level dialect file object.

My fix for C added the appropriate merged enums for each level - which means that you get the correct set for whatever dialect you include. But this means that the python generator was now appending the duplicated enums in each dialect (no longer stripped)

What this does now is re-run merging the enums in the Python parser. All the duplicates get chucked out.

This fix will need to be applied to anything else that relies on the defective stripping of nested object merged enums.

[hamishwillee]

So quick scan indicates that the same problem probably affects JavaScript at least. I wonder if we might be best off doing a "total" enums calculation in the top level and passing it to all the generators so they don't have to duplicate this code. Thoughts?

JavaScript_NextGen.zip

[hamishwillee]

@peterbarker Discussed this in the dev call with @auturgy. He raised the point, (which we are already aware of) that the fix to make C work cascades to all generators.

I made the point that I did this PR to verify the defective behaviour and test our appetite for "doing the right thing". I'm not the greatest coder and I would much prefer that someone else (you ideally) took this over. If not, then I am happy to progress this but would prefer to do so with your review of the architecture or some roll out plan.

We could for example create a pre-merged enum in the top level of mavgen and pass that to the generators like pymavlink to use instead of merging the individual files themselves. We'd have to update them all, but the code would be simpler.

Or we could make the changes I did for C-only. The other libraries would remain the same and also be strictly compatible "at the top level". The costs would be that a) anything constructed using a nested structure like C would still be wrong for the inner nesting levels, b) inconsistent implementation and a little confusing for anyone as to how they implement a new parser.

Anyway, have a chat to James and let me know how you want to progress this.

[davidbuzz]

As the author of large parts of JavaScript_NextGen, i'm here and paying attention, if i'm needed.

[hamishwillee]

That's great @davidbuzz .

I think we could modify this slightly to only impact C and Python now, allowing other runtimes to modify themselves at will.

But this is up to @auturgy - I can't invest more time without direction.

[peterbarker]

So it does look like this solution will cause issues in the javascript bindings and in the c-sharp bindings.

Is this just a C/C++-11 generation issue?

I'm wondering if we should actually stop generating any of this as multiple include files - so if you generate from ardupilotmega.xml you only get an includefile for ardupilotmega.xml - to get a common.h you'd need to generate from common.xml.

People probably won't bite on that one....

While I do see the problem here, and do agree that it needs fixing, we can't break random language bindings that people are happily using (or we get bug reports that have to be chased in short order)).

I'll keep looking at this.

[davidbuzz]

I have reviewed the output from this pr with respect to how it affects the current JavaScript bindings, and it looks good to me. ... by example... it leaves the output derived from common.xml as-is, but it increases the size of the output of 'ardupilotmega.xml and/or 'all.xml' by the appropriate number of formerly missing enums. happy for it to be merged if others are.

[hamishwillee]

This PR should work properly (now) for anything that builds per-dialect files, and break anything that generates a single flat library (other than Python which I specifically fixed). This will show up in the flat libraries as duplicate entries.

I think you're right that it is very risky to accept this, without knowing which libraries are affected.

We could however make this change restricted to C and Python and then build it out to other things following more precise testing.

People probably won't bite on that one....

IMO both the parser and the generators are much more complex than they need to be because the assumption is that you build all the dialects. I am not sure whether there are any particular benefits to resulting code either.

Upshot you're probably right, but it would not be me biting. I am awestruck by the cleverness of the code which I don't understand the justification for.

[hamishwillee]

Pretty please review @peterbarker

[tstastny]

@hamishwillee we just ran into the effects of not having this. @peterbarker can we revive this and move it forward?

[hamishwillee]

@tstastny FMI "where"/"how" did you run into this? Provided you build the top level dialect and use the top level dialect there are no issues. The only problem is when you build a dialect but then try to access the commands through a lower level generated library will you run into issues.

Right now https://github.com/mavlink/c_library_v2/ should be ordered such that if you take that library the you can use any level.

I have been nagging about this for years. @peterbarker What's the current story?

[tstastny]

@hamishwillee the issue comes when a dialect in the chain sets an enum. that means that only in the dialect header the enums are defined, so if one of the other generated headers (e.g. development) is included in some library (e.g. as in PX4 SITL..), but mavlink is built with a different dialect on the top level project (PX4), then it wont find the enums.

So in that case the only solution is to set the dialect as the include path for also the potential submodule lib (is the case in PX4 SITL). But that means a bunch of forks.

What would be better is if you can include any of the generated headers and still have access to the enums. Which is what I understood this PR to do.. but I could be misunderstanding.

I'm probably generally very poorly explaining this. @bkueng could you help add context?

[hamishwillee]

@tstastny So what is supposed to happen is that if you build a top level dialect - say ardupilotmega.xml, you can include ardupilotmega/mavlink.h and get the enums and entities for common.xml, standard.xml, minimal.xml. If you wanted to, from that same build, you should also be able to include common/mavlink.h and get everything that was defined in standard.xml, minimal.xml (but not ardupilotmega.xml) and so on.

However what happens is that if you build ardupilotmega.xml the merged enums, which include MAV_CMDs only appear in ardupilotmega/mavlink.h. If you import common/mavlink.h and ardupilotmega.xml has any MAV_CMDs then those are gone - they appear in the higher mavlink. You still get any enums that aren't merged.

If you want to use the libraries now, you should just take the top level one that has all the definitions you want and include the mavlink.h for that library. If you want to use a lower level library, rebuild for that xml and use that version.

The prebuilt library https://github.com/mavlink/c_library_v2/ SHOULD work correctly because it builds the high level libraries first - so they get all the enums - and then builds the lower level libraries (overlaying) so that each level has the correct enums, and so on. Testing indicates this works, but it may be that my testing wasn't good enough. Integrating this bug fix would mean that it would all work, but as I say, good to understand why it is needed (I don't from your explanation).

[tstastny]

@hamishwillee correct on all that you say. so the "why" in this case is particularly:

• PX4 built with a dialect will handle using that dialect on all includes within PX4 repo itself
• PX4_SITL is a submodule which specifically includes development/mavlink.h
• Since currently the enums will only appear in the higher dialect header, SITL wont work due to not having it's development enums, even though they do in principle still exist (just not in development header)

My contention is that it should not be necessary to go now down into that submodule and change to the high level dialect to make everything play nice together. But that as you say - the low-up-to-included header merged enums should be defined in all headers, such that if e.g. SITL submodule includes development/mavlink.h.. it still gets all enums development and down, despite the fact that PX4 itself was built with a higher level dialect/mavlink.h which may or may not add additional enums.

I believe this is also what you are saying.. but correct me if I'm wrong

[hamishwillee]

@hamishwillee correct on all that you say. so the "why" in this case is particularly:

• PX4 built with a dialect will handle using that dialect on all includes within PX4 repo itself

If it includes dialect/mavlink.h

• PX4_SITL is a submodule which specifically includes development/mavlink.h

You're saying that you build MAVLink once for PX4 using some dialect. If that happens to be development.xml then everything works. However if you build it with all.xml then PX4_SITL will miss the headers. Right?

Shouldn't PX4 SITL use the version of MAVlink you're building with? I.e. I would expect it to not be hard coded.

• Since currently the enums will only appear in the higher dialect header, SITL wont work due to not having it's development enums, even though they do in principle still exist (just not in development header)

Correct. Though to be very precise, it will be missing any enums merged with the higher layer, such as MAV_CMDs. It will get to keep non-merged enums.

My contention is that it should not be necessary to go now down into that submodule and change to the high level dialect to make everything play nice together.

I agree. Now we have to convince @peterbarker. This fixes pymavlink and c/c++ libraries but the other libraries might still not work. I think it is worth making them break so that they get fixed, or we might make the changes conditional on particular language. Or we might throw this all away, build the library into a single monolith, and stop pretending that the libraries can be used as we expect.

[auturgy]

ping @peterbarker