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

Reintroduce optional parameter versioning mechanism for airframe maintainers #22813

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Feb 27, 2024

Solved Problem

When working on multiple projects I found that many reintroduced a mechanism to allow automatically resetting the parameter configuration during a product update. This functionality was initially introduced in #11760 and removed again in #17279 because we (including me) thought that the param set-default mechanism of #16796 would solve all the cases. But the case where the airframe maintainer wants to enforce a reset to airframe defaults is not covered anymore with the param set-default mechanism. For products based on PX4 this is still required to ensure proper functionality after a major update.

Solution

I suggest to reintroduce what was removed in #17279 in a slightly adapted way.
The mechanism is still exactly the same. An airframe maintainer can optionally add e.g. an set PARAM_DEFAULTS_VER 2 line in the airframe file such that vehicles coming from an older (or newer) version airframe as detected using the SYS_PARAM_VER parameter get reset to the default configuration. As usual accumulating flight numbers, hours, RC, sensor and newly also airspeed calibration are preserved.

Changelog Entry

Feature: Reintroduce optional parameter versioning mechanism for airframe maintainers

Alternatives

I tried to work around the necessity to trigger a reboot for the mechanism but after some testing it's still the most useful way in my eyes. The parameter version is only available after loading the airframe but arch and board defaults need to be loaded before that and the reset has to be done before that even because otherwise all param set lines in these configurations would be reset again. So either we'd load the airframe twice per boot or just reboot the system if a reset is required.

Test coverage

I tested this on a vehicle which already had a parameter version from the previous mechanism and it seamlessly reset to the latest defaults even if I manually changed the parameters manually before the update.

Context

I was in discussion with @dagar about a possibly better less shotgun way to address the specific use cases solved by this mechanism but at least to me it's not clear yet how to solve all cases especially during bringups and major updates where a known working configuration is usually crucial to move multiple vehicles forward.

@MaEtUgR MaEtUgR requested a review from sfuhrer February 27, 2024 13:20
@MaEtUgR MaEtUgR self-assigned this Feb 27, 2024
@dagar
Copy link
Member

dagar commented Mar 1, 2024

We could add something to make it easy to skip the volatile parameters.

@dagar
Copy link
Member

dagar commented Mar 1, 2024

How about param reset_all --skip-volatile X? Then we'd only need TC and a few other little things on top.

@github-actions github-actions bot added the stale label Apr 1, 2024
@MaEtUgR MaEtUgR force-pushed the maetugr/reintroduce-sys-param-ver branch 2 times, most recently from 13db715 to fe24f94 Compare May 22, 2024 14:24
@MaEtUgR
Copy link
Member Author

MaEtUgR commented May 22, 2024

I

  • rebased on main
  • removed the unrelated cahnges like shortening the reset exclusion parameter name wildcards and adding the airspeed calibration to the exclusions

This pr is about the parameter versioning mechanism and not about airspeed calibration or revising the volatile parameter reset mechanism.

sfuhrer
sfuhrer previously approved these changes May 23, 2024
Copy link
Contributor

@sfuhrer sfuhrer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for coming back to this - I agree that we have to focus on bringing the reset mechanism in here, and should move the work around which params to not reset to another PR.
I just have some proposals for cleaner comments. Otherwise looks good to me (haven't tried it out myself though).

ROMFS/px4fmu_common/init.d/rcS Show resolved Hide resolved
ROMFS/px4fmu_common/init.d/rcS Outdated Show resolved Hide resolved
ROMFS/px4fmu_common/init.d/rcS Outdated Show resolved Hide resolved
src/lib/systemlib/system_params.c Outdated Show resolved Hide resolved
src/lib/systemlib/system_params.c Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Contributor

FYI There have been some questions in the last couple of days on discord about how parameters "change managed" across versions, following renames and deletions. Would appreciate an update to the param docs for that.

@MaEtUgR MaEtUgR force-pushed the maetugr/reintroduce-sys-param-ver branch from fe24f94 to d7437b4 Compare May 28, 2024 11:32
@MaEtUgR
Copy link
Member Author

MaEtUgR commented May 28, 2024

@sfuhrer Thanks for your suggestions, it's important to be clear 🙏 I tried to adjust the wording from what we originally had to address the valid input.

@MaEtUgR MaEtUgR force-pushed the maetugr/reintroduce-sys-param-ver branch from d7437b4 to ca6cd87 Compare June 11, 2024 15:15
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Jun 11, 2024

Rebased on main to have CI run again. The previous errors looked like the ones we fixed recently, see #23200

sfuhrer
sfuhrer previously approved these changes Jun 12, 2024
@sfuhrer
Copy link
Contributor

sfuhrer commented Jun 12, 2024

69 bytes over flash on v5

@sfuhrer sfuhrer force-pushed the maetugr/reintroduce-sys-param-ver branch 7 times, most recently from 0eb92de to e03414b Compare June 14, 2024 14:14
@sfuhrer sfuhrer removed the stale label Jun 14, 2024
The case where the airframe maintainer wants to enforce a reset to airframe is
not covered anymore with the `param set-default` mechanism. For products based
on PX4 this is still required to ensure proper functionality after a major update.
@sfuhrer sfuhrer force-pushed the maetugr/reintroduce-sys-param-ver branch from e03414b to 111be14 Compare June 17, 2024 08:16
@sfuhrer sfuhrer merged commit 9c83f84 into main Jun 18, 2024
92 of 93 checks passed
@sfuhrer sfuhrer deleted the maetugr/reintroduce-sys-param-ver branch June 18, 2024 07:32
ohyaiamhere added a commit to DL-ETeam/PX4-Autopilot-Dronelab that referenced this pull request Jun 18, 2024
autostart scripts: Reintroduce SYS_PARAM_VER (PX4#22813)
chiara-septentrio pushed a commit to flyingthingsintothings/PX4-Autopilot that referenced this pull request Jul 3, 2024
The case where the airframe maintainer wants to enforce a reset to airframe is
not covered anymore with the `param set-default` mechanism. For products based
on PX4 this is still required to ensure proper functionality after a major update.
chiara-septentrio pushed a commit to flyingthingsintothings/PX4-Autopilot that referenced this pull request Jul 4, 2024
The case where the airframe maintainer wants to enforce a reset to airframe is
not covered anymore with the `param set-default` mechanism. For products based
on PX4 this is still required to ensure proper functionality after a major update.
vertiq-jordan pushed a commit to iq-motion-control/PX4-Autopilot that referenced this pull request Aug 21, 2024
The case where the airframe maintainer wants to enforce a reset to airframe is
not covered anymore with the `param set-default` mechanism. For products based
on PX4 this is still required to ensure proper functionality after a major update.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants