GPS: Extract Septentrio Driver Into Separate Module #22904

flyingthingsintothings · 2024-03-19T09:25:54Z

Solved Problem

Making changes to the PX4 GPS drivers was hard because new features needed to be implemented for every driver. This isn't possible for most people as they don't have access to the hardware required for testing. During the previous PX4 community Q&A on Discord (2024-03-13), I discussed with @dagar what the options were to simplify this. The solution that was proposed was to separate all the drivers as they all want to specialize more than originally planned. This would allow the drivers to have their own parameters and implement features more rapidly. It would also simplify testing, as people who want to implement a new feature only need access to the hardware for a single driver.

Fixes /

Solution

Extract the Septentrio GPS driver out of the submodule into its own directory, included in the PX4 autopilot repository.

Changelog Entry

For release notes:

Feature/Bugfix XYZ
New parameter: XYZ_Z
Documentation: Need to clarify page ... / done, read docs.px4.io/...

Alternatives

We could also ...

Test coverage

Unit/integration test: ...
Simulation/hardware testing logs: https://review.px4.io/

Manual Testing

Pixhawk 6C & mosaic-go heading:
- Automatic GNSS receiver configuration works
- Position shows up in QGroundControl
- Number of reported satellites is correct
- Corrections from base station, forwarded by QGroundControl work
- Automatic startup of driver works
- Connection to receiver is stable and doesn't drop
- status command works
- Driver start and stop work

Context

Forum post about developer call where this was discussed
Original idea to split up driver
Teseo GPS driver used as reference
Other drivers in the PX4 autopilot repository were used as a guide (barometer, distance sensor...) because they are also split into separate drivers.

flyingthingsintothings · 2024-03-19T09:33:19Z

This currently compiles and runs, but it has several bugs and is not ready. I'm just looking for overall feedback about the approach and structure while I'm resolving these bugs. If this isn't the correct approach I would love some guidance about what can be improved/changed. I also haven't committed the changes to remove usage of the Septentrio driver in the submodule as I've been focused on the extracted driver first.

Some questions:

How would serial communication setup work when multiple submodules exist? Would there be a general GPS submodule responsible for forwarding calls to the actual driver modules? Is there another way to manage access to the serial ports?
This uses code from the submodule for RTCM parsing. Should this be included in the PX4 autopilot project? Should it be in every driver that needs it? I don't see much code shared in the other drivers (barometer, distance sensor...), but I think sharing RTCM parsing makes sense as it's a standardized format.

It would be nice to discuss this during the developer call this Wednesday (2024-03-20).

DronecodeBot · 2024-03-20T09:03:58Z

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-community-q-a-march-20-2024/37290/2

dagar · 2024-03-20T16:16:52Z

A few quick notes

move to srv/drivers/gnss to avoid confusion with drivers/gps
would a parameter prefix like GNSS_SBF work?

src/drivers/gnss/Kconfig

src/drivers/gnss/septentrio/septentrio.cpp

src/drivers/gnss/septentrio/septentrio.h

flyingthingsintothings · 2024-03-22T13:26:33Z

Other than the few questions I have about the code, I think this PR is ready for a first review. I tried to not rewrite too much, only copying the code that is required for Septentrio receivers. This way I hope regressions are prevented as much as possible. I updated/added documentation to most methods and variables, formatted the code as best as I could, removed any unused declarations, added licenses to all the files...

I have tested this PR on a Pixhawk 6C connected to a mosaic-go H. Configuration, positioning, injecting RTCM corrections and module functionality all seem to work fine. I will do the same tests on other autopilot hardware that I have available as well as with other receivers, and add more formal results of the tests here.

One thing I didn't copy from the original implementation was double receiver functionality, as I didn't know whether the second receiver was used (I read some comments about the receiver not actually being used for anything). If it was being used I can add double receiver code back.

I don't know yet how the merge process works (rebase, merge, one commit per PR...) but I can change the commits to follow the guidelines. I still have to squash the commits and give it a decent name as the current ones are just backups during development.

Any feedback is much appreciated 😊

julianoes · 2024-03-27T04:10:06Z

Sorry to be late to the party but how does this work alongside the GPS driver submodule? For instance, how does it work in terms of GPS parameters, auto detection, etc.?

flyingthingsintothings · 2024-03-27T08:56:11Z

I used the Teseo GPS module by @AlexKlimaj and the other modules in PX4 as a guide while writing this. Most of the other modules seem to use addresses to share a single serial connection while talking to peripherals. The GPS module is different because it uses a 1:1 connection to the receiver. The parameters used to configure the receiver are all unique to the module. That way every module uses only the parameters it needs and it's more clear to users what parameters are actually used by the driver (unlike the current ones where the description mentions if a parameter is only used by a single driver). The parameter used to select the serial port needs to be configured by the user to decide what driver they want to use. If they enable the parameter in the GPS module, the GPS module will be started (would be the default). If they enable it in the Septentrio module, the Septentrio module will be started. If the same port is selected in both modules, a single driver is started. This is what I understood was meant to happen when I discussed this with @dagar. If this approach isn't correct, I can change it according to feedback.

For now the GPS module is not changed. It still contains the driver for Septentrio receivers. I can remove it in this pull request, or make a separate pull request to remove the code for Septentrio receivers from the GPS module (some would remain in the submodule because it's used by QGroundControl).

julianoes · 2024-03-27T18:47:46Z

If this is the way forward as @dagar suggests I'm fine with it, but if it was up to me I would like it to be a clean swap without duplicate ways hanging around. And it would require changes in the docs as well, otherwise it gets even more confusing.

flyingthingsintothings · 2024-03-28T10:45:12Z

I will update the documentation. I can even do that as a PR alongside this one if that is preferred, but I would like to finish the design and decisions for the new driver before I start rewriting the documentation. Otherwise any changes to the driver would mean rewriting work in the documentation.

For the duplicated code, I think the only realistic approach is to do it over time. I can already remove the Septentrio driver from the GPS module. The Septentrio module code can be removed using makeconfig so it does not take up extra space. If another approach is preferred, please let me know 🙂

julianoes · 2024-03-28T20:20:16Z

I'll leave the design review, etc. to @dagar but that sounds fine to me then.

flyingthingsintothings · 2024-05-03T11:29:08Z

Updated the PR to use way faster automatic setup (previous one was copied from the original gps.cpp file under drivers/gps). The new implementation solves two bugs: Incorrectly identified ports (sometimes the driver would find weird ports as legitimate) and missed acknowledgement messages from the receiver (the previous implementation did not look correct and workarounds were already used to deal with that).

The previous automatic setup would take around 20 to 60 seconds where this one takes 1 to 2 seconds because of less failed ack message detection and faster returns after one has been found.

AlexKlimaj · 2024-06-04T00:02:49Z

src/drivers/gnss/septentrio/septentrio.cpp

+
+	// We should definitely match baud rates and detect used port, but don't do other configuration if not requested.
+	// This will force input on the receiver. That shouldn't be a problem as it's on our own connection.
+	if (!_automatic_configuration) {


This needs to be after the baud rate detection.

Do you mean right after line 742 with the detect_receiver_baud_rate()? Currently it's after baud rate detection, setting baud rate on flight controller and port detection as none of them touch the state of the receiver (only sends getEchoMessage and force input with SSSSSSSSSS which affects only the attached port). I've done it this way so there can be no confusion in the future about why sending commands in other parts of the driver doesn't work, as the baud rates wouldn't be matched. Automatic configuration doesn't mean that users couldn't send commands using the MAVLink console in a future use case for example. Also the current septentrio reset <hot|warm|cold> commands require matched baud rates already.

flyingthingsintothings · 2024-06-04T07:26:27Z

I saw a question about why this wasn't turned into a work queue task instead. The question is already removed, but just for future reference: The driver needs to sleep at some points. I looked up whether suspending tasks on the work queue in a cooperative way is possible, but from this forum post that doesn't seem to be the case. The original, combined GPS drivers were also not scheduled on the work queue.

flyingthingsintothings · 2024-06-04T11:39:47Z

While testing we found a bug with baud rate detection which hasn't been happening during earlier testing. It's only happening on certain ports on certain flight controllers. I'll open this again once that has been solved.

flyingthingsintothings · 2024-06-04T14:01:02Z

The issue turned out to be when both the GPS module and Septentrio module were set to use the same physical port. This was something I had asked in one of the community Q&A calls during the very early development where I was told it wouldn't cause problems. If the GPS module isn't configured to use the same ports as the Septentrio one, everything works fine (tested on Pixhawk 6X and Cube Orange+ where issues occured).

flyingthingsintothings · 2024-06-11T07:17:53Z

@mrpollo Could this PR get the label for the coordination call again to discuss blocking issues and progress?

Certain boards have too little flash to contain the new driver as well as the original ones.

DronecodeBot · 2024-06-12T15:33:36Z

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-june-12-2024/39209/1

flyingthingsintothings · 2024-06-12T16:39:00Z

I already prepared the necessary documentation for this new driver. We are currently testing it to see what extra things need to be included to make it as user-friendly as possible. I've documented all the parameters in there and cleaned the existing documentation about the hardware setup a bit. Once the documentation is finished I'll also make a pull request for that.

flyingthingsintothings · 2024-06-17T07:57:58Z

@dagar Is there something else I should change about this PR or is it ready to be merged? I can make any changes if needed.

I removed the driver in some of the configurations where it took up too much space. I see that there is a PR to free up some space on two of the three boards where this was an issue (Pixhawk 5 and 6X), so maybe the driver can be made to fit on those boards with a future PR that enables them again.

flyingthingsintothings · 2024-06-17T18:40:23Z

I'm super excited to see this big change merged. Thank you to everyone who helped during the community Q&A sessions and with feedback here on this PR. I will continue to be available should any changes be required or when any bugs show up. I will also make sure to update the documentation very soon after I've made the final tweaks to the changes I have prepared for the user guide.

…entrio module (PX4#22904) Having a generic interface over the GPS drivers makes dedicated functionality for each driver harder. Move the Septentrio driver into its own module under the `gnss` driver directory, and let it have its own parameters for only the functionality it requires. This also helps with adding new features because they only need to be implemented for the driver that wants it, simplifying testing.

DronecodeBot · 2024-07-03T15:40:28Z

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-july-3-2024/39534/1

MaEtUgR · 2024-07-17T15:40:30Z

src/drivers/gps/params.c

@@ -294,4 +275,4 @@ PARAM_DEFINE_INT32(GPS_1_GNSS, 0);
 * @reboot_required true
 * @group GPS
 */
-PARAM_DEFINE_INT32(GPS_2_GNSS, 0);
+PARAM_DEFINE_INT32(GPS_2_GNSS, 0);


My pr would catch these kinds of newline issues in CI:
#23166 (comment)

…entrio module (#22904) Having a generic interface over the GPS drivers makes dedicated functionality for each driver harder. Move the Septentrio driver into its own module under the `gnss` driver directory, and let it have its own parameters for only the functionality it requires. This also helps with adding new features because they only need to be implemented for the driver that wants it, simplifying testing.

…entrio module (PX4#22904) Having a generic interface over the GPS drivers makes dedicated functionality for each driver harder. Move the Septentrio driver into its own module under the `gnss` driver directory, and let it have its own parameters for only the functionality it requires. This also helps with adding new features because they only need to be implemented for the driver that wants it, simplifying testing.

flyingthingsintothings mentioned this pull request Mar 20, 2024

gps: fix incorrect task id in module startup #22910

Merged

flyingthingsintothings changed the title ~~Extract Septentrio Driver Into Separate Module~~ GPS: Extract Septentrio Driver Into Separate Module Mar 21, 2024

flyingthingsintothings force-pushed the pr-extract-septentrio-driver branch from de92e37 to 171dc84 Compare March 22, 2024 08:16