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

Tools: scripts: Update MAVLink parsing for wiki inclusion #27226

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ES-Alexander
Copy link
Contributor

@ES-Alexander ES-Alexander commented Jun 3, 2024

ArduPilot Wiki uses reStructuredText (rst) source files, so supporting rst output is essential for this script to generate wiki files. Ideally this can be run automatically for every vehicle type and version, like the existing parameter lists.


EDIT3: Wiki inclusion and future planning notes moved to ArduPilot/ardupilot_wiki#6065

@ES-Alexander ES-Alexander changed the title Update MAVLink parsing for wiki inclusion Tools: scripts: Update MAVLink parsing for wiki inclusion Jun 3, 2024
@ES-Alexander
Copy link
Contributor Author

@amilcarlucas where/when are you expecting/wanting yours to run?

As far as I'm aware the parameters repository already generates all the apm.pdef.xml files (with CI checks for new tags to process every 5 days), so I expect it could be reasonably straightforward to add your rsync updates there as just an extra step?

I'm not sure how much sense it would make to combine efforts on a PR in that repo, given your feature is an additional endpoint for the parameters, whereas mine is a new script to run and store the outputs of, but if you're also wanting to include the MAVLink message support in your rsync transfers then I suppose that ties them together somewhat?

@amilcarlucas
Copy link
Contributor

Nevermind, I jumped to conclusions.

@ES-Alexander ES-Alexander marked this pull request as ready for review June 10, 2024 12:59
@ES-Alexander
Copy link
Contributor Author

Did some more digging and found that adding a version selection dropdown makes more sense to handle as a patch during generation in the specific applications where it's relevant. Since both of the remaining "planned" components of this PR can be handled in separate repositories, I think this is ready for review / can be merged in 🙂

@ES-Alexander
Copy link
Contributor Author

Not really sure who should review this PR, but @Hwurzburg might have some ideas or preferences around the formatting of the generated files 🤷‍♂️

@Hwurzburg
Copy link
Collaborator

Was all the formatting in the example sub file generated by this script?

@ES-Alexander
Copy link
Contributor Author

Was all the formatting in the example sub file generated by this script?

Yeah - the .rst example file is just directly from the script output :-)

I generated it from the Sub-4.1 branch with the following invocation:

python mavlink_parse.py -v ArduSub --format rst -b Sub-4.1 --header ardupilot_wiki -gucq

(noting once again that the script requires Python 3.11 to run)

@Hwurzburg
Copy link
Collaborator

I've tagged @peterbarker and @IamPete1 to review if the groupings and generated data is correct....with PeterB's help I could add per vehicle pages into the Dev sites wiki under a MAVLink commands topic....

ArduPilot Wiki uses reStructuredText (rst) source files, so supporting rst output is essential for this script to generate wiki files.
@ES-Alexander ES-Alexander force-pushed the mavlink-messages-rst branch from 199f475 to adf87f3 Compare June 11, 2024 19:03
@ES-Alexander
Copy link
Contributor Author

I've tagged @peterbarker and @IamPete1 to review if the groupings and generated data is correct

I've just rebased over master, and generated some more rst files for different vehicle types, to make that checking a bit more straightforward:

MultiVehicle_master_MAVLink_Messages.zip

with PeterB's help I could add per vehicle pages into the Dev sites wiki under a MAVLink commands topic....

Nice! 😃
I'm happy to make a PR to generate these automatically in what is currently the parameters repository, if that's helpful, but also happy for some other approach to be taken.

I'd note that because the MAVLink protocol updates, and I'm currently including the "unsupported messages" in the output generation, the outputs for each version should ideally be re-generated each time with the latest script version, and the most recent pymavlink release, rather than tying the script to the vehicle branch and only generating once per version. That said, that's not a critical requirement, and generating once per version could be ok if we either accept that newer messages won't show up, or just don't generate the unsupported messages - I just like the idea of being able to search for a message and finding out clearly if it is definitely unsupported 🤷‍♂️

@Hwurzburg
Copy link
Collaborator

having to manually update the page generation once in a while is fine for now, IMO....a PR to generate the page would be good, then I can add it to the Dev wiki.....very few users, and only devs, will be using it but it will certainly help them, even if occasionally out of date...

@ES-Alexander
Copy link
Contributor Author

a PR to generate the page would be good

Initial files are now available for the ≥ 4.0 tags in the parameters repository :-)

They'll still need some re-jigging of the anchors (and insertion of a version selector dropdown) before they can be included in the wiki, but that'll be very similar to the existing process for parameter files, that happens during wiki generation.

@Hwurzburg
Copy link
Collaborator

sorry its taken me so long to get back to this, but I wanted to add wiki files for Plane,Copter,Rover messages as of current master....the params repo does not even have files for 4.5...could you generate these three and I will ad to dev wiki...thanks

@ES-Alexander
Copy link
Contributor Author

sorry its taken me so long to get back to this

I get it - unfortunately the list of interesting/useful things to do seems to grow much faster than the available time to do things 😅

could you generate these three and I will ad to dev wiki

It's been a while since I did those initial generations, so I'll need to figure out the relevant setup/process again. I'm hoping it's mostly covered by the Action I made in the parameters repo, although from memory that's reliant on this PR being merged, so I might need to make some custom branches to do the manual generations for now.

I'll try to sort that out this week :-)

I wanted to add wiki files for Plane,Copter,Rover messages as of current master....the params repo does not even have files for 4.5...could you generate these three

To confirm, did you want files for just the 4.5 release, or current master as well?
If I get to doing this before you respond I'll just generate both - I don't expect it to require much extra effort once I have the process working :-)

@Hwurzburg
Copy link
Collaborator

4.6 is about to drop...so master only will be fine

@ES-Alexander
Copy link
Contributor Author

Just got around to this.

4.6 is about to drop...so master only will be fine

Here ya go 🙂
MultiVehicle_master_MAVLink_Messages_Nov-5.zip

the params repo does not even have files for 4.5

Following up on this, it did have 4.5 files, per my initial pull request to that repo. Those files then got auto-removed as part of a recent metadata update, and weren't regenerated because the process for doing so is reliant on this PR being merged so the relevant parsing script option (rst format) is available. Until this PR is merged that will continue to happen, unless I make a hacky workaround that references a branch in my ardupilot fork, which would then need to be updated again once this PR is merged.

Given you're willing to put its outputs in the wiki, is there anything holding back this PR from getting merged?

@Hwurzburg
Copy link
Collaborator

@ES-Alexander no, just @peterbarker approval...will work on adding the files to the wiki that you generated....thanks

@Hwurzburg
Copy link
Collaborator

Hwurzburg commented Nov 7, 2024

@ES-Alexander PR for wiki inclusion...thanks had to reformat a few things in the files....ArduPilot/ardupilot_wiki#6379

@ES-Alexander
Copy link
Contributor Author

PR for wiki inclusion...ArduPilot/ardupilot_wiki#6379

Nice! Thanks for adding those in :-)

... had to reformat a few things in the files.

The generated files? If so, do you remember what you had to change, and are they things we should be able to edit the generator script to provide from the start?

@Hwurzburg
Copy link
Collaborator

the tag must be different in each file....I retitled the headers

@ES-Alexander
Copy link
Contributor Author

I have a vague recollection I made those the same on purpose, with the idea they'd be replaced automatically as part of the version selection support. That said, I may be misremembering, and it should be possible to at least roughly do something similar as part of the initial generator script if we want to, since it does already know the vehicle(s) being generated for, and the branch.

@Hwurzburg
Copy link
Collaborator

Hwurzburg commented Nov 8, 2024

not required at this point, I can manually run every major release to keep up to date.....going to ask @peterbarker merge as is...thanks

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to handle recursion

@@ -7,7 +7,7 @@
from dataclasses import dataclass, astuple

from pymavlink.dialects.v20 import (
common, icarous, cubepilot, uAvionix, ardupilotmega
common, icarous, cubepilot, uAvionix, ardupilotmega, development
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be just be 'all'? either that or needs minimal
does this end up with lots of duplicates via the recursion of ardupilot -> common -> minimal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be just be 'all'?

It's nice to break them out into dialects, so that the message definitions can be linked to directly within the MAVLink documentation.

either that or needs minimal

The messages defined in minimal are duplicated in the common doc, so I didn't think it was necessary to break it out further (because the links already work fine).

Happy to make that change if you think it's preferable to link to the minimal docs instead of the common docs for those couple of messages. Thoughts?

does this end up with lots of duplicates via the recursion of ardupilot -> common -> minimal?

Nope - the dialects are checked in order (see determine_dialect), so it just stops when it gets to the first one that matches. The MAVLinkDetector class maps relevant messages to their dialects (and corresponding documentation links) as needed, so the only message duplicates are if the firmware handles the same message in different ways that are checked for (e.g. SYSTEM_TIME can be incoming, (automatically) outgoing, and also requestable, so appears in all three tables).

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just try all.xml

Comment on lines +204 to +205
except FileNotFoundError: # Broken symlink
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we ignore this?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I remember having a reason for it back when I added it, but not exactly what that was. My vague recollection is it was related to an unused ardupilot library import that wasn't linked to / removed properly in one of the vehicle branches (possibly Sub?).

Whatever it was was stopping the parsing from completing, and seemed out of scope for this script to solve, so I added the check and moved on.

If you want more context I can try removing the check and re-running the script for every firmware type and version, to see if I can replicate the original issue?

@@ -7,7 +7,7 @@
from dataclasses import dataclass, astuple

from pymavlink.dialects.v20 import (
common, icarous, cubepilot, uAvionix, ardupilotmega
common, icarous, cubepilot, uAvionix, ardupilotmega, development
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing minimal.xml and/or parsing of include files and use all.xml?

Check Wiki to see if HEARTBEAT is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying part of my response to Tridge from above:

The messages defined in minimal are duplicated in the common doc, so I didn't think it was necessary to break it out further (because the [documentation] links already work fine).

Happy to make that change if you think it's preferable to link to the minimal docs instead of the common docs for those couple of messages. Thoughts?


... parsing of include files ...

Not sure what you're referring to here - this script imports the dialects from pymavlink; it doesn't know about the underlying xml files, and just checks against which messages pymavlink knows about.

In the case of sub-sets (like common < ardupilotmega), as long as the checks are performed in the correct order the relevant dialect is determined as the first match, and can be used to generate a suitable link to the MAVLink docs. It would be nice to directly query the pymavlink.dialects.v20 import for all the available dialects, but they're not stored in completeness order, and some aren't desired (e.g. test) so manual imports seem necessary at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants