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

Expanded the extended data field to contain settings that may be non-optimal for operation (Issue #1618) #1678

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

wir3z
Copy link
Contributor

@wir3z wir3z commented Apr 13, 2024

Added additional return fields to the JSON packet to indicate if the user's mobile device settings are currently in a state that negatively impacts location performance.

  • "wifi": 1=enabled, 0=disabled
  • "ps": 1=power save mode, 0=optimal power settings
  • "bo": 1=battery optimizations enabled, 0=optimal battery settings
  • "hib": 1=app can pause when unused, 0=optimal hibernation settings
  • "loc": location permission settings:
    0 = Background location, fine precision
    1 = Background location, coarse precision
    2 = Foreground location, fine precision
    3 = Foreground location, coarse precision
    4 = Disabled

CORP\vdi and others added 3 commits April 10, 2024 06:29
…user's mobile device settings are currently in a state that negatively impacts location performance.

- "wifi":  1=enabled, 0=disabled
- "ps": 1=power save mode, 0=optimal power settings
- "bo": 1=battery optimizations enabled, 0=optimal battery settings
- "hib": 1=app can pause when unused, 0=optimal hibernation settings
- "loc": location permission settings:
                0 = Background location, fine precision
                1 = Background location, coarse precision
                2 = Foreground location, fine precision
                3 = Foreground location, coarse precision
                4 = Disabled
@wir3z
Copy link
Contributor Author

wir3z commented Apr 13, 2024

Can you take a look to see if this is an acceptable direction for the PR?

Still outstanding:

  • Unit tests for this commit + changelog update
  • Connecting this data to the "friends location" drawer screen (I'd like to do that on a separate PR once this one is good)

@ckrey
Copy link
Member

ckrey commented Apr 15, 2024

I think we clearly stated #1618 this enhancement should not be done in the location message but by implementing a new status message

@growse
Copy link
Collaborator

growse commented Apr 15, 2024

Thanks for the PR!

i agree with @ckrey here - if there's device-specific, largely static status information, let's have that in a new message type published on an /info topic. Otherwise we're just bloating the location message with data that largely doesn't change from location to location.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 15, 2024

No problem! I'll get that refactored into a different message structure.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 15, 2024

Any options on using true/false vs 1/0? Thoughts on 1/0 is the end consumer could sum those results. If it was non-zero then it was not optimal (using the Windows error code type mentality here...)

@wir3z
Copy link
Contributor Author

wir3z commented Apr 18, 2024

Migrated to the status command and response as request. I still need to figure out the unit tests for it, but a couple questions:

  1. The schema was presented as this:
    `{
    "_type": "status",

... generic attributes (if any)

"android": {
"wifi": true/false,
"ps": true/false,
"bo": true/false,
"hib": true/false,
"loc": "app location permissions"
}`
Is there a need to nest that as "android" or "ios" if that is already known from the request headers?
2) If this status message is to fire on a change, where would you like that to be checked? On each location message generation? Or just keep it as an on-demand command?

@jpmens
Copy link
Member

jpmens commented Apr 18, 2024

if that is already known from the request headers

by which I assume you mean HTTP request headers. If so, please keep in mind that MQTT doesn't have those, so I'd be in favor of making clear from this payload that it's Android/iOS. Nesting isn't necessary; an attribute with the OS would suffice IMO.

@ckrey
Copy link
Member

ckrey commented Apr 18, 2024

like described here: #1618 (comment)

@jpmens
Copy link
Member

jpmens commented Apr 18, 2024

like described here

If iOS is spelled iOS, then Android should be spelled Android, whereby I would use lowercase only on both.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 18, 2024

if that is already known from the request headers

by which I assume you mean HTTP request headers. If so, please keep in mind that MQTT doesn't have those, so I'd be in favor of making clear from this payload that it's Android/iOS. Nesting isn't necessary; an attribute with the OS would suffice IMO.

Good point! I'll get that added and pushed up.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 19, 2024

Do you have guidance on what the proper formatting of the files should be? It's failing that portion of the build, but unsure what it is unhappy about.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 20, 2024

I updated the changelog + added in the MQTT unit tests. Let me know if there is anything else I'm missing. Let me know if the ktfmt failure is on my side, or part of the build process.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 23, 2024

Added in the status to be sent with a user trigger location to match iOS #778. This PR is ready for review now.

Is the ktfmt failures due to formatting in the files I touched or a PR cannot process that? Thanks!

@wir3z
Copy link
Contributor Author

wir3z commented Apr 27, 2024

Ok, figured out the ktfmt secret sauce. :) Cleanup is done and ready for submission.

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

4 participants