AP_Vehicle: store currently running firmware version into a parameter

[peterbarker]

save the currently running firmware version into a parameter. Into the future we may use this number to prevent a user from attempting to upgrade from a truly ancient version of the firmware to a more modern one where we may have removed parameter upgrade code.

Tested in SITL:

MANUAL> FW_VER_LAST      263936

pbarker@crun:~$ i 263936
263936 0x40700 01003400 0b1000000011100000000
pbarker@crun:~$ 

[tpwrules]

This seems like a good idea for the future. Do you think we should only increase it? I don't think we want to get into the game of forwards compatibility. But that might let us have a more global system for deciding to do a conversion, and let it be re-done for users who go back and forth between versions.

Do note that the read-only metadata is not absolute, and it might cause big problems for users restoring old param lists. But maybe manually increasing it could be the bypass for users who don't want to reset their EEPROM. I'm wondering instead about not making it even visible, like the secret calibration parameters.

[peterbarker]

This seems like a good idea for the future. Do you think we should only increase it? I don't think we want to get into the game of forwards compatibility. But that might let us have a more global system for deciding to do a conversion, and let it be re-done for users who go back and forth between versions.

I was imagining this would always be set - we wouldn't ratchet it to the most recent version. This would stop people going 4.1 -> 4.4 -> 4.1 -> 4.7 (numbers made up and assuming 4.1 to 4.4 is allowed but 4.1 to 4.7 is not)

Do note that the read-only metadata is not absolute, and it might cause big problems for users restoring old param lists. But maybe manually increasing it could be the bypass for users who don't want to reflash. I'm wondering instead about not making it even visible, like the secret calibration parameters.

Yep, good call, I've made the thing INTERNAL_USE_ONLY.

[timtuxworth]

I'd like to be able to access it from Lua code though. When trying to write version specific Lua that deals with parameter name changes, it would be great to be able to get the current version number.

[tpwrules]

I'd like to be able to access it from Lua code though. When trying to write version specific Lua that deals with parameter name changes, it would be great to be able to get the current version number.

Does

ardupilot/libraries/AP_Scripting/docs/docs.lua

Lines 1878 to 1911 in 6870d06

	-- desc
	FWVersion = {}
	-- get field
	---@return string
	function FWVersion:hash() end
	-- get field
	---@return integer
	function FWVersion:patch() end
	-- get field
	---@return integer
	function FWVersion:minor() end
	-- get field
	---@return integer
	function FWVersion:major() end
	--get APM_BUILD_? value from AP_Vehicle/AP_Vehicle_Type.h that is checked against APM_BUILD_TYPE()
	---@return integer
	---| '1' # Rover
	---| '2' # ArduCopter
	---| '3' # ArduPlane
	---| '4' # AntennaTracker
	---| '7' # ArduSub
	---| '9' # AP_Periph
	---| '12' # Blimp
	---| '13' # Heli
	function FWVersion:type() end
	-- get field
	---@return string
	function FWVersion:string() end

meet your needs? If this goes in as is then the values returned by Lua there will always match the param (or presumably the system will fail to start with a config error). No need to get the param or wait for this PR.

[peterbarker]

I'd like to be able to access it from Lua code though. When trying to write version specific Lua that deals with parameter name changes, it would be great to be able to get the current version number.

Better to just look for the old parameter and then look for the new parameter, surely?

i.e. test for what you're really trying to test for (i.e. the existence of a parameter). No reason to infer from a version number.

[timtuxworth]

I'd like to be able to access it from Lua code though. When trying to write version specific Lua that deals with parameter name changes, it would be great to be able to get the current version number.

Better to just look for the old parameter and then look for the new parameter, surely?

i.e. test for what you're really trying to test for (i.e. the existence of a parameter). No reason to infer from a version number.

Sure that's what I do now, but "better?" - nah, it's a workaround.

[timtuxworth]

I'd like to be able to access it from Lua code though. When trying to write version specific Lua that deals with parameter name changes, it would be great to be able to get the current version number.

Does

ardupilot/libraries/AP_Scripting/docs/docs.lua

Lines 1878 to 1911 in 6870d06

	-- desc
	FWVersion = {}
	-- get field
	---@return string
	function FWVersion:hash() end
	-- get field
	---@return integer
	function FWVersion:patch() end
	-- get field
	---@return integer
	function FWVersion:minor() end
	-- get field
	---@return integer
	function FWVersion:major() end
	--get APM_BUILD_? value from AP_Vehicle/AP_Vehicle_Type.h that is checked against APM_BUILD_TYPE()
	---@return integer
	---| '1' # Rover
	---| '2' # ArduCopter
	---| '3' # ArduPlane
	---| '4' # AntennaTracker
	---| '7' # ArduSub
	---| '9' # AP_Periph
	---| '12' # Blimp
	---| '13' # Heli
	function FWVersion:type() end
	-- get field
	---@return string
	function FWVersion:string() end

meet your needs? If this goes in as is then the values returned by Lua there will always match the param (or presumably the system will fail to start with a config error). No need to get the param or wait for this PR.

This is perfect! Thanks Thomas!

[IamPete1]

We could hide the param from the user altogether with AP_PARAM_FLAG_HIDDEN, I don't see any reason they would want to see it. Although I guess it might be useful if you have only a param set but no log.

[timtuxworth]

We could hide the param from the user altogether with AP_PARAM_FLAG_HIDDEN, I don't see any reason they would want to see it. Although I guess it might be useful if you have only a param set but no log.

It would be VERY useful to have the value as a parameter value in a parameter file!

[IamPete1]

It would be VERY useful to have the value as a parameter value in a parameter file!

If this is the goal, then we should add vehicle type in there somewhere and a little decoder python script like the one for devids.

[LupusTheCanine]

Don't we have FORMAT_VERSION?

[peterbarker]

Don't we have FORMAT_VERSION?

Not the same thing; if FORMAT_VERSION is different to what we are expecting we wipe all of your parameters and you get to configure everything again.

We haven't reset one of those since I've been with the project.

[LupusTheCanine]

Don't we have FORMAT_VERSION?

Not the same thing; if FORMAT_VERSION is different to what we are expecting we wipe all of your parameters and you get to configure everything again.

We haven't reset one of those since I've been with the project.

Ok. Good to know.

How about tying the parameter to firmware version we tie it to "parameter set revision".

Where following actions don't increment:

• Adding a new parameter
• Range extending conversion (though I am not sure how well it is handled when rolling back and updating)

Increment:

• Parameter conversion or functional change
• Value range or out of bounds handling changed (eg. issue in Dangerous change in behaviour of TECS_PITCH_MIN #30156)

I think this type of parameter should be able to have multiple instances. eg. to handle vendor specific logic or maybe manage subsystems so we can tell users which subsystem is affected. It also shouldn't be (fully) autoincremented after update without user acknowledgement eg. use the last bit to store user accepting the change.

[magicrub]

Don't we have FORMAT_VERSION?

Not the same thing; if FORMAT_VERSION is different to what we are expecting we wipe all of your parameters and you get to configure everything again.

We haven't reset one of those since I've been with the project.

Maybe for v5.0.0 with String params! :) I can only hope LOL

[magicrub]

Personally, I don't think this extra parameter is worth it. Better to reallocate 4 bytes (int or 4 uint8's?) into the flash storage page and don't make it visible.. like the missions and DNA list

[magicrub]

Let's sneak it in here:
https://github.com/ArduPilot/ardupilot/blob/master/libraries/StorageManager/StorageManager.cpp#L101
If we're consuming a few bytes of flash anyway might as well take it out of the last 4 bytes of StorageParam or StorageParamBak

downside of using the Bak is it's only for larger chips and downside of touching StorageParam is we run the risk of corrupting the whole flash space, and that chance goes up the lower (and thus small memory requirement) we go.

note: once we boot with an old-to-new param value and we do whatever param migrations that are needed then we replace that param with the current version, which is the new default, which then will consume zero storage space

I dunno.. adding a param here just feels like it goes against the don't-add-a-param-unless-you-reaaaaally-need-to methos which we have in place for good reasons.