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

AP_Param: fixed param class conversion code #20388

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Mar 28, 2022

param class conversion was unconditionally overwriting the parameter from the old parameter. This meant if the user has set a value in an old firmware they could not change it in a new firmware.

I hit this with ARSPD_TYPE. I had previously set this to 0 in a previous use of the board, and found that it kept resetting to 0 on
the new firmware when I tried to enable airspeed

To reproduce this bug:

  • checkout plane 4.1
  • set ARSPD_TYPE to 8 then set it back to 0
  • now start 4.2 with the same eeprom.bin
  • set ARSPD_TYPE to 8 and then reboot
  • fetch parameters and ARSPD_TYPE will be 0, reset by this bug

param class conversion was unconditionally overwriting the parameter
from the old parameter. This meant if the user has set a value in an
old firmware they could not change it in a new firmware.

I hit this with ARSPD_TYPE. I had previously set this to 0 in a
previous use of the board, and found that it kept resetting to 0 on
the new firmware when I tried to enable airspeed
@magicrub
Copy link
Contributor

I think this is a gremlin that has been haunting us for a while!

@hendjoshsr71
Copy link
Member

hendjoshsr71 commented Mar 28, 2022

I think the bug fix is correct and makes sense.

But I can't actually reproduce it with your steps.... which is bugging me a bit

I also tried it a on CubeBlack in addition to SITL (I tried a couple time with each)

@tridge tridge added this to merged in Plane 4.2 Mar 28, 2022
@tridge
Copy link
Contributor Author

tridge commented Mar 28, 2022

I think this is a gremlin that has been haunting us for a while!

the only user of this code is the airspeed conversion, so only impacts airspeed so far

@tridge
Copy link
Contributor Author

tridge commented Mar 28, 2022

@hendjoshsr71 an easy way to reproduce is to use this eeprom.bin:
http://uav.tridgell.net/tmp/eeprom-airspeed-bug.bin
copy that to eeprom.bin for SITL and run master without this fix.
That file was fetched off a CubeOrange that had this bug using ftp of "@SYS/storage.bin"

@hendjoshsr71
Copy link
Member

hendjoshsr71 commented Mar 28, 2022

@hendjoshsr71 an easy way to reproduce is to use this eeprom.bin: http://uav.tridgell.net/tmp/eeprom-airspeed-bug.bin copy that to eeprom.bin for SITL and run master without this fix. That file was fetched off a CubeOrange that had this bug using ftp of "@SYS/storage.bin"

Ok I can now reproduce it now with my own bin files. The difference was I doing: 4.1.7 -> 4.2Beta not master.

When I went from 4.2Beta -> Master it got reset to zero after doing the 4.1.7->4.2Beta.

There must be some difference between the two.

@tridge
Copy link
Contributor Author

tridge commented Mar 28, 2022

Yes, I have merged this fix in beta4

@peterbarker peterbarker merged commit 0879b49 into ArduPilot:master Mar 28, 2022
@hendjoshsr71
Copy link
Member

hendjoshsr71 commented Mar 29, 2022

@tridge I think I figured out how my testing missed this. I thought had I tested this scenario but ....

When using mavproxy I realized that after reboot, even though it restarts SITL, you must then call param fetch before param show ARSPD*.

Might it make sense when using sitl that reboot in mavproxy automatically calls fetch?

@rmackay9 rmackay9 added this to Pending in Copter 4.2 Mar 29, 2022
@rmackay9 rmackay9 moved this from Pending to 4.2.0-beta3 in Copter 4.2 Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Copter 4.2
4.2.0-beta3
Development

Successfully merging this pull request may close these issues.

None yet

5 participants