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.

Sign up for GitHub

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

Jump to bottom

Fix GNSS new data #12725

Merged
merged 1 commit into from May 10, 2023
Merged

Fix GNSS new data #12725

merged 1 commit into from May 10, 2023

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Apr 24, 2023 •
edited

Fixes gating for sending data in onGpsNewData

@haslinghuis haslinghuis added this to the 4.5 milestone Apr 24, 2023
@haslinghuis haslinghuis self-assigned this Apr 24, 2023
haslinghuis
haslinghuis commented Apr 24, 2023
View reviewed changes
src/main/io/gps.c Outdated
@@ -1955,7 +1955,7 @@ void onGpsNewData(void)

lastTimeUs = timeUs;

if (!(STATE(GPS_FIX) && gpsSol.numSat >= GPS_MIN_SAT_COUNT)) {
if (!(STATE(GPS_FIX) || gpsSol.numSat >= GPS_MIN_SAT_COUNT)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

// not (A or B) = (not A) and (not B)
// not (A and B) = (not A) or (not B)

Copy link
Member

Choose a reason for hiding this comment

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

Good old DeMorgan… https://en.m.wikipedia.org/wiki/De_Morgan%27s_laws

Copy link
Member

@ctzsnooze ctzsnooze Apr 25, 2023 •
edited

Choose a reason for hiding this comment

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

Good catch!
But lets think about this.
The intent is, while flying home, to not interrupt the rescue unless the GPS data is such that the quad would likely be going completely haywire.
So long as the GPS reports a 3D fix, regardless of the satellite count, it is probably sufficient to keep flying home.
However just having a 3D fix, and ignoring the sat count, would not be a sufficient requirement for determining a valid home point.
So we would need to look at this in relation to the rescue process itself.
The test for arming in GPS Rescue mode, if a valid home point is required, should be the presence of both a 3D fix and at least min satellites.
The test for continuing the rescue should be only that we have a 3D fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, we should remove the second part of the condition here ... done

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think we only require the 3D fix check here.

@github-actions

This comment has been minimized.

Sign in to view
haslinghuis
haslinghuis commented Apr 24, 2023
View reviewed changes
src/main/io/gps.c Outdated Show resolved Hide resolved
@haslinghuis haslinghuis mentioned this pull request Apr 24, 2023
Closed
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@blckmn
Copy link
Member

blckmn commented Apr 25, 2023 •
edited

AUTOMERGE: (FAIL)

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

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
ctzsnooze
ctzsnooze reviewed Apr 25, 2023
View reviewed changes
src/main/io/gps.c Outdated Show resolved Hide resolved
@haslinghuis
Fix gps new data
bb59a7d 
Initialize next_fix
@github-actions

This comment has been minimized.

Sign in to view
@ctzsnooze ctzsnooze requested a review from KarateBrot May 1, 2023 02:20
@haslinghuis haslinghuis changed the title Fix gps new data Add UBX_MON_VER May 3, 2023
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@ZzyzxTek
Copy link
Contributor

ZzyzxTek commented May 4, 2023

The original change ([Fix gps new data) was to fix the gating for sending data to GPS rescue.

The PR added a a version info poll message and changed the PR name to that. That seems something different than the original fix.

The version info isn't currently processed--is the intent to add parsing the return message and saving and/or doing something with the info?

That would be cool, I'd like to use that for the GPS status info I'm working on. I was planning on adding a version info poll at some point. Also a lot of discussion in the discord #imu-development channel on using version info to select which protocol version of config commands to use (to support M10).

@haslinghuis
Copy link
Member Author

Yes - you are right. I will revert second commit and open a new PR as I was following along the discord channel to keep fixes and improvements separated.

The version info isn't currently processed--is the intent to add parsing the return message and saving and/or doing something with the info?

That is the intent.

@haslinghuis haslinghuis changed the title Add UBX_MON_VER Fix GPS new data May 4, 2023
@github-actions
Copy link

github-actions bot commented May 4, 2023

Do you want to test this code? Here you have an automated build:
Assets
WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

@ctzsnooze ctzsnooze requested a review from ledvinap May 10, 2023 01:00
@ctzsnooze
Copy link
Member

@ZzyzxTek @haslinghuis @KarateBrot @ledvinap @KarateBrot ...

I think this PR now makes just a small change, and a useful one. The GPS code should keep receiving data even if sat count is low, so long as the unit provides a 3D fix the data should be usable. If the data is not usable, or there is no 3D fix, a sanity check will handle that.

If everyone is OK with the PR as it is, please approve so we can merge it.

@ctzsnooze ctzsnooze self-requested a review May 10, 2023 01:07
@haslinghuis haslinghuis mentioned this pull request May 10, 2023
Merged
@haslinghuis haslinghuis changed the title Fix GPS new data Fix GNSS new data May 10, 2023
@haslinghuis haslinghuis merged commit f3fab66 into betaflight:master May 10, 2023
38 checks passed
@haslinghuis haslinghuis deleted the fix-gps-new-data branch May 10, 2023 22:47
AkankshaJjw pushed a commit to AkankshaJjw/betaflight that referenced this pull request May 29, 2023
@haslinghuis @AkankshaJjw
Fix GNSS new data (betaflight#12725)
3c41147 
Fix gps new data

Initialize next_fix
davidbitton pushed a commit to davidbitton/betaflight that referenced this pull request Feb 5, 2024
@haslinghuis @davidbitton
Fix GNSS new data (betaflight#12725)
e7fb65c 
Fix gps new data

Initialize next_fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveCEvans SteveCEvans SteveCEvans left review comments

@blckmn blckmn blckmn approved these changes

@ctzsnooze ctzsnooze ctzsnooze approved these changes

@KarateBrot KarateBrot KarateBrot approved these changes

@ledvinap ledvinap Awaiting requested review from ledvinap

@sugaarK sugaarK Awaiting requested review from sugaarK

@McGiverGim McGiverGim Awaiting requested review from McGiverGim

Assignees

@haslinghuis haslinghuis

Labels
RN: BUGFIX
Projects
Finalizing Firmware 4.5 Release
Status: COMPLETED
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@haslinghuis @blckmn @ZzyzxTek @ctzsnooze @SteveCEvans @KarateBrot