CRCS and lengths for mulitple inclusion levels #339

hamishwillee · 2019-07-12T06:20:33Z

The current code to include multiple dialect levels is a bit broken (which didn't matter too much because no one was using this).
The problem is that dialect CRC lists have to include the CRCS of all included files (because the various bits are only included/#defined in the C code once). The previous version only added the previous parent CRCs.

What this code does is add all the CRCs for all the parents. It does this by calculating all the parents of each file in the first iteration. Then it works runs check to find all the children from all the parents. At that point it iterates the xml files a final time to add CRCs and other files.

I'm still running tests, but I think this is good for review.

@peterbarker ?

hamishwillee · 2019-07-15T01:16:48Z

From Ollie - I have not yet analysed.

@hamishwillee
as regards the generator, I think your version is NOT correct. Your main subfolder may come out correctly, but I think you miss to see that your include subfolders are going to be incorrect! E.g., for ardupilot.xml, the files in the ardupilot subfolder may be correct, but I don't think the files in the other subfolders will be identical to then you would start your generator with them as root!!! But this is how it has to be, anything else is a bug. Please carefully compare the result of my version to the result of your version as regards ALL generated files, compared to when the an included file is used in the generator. You when will also find that in my version the external correction for payload length is not needed, another indication of that it does something more correct. You generally can assume that there are reasons for what I do. :)

hamishwillee · 2019-07-15T03:41:39Z

generator/mavgen.py

-            # stop when loop doesn't add any more included files
-            break
-
-    # work out max payload size across all includes


This did max payload of all dialects. I think incorrect so removed. Should be max of payloads that current dialect includes. Note the assumption (which appears to be true by inspection) is that the the value is set by the top level dialect.

hamishwillee · 2019-07-15T03:46:40Z

@olliw42

as regards the generator, I think your version is NOT correct. Your main subfolder may come out correctly, but I think you miss to see that your include subfolders are going to be incorrect! E.g., for ardupilot.xml, the files in the ardupilot subfolder may be correct, but I don't think the files in the other subfolders will be identical to then you would start your generator with them as root!!! But this is how it has to be, anything else is a bug. Please carefully compare the result of my version to the result of your version as regards ALL generated files, compared to when the an included file is used in the generator.

I am fairly sure that my code is correct in this respect.

I ran both my version and your version of the generator separately against each of the dialect files and compared between dialects and generator outputs. The output of both generators are the same when any of the files has the same root.

You when will also find that in my version the external correction for payload length is not needed, another indication of that it does something more correct. You generally can assume that there are reasons for what I do. :)

It may well be that Peter will like your approach more. That's fine by me - he will almost certainly require you to correct some of those issues highlighted first.

hamishwillee · 2019-07-15T03:51:48Z

@peterbarker I've run a bunch more tests on this and I am convinced that:

Existing generation of current files will not be broken by this.
This correctly generates the CRCs and other values such that parents get all the values of their children.
I tested on Python, Java, C.

So this is ready for your review.

peterbarker

There are more flake8 warnings in this file than when you started.

Commit messages needs help. And probably only a single commit.

I generally understand what's going on in this PR and it seems reasonable. It also doesn't trivially break ArduPilot's autotest, which is a plus.

peterbarker · 2019-08-06T10:01:29Z

generator/mavgen.py

@@ -64,38 +64,82 @@ def mavgen(opts, args):
            print("WARNING: Unable to load XML validator libraries. XML validation will not be performed", file=sys.stderr)
            opts.validate = False

+


Extra whitespace.

peterbarker · 2019-08-06T10:05:03Z

generator/mavgen.py

+        """Expand includes. XML files from arguments already parsed inot xml list."""
+
+        def expand_oneiteration():
+            noincludeadded = True


Suggested change

noincludeadded = True

includeadded = False

generator/mavgen.py

peterbarker · 2019-08-06T10:07:07Z

generator/mavgen.py

-                x.message_target_component_ofs.update(xml[-1].message_target_component_ofs)
-                x.message_names.update(xml[-1].message_names)
-                x.largest_payload = max(x.largest_payload, xml[-1].largest_payload)
+        """Expand includes. XML files from arguments already parsed inot xml list."""


Please translate into English.

generator/mavgen.py

peterbarker · 2019-08-06T10:10:27Z

generator/mavgen.py

+            noincludeadded = True
+
+            for x in xml[:]:
+                try:


This can be rewritten using getattr; see example in other comment.

peterbarker · 2019-08-06T10:15:00Z

generator/mavgen.py

+                        continue
+                    all_files.add(fname)
+                    # Validate XML file with XSD file if possible.
+                    if opts.validate:


We're also validating at the top level. What's up with that?

We've validated the top level files already. This is validating (just) the included files as well.

peterbarker · 2019-08-06T10:30:56Z

generator/mavgen.py

+                    parsed_xml = mavparse.MAVXML(fname, opts.wire_protocol)
+                    #add all parent(s) into included file
+                    parsed_xml.parent=set()
+                    parsed_xml.parent.add(x.basename)


You appear to be adding apples and oranges to the same set here. Probably just the basename is supposed to go in?

No, this is actually intentional - though it might not be the best way to do it.

The x.basename is just the immediate parent. The parsed_xml.parent.update(x.parent) is built up from all previous parents (and is what I am really after). You need both so that this works when run on the first included file.

So basically this builds up the full parent tree at each level. Later on I use it to generate the full child tree at all levels, and that is what gives me the ability to calculate CRCs etc.

peterbarker · 2019-08-06T10:32:17Z

generator/mavgen.py

+                    noincludeadded = False
+            return noincludeadded        
+
+        for i in range(MAXIMUM_INCLUDE_FILE_NESTING):


Throwing an error if we go too far would seem to be a good idea.

We can't go too far - MAXIMUM_INCLUDE_FILE_NESTING is the maximum expansion.

Not sure if you really mean throw, so have added this code on next line:

if expand_oneiteration(): print("ERROR Nesting level greater than maximum: %s" % MAXIMUM_INCLUDE_FILE_NESTING) exit(1)

So basically if there is more expansion that can be done this will do it and exit. Otherwise this will run but do nothing (no new files).

peterbarker · 2019-08-06T10:33:04Z

generator/mavgen.py

-                x.largest_payload = max(x.largest_payload, xml[-1].largest_payload)
+        """Expand includes. XML files from arguments already parsed inot xml list."""
+
+        def expand_oneiteration():


is there some reason not to go fully recursive here?

Yes, effort and clarity. The code that gets expanded here is discrete and separate from the rest.

hamishwillee · 2019-08-21T05:08:30Z

Thanks for your review @peterbarker. I've fixed the issues I could fixed and answered the open questions. I also ran flake8 and fixed all the whitespace errors. I left in the lines too long and module in wrong place issues, as they pre-exist anything I did.

I re-ran the tests for C library (only) and the output matches what I had before. I also tested with an extra level of include to verify that this catches the case where things are too deeply nested.

Thoughts?

olliw42 · 2019-08-21T06:23:36Z

I am sorry to be the pain in the ass, but I really don't understand what you guys drives you here. I would think that one wants to listen to arguments.

You don't want your "compiler" to only work for a particular limited structure of the include tree, you want your compiler to (1) correctly resolve ANY include tree which is resolvable and (2) throw an error for ANY include tree which is not resolvable. With really nothing inbetween like throwing an error for an include tree which is resolvable or not throwing an error for an include tree which is not resolvable (and producing code for this).

This to me sounds so simple and obvious that I struggle to see why you guys even would bother to argue, or not see.

The above algorithm/code fails in this simple regard.

Given that, and given that there is a suggestion for an algorithm/code which exactly already does it, it's simply a mystery why one would want to spend time on an incomplete algorithm/code. There are certainly many algorithms to do the resolving of the include tree, and the suggested one is just one of them, but so far no one has provided a functional argument against it, which even more raises the question why one would spend time on this here. It seems the only argument for clinging to the above is "we don't like it, or the messenger". But not doing a correct code for that reason really doesn't point to me as the kid in the block.

Frankly, this effort here comes across as driven by stubbornness. Doing a correct code obviously isn't the driving force.

With having made this effort to drive you guys into a direction which results into proper solutions I will TO of the topic. Good luck to all.

hamishwillee · 2019-08-21T11:53:16Z

Hi @olliw42,

I'm my fixing review feedback. That is what a good community member does to make it easier to integrate.

As to the rest, really not up to me. I don't really understand your code, and hence reasoning why it is better. That's why I deferred to Peter.

hamishwillee · 2020-07-20T05:32:47Z

Superseded by #436

CRCS and lengths for mulitple inclusion levels

4d3205a

hamishwillee mentioned this pull request Jul 12, 2019

mavgen.py: correct handling of includes #338

Closed

hamishwillee mentioned this pull request Jul 15, 2019

COMPONENT_INFORMATION mavlink/mavlink#1176

Merged

hamishwillee added 2 commits July 15, 2019 12:50

remove first instance largest payload

2f018c3

Fix up CRC calculation

01a4cc8

hamishwillee commented Jul 15, 2019

View reviewed changes

peterbarker requested changes Aug 6, 2019

View reviewed changes

Flake8 and review fixes

ccd4dfd

hamishwillee mentioned this pull request Jul 20, 2020

mavgen.py: correct handling of includes (updated) #436

Merged

hamishwillee closed this Jul 20, 2020

hamishwillee deleted the fix_crcs branch July 20, 2020 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRCS and lengths for mulitple inclusion levels #339

CRCS and lengths for mulitple inclusion levels #339

hamishwillee commented Jul 12, 2019

hamishwillee commented Jul 15, 2019

hamishwillee Jul 15, 2019

hamishwillee commented Jul 15, 2019

hamishwillee commented Jul 15, 2019

peterbarker left a comment

peterbarker Aug 6, 2019

peterbarker Aug 6, 2019

peterbarker Aug 6, 2019

peterbarker Aug 6, 2019

peterbarker Aug 6, 2019

hamishwillee Aug 21, 2019

peterbarker Aug 6, 2019

hamishwillee Aug 21, 2019 •

edited

Loading

peterbarker Aug 6, 2019

hamishwillee Aug 21, 2019

peterbarker Aug 6, 2019

hamishwillee Aug 21, 2019

hamishwillee commented Aug 21, 2019

olliw42 commented Aug 21, 2019

hamishwillee commented Aug 21, 2019

hamishwillee commented Jul 20, 2020

		@@ -64,38 +64,82 @@ def mavgen(opts, args):
		print("WARNING: Unable to load XML validator libraries. XML validation will not be performed", file=sys.stderr)
		opts.validate = False

CRCS and lengths for mulitple inclusion levels #339

CRCS and lengths for mulitple inclusion levels #339

Conversation

hamishwillee commented Jul 12, 2019

hamishwillee commented Jul 15, 2019

Choose a reason for hiding this comment

hamishwillee commented Jul 15, 2019

hamishwillee commented Jul 15, 2019

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee commented Aug 21, 2019

olliw42 commented Aug 21, 2019

hamishwillee commented Aug 21, 2019

hamishwillee commented Jul 20, 2020

hamishwillee Aug 21, 2019 •

edited

Loading