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.

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

Add Option to Toggle Automatic Receiver Setup #152

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

flyingthingsintothings
Copy link
Contributor

@flyingthingsintothings flyingthingsintothings commented Feb 28, 2024

This adds an option to disable automatic setup of the attached receiver during the configuration. It is only implemented by the SBF driver for now. Other drivers can implement it if they want. This is linked to another PR in the PX4-Autopilot project to add a parameter that lets users disable automatic driver configuration.

@SeptentrioGNSS

Add a field to `GPSConfig` that allows disabling automatic setup of attached
receivers.
@flyingthingsintothings
Copy link
Contributor Author

This is branched from an earlier version of the main branch as branching from the head of the main branch caused compilation issues when updating the submodule in the PX4-Autopilot project (due to other changes that have happened to main). I don't know what the preferred approach is to add functionality to this repository and use it in the autopilot one without getting compilation problems from the other changes.

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Can you add it to all drivers?

This is branched from an earlier version of the main branch as branching from the head of the main branch caused compilation issues when updating the submodule in the PX4-Autopilot project (due to other changes that have happened to main). I don't know what the preferred approach is to add functionality to this repository and use it in the autopilot one without getting compilation problems from the other changes.

As long as it cleanly rebases this is fine.

@flyingthingsintothings
Copy link
Contributor Author

I'm still getting used to the code. Would these be all the drivers that this would need to be implemented for?

thomas@thomas-laptop-vm-ubuntu:~/Documents/development/cpp/px4_gps_drivers/main$ grep 'int configure('
src/gps_helper.h:215:	virtual int configure(unsigned &baud, const GPSConfig &config) = 0;
src/emlid_reach.h:150:	int configure(unsigned &baudrate, const GPSConfig &config) override;
src/ubx.h:1002:	int configure(unsigned &baudrate, const GPSConfig &config) override;
src/femtomes.h:175:	int configure(unsigned &baudrate, const GPSConfig &config) override;
src/mtk.h:95:	int configure(unsigned &baudrate, const GPSConfig &config) override;
src/nmea.h:70:	int configure(unsigned &baudrate, const GPSConfig &config) override;
src/ashtech.h:62:	int configure(unsigned &baudrate, const GPSConfig &config) override;
src/sbf.h:380:	int configure(unsigned &baudrate, const GPSConfig &config) override;

Unlike for the SBF driver, I don't have access to receivers for all of the other ones, so I can't thoroughly test them. I hope that is OK? The other option would be to document that it is only supported for Septentrio receivers for now.

@bkueng
Copy link
Member

bkueng commented Mar 5, 2024

Yes correct, that's ok.

@flyingthingsintothings
Copy link
Contributor Author

I was wondering whether this PR could be merged separately from the implementation for other drivers which could be its own PR? Implementing the bypass for Septentrio receivers is quite easy as I have access to hardware to test and the driver doesn't keep that much state. I don't have hardware for the other drivers and they keep lots of state. It would require a bigger PR, potentially with some help from others.

@bkueng
Copy link
Member

bkueng commented Mar 13, 2024

For this simple change I prefer to keep them in sync right away. Once you do the change I can have a closer look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants