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

Fixes [#3172] possible position leak when location is disabled #3215

Closed
wants to merge 2 commits into from
Closed

Fixes [#3172] possible position leak when location is disabled #3215

wants to merge 2 commits into from

Conversation

Gabrielerusso
Copy link
Contributor

@Gabrielerusso Gabrielerusso commented Feb 12, 2024

Related to #3172 fixes location leaks when location is disabled and we don't want our position to be given.
basically we send our position only if the location is enabled, and if it get disabled we reset the local data to avoid any unwanted data sharing.

This is also part of PR #3163

fixes location leaks when location is disabled and we don't want our position to be given.
@Gabrielerusso Gabrielerusso changed the title #3172 Fixes possible position leak when location is disabled Fixes [#3172] possible position leak when location is disabled Feb 12, 2024
@garthvh
Copy link
Member

garthvh commented Feb 12, 2024

This changes the meaning of a bunch of settings, there is no leak, someone sent a position to the node.

@Gabrielerusso
Copy link
Contributor Author

Gabrielerusso commented Feb 12, 2024

This changes the meaning of a bunch of settings.

What does change? Everything works the same as before, just when the gps is manually disabled no more data is streamed. If a fixed position is set then nothing changes.

location enabled or fixed position enabled -> send location
location disabled and fixed position disabled -> don't send anything

@Gabrielerusso
Copy link
Contributor Author

Gabrielerusso commented Feb 12, 2024

there is no leak, someone sent a position to the node

The local node position is never deleted when location is disabled, unless you restart the board after disabling the location it will keep that.

@garthvh
Copy link
Member

garthvh commented Feb 12, 2024

there is no leak, someone sent a position to the node

The local node position is never deleted when location is disabled, unless you restart the board after disabling the location it will keep that.

There is no disable location setting, that turns off the hardware GPS.

@Gabrielerusso
Copy link
Contributor Author

there is no leak, someone sent a position to the node

The local node position is never deleted when location is disabled, unless you restart the board after disabling the location it will keep that.

There is no disable location setting, that turns off the hardware GPS.

It's called "meshtastic_Config_PositionConfig_GpsMode_DISABLED" this does enable or disable location (position). GPS was just used to refer for the location, doesn't actually exist a disable option for every single GNSS.

@garthvh
Copy link
Member

garthvh commented Feb 12, 2024

there is no leak, someone sent a position to the node

The local node position is never deleted when location is disabled, unless you restart the board after disabling the location it will keep that.

There is no disable location setting, that turns off the hardware GPS.

It's called "meshtastic_Config_PositionConfig_GpsMode_DISABLED" this does enable or disable location (position). GPS was just used to refer for the location, doesn't actually exist a disable option for every single GNSS.

It turns off the GPS module itself, you can still get positions from the phone, or broadcast the position already stored in the nodedb. There is no "I sent a position to the device but I don't ever want it to be used" option currently, if we have a position it gets used. It only gets broadcast over the mesh if you have fixed position on.

@thebentern
Copy link
Contributor

Yes, GPS != Position module is where the confusion comes from

@Gabrielerusso
Copy link
Contributor Author

there is no leak, someone sent a position to the node

The local node position is never deleted when location is disabled, unless you restart the board after disabling the location it will keep that.

There is no disable location setting, that turns off the hardware GPS.

It's called "meshtastic_Config_PositionConfig_GpsMode_DISABLED" this does enable or disable location (position). GPS was just used to refer for the location, doesn't actually exist a disable option for every single GNSS.

It turns off the GPS module itself, you can still get positions from the phone, or broadcast the position already stored in the nodedb. There is no "I sent a position to the device but I don't ever want it to ever be used" option currently, if we have a position it gets used. It only every gets broadcast over the mesh if you have fixed position on.

If it was a setting to disable one by one every GNSS systems then i would agree that this would change the dynamics, but in our case the "GPS" name is just used to refer to the location, if there is an hardware capable of GLONASS or whatever it's enabled and disabled with the same flag.
I uderstand what do you mean, but i think this is too general to be used in that way, most people call GPS the location services.

@garthvh garthvh added the bad config Problematic or conflicting config settings, not a bug label Feb 12, 2024
@Gabrielerusso
Copy link
Contributor Author

Yes, GPS != Position module is where the confusion comes from

in that case would make more sense to use two setting, like in car and mobile devices the user enabled/disable location, not the GNSS.

  1. location
  2. hardware GNSS


// reset position data if location is disabled and not fixed
if (config.position.gps_mode == meshtastic_Config_PositionConfig_GpsMode_DISABLED && !config.position.fixed_position) {
positionModule->clearLastPosition();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this one is still valid even if the "gpsmode" is used for the hardware GNSS module, as when it's decativated there is no use for the previous data if "fixed position" is disabled.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a clean pull, probably doing this on nodedb reset. @jp-bennett I think this could be related to the preventing a node from deleting itself from the nodedb, should we clear the last position from the current node when we reset the nodedb?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be related to the preventing a node from deleting itself from the nodedb

Does that change apply when doing a NodeDB reset? A full reset should probably delete the local node's entry, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that change apply when doing a NodeDB reset?

The change i did applies only when GPS::disable() is called.

src/GPSStatus.h Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all the changes on this file on the other hand could be omitted until there is a location option.

@jp-bennett
Copy link
Collaborator

I'm working on a per-channel GPS setting that might be a better way to do this. It's going to be a setting that explicitly controls the sending of locations.

@Gabrielerusso
Copy link
Contributor Author

Gabrielerusso commented Feb 12, 2024

I'm working on a per-channel GPS setting that might be a better way to do this. It's going to be a setting that explicitly controls the sending of locations.

I'm working on a per-channel GPS setting that might be a better way to do this. It's going to be a setting that explicitly controls the sending of locations.

I think it's better to start calling things by their names, otherwise a lot of confusion is generated, by "GPS settings" you mean the general position settings or the hardware GNSS module settings?
If we use "GPS" to refer to the Hardware GNSS module and then we use "GPS" for the location service and then we use GPS for the signle GNSS mode of Hardware GNSS this becomes a nigthmare :D .

In the future multiple options to be activated could be implemented as newer GNSS modules support GPS/Galileo/GLONASS/.., so a more precise definition migh be better.

For the location setting i think that a single option to disable location data from any source would be the best choiche, usable directly from the device like it's done with the triple click now.

@thebentern thebentern closed this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bad config Problematic or conflicting config settings, not a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants