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_Stats: Only reset statistics if the user explicitly sets AP_Stats_… #6402

Merged

Conversation

amilcarlucas
Copy link
Contributor

…RESET parameter to zero.

This allows users to load parameter files (in MP, MAVProxy or any other GCS) without
accidentally reseting the statistics, because the AP_STATS_RESET value contained in
the parameter file will be ignored (unless it is zero and it is usually not zero).
The other statistics parameters are read-only, and the GCS should be clever enough to not set those.
Some documentation improvements

A side effect is that if AP_STATS_RESET != 0 the GCS will probably retry setting the AP_STATS_RESET parameter a couple of times without success and will give up with a timeout or something like that. But that is OK I think. Better than accidentally reseting the stats just because a new parameter file got loaded.

@magicrub
Copy link
Contributor

magicrub commented Jun 9, 2017

this looks like it changes the default behavior from resetting to not resetting. Is that the default behavior we want?

@amilcarlucas
Copy link
Contributor Author

Any updates ?

@amilcarlucas
Copy link
Contributor Author

I still think this is useful, but I will close this one if nobody else feels it is an improvement.

@amilcarlucas amilcarlucas force-pushed the pr-ap-stats-reset-only-when-zero branch from 3b782a7 to 5538472 Compare May 3, 2018 10:52
@amilcarlucas amilcarlucas force-pushed the pr-ap-stats-reset-only-when-zero branch from 5538472 to d56c94b Compare February 24, 2019 22:46
@peterbarker
Copy link
Contributor

@amilcarlucas It was done this way originally so that if the vehicle never had an idea of Unix epoch time (e.g. no GPS and no times sent from GCS), then you could still set a reasonable number in here from the GCS doing the reset.

@amilcarlucas
Copy link
Contributor Author

@amilcarlucas for the reasons described above, I like this change and have it on my local branch. Should we bring this up in the dev meeting?

@@ -32,10 +32,9 @@ const AP_Param::GroupInfo AP_Stats::var_info[] = {
AP_GROUPINFO("_RUNTIME", 2, AP_Stats, params.runtime, 0),

// @Param: _RESET
// @DisplayName: Reset time
// @Description: Seconds since January 1st 2016 (Unix epoch+1451606400) since reset (set to 0 to reset statistics)
// @DisplayName: Statistics Reset Time
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how we're adding "Statistics" to the param descriptions. I think in general all parameter descriptions should, if possible, start with the feature or library name.

@peterbarker
Copy link
Contributor

I'm going to have another look at this.

tridge pointed out we probably are overflowing this number ATM, so we'll move to minutes in this field...

@amilcarlucas
Copy link
Contributor Author

The change to minutes is scope creeping and should be done with a future PR.

@khancyr
Copy link
Contributor

khancyr commented Sep 1, 2020

@amilcarlucas @peterbarker This one is not a bad change, it would need a rebase.... I think it is safe change to merge to prevent unexpected reset of statistics. But I don't think that the removal of the @readonly flag is usefull
@peterbarker I failed to see how you can the usage of reset can change the time correctly

@amilcarlucas
Copy link
Contributor Author

If you can write to it, it is no longer read_only. Should I change it?

@amilcarlucas amilcarlucas force-pushed the pr-ap-stats-reset-only-when-zero branch from d56c94b to 5381764 Compare September 1, 2020 10:09
@khancyr
Copy link
Contributor

khancyr commented Sep 1, 2020

The readonly flag is abused here as notSettable. You could send some value but they won't be settled

@amilcarlucas
Copy link
Contributor Author

Did the requested changes

@peterbarker
Copy link
Contributor

@amilcarlucas we've recently come down hard on someone for doing the same "magic parameter" thing that I did in the original stats stuff - resetting one parameter chains on to doing others. I'm pretty sure for consistency we should probably move away from magic parameters - and if we're going to do that should probably not change things twice :-)

I propose we add something to an existing mav-command-long message that clears the onboard flight statistics. This can work in parallel with the existing clear code - which people currently expect.

Now - if we can decide where to add it, that would be great! Remembering we also need to get that mavlink change upstream....

Maybe adding a bit to param7 in MAV_CMD_PREFLIGHT_CALIBRATION which, if set, clears the flight statistics?

@davidbuzz
Copy link
Collaborator

davidbuzz commented Aug 17, 2021

@peterbarker - "Maybe adding a bit to param7 in MAV_CMD_PREFLIGHT_CALIBRATION which, if set, clears the flight statistics?" - are you suggesting that this PR should be closed in favour of a different one that does thius alternativeive..?

@davidbuzz
Copy link
Collaborator

nudge

…RESET parameter to zero.

This allows users to load parameter files (in MP, MAVProxy or any other GCS) without
accidentally reseting the statistics, because the AP_STATS_RESET value contained in
the parameter file will be ignored (unless it is zero and it is usually not zero).
The other statistics parameters are read-only, and the GCS should be clever enough to not set those.
@amilcarlucas amilcarlucas force-pushed the pr-ap-stats-reset-only-when-zero branch from e7927bb to 3e55bf7 Compare February 20, 2024 15:42
@amilcarlucas
Copy link
Contributor Author

Rebased once more. This has been lingering since 6 and a half years.

@tridge tridge merged commit 4996c17 into ArduPilot:master Feb 21, 2024
92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants