-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
SIM_GPS_GSOF - support dcol parser via new bidirectional data parsing #25541
Merged
peterbarker
merged 2 commits into
ArduPilot:master
from
Ryanf55:gsof-sim-support-dcol-parser
Nov 28, 2023
Merged
SIM_GPS_GSOF - support dcol parser via new bidirectional data parsing #25541
peterbarker
merged 2 commits into
ArduPilot:master
from
Ryanf55:gsof-sim-support-dcol-parser
Nov 28, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ryanf55
changed the title
Gsof sim - support dcol parser via new bidirectional data parsing
SIM_GPS_GSOF - support dcol parser via new bidirectional data parsing
Nov 15, 2023
tridge
requested changes
Nov 15, 2023
Ryanf55
force-pushed
the
gsof-sim-support-dcol-parser
branch
from
November 15, 2023 23:22
fb9daa9
to
2ce241b
Compare
Ryanf55
commented
Nov 21, 2023
Ryanf55
commented
Nov 21, 2023
rmackay9
reviewed
Nov 21, 2023
rmackay9
reviewed
Nov 21, 2023
rmackay9
reviewed
Nov 21, 2023
peterbarker
requested changes
Nov 21, 2023
Signed-off-by: Ryan Friedman <[email protected]>
Ryanf55
force-pushed
the
gsof-sim-support-dcol-parser
branch
from
November 21, 2023 07:06
2ce241b
to
1c41f0d
Compare
* Implement DCOL command support for GSOF simulator * Only send GSOF when enabled * Publish only at the configured rate * Only build GSOF packets when needed * This saves CPU * Make physics and read loop run at full rate * The logic to rate-limit writes is now pushed to the backend * Indent errors were fixed too Signed-off-by: Ryan Friedman <[email protected]>
Ryanf55
force-pushed
the
gsof-sim-support-dcol-parser
branch
from
November 21, 2023 07:11
1c41f0d
to
7815da1
Compare
peterbarker
approved these changes
Nov 28, 2023
@@ -235,7 +354,8 @@ class GPS : public SerialDevice { | |||
|
|||
int ext_fifo_fd; | |||
|
|||
uint32_t last_update; // milliseconds | |||
// The last time GPS data was written [mS] |
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.
Suggested change
// The last time GPS data was written [mS] | |
// The last time GPS data was written [ms] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
In #25525, the driver changes require active configuration parsing and response from the driver. That PR can't merge easily, and it wasn't trivial to implement because bidirectional serial simulators for GPS did not exist. This performs the leg work to set up the GPS as a full bidirectional capability.
How it works (physics, throttling, and updates)
Previous behavior
update_publish(const GPS_Data *d)
in the backend builds the packet and immediately publishes, so this needs to be throttledupdate_read(const GPS_Data *d)
in the backend reads incoming data. Previously, it read one bit per loop, and would just throw them on the floorNew behavior
How the GSOF parser works
I added a DCOL parser, which receives incoming configuration packets. Upon receipt of a successful packet, it spits back out an ACK. This will allow active configuration in #25525 to actually work.
The parser is pretty standard, using a switch statement, keeping track of parser state, and then I have a bunch of little routines for doing the parsing and utilities. A key important part is the fact that the GSOF simulator rates are now configured from ArduPilots driver, just like the real device, instead of the previous hard coded behavior of immediately outputting data streams. This now enforces whatever configuration packets are sent are probably valid. Coverage is not complete for all cases, but it at least covers the data channel enable/rate configuration that's currently in GSOF.
In order to handle scenarios where the simulator is missing configuration parsing, I want to let developers know with a a flow_of_control error. This is a good indication that you added new config in the driver without supporting it in the simulator, which is much better than silently failing.
As an aside, indent was totally off in SIM_GPS, so I fixed it.
Complications with SITL params interfering with GPS params
The GSOF driver is configuring the output rates with the APP File command, but then the ArduPilot simulator also gates write speeds with
_sitl->gps_hertz[instance]
. This param will interfere if it's lower than 10Hz because the GSOF driver should ideally be driven byGPS_RATE_MS
parameter. In the land of GSOF in this PR, there is no reason SITL should also have a parameter. One idea is to makeGPS_Backend::update(const GPS_Data &d)
virtual, and override in the GSOF driver to skip the throttling. In Dev call, Tridge thought it would good for the driver to set the simulation behavior instead of separate SIM params since this is more like the real device.Future work
Testing performed
./Tools/autotest/autotest.py build.Copter test.CopterTests2b.GPSTypes