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

SITL : added a basic simulated SBF GPS receiver #27859

Merged
merged 4 commits into from
Aug 20, 2024
Merged

SITL : added a basic simulated SBF GPS receiver #27859

merged 4 commits into from
Aug 20, 2024

Conversation

chiara-septentrio
Copy link
Contributor

A basic simulator that returns PVT with the PVTGeodetic v1 SBF block

@Ryanf55 Ryanf55 self-requested a review August 16, 2024 16:49
Ryanf55
Ryanf55 approved these changes Aug 16, 2024
View reviewed changes
Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

LGTM, nice and simple. Always great to see more simulators added!

libraries/SITL/SIM_GPS.h Show resolved Hide resolved
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Aug 16, 2024

Did you want this added to the autotest suite?

chiara-septentrio and others added 3 commits August 19, 2024 10:03
@chiara-septentrio @peterbarker
SITL: SBF driver
72d3ffb
@peterbarker
AP_GPS: SBF: avoid FPE when undulation is DNF
4b168f8
@peterbarker
SITL: correct and augment SBF simulator
40cb903 
 - checksum calculation was including header parts in CRC twice
 - need to send DOP message to make EKF happy
 - need to supply own number of satellites
 - must pad packets to a multiple of 4 bytes
@peterbarker
Copy link
Contributor

@chiara-septentrio I have made this simulator work.

Next time you open a PR, please state what testing it has had. @Ryanf55 assumed a minimal level of testing, probably thinking you had at least verified this was working in SITL. Given the several problems I encountered, surely the only level of testing this could have had was "it compiles"?

I have added it to the autotest suite, so now we have some level of protection against regressions in the Septentrio driver.

I have also made a patch to the Septentrio driver to not raise a numeric exception when the undulation comes in as DNU; the simulator was supplying DNU there. Please verify that that patch is correct.

@chiara-septentrio
Copy link
Contributor Author

It's my bad, I misunderstood how to test for it correctly. I launched a simulation without the console or map and since I didn't see any error, I thought it was fine. I had never used SITL before and I obviously didn't correctly understand to documentation, I'm sorry.
I tested the changes with the console and map this time, using mode guided and circle, and it seemed to work fine.
I just changed the function to check the configuration is done to take simulation into account otherwise the function was waiting for the GPS to respond to commands infinitely. From what I understand it should still be able to use an actual GPS with the simulation.

@peterbarker
Copy link
Contributor

OK, LGTM. Thanks for re-testing it!

I assume you're OK with the change in the actual driver, and I'm marking it MergeOnCIPass.

Thanks for coming back and creating this for us!

@peterbarker peterbarker merged commit 2e1364e into ArduPilot:master Aug 20, 2024
93 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks @chiara-septentrio !

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

@peterbarker peterbarker peterbarker left review comments

@Ryanf55 Ryanf55 Ryanf55 approved these changes

Assignees
No one assigned
Labels
MergeOnCIPass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chiara-septentrio @Ryanf55 @peterbarker