Skip to content

Conversation

@TransientTetra
Copy link
Contributor

I'm proposing a new field to be added to stats, flight counter, i.e. number of flights. I used an assumption that a flight counts once the craft is armed and disarmed and a certain time passes between the two (same as other fields); but only once per each battery pack. I also used uint32 for the field, which I feel is a bit excessive; for uint16 max number of flights would be 32768, I'm leaning on your judgement on that matter.

@MrD-RC
Copy link
Member

MrD-RC commented May 12, 2024

I think uint16_t would be plenty to be honest. That's enough for 1 flight a day for 179 years 🤣

@MrD-RC
Copy link
Member

MrD-RC commented May 12, 2024

I'm wondering if it needs better detection for incrementing the number of flights. Just relying on the arm/disarm action can provide false positives.

MrD-RC
MrD-RC previously requested changes May 12, 2024
Copy link
Collaborator

@stronnag stronnag left a comment

Choose a reason for hiding this comment

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

Please update the PG Template version as you've added a field;

PG_REGISTER_WITH_RESET_TEMPLATE(statsConfig_t, statsConfig, PG_STATS_CONFIG, 1);

i.e. from 1 to 2

@TransientTetra TransientTetra requested a review from stronnag May 12, 2024 19:49
@TransientTetra
Copy link
Contributor Author

Any updates on this?

@breadoven
Copy link
Collaborator

breadoven commented Jun 13, 2024

Any updates on this?

Seems OK to me other than the minor formatting comment I made.

Copy link
Contributor

@OptimusTi OptimusTi left a comment

Choose a reason for hiding this comment

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

I'd increase this number to avoid false flights due to distance accumulation during armed workbench tests, if that makes sense.

Co-authored-by: OptimusTi <45466510+OptimusTi@users.noreply.github.com>
@TransientTetra
Copy link
Contributor Author

Any additional suggestions or requests on this? Style now is according to @breadoven request, min flight distance was changed to 30m

@TransientTetra
Copy link
Contributor Author

Hey, how do we go forward with this?

@breadoven breadoven dismissed stale reviews from stronnag and MrD-RC July 26, 2024 22:07

completed

@sensei-hacker
Copy link
Member

It sounds like all of the comments have been addressed.

Has anyone other than the developer tested this? If not, can someone please do so?
In particular, we're interested in testing for anything that might break things, as opposed to it working as expected when all conditions are good. Is there any way this can break the stats screen or otherwise cause a problem?

@sensei-hacker
Copy link
Member

/review

@qodo-code-review
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
⚡ Recommended focus areas for review

Possible Overflow

stats_flight_count is defined as uint16_t, but increments are done via prev_flight_count + 1 without guarding against wraparound or capping at UINT16_MAX. Confirm intended wrap behavior or add bounds checking.

void statsOnDisarm(void)
{
    if (statsConfig()->stats_enabled) {
        uint32_t dt = (millis() - arm_millis) / 1000;
        if (dt >= MIN_FLIGHT_TIME_TO_RECORD_STATS_S) {
            statsConfigMutable()->stats_total_time += dt;   //[s]
            statsConfigMutable()->stats_total_dist += (getTotalTravelDistance() - arm_distance_cm) / 100;   //[m]
#ifdef USE_GPS
            // flight counter is incremented at most once per power on
            if (sensors(SENSOR_GPS)) {
                if ((getTotalTravelDistance() - arm_distance_cm) / 100 >= MIN_FLIGHT_DISTANCE_M) {
                    statsConfigMutable()->stats_flight_count = prev_flight_count + 1;
                }
            } else {
                statsConfigMutable()->stats_flight_count = prev_flight_count + 1;
            }
#else
            statsConfigMutable()->stats_flight_count = prev_flight_count + 1;
#endif // USE_GPS
Init Timing

statsInit is called late in init(); ensure no arm/disarm events can occur before prev_flight_count is captured, or make prev_flight_count lazy-loaded from statsConfig on first use to avoid a stale zero if ordering changes.

void statsInit(void)
{
    prev_flight_count = statsConfig()->stats_flight_count;
}
GPS Distance Threshold

When GPS is present, flights shorter than MIN_FLIGHT_DISTANCE_M are not counted; when GPS absent, all qualifying time flights are counted. Verify this asymmetric behavior is intended and won’t penalize indoor GPS-enabled use or cold-starts with low distance due to poor fix.

#ifdef USE_GPS
            // flight counter is incremented at most once per power on
            if (sensors(SENSOR_GPS)) {
                if ((getTotalTravelDistance() - arm_distance_cm) / 100 >= MIN_FLIGHT_DISTANCE_M) {
                    statsConfigMutable()->stats_flight_count = prev_flight_count + 1;
                }
            } else {
                statsConfigMutable()->stats_flight_count = prev_flight_count + 1;
            }
#else
            statsConfigMutable()->stats_flight_count = prev_flight_count + 1;
#endif // USE_GPS

@MrD-RC MrD-RC added this to the 9.0 milestone Oct 26, 2025
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.

6 participants