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

Possible fix for use of pin 0 as PIN_GPS_EN #6434

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

macvenez
Copy link
Contributor

As discussed on Discord here
we are using PIN 0.00 on NRF52 for PIN_GPS_EN this causes an issue because if it's set to 0 this skips all the code that creates the gps enable pin instance.

I thought about this for a possible solution, this is working correctly on our board.
Will this be ok?
Thanks!

@ponzano
Copy link
Contributor

ponzano commented Mar 28, 2025

Tested and working 👌

@@ -1369,7 +1369,7 @@ GPS *GPS::createGps()

GpioVirtPin *virtPin = new GpioVirtPin();
new_gps->enablePin = virtPin; // Always at least populate a virtual pin
if (_en_gpio) {
#if defined(PIN_GPS_EN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this skip for runtime configured pins with config.position.gps_en_gpio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's possible, do you think we can still go with this approach or we should switch to using our device specific #define as did for GNSS_AIROHA in the same file?
I understand there may be devices wich use GPS but don't have EN pin connected for GPS so we must use value 0 to identify those? (The issue is that with our device 0 is the correct gpio value instead...)

Copy link
Contributor

@ponzano ponzano Mar 28, 2025

Choose a reason for hiding this comment

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

Won't this skip for runtime configured pins with config.position.gps_en_gpio?

Perhaps we could do this (?)

if(_en_gpio){
//code to execute
}
else{
#if defined(PIN_GPS_EN)
//same code to execute
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Check out line 1355

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest trying to modify that area rather than changing the branching here. Try and make it a generic solution rather than one specific to a board, the airoha stuff is ... not the best :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'm trying to figure it out.
The issue is this probably won't support setting PIN_GPS_EN to pin 0 by app or CLI as we need a value for boards which don't have this pin. Am I right?
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In alternative, but I don't know if that's feasible, we could use -1 as value for boards which don't use Enable pin for GPS so we could easily distinguish between zero or positive values (correct pin to use) and -1 which would mean don't use

@macvenez
Copy link
Contributor Author

macvenez commented Apr 1, 2025

Hi @fifieldt @caveman99 @thebentern any updates on what would be the best choice?

In alternative, but I don't know if that's feasible, we could use -1 as value for boards which don't use Enable pin for GPS so we could easily distinguish between zero or positive values (correct pin to use) and -1 which would mean don't use

I've never seen -1 used in the Android app so I'm wondering if it would be admissible.

Alternatively we could only enable pin 0 usage for boards which have #define PIN_GPS_EN declared

Thanks!

@macvenez macvenez mentioned this pull request Apr 3, 2025
3 tasks
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.

4 participants