feat: Adding and implementing set_mavlink_version function in mavutil.py - #1207
feat: Adding and implementing set_mavlink_version function in mavutil.py#1207noahredon wants to merge 5 commits into
Conversation
|
Tested with a v2-only dialect : set_dialect's auto-gen fallback hits an XML lookup that appearently doesn't exist for a single-version dialect... its not a bug in this PR that you made (existing set_dialect behaviour) but the new docstring is the obvious place to flag it since you are adding it :) something like an alert/warning would be nice to see |
5d01363 to
9c07a2e
Compare
|
Thanks for testing this and for the feedback! I just updated the PR to add a warning in the docstring about this specific edge case. To be honest, this is actually the behavior I was aiming for. It makes sense that the script fails and stops execution if it tries to load a dialect that doesn't exist for the newly selected MAVLink version. In this case, you only need to set the MAVLink version before the dialect, which makes sense. And for most cases (for example when using a common dialect like ardupilotmega), it works perfectly fine. Let me know if the updated docstring looks good to you! |
fallenmi
left a comment
There was a problem hiding this comment.
Reviewed with OpenAI Codex assistance.
Requesting changes for one state-integrity issue in set_mavlink_version():
mavutil.py:147-158mutatesMAVLINK09/MAVLINK20before attempting to load the target dialect, but does not restore them ifset_dialect()raises. The documented incompatible-dialect exception is reasonable; leaving the process in a mixed wire-protocol state after that exception is not. With a legitimate v2-only generated dialect loaded,set_mavlink_version(1.0)raises the documentedFileNotFoundError, but after catching itMAVLINK20is absent andmavlink20()returnsFalsewhile the active module is stillpymavlink.dialects.v20.foozwithWIRE_PROTOCOL_VERSION == "2.0". Callers can therefore observe or create connections from contradictory global version state. Please make the switch transactional (for example, restore the prior environment/version state if loading the target dialect fails) and add a regression for the failed-switch path.
I reproduced this on exact head 9c07a2e9d97203a1971461631e35f247b0fddcac and the current-master merge tree a66dddcea500364cd33667eb919c8819f5a49821; their normalized oracle outputs were byte-identical. A 50-switch common-dialect 1.0↔2.0 control passed on both trees. The exact-head GitHub matrix is 22/22 successful across four completed workflows. Locally, the remaining test slice passed 51/51 on head and 59/59 on the current merge; the unrelated localhost:14550 logging test passed 3/3 independently on each tree but collides when run after the full macOS suite.
9c07a2e to
5652c10
Compare
fallenmi
left a comment
There was a problem hiding this comment.
Reviewed exact head be8248c018508e8ada83096049aab8624edc7e3f.
The rollback fixes the state-integrity issue from my earlier review. With an actual generated v2-only dialect loaded, a failed 2.0 → 1.0 switch now leaves MAVLINK09, MAVLINK20, the version helpers, current dialect, and active module exactly unchanged on both this head and the current merge. Successful common-dialect 1.0/2.0 cycles remain intact. A separate seven-case rollback matrix covering failed transitions from 2.0, 1.0, and 0.9—including custom pre-existing environment values—passes on head and merge; the prior head failed all four rollback cases.
The focused mavutil tests pass 3/3 on both trees, and all 21 live checks across four workflows are green. I found no remaining blocker.
Disclosure: I used OpenAI Codex to re-review the changed head, inspect the implementation and live repository state, and run the exact-head/current-merge oracles and focused tests described above; I independently verified the result.
be8248c to
3b92ab1
Compare
…tate if exception in set_dialect
Hi! I thought having a simple function capable of changing the MAVLink version could be useful. For example, if you create a custom MAVLink dialect for MAVLink 2 only and don't change the version before setting the dialect, the script will try to import the default MAVLink 1 dialect, which will crash.
I've tested it, and it works as expected.
I also implemented it in mavfile.auto_mavlink_version for better readability.