-
Notifications
You must be signed in to change notification settings - Fork 17.2k
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: create mavlink_parse.py #22278
Tools: scripts: create mavlink_parse.py #22278
Conversation
efdd6fc
to
d95d071
Compare
Very nice!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
73881bb
to
1693bef
Compare
@Hwurzburg no idea if you're relevant for this getting merged, but thought you might be interested to know about it, in case it's useful for the ArduPilot wiki :-) |
bc06531
to
ca122bb
Compare
can you give some sample command lines? not quite sure what you are parsing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please give command line example
ca122bb
to
dcb18dd
Compare
I've spent some time today doing cleanup, as well as adding some extra functionalities. Parsing overview
Running Examples
Sure :-)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It would be nice to merge the development commits before merging the PR
- A list of valid
MAV_CMD
supported messages would be really nice, since a good part of the vehicle functionality is under it- MAV_CMD_COMPONENT_ARM_DISARM, MAV_CMD_DO_SET_MODE..
I think you mean "squash" the commits into one....a normal requirement...only one commit per library element (ie Tools in this case) |
@patrickelectric thanks for the review :-)
Should they just all be squashed together?
I agree, which is why I added that in the latest set of updates - I'm assuming you didn't see that part? :-) |
thanks for the examples...will help writing the wiki info for this |
Yes. If the code is not yet on the master branch, there is no reason to have development commits for such script on it.
By 3 minutes 😄 |
dcb18dd
to
f34370e
Compare
No worries @Hwurzburg :-) That said, I was more expecting that the output of this script could potentially be useful to include in the wiki, rather than necessarily needing info in the wiki about the script itself :-) |
f34370e
to
dae6e42
Compare
I've added a default introduction for the markdown output, so it's not just straight into the tables. I'm done now - happy for this to be merged if there are no outstanding changes that need to be made :-) |
25055f1
to
b934926
Compare
let us know when you are finished changing... |
@Hwurzburg There's nothing left that I'm planning to do. I ended up doing two unplanned updates because I found a couple of bugs, and I wasn't sure if the failed plane test was holding up merging (even though the tests are completely unrelated to the functionality here). |
It would make sense to run this in the build pipelines so that the webpages are kept up-to-date. |
this does not run for me: |
@Hwurzburg sorry for the delay on this - I've been away and just got back.
Python's If you have already installed Python 3.11 it is possible it is not mapped to the If it's important for you to be able to run programs from multiple Python versions (especially for testing purposes) then you may need to look into how to do so for your operating system. I personally use |
Given the trouble we have getting people off python 2 even, requiring 3.11+ specifically doesn't seem great to me. 3+ would be a reasonable requirement IMO. My $0.02 only! |
While compatibility with existing systems is nice, and is valuable by reducing update friction, not allowing developers to use modern features and tools discourages them from contributing, and also means the entire technology stack is constantly held back by the most restrictive user's configuration, instead of being enabled by technology improvements as they're released. I agree it generally doesn't make much sense to intentionally break existing functionality on old systems just to refactor (e.g. to use an improved syntax or data structure), but I equally don't think new features should be necessarily beholden to old systems and technologies. If new features can't enter the codebase via even a benign program like a standalone script that generates some useful documentation (which most users will likely never know about or need to run), then I fear the message that sends to potential contributors - especially those starting out, who learn to program and then get told their tools are too good/nice to be allowed in a given project (like not being able to use a calculator for a maths assignment, except not even for the supposed purpose of learning). As I understand it, software requires updates to get new features, and sometimes new features are reliant on improvements in the underlying technologies, and aren't available without a recent enough update of the base. That doesn't seem unreasonable (e.g. a nail requires only a hammer, but having a hammer doesn't mean it's not also valuable to get a screwdriver so you can use screws). As is, the significant technical debt in ArduPilot's Python tools, caused/required by the extensive backwards compatibility, is one of the primary reasons I don't contribute more to the ArduPilot codebase. I love the mission and open source nature of ArduPilot, and at times want to contribute, but I like the programming tools and features I'm used to, and feel like it's a regression to not be able to use them (not to mention the additional reasoning and programming time required to develop workarounds, and often hacky homebrew alternatives). Accordingly, I either avoid making any changes (and just be a bit disgruntled with ArduPilot while I instead work on other projects where I have more development freedom), or I develop features in a local branch and don't bother trying to contribute upstream on the grounds that there'll likely be pushback that I don't have the time or motivation to deal with. I appreciate and understand your 2 cents - hopefully my perspective and experience are also valuable :-) |
I agree with @ES-Alexander and share the same feelings as developer. As open source organization, adopting and improving is one of the best ways to have more collaborators and engage the community. Starting with a standalone script is the best way to go to slowly adopt new python versions. Maybe @khancyr and @Williangalvani wants to add something on the discussion |
I don't think we should continue support py2, and we already have some tools that don't support it. But we should focus was is on the latest Ubuntu LTS I would said and not trying to get all the latest features, as that is what we propose to install anyway. So max python 3.10 for now. |
Fair enough
I don't see why Ubuntu's system defaults get to determine the dependencies used for all operating systems, but that's not my call to make. As far as I'm aware it's generally not recommended to use the system Python install for user programs, so at least in my head this is a question of "which Python version should be installed?", rather than "can we avoid people needing to install Python on Ubuntu?".
Ok. I believe
I don't really understand the reasoning here (i.e. why would it be an issue?). Is the idea that an active Ubuntu LTS version should be able to fetch and make use of any updates to the ArduPilot codebase without needing to install or update any dependencies or programs? If so, why is there a script to install prerequisite packages and libraries for Ubuntu - by that reasoning shouldn't they already be built into Ubuntu, or be included directly (or as submodules) in the ArduPilot codebase? Or is the idea just that it should be possible to install prerequisites once, and not need to do any updates until also updating to a new version of Ubuntu? If so, why is it ok to need to fetch new code to get new tools/features, but not ok to also need to update dependencies to be able to use those new tools/features? |
b934926
to
e697e28
Compare
Fixed a typo, improved some descriptions, and improved sort order from initial output usage feedback. |
To the subset of Ubuntu folks who aren't currently managing multiple Python versions, you should be able to test this by installing Python 3.11 through I'm not a huge fan of blanket shell scripts, but it does at least make things easy to follow... Something like this should work, without generally changing your system behaviour afterwards (beyond being able to use pyenv again later without needing to re-install it): # Install pyenv
curl https://pyenv.run/ | bash
# Restart the shell to ensure its aliases are available
exec $SHELL
# Use it to install the desired python version
pyenv install 3.11
# Make sure you're in the ArduPilot directory
# Can navigate into `Tools/scripts/` if you want to be extra specific
# -> but would need to adjust the running line appropriately
cd /path/to/ArduPilot/repo/
# Set local Python version (for cwd and its children) temporarily
pyenv local 3.11
# Make sure latest pymavlink is installed
python -m pip install --upgrade pymavlink
# Run the MAVLink parser script, with whichever arguments you want
python Tools/scripts/mavlink_parse.py --help
# Revert local Python version to system python
# As mentioned, I don't actually think this should be being used,
# but you can continue doing so if you want.
pyenv local system |
Ubuntu is the most used Distribution on robotics and for ArduPilot, that is what we recommand as starter and the default propose installation is LTS. Bringing new feature to ArduPilot is great and we love your contribution, but we shouldn't forget that a large part of the ArduPilot community doesn't know a lot about programming, so having to deal with multiple Python version is a pain.
I will try to push a patch for this as the script is really interesting and I don't want to lost it into the PR pile...
As said previouly, most people don't really know what all ArduPilot ecosystem is, so they just run the install script blindly once and trust us that is work. Fetching new code is generally fine as we don't need to install anything, here we need a specific python version. We cannot enforce that for now, and dealing with an update with ask a lot of time on support, so that is best to avoid. So in resume we aren't opposed to this, on contrary. But we need to rework it a bit to lower the python version requirement as that will make it available simply for most and don't break any feature. |
Hi @khancyr, I just saw your reply.
I'm not suggesting that people should need to actively manage multiple Python versions - I'm querying whether using the Ubuntu system Python as the required Python for everyone is reasonable when there's a more recent stable Python version available. The macOS 'install-prereqs' script installs pyenv and a relevant Python version. I don't understand why Ubuntu couldn't or shouldn't do the same, particularly given using a system's Python install is generally not recommended (as far as I'm aware).
This program is for generating some useful documentation, and will likely be used by very few users. It seems reasonable (to me) that new users could start on a Python version that supports it, and old users who want access to the new functionality could upgrade. This kind of incremental upgrade seems significantly less painful than requiring lots of people to update at once at a particular event that forces lots of things to lose their backwards compatibility, and attempting to avoid such an event seems like it could be a significant driver that's preventing adoption of up to date programming language and library versions in the ArduPilot ecosystem.
I still don't really understand or agree with the reasoning to push back the language version here, but I appreciate that you're approaching this in good faith, and Python 3.10 at least has many of the more recent features as compared to something like 3+, and is still receiving security updates. If it's helpful, I believe the uses of from enum import Enum # TODO: replace with StrEnum once Python 3.11 available
class StrEnum(str, Enum):
""" Temporary filler until Python 3.11 available. """
def __str__(self):
return self.value |
I think that this is used by very few users. And it should go in as it is. |
@peterbarker / @tridge, do you guys have any input on the Python versioning compatibility requirements discussion here? As it stands,
|
e697e28
to
4f8e67d
Compare
A parser that finds incoming, requestable, and outgoing MAVLink messages for each vehicle. May not indicate full support, but at least shows the messages which are handled in the code. Optionally also: - finds incoming commands - finds unsupported messages (and commands) - breaks out messages in the search groups for the selected vehicle - allows specifying a header for the markdown file output Requires Python >= 3.11
4f8e67d
to
373f6f1
Compare
Force pushed to improve some error messages if expected things/versions aren't installed, and to rebase over upstream |
we merged a change this week to require python 3.8 to build ArduPilot. |
Great to hear! :-)
That wouldn't actually help (much) - It could help marginally for people on 3.10, but I've also added a comment for the
This comment (from above) should be helpful for that :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super useful and now fails with a nice error message on earlier python versions.
I just had to install Python3.11 and do python3.11 -m pip install pymavlink
and then run it.
I'll merge this as it doesn't really harm anyone and we want it for properly documenting ArduSub's MAVLink support =]
A parser that finds incoming, requestable, and outgoing MAVLink messages for each vehicle. May not indicate full support, but at least shows the messages which are handled in the code.
Requirements
git
command accessible from terminal-b
/--branch
argumentFeatures
NAMED_VALUE_FLOAT
andNAMED_VALUE_INT
message names--vehicle ALL
)