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

Expanding the extended data field to contain settings that may be non-optimal for operation #1618

Open
wir3z opened this issue Feb 18, 2024 · 23 comments
Milestone

Comments

@wir3z
Copy link
Contributor

wir3z commented Feb 18, 2024

I added additional functional to a fork of 2.4 that added:
@JsonProperty("wifi") -- wifi is on/off
@JsonProperty("ps") -- phone power save mode
@JsonProperty("bo") -- app is configured with battery optimizations
@JsonProperty("hib") -- app can hibernate if not used
@JsonProperty("loc") -- app location permissions

They get returned in the extended attributes to allow the central host to see misconfigurations that can impact good location operation. And then added a summary to the friends tile drawer (including a battery icon that changes when charging):
image

If I do a pull request, will this be considered?

@ckrey
Copy link
Member

ckrey commented Feb 18, 2024

I don't think this more or less "static" data should go into each location message. Rather put it into an extra attribute when the configuration is dumped (or requested by cmd/dump

@ckrey
Copy link
Member

ckrey commented Feb 18, 2024

The current network status is already in then location message as conn booklet

@jpmens
Copy link
Member

jpmens commented Feb 18, 2024

I agree that this rather static information should be reported otherwise if at all.

@wir3z
Copy link
Contributor Author

wir3z commented Feb 18, 2024

The current network status is already in then location message as conn booklet

It reports mobile/wifi data source, not if the wifi is disabled which basically breaks significant monitoring mode.

@wir3z
Copy link
Contributor Author

wir3z commented Feb 18, 2024

All of the above settings can be changed by the user and basically render decent presence detection useless. Is there a cmd/dump command (I was looking for that!)? Retrieving it that way would be just as valuable.

@jpmens
Copy link
Member

jpmens commented Feb 18, 2024

Our Booklet details the supported commands on the JSON page.

@ckrey
Copy link
Member

ckrey commented Feb 18, 2024

The current network status is already in then location message as conn booklet

It reports mobile/wifi data source, not if the wifi is disabled which basically breaks significant monitoring mode.

It is not supposed to report a "data source" but

conn Internet connectivity status (route to host) when the message is created

@ckrey
Copy link
Member

ckrey commented Feb 18, 2024

I don't think this more or less "static" data should go into each location message. Rather put it into an extra attribute when the configuration is dumped (or requested by cmd/dump

There are the configuration settings as described in [booklet|(https://owntracks.org/booklet/tech/json/#_typeconfiguration)

You could add an object to this object like:

{
   "_type":"configuration",
   ...
   "local": {
     "wifi": true/false,
     "ps": true/false,
     "bo": true/false,
     "hib": true/false,
     "loc": "app location permissions"
     }
}

@growse
Copy link
Collaborator

growse commented Feb 18, 2024

I'm not sure about putting it into configuration - this is meant to be for the configuration of the app, rather than the status of the device. I also agree that the location message type is meant to just contain fields that might conceivably change from one location message to the next.

I think there might be value in getting the device to report its status (in a separate message type), on request from the remote endpoint. Open question as to what extend it's android/ios/whatever specific, rather than genericised.

including a battery icon that changes when charging

Does the icon currently not reflect charging status? That's a bug if so.

@ckrey
Copy link
Member

ckrey commented Feb 18, 2024

Good idea @growse: How about defining a new "_type":"status" message, which can be retrieved by a {"_type":"cmd", "action":"status"} command. In addition, the app may send this message when requested by the user or when the status changes

@ckrey
Copy link
Member

ckrey commented Feb 18, 2024

If we do have the "status" message, we can have a genericised part and an iOS/android part like

{
  "_type": "status",

  ... generic attributes (if any)
  
  "android": {
     "wifi": true/false,
     "ps": true/false,
     "bo": true/false,
     "hib": true/false,
     "loc": "app location permissions"
     }
     
... or ...
   
  "iOS": {
    "altimeterAuthorizationStatus": "CMAuthorizationStatusAuthorized",
    "altimeterIsAbsoluteAltitudeAvailable": false,
    "altimeterIsRelativeAltitudeAvailable": true,
    "locationManagerAuthorizationStatus": "kCLAuthorizationStatusAuthorizedAlways",
    "UIBackgroundRefreshStatus": "UIBackgroundRefreshStatusAvailable",
    "version": "17.2.3/de_DE"
  }
}

@growse
Copy link
Collaborator

growse commented Feb 18, 2024

Which topic should status messages live on? /info?

@ckrey
Copy link
Member

ckrey commented Feb 18, 2024

I think on a new topic owntracks/user/device/status. This can be optionally subscribed to by the backend.
.../info is already used for cards and is subscribed by other clients

@growse
Copy link
Collaborator

growse commented Feb 18, 2024

Worth sketching out a quick schema with some types for each value?

@wir3z
Copy link
Contributor Author

wir3z commented Feb 18, 2024

https://owntracks.org/booklet/tech/json/#_typeconfiguration

Is cmd/dump applicable for Android? It's listed as iOS only: "dump Triggers the publish of a configuration message (iOS)"

@wir3z
Copy link
Contributor Author

wir3z commented Feb 18, 2024

Does the icon currently not reflect charging status? That's a bug if so.

No, did not appear to be so in the 2.4.12 PlayStore version.

@wir3z
Copy link
Contributor Author

wir3z commented Feb 24, 2024

Would you like a PR for this? I realize that it's in the location message vs status, but you can cherry pick the API's to collect the desired info from Android.

@growse
Copy link
Collaborator

growse commented Feb 25, 2024

Yes please - I suspect at a minimum we'll need:

  • New message type for status message
  • New enum value for command message action
  • handler in the service to process the incoming command, and generate the status message, before sticking it on the outbound queue.
  • somewhere between 2 and 385 tests.

I've probably missed something.

Worth also saying that these are incremental - we can introduce a new message type as an atomic change without doing the other bits, so might be easier to break it up.

@growse growse added this to the v2.6 milestone Feb 25, 2024
@wir3z
Copy link
Contributor Author

wir3z commented Mar 1, 2024

Sorry about the late reply -- just tied up on another project. I'll try to get the first bits into a PR this weekend to start chipping away at this.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 3, 2024

@growse Other project wrap-up took longer than expected so I'm just getting back to this one. How would you like this PR structured? I can submit it as is, or I can add in the message types that you listed above? Do you have specific ENUM's in mind, or that can get scrubbed in the PR? Let me know!

@growse
Copy link
Collaborator

growse commented Apr 6, 2024

I don't think I have strong opinions, my only real criteria for contributions are:

  • Tests pls (unit and espresso if you appropriate)
  • Update the CHANGELOG.

I'm generally ok with long-lived PRs that we can work on until it's good to merge, so feel free to shove something up and mark it as a draft or something until you're happy.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 20, 2024

I've added the status command/response to the PR. Downfall of using a separate status message is being able to display friends status on the friends drawer isn't possible (the UX image from the first post) since that information cannot be returned with contact card updates. I'll need to add a way to store the past status and then trigger a new adhoc one if there is a change if that is a desired operation.

Thoughts on putting the data as a subset of the extended data that only gets populated on change for the adhoc changes? That way the information is cached for the user as well as the friends. I do like the separate status command to allow a query of state as well.

@wir3z
Copy link
Contributor Author

wir3z commented Apr 27, 2024

Ok, PR is complete to support the status command:

  • A status response is sent when a status command is received.
  • A status response is sent when a user initiated location is triggered.

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

No branches or pull requests

4 participants