Add GPS Lap Timer #11856

SpencerGraffunder · 2022-09-26T15:10:47Z

Resubmitting #10968. See discussion there.
Feature described here: https://youtu.be/TA5cWwFafY4

Test with:
make XYZ EXTRA_FLAGS="-DUSE_GPS -DUSE_GPS_LAP_TIMER"

ledvinap · 2022-09-26T15:59:51Z

src/main/cli/settings.h

@@ -185,8 +187,8 @@ typedef enum {
 #define VALUE_MODE_MASK (0xE0)

 typedef struct cliMinMaxConfig_s {
-    const int16_t min;
-    const int16_t max;
+    const int32_t min;


This will double size of cliValueConfig_t, considerably increasing flash size.
Maybe just assume that int32 has symmetric limits and use d32Max. But it will need some more general six in future

ledvinap · 2022-09-26T16:04:09Z

src/main/fc/gps_lap_timer.h

+
+typedef struct gpsLapTimerData_s {
+    gpsLocation_t gateLocation;
+    uint32_t previousLaps[3];


Maybe use #define for lap history. It bill make rest of code slightly cleaner (for instead of inlining 3 readings)

ledvinap · 2022-09-26T16:06:02Z

src/main/io/gps.c

@@ -683,7 +685,17 @@ void gpsInitUblox(void)
                        }
                        break;
                    case 12:
-                        ubloxSetNavRate(0x64, 1, 1); // set rate to 10Hz (measurement period: 100ms, navigation rate: 1 cycle) (for 5Hz use 0xC8)
+                        switch (gpsConfig()->gps_update_rate) {


Using array mapping index to {0xc8,0x64,0x35} and maybe #defining it's context near GPS_UPDATE_RATE_xHZ will be cleaner,

I think I want to just use an int to store the update rate.
The easiest way to do that would be to store it as a period in milliseconds, because that can be passed straight to the existing ubloxSetNavRate function.
The other way, which would be easier for the user, would be to store it as a rate in Hz and have ubloxSetNavRate convert the Hz to a period. The problem here is that the rates may not be integers. The max rate I found to work on my units was 53ms, or 18.86Hz. So then we would set the max allowed value to 19Hz, but clamp the calculated period to 53ms.

IMO using enum is a bit better here. It will prevent users using values that don't work. Without error message, this can be support nightmare.

ledvinap · 2022-09-26T16:07:05Z

src/main/io/gps.h

+    GPS_UPDATE_RATE_10HZ,
+    GPS_UPDATE_RATE_19HZ
+} gpsUpdateRate_e;
+


#define GPS_UPDATE_RATE__MAP {0xc8,0x64,0x35} or similar, see above

ledvinap · 2022-09-26T16:09:50Z

src/main/osd/osd.c

+        uint32_t lapTimeSeconds;
+        uint32_t lapTimeDecimals;
+        if (gpsLapTimerData.best3Consec != 0) {
+            lapTimeSeconds = gpsLapTimerData.best3Consec / 1000;
+            lapTimeDecimals = (gpsLapTimerData.best3Consec % 1000) / 10;
+            tfp_sprintf(buff, "%3u.%02u", lapTimeSeconds, lapTimeDecimals);
+        } else {
+            tfp_sprintf(buff, "  -.--");
+        }
+


This can be replaced with function (maybe including optional prefix character/string /empty value), removing lot of code duplication

ledvinap · 2022-09-26T16:10:50Z

src/main/osd/osd.c

+        lapTimeSeconds = gpsLapTimerData.bestLapTime / 1000;
+        lapTimeDecimals = (gpsLapTimerData.bestLapTime % 1000) / 10;


unused code

ledvinap · 2022-09-26T16:12:20Z

src/main/osd/osd_elements.c

@@ -1014,8 +1015,18 @@ static void osdElementGpsHomeDirection(osdElementParms_t *element)
 {
    if (STATE(GPS_FIX) && STATE(GPS_FIX_HOME)) {
        if (GPS_distanceToHome > 0) {


(technically, gpsLapTimerData.timerRunning will be ignored with GPS_distanceToHome == 0)

Also, does code below handle correctly case when distance to gate is zero?

ledvinap · 2022-09-26T16:18:24Z

src/main/osd/osd_elements.c

+#ifdef USE_GPS_LAP_TIMER
+            // Change the "home" point to the start/finish location if the lap timer is running
+            if (gpsLapTimerData.timerRunning) {
+                const int h = gpsLapTimerData.dirToPoint/100 - DECIDEGREES_TO_DEGREES(attitude.values.yaw);
+                element->buff[0] = osdGetDirectionSymbolFromHeading(h);
+            } else {
+#endif
            const int h = DECIDEGREES_TO_DEGREES(GPS_directionToHome - attitude.values.yaw);
            element->buff[0] = osdGetDirectionSymbolFromHeading(h);
+#ifdef USE_GPS_LAP_TIMER
+            }
+#endif


Suggested change

#ifdef USE_GPS_LAP_TIMER

// Change the "home" point to the start/finish location if the lap timer is running

if (gpsLapTimerData.timerRunning) {

const int h = gpsLapTimerData.dirToPoint/100 - DECIDEGREES_TO_DEGREES(attitude.values.yaw);

element->buff[0] = osdGetDirectionSymbolFromHeading(h);

} else {

#endif

const int h = DECIDEGREES_TO_DEGREES(GPS_directionToHome - attitude.values.yaw);

element->buff[0] = osdGetDirectionSymbolFromHeading(h);

#ifdef USE_GPS_LAP_TIMER

}

#endif

do {

#ifdef USE_GPS_LAP_TIMER

// Override the "home" point to the start/finish location if the lap timer is running

if (gpsLapTimerData.timerRunning) {

const int h = gpsLapTimerData.dirToPoint/100 - DECIDEGREES_TO_DEGREES(attitude.values.yaw);

element->buff[0] = osdGetDirectionSymbolFromHeading(h);

break;

}

#endif

const int h = DECIDEGREES_TO_DEGREES(GPS_directionToHome - attitude.values.yaw);

element->buff[0] = osdGetDirectionSymbolFromHeading(h);

} while(0);

Definitely need to fix this section. Is there any reason not to do it like this? It uses a non-const int, but I assume that doesn't make it to the machine code anyways, right?

Suggested change

#ifdef USE_GPS_LAP_TIMER

// Change the "home" point to the start/finish location if the lap timer is running

if (gpsLapTimerData.timerRunning) {

const int h = gpsLapTimerData.dirToPoint/100 - DECIDEGREES_TO_DEGREES(attitude.values.yaw);

element->buff[0] = osdGetDirectionSymbolFromHeading(h);

} else {

#endif

const int h = DECIDEGREES_TO_DEGREES(GPS_directionToHome - attitude.values.yaw);

element->buff[0] = osdGetDirectionSymbolFromHeading(h);

#ifdef USE_GPS_LAP_TIMER

}

#endif

int direction = GPS_directionToHome

#ifdef USE_GPS_LAP_TIMER

// Override the "home" point to the start/finish location if the lap timer is running

if (gpsLapTimerData.timerRunning) {

direction = gpsLapTimerData.dirToPoint/10; // dirToPoint is centidegrees

}

#endif

element->buff[0] = osdGetDirectionSymbolFromHeading(DECIDEGREES_TO_DEGREES(direction - attitude.values.yaw));

Yes, overriding direction is fine.
My comment was mainly about two #ifdef mixed into C block, it is too easy to break with future changes.

ledvinap · 2022-09-26T16:21:30Z

src/main/osd/osd_elements.c

+#ifdef USE_GPS_LAP_TIMER
+static void osdElementGpsLapTimeCurrent(osdElementParms_t *element)
+{
+    uint32_t lapTimeSeconds = gpsLapTimerData.currentLapTime / 1000;


(see above; prefix variant)

Also, isn't -:-- possible here too ?

ledvinap · 2022-09-26T16:22:44Z

src/main/target/common_pre.h

@@ -373,7 +373,7 @@ extern uint8_t _dmaram_end__;
 #define USE_CANVAS
 #define USE_FRSKYOSD
 #define USE_GPS
-#define USE_GPS_NMEA


This seems like quite intrusive change

I agree. This was done mostly just let me test. We will have to discuss feature priority. I understand keeping NMEA for whacky receivers that can't do UBLOX. How do we decide which features go away when making room for new ones?

blckmn · 2022-09-26T18:17:26Z

AUTOMERGE: (FAIL)

github identifies PR as mergeable -> PASS
assigned to a milestone -> PASS
cooling off period lapsed -> PASS
commit count less or equal to three -> FAIL
Don't merge label NOT found -> PASS
at least one RN: label found -> PASS
Tested label found -> FAIL
assigned to an approver -> FAIL
approver count at least three -> PASS

SpencerGraffunder · 2022-09-26T19:24:48Z

Thanks for the through review, @ledvinap! Just what I've been waiting for.
I'll have to take some time to work through your comments.

ledvinap · 2022-09-27T07:31:50Z

BTW: What about using home position as a gate? Gate positioning code would apply to home (which may be useful for other purposes), only problem I see is unintended flyaway when someone saves different site as home (I heard of $20k drone flying back to Africa on first test)

SpencerGraffunder · 2022-09-27T12:41:49Z

BTW: What about using home position as a gate? Gate positioning code would apply to home (which may be useful for other purposes)

Can you explain more? I don't see how it would be helpful.

ledvinap · 2022-09-27T12:53:42Z

Current code uses concept of 'home' location. It is initialized on arming and used for HOME info on OSD and RTH code. But it is quite similar to your gate location.
If user can specify Home location (using mechanisms from this PR), it can be used as Gate. And it will allow specifying RTH/OSD home that is different from arming location (possibly useful if you want clear place for RTH / OSD landing). Eventually, it can be extended to support geofencing etc.

SpencerGraffunder · 2022-09-27T18:30:51Z

Current code uses concept of 'home' location. It is initialized on arming and used for HOME info on OSD and RTH code. But it is quite similar to your gate location. If user can specify Home location (using mechanisms from this PR), it can be used as Gate. And it will allow specifying RTH/OSD home that is different from arming location (possibly useful if you want clear place for RTH / OSD landing). Eventually, it can be extended to support geofencing etc.

I see what you're talking about now. I think a non-volatile RTH position wouldn't be desirable because of flyaways. I could see it being cool to have a completely separate "POI" with a distance and pointer that you could have set at your vacation home or an airport, but I don't feel like betaflight is the right place for a secondary airfield feature.
If we don't like the idea of hijacking the home OSD elements, I could create a second set that's just for the lap timer.

ctzsnooze · 2022-10-12T02:15:56Z

Very cool!

Thanks @ledvinap for your assistance.

Can I clarify that GPS Rescue can still be activated via switch or RC loss, while this function is active? I can imagine this opening up the possibility of relatively long-distance races where a functional GPS Rescue could be required, even if the user wants to use GPS to time their laps.

Also, GPS Rescue now does good landings. I can imagine that if the user chose the 'set home point once' rescue option, and arms once at the landing point, then they could go racing with this, do their laps, and at the end just hit GPS Rescue switch, and it would bring back and land at the correct spot. Hence I think that keeping the GPS Rescue capability and its home point separate, and both functional, would be good.

How much extra flash space is required?

SpencerGraffunder · 2022-10-13T15:38:55Z

I haven't tested GPS Rescue while the timer is running, but I can't imagine any way it would interfere. I'm not planning to overwrite the home point at all other than hijacking the OSD element. Even that isn't necessary if we want to make two new elements for distance and direction to the gate. I'm not controlling flight or injecting positioning data to existing structs.

Not sure on flash space. After implementing some of the suggestions here, I was able to build for all targets within the flash limits without removing any other features.

Unfortunately I am too busy to work on this any more this month. The code still needs some changes done, so this isn't ready for testing yet.

ctzsnooze · 2022-10-13T23:03:03Z

OK thanks for the heads up.
Temporary hijacking of home point is OK to me.
Later on we need to test and confirm that GPS Rescue still works while this is active.
We will put this PR 'on ice' until you get back to us.

src/main/cms/cms_menu_gps_lap_timer.c

src/main/fc/gps_lap_timer.c

KarateBrot · 2022-10-17T13:04:02Z

src/main/fc/gps_lap_timer.c

+#define MIN(a,b) (((a)<(b))?(a):(b))
+#define MAX(a,b) (((a)>(b))?(a):(b))
+


Suggested change

#define MIN(a,b) (((a)<(b))?(a):(b))

#define MAX(a,b) (((a)>(b))?(a):(b))

These macros are defined in common/maths.h, so we can remove them here.

KarateBrot · 2022-10-17T13:18:19Z

Later on we need to test and confirm that GPS Rescue still works while this is active.

Why would GPS Rescue not work with this active? This is the normal acro mode we are used to, just with OSD elements for lap times that is using the GPS data to get times. This is not some fancy flight mode that could potentially block other flight modes.

howels · 2022-11-02T13:23:51Z

@SpencerGraffunder can you update this so it's ready for 4.5? Looks like some changes were requested but the feature freeze was missed. However it's a great looking feature so please update as I'm sure this will be popular.

KarateBrot · 2022-11-02T14:27:53Z

@SpencerGraffunder can you update this so it's ready for 4.5? Looks like some changes were requested but the feature freeze was missed. However it's a great looking feature so please update as I'm sure this will be popular.

Feature freeze wasn't missed. This PR was opened well before the feature freeze so no problem 👍

howels · 2022-11-02T14:48:08Z

@SpencerGraffunder can you update this so it's ready for 4.5? Looks like some changes were requested but the feature freeze was missed. However it's a great looking feature so please update as I'm sure this will be popular.

Feature freeze wasn't missed. This PR was opened well before the feature freeze so no problem +1

thanks for the correction, was worried this wasn't making the cut. Even better news!

Co-authored-by: Jan Post <Rm2k-Freak@web.de>

src/main/osd/osd_elements.c

haslinghuis · 2023-05-20T11:55:27Z

@SteveCEvans are the requested changes resolved?

SteveCEvans · 2023-05-21T21:57:58Z

src/main/fc/gps_lap_timer.c

+        } else {
+            // Just left the circle, so record the lap time
+            if (wasInCircle) {
+                // Not the first time through the gate


I'm happy with this PR, but as a possible future enhancement would we be able to get better timing accuracy with a smaller circle size qualified with an exit direction? Setting this direction would be a potential challenge, perhaps requiring training on a practice lap?

If you have a compass you could dial in the heading of the gate directly through the CMS menu or CLI.

@SteveCEvans IIRC gate time is taken at closest point to gate (minimum distance is tracked, with timestamp). So actual gate size should not matter.

An exit direction could help by letting us make the circle bigger and not be caught by other parts of the track. Since it's using the closest point to the gate waypoint, I think that's the most accurate we can be without correcting the gate waypoint. I'm sure there is a lot more we could do with this in the future. Split times and auto-correcting the start/finish point are two things I've thought of.

src/main/cms/cms_menu_gps_lap_timer.c

SpencerGraffunder · 2023-05-22T12:17:19Z

I think I've resolved everything. Does the INT32 stuff still need to be changed, @ledvinap?

src/main/cms/cms_menu_gps_lap_timer.c

src/main/cms/cms_menu_gps_lap_timer.h

Co-authored-by: Jan Post <Rm2k-Freak@web.de>

github-actions · 2023-05-24T13:51:38Z

Do you want to test this code? Here you have an automated build:
Assets

See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions about installing this unified targets!
An easier way to test this, using the Configurator cloud build, is simply put #11856 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

Easy4Racing · 2023-05-28T01:59:51Z

Hi , I want to run a test for this new feature but failed.

I flashed the new Hex file cloud built by Configurator(version 10.10.0-debug-e6f3b5b).
Selected sensor input 'GPS' in Ports tab, enabled GPS & OSD in Configuration tab, enabled 'Unknown 1.2.3' in OSD tab. Nothing of GPS_lap_timer displayed but the craft_name and on time is ok.

What should I do next ?
My board is OMNIBUSF4

SpencerGraffunder · 2023-05-28T02:42:33Z

What should I do next ? My board is OMNIBUSF4

Did you add USE_GPS_LAP_TIMER in custom defines?

Your GPS also needs a lock before the elements will show.

Easy4Racing · 2023-05-28T02:55:06Z

What should I do next ? My board is OMNIBUSF4

Did you add USE_GPS_LAP_TIMER in custom defines?

Your GPS also needs a lock before the elements will show.

Thx for reply.
I set the custom defines but have not connect the GPS module. So, this is the reason.
I thought here may be an initialization value, even if I don't pick up GPS, or the GPS module work improperly .

haslinghuis · 2023-05-28T08:49:27Z

Don't use USE in Custom Defines: GPS_LAP_TIMER

* Add gps lap timer * change timing to GPS time instead of local time * rebase and minor changes * implement KarateBrot's suggestions * follow ledvinap's suggestions, some OSD symbol changes * move platform.h include to the top Co-authored-by: Jan Post <Rm2k-Freak@web.de> * fix osd elements not showing, remove useless block * cleanup, move pg stuff to pg folder * cleanup from review * minor mods to gps lap timer update, add number of laps tracked * rename time variable * add const to timeMs Co-authored-by: Jan Post <Rm2k-Freak@web.de> * Update licenses, add is_sys_element macro * update licenses * round to nearest centisecond Co-authored-by: Jan Post <Rm2k-Freak@web.de> --------- Co-authored-by: Jan Post <Rm2k-Freak@web.de>

SpencerGraffunder changed the title ~~Add gps lap timer and supporting type changes~~ Add GPS Lap Timer Sep 26, 2022

This comment has been minimized.

Sign in to view

KarateBrot assigned SpencerGraffunder Sep 26, 2022

KarateBrot added RN: MAJOR FEATURE Testing Required Needs coordination with betaflight-configurator labels Sep 26, 2022

KarateBrot added this to the 4.4 milestone Sep 26, 2022

ledvinap reviewed Sep 26, 2022

View reviewed changes

SpencerGraffunder force-pushed the gps-lap-timer branch from e90cab0 to faa3465 Compare September 27, 2022 18:22

This comment has been minimized.

Sign in to view

SpencerGraffunder force-pushed the gps-lap-timer branch 2 times, most recently from 86272c1 to 48a8c56 Compare September 30, 2022 02:41

haslinghuis added the Rebase / Squash required label Oct 3, 2022

KarateBrot requested changes Oct 17, 2022

View reviewed changes

KarateBrot mentioned this pull request Nov 4, 2022

Altitude sensor fusion (GPS+Barometer) #11958

Draft

SpencerGraffunder and others added 2 commits May 19, 2023 14:03

rename time variable

8f5adf4

add const to timeMs

8ee2e74

Co-authored-by: Jan Post <Rm2k-Freak@web.de>

SpencerGraffunder force-pushed the gps-lap-timer branch from eab3a02 to 8ee2e74 Compare May 19, 2023 18:08

This comment has been minimized.

Sign in to view

haslinghuis removed the Rebase Required label May 19, 2023

haslinghuis approved these changes May 19, 2023

View reviewed changes

KarateBrot reviewed May 20, 2023

View reviewed changes

src/main/osd/osd_elements.c Outdated Show resolved Hide resolved

blckmn approved these changes May 21, 2023

View reviewed changes

SteveCEvans approved these changes May 21, 2023

View reviewed changes

haslinghuis requested changes May 21, 2023

View reviewed changes

src/main/cms/cms_menu_gps_lap_timer.c Outdated Show resolved Hide resolved

Update licenses, add is_sys_element macro

81d98d9

This comment has been minimized.

Sign in to view

haslinghuis approved these changes May 22, 2023

View reviewed changes

KarateBrot requested changes May 23, 2023

View reviewed changes

src/main/cms/cms_menu_gps_lap_timer.c Outdated Show resolved Hide resolved

src/main/cms/cms_menu_gps_lap_timer.h Outdated Show resolved Hide resolved

haslinghuis self-requested a review May 23, 2023 09:50

update licenses

5638d36

This comment has been minimized.

Sign in to view

haslinghuis approved these changes May 23, 2023

View reviewed changes

KarateBrot approved these changes May 23, 2023

View reviewed changes

round to nearest centisecond

955ed9c

Co-authored-by: Jan Post <Rm2k-Freak@web.de>

haslinghuis merged commit aad197f into betaflight:master May 25, 2023

		lapTimeSeconds = gpsLapTimerData.bestLapTime / 1000;
		lapTimeDecimals = (gpsLapTimerData.bestLapTime % 1000) / 10;

		#define MIN(a,b) (((a)<(b))?(a):(b))
		#define MAX(a,b) (((a)>(b))?(a):(b))

Add GPS Lap Timer #11856

Add GPS Lap Timer #11856

Conversation

SpencerGraffunder commented Sep 26, 2022 • edited by KarateBrot Loading

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blckmn commented Sep 26, 2022 • edited Loading

SpencerGraffunder commented Sep 26, 2022 • edited Loading

ledvinap commented Sep 27, 2022

SpencerGraffunder commented Sep 27, 2022

ledvinap commented Sep 27, 2022

This comment has been minimized.

SpencerGraffunder commented Sep 27, 2022

ctzsnooze commented Oct 12, 2022

SpencerGraffunder commented Oct 13, 2022

ctzsnooze commented Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

KarateBrot commented Oct 17, 2022 • edited Loading

howels commented Nov 2, 2022

KarateBrot commented Nov 2, 2022

howels commented Nov 2, 2022

This comment has been minimized.

haslinghuis commented May 20, 2023

Choose a reason for hiding this comment

KarateBrot May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

SpencerGraffunder commented May 22, 2023

This comment has been minimized.

github-actions bot commented May 24, 2023

Easy4Racing commented May 28, 2023

SpencerGraffunder commented May 28, 2023

Easy4Racing commented May 28, 2023

haslinghuis commented May 28, 2023

SpencerGraffunder commented Sep 26, 2022 •

edited by KarateBrot

Loading

blckmn commented Sep 26, 2022 •

edited

Loading

SpencerGraffunder commented Sep 26, 2022 •

edited

Loading

ctzsnooze commented Oct 13, 2022 •

edited

Loading

KarateBrot commented Oct 17, 2022 •

edited

Loading

KarateBrot May 22, 2023 •

edited

Loading