-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
[WIP] Geofences with Google Location API #914
Conversation
Thanks for working on this! Also note that @Tony-hu is currently working on #880, which imho is pretty much related. What I have discussed with him there is that it would be great to have some concepts of "person x being at location y" and the apps providing this information. Saying "person x is near beacon y" or "person x is in geofence z" is pretty much the same - the logic of what should happen in such a case should imho definitely be on the server side (as rules) and not in the app. |
Yes, I am aware of the other PRs. |
Hello @CasualTriangle , we are doing the features with something in common with #880 . Perhaps we can make further discussion in the future and I doubt if we will share the same configuration activity in the future.
I have the similar situation to deal with the question: how to make the connection between detecting location changes and controlling thing(s)? My solution is, as @kaikreuzer said, leave the controlling authority to the server and all the cellphone did are just detecting and reporting state changes. Once the server gets the state changes, it is the server's responsibility to do some stuff based on existing rules. |
I have implemented a basic activity with a recyclerview that lets the user add and define openhab-switch-items (the name and label). |
Cool! I almost did the same thing also using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments after a quick code inspection. Didn't try it yet; will look at it more in detail after 'WIP' status is resolved.
@@ -13,6 +16,13 @@ | |||
</intent-filter> | |||
</receiver> | |||
|
|||
<activity android:name="org.openhab.habdroid.ui.OpenHABGeofenceFragment" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fragment is not an activity. What exactly is the reason for adding this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had the fragment initaly as a activity... will delete that now that its a fragment
|
||
public static boolean isGeofenceFeatureAvailable() { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unneeded/misplaced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this function will return false
is the foss flavor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is for the foss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the same method exists in GeofencingHelper
, and is placed correctly there. The class GeofencingService
doesn't even exist for foss.
return true; | ||
} | ||
|
||
public static class GeofenceNameNotUnique extends IllegalArgumentException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GeofenceNameNotUniqueException
if (loadOpenHABGeofencesFromMemory(activity).contains(newOpenHABGeofence)) { | ||
throw new GeofenceNameNotUnique();//Geofence name already registered | ||
} | ||
List<OpenHABGeofence> newOpenHABGeofences = new ArrayList<>(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not only apply here, but also elsewhere: please drop 'OpenHAB' from variable and class names. Yes, other places in the code base have it like that, but those places are going to be changed accordingly ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will do that, did that on the geofences to differenciate the OpenHAB instance form the google one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe name it StoredGeofence
or AssignedGeofence
or ItemGeofence
or something among those lines?
* @param geofencesForRemoval | ||
*/ | ||
public static void removeGeofences(Activity activity, List<OpenHABGeofence> geofencesForRemoval) { | ||
if(geofencesForRemoval.isEmpty()) {return;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also doesn't apply here only, but in other places as well: Please try to follow the code style - we use the AOSP code style: https://source.android.com/setup/contribute/code-style
In this particular place the corrected version would be
if (geofencesForRemoval.isEmpty()) {
return;
}
public static final String SUBSCREEN_REMOTE_CONNECTION = "default_openhab_remote_connection"; | ||
public static final String SUBSCREEN_SSL_SETTINGS = "default_openhab_ssl"; | ||
|
||
public static final int REQUEST_LOCATION_REQUEST_CODE = 42; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably define this in OpenHABMainActivity
* @param longitude the longitude | ||
* @return | ||
*/ | ||
public static String coordinatesStringFromValues(double latitude,double longitude) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too specific to live here; I don't think there's a realistic chance of reusing this.
@@ -0,0 +1,44 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These layout files should be moved to the full package.
<string name="geofence_coordinates">Coord.</string> | ||
<string name="geofence_radius">Radius</string> | ||
<string name="geofence_dialog_create">Create</string> | ||
<string name="geofence_dialog_cancel">Cancel</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those strings are - AFAICT - full package specific, why not move them to there?
<string name="geofence_dialog_new">New Geofence</string> | ||
<string name="geofence_label">Label</string> | ||
<string name="geofence_name">Name</string> | ||
<string name="geofence_coordinates">Coord.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why shorten this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks bad on my phone if written out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In abbreviated form it'll likely look bad on someone else's phone. This sounds like it could/should be solved by a better placement of controls instead. Can you provide a screenshot of the dialog?
I agree that geofences and beacons should have a common navigation drawer entry. Opening that should show all currently configured fences and beacons. A click on the "add" fab could expand it to allow adding geofences and beacons: https://material.io/design/assets/1fohUHLBjjut8g3UsgGGHHBV6PBboy8EK/fab-transitions-speeddial-do-labels.png. Those "adding" screens can be differ between the two things. I'm not sure if it's the best way to create the switch item during the configuration. Having a searchable list with all items that are defined server side seems a better option to me. When a user defines a lot of fences or beacons it might be easier for him to check for one text item that has the name of the fence or beacon he is near as state or null/undefined if he isn't near a beacon and outside all fences. This should be optional. |
I love this kind of design. Perhaps CasualTriangle and I could share the same drawer entry activity. And once the user clicks on the fab, the app will open specified fragment.
I'm wondering that if each phone should be treated as a location item(possibly 2 items for both beacon and geo-fence, not sure). It can have some states like "kitchen", "living room", "absent" and etc. And the app not only reports for the state but also adds new states if more beacons/geofences are introduced. So, you know, in this situation, the item can hold more states than a switch does.
Agreed. The app only shows the nearest beacon/geo-fence(of course this should be the same as the current state) on the entry activity. But leaves the rest stuff in the configuration fragment. Right? |
As mentioned above already, I am sure that it is NOT the best way ;-)
As we are just about to create an initial ESH tag library, my suggestion would be to align this what we need here: We could use All our rooms/homes/offices/otherplaces are usually already modelled as Group items - if those were tagged with Whenever the phone is near a beacon or within a geofence, it could simply send the name of the associated place-item to the person-item. To be able to do this, Location items should not only allow geo-coordinates as a state, but also place-item names, but this is a change that was anyhow considered when introducing Location items. One assumption that is implicitly made here is that we won't have any intersections between places, but only containments - i.e. all beacons that are in my rooms at home can be considered to be within my "home geofence" and my "office geofence" is not intersecting with my "home-geofence". I would think that as we want to locate a person with that feature, this assumption is perfectly fine, though - but feel free to disagree, if I miss anything. Wdyt: Does that concept make sense? |
I'd actually also like to reference eclipse-archived/smarthome#582, which was created in 2015 "to better support use cases like geo-fencing, iBeacons / BLE tags, wearables and rules with user-context" - maybe we can finally get there :-) |
@CasualTriangle Any feedback from your end on my proposal? |
@kaikreuzer after looking through the proposals i started to like the idear of "pinning" location tags on a items and then having items for every person.
If i understood the concept right then even that would be possible, wouldn't it ? |
Yes, he would have to create those items, but it is likely that they even already exist - e.g. when using the HomeBuilder, it creates an item
which would be the perfect target for a geofence.
Yes, this should be a screen like @mueller-ma described in #914 (comment): Opening that should show all currently configured fences and beacons. A click on the "add" fab could expand it to allow adding geofences and beacons. Those "adding" screens can be differ between the two things.
No, persons would be a "Location" item, i.e. the item state defines their location (as this is something dynamic and not statically defined). And there can always only be one state, i.e. one location at a time (which somewhat makes sense). For house+town it would still be easy, because house is within town, so it is simply a more precise location (and from being "in house" you can implicitely derive that you are also "in town". |
I have written an activity for displaying the binding pairs of It's displaying nearest beacon with distance as the title of the activity. And all the paired beacon name - frame has been displayed. What's more, as @mueller-ma suggested, it can also prompt "Not in range" if a certain paired beacon is not detected. And for the code of this activity, you can go to |
For the speed dial FAB, see https://stackoverflow.com/questions/30699302/android-design-support-library-expandable-floating-action-buttonfab-menu/31422678 ... it both mentions some 3rd party libraries as well as a moderately simple solution for it without 3rd party lib. |
The title should be something like "Location tracking". If the mac is displayed when no name is present, it should have the same visual look. At some point we can also add location tracking via wifi. |
looks good so far :)
... does that make sense ? |
So, it's allowed to introduce with 3-rd party libraries, right?
Yes, it definitely makes sense. And that's why, you know, I claimed that it's an ugly debugging view.
Yes, that could be looked nice.
I'm not sure what's the difference between in range and inside? |
Yes, sure, it's fine to add them in case they're needed :-) |
Content wise it is a good start, but sorry, the UI still looks like a debugging interface. So yes, a FAB as @mueller-ma suggested above would be nice, but additionally, the list on the screen should also use a design like the one shown here, i.e. similar to a mail inbox (with a round large coloured icon, proper padding and so on). |
I fully agree that padding is needed, but the icon(s) IMHO shouldn't be colored, but be monochrome (match primary or secondary text color). Additionally the FAB also needs padding and a proper (white) icon. |
What I meant is "having a color", i.e. not being b/w. - just like the standard material design as in the linked example. |
I got that part, but that's what I specifically not would want to see. Unlike an email app, the icons in our list only depict a very limited set of 'sources' (geofence, beacon, wifi and that's about it?), so in our case we don't need color to differentiate between the items. What I'm thinking about is something similar to this screen from another project of mine (https://github.com/slapperwan/gh4a/): |
That's true. Basically, I'm keeping the UI to improve. But it still takes time to make the UI better and better.
That a good design. And I think it's quite similar to the UI below.
I downloaded and used the app from my phone. Definitely, that's a good app with beautiful items UI which I should learn from. And it's similar to the UI Kai posted. Thanks for offering the link! |
Looks good, but there is still place for some improvements: |
Any news here? |
I would like to ask you to leave a short comment here if this function will ever be developed further. |
There's an open feature request for that (#343), so I'd never say never. Currently I'm not planing to implement this feature, though. |
Thank you, I know all the feature requests, and have also read through almost all of them. But what I still don't understand is what is the blocker that prevents it from going further? |
For starters, the list of things listed under 'not implemented yet' in the PR description + code review comments + adaption to the codebase changes in the meantime (Kotlin conversion, among other things). In other words, it would need somebody to take what's in this PR, rewrite it to Kotlin and add the missing pieces. |
Hello there,
I have started to implement geofences in the app.
the idear was to let the user define a new switch-item in the App and then change the state (through a REST Api call) on the OP2 Server when the Phone gets into or out of the geofence.
Currently implemented:
Not implemented yet / up for debate:
Thanks,
Nils