Correct saving of parameters in the face of runtime defaults

[peterbarker]

Summary

Fixes a non-forced parameter save being silently dropped when the value equals the parameter's compiled-in default but set_default() has changed the default at runtime. After a reboot the parameter came back as the runtime default rather than the saved value.

Classification & Testing (check all that apply and add your own)

• Checked by a human programmer

• Non-functional change

• No-binary change

• Infrastructure change (e.g. unit tests, helper scripts)

• Automated test(s) verify changes (e.g. unit test, autotest)

• Tested manually, description below (e.g. SITL)

• Tested on hardware

• Logs attached

• Logs available on request

• New test.Copter.ParamSaveAfterRuntimeDefault autotest:

  • With a MaxSonar serial rangefinder configured (SERIAL4_PROTOCOL 9, RNGFND1_TYPE 13), the backend's constructor calls params.scaling.set_default(0.0254f), so RNGFND1_SCALING comes up as 0.0254 rather than its compiled-in 3.
  • A Lua script saves 3 with param:set_and_save(), which doesn't force the save.
  • The test removes the script, reboots, and checks the value.

• Without the fix it fails: "parameter RNGFND1_SCALING want=3.000000 got=0.025400". With it, it passes.

• Run on this branch (current master plus these two commits). test.Copter.ParameterChecks, next to the new test, also passes.

• Not tested on hardware.

Description

AP_Param::save_sync() doesn't write a new parameter record when the value equals the default from var_info (get_default_value()). The reasoning is that an unsaved parameter comes back as that default on reboot. Once set_default() has changed the default, that no longer holds: an unsaved parameter comes back as the runtime default instead. A non-forced save of the compiled-in default was therefore dropped, and the runtime default returned after a reboot.

GCS PARAM_SET isn't affected, because it forces the save whenever the value changes. Non-forced saves are:

• scripting's param:set_and_save();
• MAVFTP @PARAM/param.pck uploads (AP_Filesystem_Param.cpp calls save_sync(false, false));
• AP_Param::set_and_save_by_name() and the set_and_save() calls made by firmware code.

This affects any parameter given a runtime default with set_default(). The tree has about 80 such calls; the rangefinder's RNGFNDx_SCALING used by the test is one. #19132 adds backend defaults for RNGFNDx_MIN/MAX, which makes the problem easier to hit, e.g. scripting saving RNGFND1_MAX=7 on a TF02 whose default is now 10.

The fix: before the skip check, save_sync() looks up the runtime default with the existing check_default(). If it differs from the var_info default, the save is forced.

• It only adds saves, never removes any. A value equal to the runtime default but not the compiled-in one was already saved and still is.
• Builds without AP_PARAM_DEFAULTS_ENABLED (which follows HAL_GCS_ENABLED) don't record runtime defaults, so they keep the old behaviour.
• Cost: check_default() walks the runtime-defaults list, and only when a parameter with no stored record is saved.

This PR was prepared with AI assistance (Claude), which found the save path, wrote the fix and test, and ran the SITL testing described above. The issue was first reported by an AP-Review round on #19132.

[peterbarker]

1. Build master and start SITL with a wiped EEPROM:
   git checkout origin/master
   ./waf configure --board sitl && ./waf copter
   Tools/autotest/sim_vehicle.py -v ArduCopter -w --no-rebuild

2. In MAVProxy, set up a MaxSonar serial rangefinder and enable scripting:
   param set SERIAL4_PROTOCOL 9
   param set RNGFND1_TYPE 13
   param set SCR_ENABLE 1
   reboot
   param fetch RNGFND1_SCALING
   RNGFND1_SCALING should now read 0.0254: the MaxSonar constructor's set_default() replaces the compiled-in 3.

3. In another shell, in the directory you ran sim_vehicle.py from, add a script that saves 3 without forcing the save:
   mkdir -p scripts
   cat > scripts/save-scaling.lua <<'EOF'
   assert(param:set_and_save('RNGFND1_SCALING', 3))
   gcs:send_text(6, 'saved RNGFND1_SCALING')
   EOF

4. In MAVProxy, reboot so the script runs:
   reboot
   param fetch RNGFND1_SCALING
   You should see "saved RNGFND1_SCALING", and the value reads 3. That's only in RAM: save_sync() skipped the write because 3 matches the
   var_info default.

5. Move the script out of the way so it can't save 3 again:
   mv scripts/save-scaling.lua scripts/save-scaling.lua.bak

6. In MAVProxy, reboot and check the value:
   reboot
   param fetch RNGFND1_SCALING

• Master: it's back to 0.0254. That's the bug.
• pr/param-save-runtime-default: it stays at 3.

For contrast, param set RNGFND1_SCALING 3 from MAVProxy in step 4 would stick even on master. A GCS PARAM_SET forces the save when the value
changes, which is why you didn't see this with SCHED_LOOP_RATE.

[github-actions]

Global Size Summary (Total Flash delta in bytes)

Built from 280f396 rebased onto dc78bc5

Board	Periph	tracker	blimp	bootloader	copter	heli	iofw	plane	rover	sub
DrotekP3Pro	_	_	_	*	_	_	_	_	_	_
Durandal	_	+32	+32	_	+32	+32	_	+32	+32	+32
Hitec-Airspeed	*	_	_	_	_	_	_	_	_	_
KakuteF7	_	+32	+32	_	+32	+32	_	+32	+32	+32
MambaH743v4	_	+76	+76	_	+76	+76	_	+76	+76	+76
MatekF405-CAN	_	_	_	*	_	_	_	_	_	_
MatekF405	_	+32	+32	_	+32	+32	_	+32	+32	+32
MatekH743-bdshot	_	+32	+32	_	+32	+32	_	+32	+32	+32
Pixhawk1-1M	_	+32	+32	_	+32	+32	_	+32	+32	+32
disco	_	+66	+56	_	+64	+56	_	+60	+64	+52
f103-GPS	*	_	_	_	_	_	_	_	_	_

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-10-19)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: REQUEST CHANGES

Reviewed at head 4b69e92afa.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34813/1.html#pr34813

Thanks for the reproduction steps. The AP_Param fix works: I reran the new test in SITL, and with only AP_Param.cpp reverted to the merge-base it fails on the last check (want 3, got 0.0254).

The new test is what makes sitltest-copter-tests1d fail, though. In that bucket all tests share one eeprom. AC_Avoidance_Proximity_AVOID_ALT_MIN runs first and puts RNGFND1_SCALING back to 3 with a PARAM_SET that changes the value. That save is forced, so 3 gets stored, set_default(0.0254) doesn't apply, and the first assertion here fails with want=0.025400 got=3.000000 (https://github.com/ArduPilot/ardupilot/pull/34813/files#diff-a9490928a9e9ea35ce280aac604533d4d0ce04482033429a1a5e5e947ebe849dR13200). Running just those two tests reproduces it. It passes on its own only because each test.Copter.X argument starts a fresh SITL.

Adding self.customise_SITL_commandline([], wipe=True) as the first line of the test fixed it for me. Both tests pass, and the test still fails against the old AP_Param.cpp.

Minor, no change needed here: check_default() now walks default_list from the IO thread with no lock against purge_defaults_list_overrides(). master already does the same with param_overrides via get_default_value(), and I found no reachable interleaving on firmware.

[AP-Review]

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: ACCEPT

Reviewed at head 280f396aba.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34813/2.html#pr34813

The head has moved since my last note, from 4b69e92 to 280f396. Thanks for adding the wipe.

I reran AC_Avoidance_Proximity_AVOID_ALT_MIN followed by ParamSaveAfterRuntimeDefault in one SITL session at the new head, and both pass. With only AP_Param.cpp reverted to the merge-base, the test still fails on the last check (https://github.com/ArduPilot/ardupilot/pull/34813/files#diff-a9490928a9e9ea35ce280aac604533d4d0ce04482033429a1a5e5e947ebe849dR13220): want 3, got 0.0254, so it still catches the bug. sitltest-copter-tests1d is green at this head.

The one red job, sitltest-quadplane-tests1c, doesn't run this test, and it fails on master often too. A local run of that bucket at this head passed. I couldn't see which test failed in CI, because the log isn't public.

Not checked: MAVFTP param upload, other firmware set_and_save() callers, and hardware.