-
Notifications
You must be signed in to change notification settings - Fork 585
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
mavgen.py: correct handling of includes (updated) #436
mavgen.py: correct handling of includes (updated) #436
Conversation
@peterbarker Reminder. This would be damn nice! |
@hamishwillee this looks like an unintended but beneficial change?
And when generating mavlink1:
That change actually has interesting implications for anyone allocating memory based on it; malloc of zero bytes is problematic. |
898de0c
to
7e84610
Compare
- add large comment explaining algorithm used - add comments - remove some state variables which could be infered from other state variables - invert the return values of the helper functions, so "expand_oneiteration" returns true if it expanded something - removed a couple of bad comments - simplified a few state variables (e.g. inverting sense of one of them)
7e84610
to
f054337
Compare
OK, after reading through the code it's rather less daunting than I'd initially thought. I've folded up Hamish's trivial fixes (thanks!) back into @olliw42 's original commit. I've added to the repository a simple structure I used to test the generation - relatively easy to tell at a glance if things are working correctly and may form the basis of a regression test later. I've also added a patch on top in which I:
Hopefully I haven't broken anything! More testing tomorrow and then I'll probably merge (@tridge). |
Thanks @peterbarker . The comments will be very useful - it all makes sense, but since Ollie doesn't think like I do it took some parsing for me to understand. With respect to #436 (comment) This indicates that the payload sizes have changed (e.g. from 0 to 255), right? That needs to be sanity checked. I would expect any dialect that includes common.xml to be 255 so it is possible that the calculation was broken before. |
Merged! Thanks @olliw42 , @hamishwillee and @auturgy for your patience on this! |
Quite the opposite. These "mixin" files were getting an incorrect size of 255 (the diff is reversed) |
@peterbarker Thank you. Much appreciated. @olliw42 And you thought that our glacial slowness meant this would never happen :-) |
This supersedes #338 and #339. It updates @olliw42 recursive nesting code so I hope it can be merged. #339 will be closed.
Changes w.r.t 338.
@peterbarker I have taken the time to convince myself that this code works. I have also run both Ollie test code and some of my own for both c and Python. The only differences are those that are expected - ie it fixes up the parent dialect headers with the CRCs etc of the nested dialects.
I have not included the test xml because I'm not sure where it would make sense to do so if the testing cannot be automated - and this case would be difficult to do.
I will withdraw my original PR. What do we need to do to get this in?
@auturgy FYI