Parameter migration: code cleanup, migration script, and land_detector trial #19489

coderkalyan · 2022-04-14T19:20:59Z

Describe problem solved by this pull request
As discussed on Slack, legacy c parameter files can/should be migrated to the newer YAML module definitions.

Describe your solution
This PR:

Performs some minor code cleanup on the parameter parsing and validation code in favor of new Python3 idioms
Adds a python script that uses this parsing code to autogenerate yaml module definitions
Uses the above script to migrate land_detector to YAML definitions.

Test data / coverage
Not flight tested. Build completes successfully locally, so uploading this to run on CI.

coderkalyan · 2022-07-07T22:48:14Z

@dagar Could you take a look at this please?

The only issue I can see so far is MAVROS which is running a bionic container with Python 3.6. I will file a PR to upgrade that to 3.7 or 3.8 as long as there no other side effects of doing so.

Also should do a rebase and pass clang tidy, but functionally I think this is complete. If this looks good to you I can continue the work on the other modules.

coderkalyan · 2022-07-13T18:33:42Z

As per slack, we're going to go aggressive on this and just migrate the entire codebase + scrap the old junk. In the meantime a sample (land_detector) is pushed.

coderkalyan · 2022-07-13T18:51:01Z

coderkalyan · 2022-07-14T07:53:43Z

@dagar All of modules and drivers has been migrated. diffing parameters.xml shows no technical changes, only a couple formatting irregularities and a couple strange parameters i had to update (especially the rc_update module). Nonetheless it would be good to get a second eye on it.

There are a few remaining files in the libraries and other directories. Will the module.yaml work there as well? (I'll try it tomorrow).

Multiple small bugs in the parameter pipeline have been found and fixed. Once we're completely free of C source I'll start simplifying the scripts.

Update: something wrong with simulator/battery_simulator, probably because I testing for the fmu-v5x target. Will explore tomorrow.

coderkalyan · 2022-07-16T04:09:49Z

Everything is now successful except for a new dependency (dataclasses to be backwards compatible with older Python versions). Updated in this PR: PX4/PX4-containers#318

@dagar This is ready when you want to take a look.

coderkalyan · 2022-07-16T04:10:44Z

Hopefully we can get this in soon to minimize merge conflicts. The tooling will be moved to a separate PR.

coderkalyan · 2022-07-18T21:04:20Z

sitl.diff.txt
fmu-v5x.diff.txt

@dagar Here are the diffs

bkueng

Thanks for going through this.

bkueng · 2022-07-19T06:19:12Z

Tools/validate_yaml.py

-try:
-    import yaml
-except ImportError as e:
-    print("Failed to import yaml: " + str(e))
-    print("")
-    print("You may need to install it using:")
-    print("    pip3 install --user pyyaml")
-    print("")
-    sys.exit(1)
-
-try:
-    import cerberus
-except ImportError as e:
-    print("Failed to import cerberus: " + str(e))
-    print("")
-    print("You may need to install it using:")
-    print("    pip3 install --user cerberus")
-    print("")
-    sys.exit(1)


Can you keep these?

Well, to be honest I find them to be a bit clunky and not super useful. The import statement raises an error if it can't find it and that's pretty readable. Also, pip3 is not the only way to install a package, i.e. system package managers. I feel like its better to just maintain a requirements.txt in the tree that we point to in documentation. What do you think?

@dagar thoughts?

I know, advanced users will know what to do. We added them when adding new external requirements, for people who updated their branch and it didn't compile anymore.

Tools/validate_json.py

Tools/validate.py

src/lib/parameters/px4params/jsonout.py

validation/module_schema.yaml

A little bit of code cleanup in preparation for using srcparser to migrate the c parameter files to yaml definitions. * dataclasses * PEP 8-ish

TODO: not completed, edit commit when finished Migrates all modules to use the new yaml parameter definitions instead of C.

This python script uses the existing parameter build infrastructure to parse c parameter files and generate yaml module definitions.

Fixes a bug where any boolean tag that exists, whether or not the value is actually true, is recorded as true in parameter generation.

bkueng

A couple more details, generally looks good, but I haven't looked at the param diff.

can you squash the wip commit ( src/modules: parameter format migration (WIP))?
for all the generated commits, can you add the exact shell command(s) to the commit message? This will help for people having merge conflicts.

bkueng · 2022-07-22T06:04:16Z

Tools/validate.py

+
+
+def main():
+    parser = argparse.ArgumentParser(description='Validate YAML or JSON file(s) against a schema')


Where is json being handled?

bkueng · 2022-07-22T06:07:27Z

requirements.txt

@@ -0,0 +1,24 @@
+argparse


There's already a requirements.txt in https://github.com/PX4/PX4-Autopilot/blob/main/Tools/setup/requirements.txt

Oh, I didn't see that. I'll fix.

bkueng · 2022-07-22T06:07:59Z

.github/workflows/clang-tidy.yml

@@ -17,5 +17,8 @@ jobs:
      with:
        token: ${{secrets.ACCESS_TOKEN}}

+    - name: install dependencies
+      run: pip3 install dataclasses


Where is this required?

px4params/scrparser.py. When I wrote that I was targeting Python 3.7 compatibility since that's the oldest "supported" Python but I then realized we use 3.6 in some places. This is backwards compatibility for 3.6, dataclasses are built into 3.7+

Daniel and I decided to add it directly here because the bionic docker containers aren't behaving (unrelated to my changes) and I couldn't build a new one.

IMO worth adding a comment in the file or commit message, so we can remove it later on.

bkueng · 2022-07-22T06:08:42Z

Tools/serial/generate_config.py

@@ -269,7 +269,7 @@ def parse_yaml_serial_config(yaml_config):
            else:
                default_port_str = port_config['default']

-            if default_port_str != "":
+            if default_port_str != "" and default_port_str != 0:


When is this a number?

Daniel had used that as a "null" default for the sagetech transponder driver. However I see that just two days back he fixed that. I no longer need this, I'll remove it.

coderkalyan · 2022-07-22T07:02:48Z

Thanks @bkueng for the review, I will implement your changes in the morning, and squash the WIP commit. If you have time, could you please check the XML and json/md diff on main? The diffs I posted are probably out of date, and I've checked but I'd like another eye on it if you are able to.

bkueng · 2022-07-26T09:01:06Z

I looked at the diff you provided and it looks good. Might be worth checking if QGC doesn't have a problem with the scientific float notation (3.0e-3), and the case change of reboot_required.

mcsauder · 2022-08-30T16:11:11Z

Hi @coderkalyan , would it help get this PR moving again if we started creating a few smaller *_param.c file to *.yaml file PRs?
I'd be happy to help with a few of those.

coderkalyan · 2022-08-31T14:42:10Z

Hi @mcsauder thanks for pitching in. There are two parts to this PR:

multiple bug fixes and feature improvements of the parameter build system
the actual migration, facilitated by tools/migrate_c_params.py with some amount of manual editing and merging.

I was talking with the dev team and I am going to try to pull the build system improvements into a separate PR. Merging that will unblock this PR. The main issue with migrating everything here is that we start spending hours on rebasing the generated files all the time.

I'm going to additionally split drivers into a new PR, since they don't get updated nearly as often. I think I can get both of those merged soon. Could you please take a couple modules at a time and rebase/create new PRs? I think if you start off with all the *_control modules that would be a good chunk done and we can touch base after that.

Please keep in mind that some of the bug fixes I made are required for the system to accept all the new yaml files. If you find any errors building or missing parameters in the generated output, double check with this PR or rebase with my new tooling PR once I push that. Thanks!

mcsauder · 2022-09-08T21:20:17Z

Hi @coderkalyan , just checking in. I'm chipping away on branches to further this PR, but waiting now on #20108 and #20172. I will continue work to get this across the finish line after those PRs.

coderkalyan · 2022-10-09T22:39:53Z

Sorry all for the absence. I'm pulling the python tooling updates into a separate PR right now. I'll get this all wrapped up within the next few days.

coderkalyan force-pushed the master branch 4 times, most recently from c6b3373 to ae29f90 Compare April 15, 2022 02:24

dagar self-requested a review July 13, 2022 17:41

dagar added Admin: Enhancement (improvement) 💡 Admin: Cleanup (housekeeping) 🧹 labels Jul 13, 2022

coderkalyan force-pushed the master branch from ae29f90 to 1ad2085 Compare July 13, 2022 18:30

coderkalyan force-pushed the master branch 2 times, most recently from ecb7465 to 21c6777 Compare July 14, 2022 07:47

coderkalyan force-pushed the master branch 7 times, most recently from 14773db to bf19ac7 Compare July 15, 2022 23:35

coderkalyan force-pushed the master branch 2 times, most recently from e513654 to 98808e8 Compare July 16, 2022 18:45

coderkalyan force-pushed the master branch 2 times, most recently from e1a0de6 to 88dfc98 Compare July 18, 2022 23:29

bkueng reviewed Jul 19, 2022

View reviewed changes

coderkalyan force-pushed the master branch from 88dfc98 to fb15da1 Compare July 20, 2022 19:02

coderkalyan force-pushed the master branch 3 times, most recently from 0d77f38 to 8f4807f Compare July 21, 2022 17:51

coderkalyan added 15 commits July 21, 2022 10:58

tools/validate_yaml: code cleanup

108602b

tools/validation: allow json with cerberus for future migration

1292fdc

parameters: srcparser code cleanup

179255b

A little bit of code cleanup in preparation for using srcparser to migrate the c parameter files to yaml definitions. * dataclasses * PEP 8-ish

land_detector: migrate c parameter definitions to yaml

9e9a087

src/modules: parameter format migration (WIP)

935d277

TODO: not completed, edit commit when finished Migrates all modules to use the new yaml parameter definitions instead of C.

tools: add migrate_c_params helper script

0817791

This python script uses the existing parameter build infrastructure to parse c parameter files and generate yaml module definitions.

tools/generate_params: fix boolean property bug

7358465

Fixes a bug where any boolean tag that exists, whether or not the value is actually true, is recorded as true in parameter generation.

modules: migrate all modules to yaml parameter definitions

4b6179f

drivers: migrate all drivers to yaml parameter definitions

a5ffbe7

python/pip: add requirements.txt

dfbb110

validation: add new units and increase short description length

5cad5c2

ci: temporarily add dataclasses dependency

22ff3b3

battery: capitalize parameter category

7fe65c6

tools/generate_config: fix empty default value bug

066ac8a

parameters: update injected parameter descriptions

ed941e7

coderkalyan force-pushed the master branch from 8f4807f to ed941e7 Compare July 21, 2022 17:59

coderkalyan requested a review from bkueng July 21, 2022 18:13

bkueng reviewed Jul 22, 2022

View reviewed changes

bkueng mentioned this pull request Aug 31, 2022

Improve Parameter Metadata display in QGC #20125

Closed

coderkalyan mentioned this pull request Oct 9, 2022

Param tooling #20385

Open

bresch mentioned this pull request Apr 5, 2024

EKF2: migrate params to YAML #22982

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter migration: code cleanup, migration script, and land_detector trial #19489

Parameter migration: code cleanup, migration script, and land_detector trial #19489

coderkalyan commented Apr 14, 2022

coderkalyan commented Jul 7, 2022

coderkalyan commented Jul 13, 2022

coderkalyan commented Jul 13, 2022 •

edited

Loading

coderkalyan commented Jul 14, 2022

coderkalyan commented Jul 16, 2022

coderkalyan commented Jul 16, 2022

coderkalyan commented Jul 18, 2022

bkueng left a comment

bkueng Jul 19, 2022

coderkalyan Jul 19, 2022

bkueng Jul 20, 2022

bkueng left a comment

bkueng Jul 22, 2022

bkueng Jul 22, 2022

coderkalyan Jul 22, 2022

bkueng Jul 22, 2022

coderkalyan Jul 22, 2022

bkueng Jul 26, 2022

bkueng Jul 22, 2022

coderkalyan Jul 22, 2022

coderkalyan commented Jul 22, 2022

bkueng commented Jul 26, 2022

mcsauder commented Aug 30, 2022

coderkalyan commented Aug 31, 2022

mcsauder commented Sep 8, 2022

coderkalyan commented Oct 9, 2022 •

edited

Loading



		def main():
		parser = argparse.ArgumentParser(description='Validate YAML or JSON file(s) against a schema')

Parameter migration: code cleanup, migration script, and land_detector trial #19489

Are you sure you want to change the base?

Parameter migration: code cleanup, migration script, and land_detector trial #19489

Conversation

coderkalyan commented Apr 14, 2022

coderkalyan commented Jul 7, 2022

coderkalyan commented Jul 13, 2022

coderkalyan commented Jul 13, 2022 • edited Loading

coderkalyan commented Jul 14, 2022

coderkalyan commented Jul 16, 2022

coderkalyan commented Jul 16, 2022

coderkalyan commented Jul 18, 2022

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderkalyan commented Jul 22, 2022

bkueng commented Jul 26, 2022

mcsauder commented Aug 30, 2022

coderkalyan commented Aug 31, 2022

mcsauder commented Sep 8, 2022

coderkalyan commented Oct 9, 2022 • edited Loading

coderkalyan commented Jul 13, 2022 •

edited

Loading

coderkalyan commented Oct 9, 2022 •

edited

Loading