PX4-Autopilot icon indicating copy to clipboard operation
PX4-Autopilot copied to clipboard

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

Open coderkalyan opened this issue 2 years ago • 12 comments

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 avatar Apr 14 '22 19:04 coderkalyan

@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 avatar Jul 07 '22 22:07 coderkalyan

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 avatar Jul 13 '22 18:07 coderkalyan

  • [x] land_detector
  • [x] airspeed_selector
  • [x] angular_velocity_controller
  • [x] attitude_estimator_q
  • [x] battery_status
  • [x] commander
  • [x] commander/failure_detector
  • [x] dataman
  • [x] ekf2
  • [x] events
  • [x] flight_mode_manager
  • [x] gimbal
  • [x] gyro_calibration
  • [x] gyro_fft
  • [x] landing_target_estimator
  • [x] load_mon
  • [x] local_position_estimator
  • [x] logger
  • [x] mag_bias_estimator
  • [x] manual_control
  • [x] mavlink
  • [x] mc_hover_thrust_estimator
  • [x] navigator
  • [x] rc_update
  • [x] *_control
  • [x] sensors
  • [x] sih
  • [x] simulator
  • [x] temperature_compensation
  • [x] verify no change to parameters.xml
  • [ ] remove parser for old parameter definitions
  • [ ] remove script middle steps
  • [ ] scripting/tooling upgrades

coderkalyan avatar Jul 13 '22 18:07 coderkalyan

@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 avatar Jul 14 '22 07:07 coderkalyan

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

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

coderkalyan avatar Jul 16 '22 04:07 coderkalyan

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

coderkalyan avatar Jul 16 '22 04:07 coderkalyan

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

@dagar Here are the diffs

coderkalyan avatar Jul 18 '22 21:07 coderkalyan

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.

coderkalyan avatar Jul 22 '22 07:07 coderkalyan

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.

bkueng avatar Jul 26 '22 09:07 bkueng

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.

mcsauder avatar Aug 30 '22 16:08 mcsauder

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!

coderkalyan avatar Aug 31 '22 14:08 coderkalyan

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.

mcsauder avatar Sep 08 '22 21:09 mcsauder

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 avatar Oct 09 '22 22:10 coderkalyan