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

mavgen_python: Multiple improvements and bug fixes #664

Merged
merged 20 commits into from
Jul 27, 2022
Merged

mavgen_python: Multiple improvements and bug fixes #664

merged 20 commits into from
Jul 27, 2022

Conversation

alehed
Copy link
Contributor

@alehed alehed commented Apr 27, 2022

This PR is best reviewed commit-by-commit starting where the reformat_output branch leaves off.

This lays the groundwork for the following PR which adds type annotations in Python3 code.

@alehed alehed changed the title Multiple improvements and fixes for the python target mavgen_python: Multiple improvements and bug fixes Apr 28, 2022
@alehed
Copy link
Contributor Author

alehed commented May 17, 2022

@peterbarker I modified the PayLoadTrimZeros test because I think it is wrong.

I'm a bit confused about the test that failed. It uses code that is generated for mavlink 1.0 and sets the version to 2. Because the version is set to an int and not a string, in the past only parts of the generated code used to assume mavlink 1.0. Should code generated with protocol 1.0 allow sending mavlink 2.0 messages?

@tridge
Copy link
Contributor

tridge commented Jun 8, 2022

on the name attribute problem, it would be nice to cope with .name in existing code (eg. in msgrates module in mavproxy)
maybe use a __getattr__() to check dictionary for the field, if field is 'name' and if no field and have msgname then return msgname
and a comment that this is a temporary workaround, can be removed in 1 years time

@alehed
Copy link
Contributor Author

alehed commented Jun 8, 2022

As discussed, added the name class variable property back, but in such a way that using it will raise an exception when using on an instance and printing a warning when using on a class. It is also added in such a way that mypy doesn't know about it (so it doesn't print warnings about it or suggests using it). Like this it should be ready for merging.

alehed added 20 commits July 15, 2022 13:05
This handles the case where the description contains newlines.
Code that is probably deep in some library should not print to stdout.
This changes the generated code to use per-module logging (per-module,
such that it can be turned off by the user of the library).
The default log level in python is warning.
The dialect common.xml defines various messages that have a name
field, which causes a name instance variable to be defined on the
class. This would shadow the class variable that holds the name
of the message the class corresponds to.
It is not needed, instead the unpacker should be used. It is used
in MAVProxy, where its use needs to be removed.
This removes all external dependencies from the generated file.
Otherwise it prints something like 'b"a"', instead of '0x61'.
In general it is desirable that subclasses don't violate the Liskov
substitution principle. Given that users of the library almost never
interact with MAVLink_message directly, this should break fewer
implementations than renaming subclass.pack().
This make the code more readable and will fix some mypy warnings in
the future.
Turns out in some places WIRE_PROTOCOL_VERSION is set to an int,
in which case the string comparisons fail.
The code the currently generated default values didn't work.
Using lists as default arguments can cause unforseen consequences.
This is because the default argument can be modified and will then
have a different value the next time it is called.

This might cause some breakage downstream, but the chances are low.
This module is in the stdlib and not having it will cause exceptions later.
This is not done automatically and logging won't work properly without
it.
This ensures backwards compatibility, but encourages users to migrate
(via log messages) and does it an a way that doesn't produce mypy
warnings.
@tridge tridge merged commit e556217 into ArduPilot:master Jul 27, 2022
@alehed alehed deleted the feature/python_convenience_improvements branch July 27, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants