Skip to content
This repository has been archived by the owner. It is now read-only.

Astro Background discovery for location changes #3725

Merged

Conversation

@triller-telekom
Copy link
Contributor

@triller-telekom triller-telekom commented Jun 22, 2017

Signed-off-by: Stefan Triller stefan.triller@telekom.de

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@@ -187,3 +189,7 @@ then
...
end
```

##Tipps
Copy link
Contributor

@maggu2810 maggu2810 Jun 22, 2017

Shouldn't there be a space between ## and Things

@Override
public void run() {
PointType currentLocation = astroDiscoveryService.getLocationProvider().getLocation();
if (currentLocation != previousLocation) {
Copy link
Contributor

@maggu2810 maggu2810 Jun 22, 2017

Do you really want to compare the reference, regardless if another point type variable points to the same location? Shouldn't we use equals?

Copy link
Contributor Author

@triller-telekom triller-telekom Jun 23, 2017

Oh damn... It was late and warm in the office yesterday... O_o

.withProperty("geolocation", propGeolocation).build());
}

public LocationProvider getLocationProvider() {
Copy link
Contributor

@maggu2810 maggu2810 Jun 22, 2017

We could also thing about adding the location provider to the constructor argument of AstroDiscoveryLocationChangedTask without introducing a new function.

Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

Just completely remove AstroDiscoveryLocationChangedTask and use a lambda here. No need for this function either in that case.

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@@ -10,17 +10,19 @@ This binding supports two Things: Sun and Moon

## Discovery

Discovery is not necessary, because all calculations are done within the binding.
You can use the auto discovery if you have a location set in "Configuration - System - Regional Settings - Location". This will automatically create a "Local Sun" and a "Local Moon" item for this location.
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

Simply say "if you have a system location set". There might be many ways how to do that, it is not up to the binding to know how.


## Binding Configuration

No binding configuration required.

## Thing Configuration

A thing requires the geolocation (latitude, longitude) for which the calculation is done.
A thing requires the geolocation (latitude, longitude, (altitude)) for which the calculation is done.
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

The comma should be optional as well. And as commented on the PR, I would prefer the syntax

(latitude,longitude[,altitude])

especially as we now have two levels of brackets.


##Tipps

Do not worry if for example the "astro dawn" is "-" in the place that you set for your item. The reason might be that you live in a northern country and it is summer, because then by definition the sun is not 18 degrees below the horizon in the morning. For details see: https://en.wikipedia.org/wiki/Dawn Also while reading this wikipedia article you might come to the conclusion that you want to use "civil dawn" anyway.
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

Please do every sentence on a new line for easier diffing.

@@ -0,0 +1,23 @@
package org.eclipse.smarthome.binding.astro.discovery;
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

license?


import org.eclipse.smarthome.core.library.types.PointType;

public class AstroDiscoveryLocationChangedTask implements Runnable {
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

I wouldn't know why we would need this logic in its own public (and exported) class.

Copy link
Contributor Author

@triller-telekom triller-telekom Jun 23, 2017

I am now using a lambda expression instead

if (expression != null && backgroundJob == null) {
AstroDiscoveryLocationChangedTask job = new AstroDiscoveryLocationChangedTask(this);
scheduledExecutor.schedule(job, expression);
logger.info("Scheduled astro location-changed job every {} seconds", LOCATION_CHANGED_CHECK_INTERVAL);
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

debug is enough

.withProperty("geolocation", propGeolocation).build());
}

public LocationProvider getLocationProvider() {
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

Just completely remove AstroDiscoveryLocationChangedTask and use a lambda here. No need for this function either in that case.

protected void startBackgroundDiscovery() {
Expression expression = null;
try {
expression = new CronExpression(
Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

Why do we need the complexity of a cron job here?

Copy link
Contributor Author

@triller-telekom triller-telekom Jun 23, 2017

@SJKA suggested that we can make use of our scheduler. I thought its a good idea.

Copy link
Contributor

@kaikreuzer kaikreuzer Jun 23, 2017

I doubt that this is what he meant.
Every discovery service already HAS a scheduler available. I see no reason why this should not be used here. Check the docs at https://www.eclipse.org/smarthome/documentation/development/bindings/discovery-services.html#background-discovery (and please improve them, if they weren't clear enough on that point!).

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
amitjoy
Copy link
Contributor

amitjoy commented on 6d0005c Jun 23, 2017

For better logging, you can add - logger.debug("Stopping Astro device background discovery..."); and add logger.debug("Stopped Astro device background discovery"); after you cancel it.

amitjoy
Copy link
Contributor

amitjoy commented on 6d0005c Jun 23, 2017

Just to be concise, you can also use - if (!previousLocation.equals(currentLocation)) but this will work if you are certain that previousLocation is not null.

maggu2810
Copy link
Contributor

maggu2810 commented on 6d0005c Jun 23, 2017

What about

if (!Objects.equals(currentLocation, previousLocation)) {
...
}
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@triller-telekom
Copy link
Contributor Author

@triller-telekom triller-telekom commented Jun 23, 2017

previousLocation can also be null. And I can only proceed if I have a currentLocation != null, so I think my check is correct.

PS: can you make you comments next to the code next time please? its easier to reply then.

@amitjoy
Copy link
Contributor

@amitjoy amitjoy commented Jun 23, 2017

@triller-telekom I actually commented at the source but I don't why it showed this way. I believe concurrent reviews and changes to the same files caused github to arrange it differently. Anyways thanks for noticing it.

protected void stopBackgroundDiscovery() {
logger.debug("Stopping Astro device background discovery");
if (astroDiscoveryJob != null && !astroDiscoveryJob.isCancelled()) {
astroDiscoveryJob.cancel(true);
Copy link
Contributor

@amitjoy amitjoy Jun 23, 2017

you can actually use the returned boolean value to nullify astroDiscoveryJob and print the log.

if (astroDiscoveryJob == null) {
astroDiscoveryJob = scheduler.scheduleAtFixedRate(() -> {
PointType currentLocation = locationProvider.getLocation();
if ((currentLocation != null) && !currentLocation.equals(previousLocation)) {
Copy link
Contributor

@amitjoy amitjoy Jun 23, 2017

As suggested, you can use Objects#equals(Object, Object) as well

@@ -187,3 +189,7 @@ then
...
end
```

## Tipps
Copy link
Contributor

@amitjoy amitjoy Jun 23, 2017

English: Tip German: Tipp 😉

private PointType previousLocation;

private static ThingUID SUN_THING = new ThingUID(THING_TYPE_SUN, LOCAL);
private static ThingUID MOON_THING = new ThingUID(THING_TYPE_MOON, LOCAL);
Copy link
Contributor

@amitjoy amitjoy Jun 23, 2017

Capitalized variables are primarily used for constant literals.

Copy link
Contributor

@sjsf sjsf Jun 23, 2017

...so that okay - let's worry why they're not declared final here...?! 😉

Copy link
Contributor

@amitjoy amitjoy Jun 23, 2017

You are right. That's also my concern 😉.

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@kaikreuzer kaikreuzer merged commit 76f70a1 into eclipse-archived:master Jun 24, 2017
2 checks passed
@triller-telekom triller-telekom deleted the astroLocationChanged branch Jun 26, 2017
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Jun 26, 2017
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Jun 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants