Replace iterative IDX with hash of dialect name to remove build order dependencies

[hamishwillee]

The C generator assigns to each nested dialect an increasing MAVLINK_THIS_XML_IDX. In the mavlink.h at each level it defines the same value of MAVLINK_PRIMARY_XML_IDX and uses the comparison to decide if it should define the arrays MAVLINK_MESSAGE_INFO and MAVLINK_MESSAGE_NAMES.

So whichever dialect you import (e.g. common.xml) the MAVLINK_THIS_XML_IDX==MAVLINK_PRIMARY_XML_IDX and you pull in the info and names for the current dialect. However they don't get pulled in again, because the values ONLY match for the mavlink.h you pulled in first.

However this is fragile in that if you generate (say) ardupilotmega.xml and common.xml the libraries will be different, and depending on the order you can end up with two libraries with the same IDX value. Consider building ardupilot first and you get these IDX values at each level.
ap primary 0
common 1
min 2

If you then build common at the same location you end up with:
ap primary 0
common 0
min 1

So if you import ap, when common is loaded it will try to reimport the definitions.

What this PR does is replace this complicated approach with an #ifndef MAVLINK_MESSAGE_INFO. That way the info is defined when only once, in the top level file you call first.

[hamishwillee]

Well, some depressing test fails. Wonder what I missed.

[hamishwillee]

@peterbarker I can see that the tests aren't working properly because messages generated with this are all empty sysid:11 compid:10 seq:5 EMPTY { }.

To me the logic of this change looks sane. Can you help me debug (I can't run this locally or find any docs on how the tests work)?

[hamishwillee]

@auturgy This is worth fixing but I don't know how - independent of the enum problem. Essentially we use a distinct index number at each level of dialect inclusion to determine whether enums should be included. This contravenes the assumption that generated dialect files should be the same whether generated at top level or as included file.

Elsewhere we have just used the first inclusion to set the def, so we pull in the top level definitions/first time definitions included, whatever they happen to be. Would be good if @peterbarker could look at this at some point.

[peterbarker]

@hamishwillee surely this #ifndef approach can't work as-written as the "base includes" must already have defined it. That would fit with what you saw in the tests, I think - where most of the messages are empty. To (try to be) clear, ardupilotmega.xml includes common.h which includes minimal.h - minimal.h sees that MAVLINK_MESSAGE_INFO isn't defined so defines it - then we resume processing of common.h and it sees MAVLINK_MESSAGE_INFO as being defined (as minimal just did that), and likewise ardupilotmega.h now sees it as defined.

I think your PR might be fixed by just doing the base includes after doing the define?

Various comments indicate some semantics for mavlink.h - where you can generate for multiple xml files and nominate one as "primary". Your patch removes those semantics. Those comments should also be adjusted, I think, and the change in semantics explicitly called out in the PR message here....

[hamishwillee]

@peterbarker Thanks for reviewing. Yes to all. I'll try fix up Weds or Thurs.

[hamishwillee]

@peterbarker EDITED.
I updated this as suggested. I now understand why it was done with IDX, and why this fix doesn't work.

The process needs to include all the identifiers in all nested dialects, and then build the array of them once it has all the identifiers.

The code I had broke because it imported the bottom level identifiers, created the array, then imported the higher level identifiers - but could not redefine the array. I don't think this approach can work because after the array is imported we also import a file that uses it - that HAS to only happen once at the top level.

The new code doesn't work either. It builds the table of all message info identifiers correctly in the top level, but at this point the generated C code is invalid because it does not know what those identifiers are (because it hasn't included the dialect files yet).

The original code solved this by having index comparisons that only matched at the top level. So the top level first includes the base files to get all the identifiers it needs and then creates the array - this array code is present in the base files, but is never true because the indexes don't match.

The only reason that the above approach is a problem is because when we overlay multiple rebuilds the index numbers change and we can end up with several dialects that might have the same IDX numbers.

A better solution is therefore to give them unique numbers that can't overlap. We can't do a string match in preprocessor macros but is there a way to create a numeric hash of the name?

If not, the obvious options are to use C functions to build the table or to just build the whole library in one C file.

Thoughts?

[hamishwillee]

@peterbarker Tests pass. Works :-)

This now uses the Python inbuild hash() function to generate the hash for each file. To match this change I modified the name of the definitions to have extension _HASH (rather than _IDX) - otherwise it would have just been a one line change.
Provided that we don't use duplicate dialect names this should be unique for any build (very low chance of collision).

Should fix the problem because the number will always be the same for building a particular dialect.

[hamishwillee]

@peterbarker @auturgy Could I please get this reviewed?

This will fix https://github.com/mavlink/c_library_v2 builds . I know several who have already worked around the problem in various ways, but would be good to fix this for those who might not have done so yt.

[peterbarker]

Rebased, squashed the commits and fixed the commit message a little.

I really like this change.

I think we might be emitting the undef/define for the hash when we don't need to.

Can you think of a reason we couldn't emit MAVLINK_COMMON_XML_HASH and MAVLINK_MINIMAL_XML_HASH instead of having to undef/redef MAVLINK_THIS_XML_HASH all the time?

I'm inclined to merge this as is, however.

[hamishwillee]

@peterbarker Yes, it's a good fix, provided that we never allow a file to be included with the same name. If we did that we'd have to hash the full file path.

Can you think of a reason we couldn't emit MAVLINK_COMMON_XML_HASH and MAVLINK_MINIMAL_XML_HASH instead of having to undef/redef MAVLINK_THIS_XML_HASH all the time?

I "think" that would work - you would of course still have to def the value. If it works it would result in much more readable output code.
So I think it would be an optimization worth trying.

My only concern is that it is very easy to screw up. If you merge this though I'll try a follow on patch fix later today.

[peterbarker]

Merged, thanks!