-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
AP_Stats: Only reset statistics if the user explicitly sets AP_Stats_… #6402
Conversation
this looks like it changes the default behavior from resetting to not resetting. Is that the default behavior we want? |
Any updates ? |
I still think this is useful, but I will close this one if nobody else feels it is an improvement. |
3b782a7
to
5538472
Compare
5538472
to
d56c94b
Compare
@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 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 |
There was a problem hiding this comment.
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.
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... |
The change to minutes is scope creeping and should be done with a future PR. |
@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 |
If you can write to it, it is no longer read_only. Should I change it? |
d56c94b
to
5381764
Compare
The readonly flag is abused here as notSettable. You could send some value but they won't be settled |
5381764
to
e7927bb
Compare
Did the requested changes |
@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 |
@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..? |
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.
e7927bb
to
3e55bf7
Compare
Rebased once more. This has been lingering since 6 and a half years. |
…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.