Astro Background discovery for location changes #3725

triller-telekom · 2017-06-22T16:23:25Z

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

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

maggu2810 · 2017-06-22T16:45:06Z

extensions/binding/org.eclipse.smarthome.binding.astro/README.md

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


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

maggu2810 · 2017-06-22T16:46:08Z

...in/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryLocationChangedTask.java

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


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

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

maggu2810 · 2017-06-22T16:47:34Z

...astro/src/main/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryService.java

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

+    public LocationProvider getLocationProvider() {


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

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>

kaikreuzer · 2017-06-23T06:52:20Z

extensions/binding/org.eclipse.smarthome.binding.astro/README.md

@@ -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.


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.

kaikreuzer · 2017-06-23T06:54:49Z

extensions/binding/org.eclipse.smarthome.binding.astro/README.md


 ## 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.


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.

kaikreuzer · 2017-06-23T06:56:11Z

extensions/binding/org.eclipse.smarthome.binding.astro/README.md

+
+##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.


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

kaikreuzer · 2017-06-23T06:56:17Z

...in/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryLocationChangedTask.java

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


kaikreuzer · 2017-06-23T06:57:36Z

...in/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryLocationChangedTask.java

+
+import org.eclipse.smarthome.core.library.types.PointType;
+
+public class AstroDiscoveryLocationChangedTask implements Runnable {


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

I am now using a lambda expression instead

kaikreuzer · 2017-06-23T06:58:34Z

...astro/src/main/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryService.java

+        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);


debug is enough

kaikreuzer · 2017-06-23T06:59:19Z

...astro/src/main/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryService.java

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

+    public LocationProvider getLocationProvider() {


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

kaikreuzer · 2017-06-23T07:00:26Z

...astro/src/main/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryService.java

+    protected void startBackgroundDiscovery() {
+        Expression expression = null;
+        try {
+            expression = new CronExpression(


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

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

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>

triller-telekom · 2017-06-23T08:40:19Z

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 · 2017-06-23T09:13:23Z

@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.

amitjoy · 2017-06-23T09:15:16Z

...astro/src/main/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryService.java

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


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

amitjoy · 2017-06-23T09:17:57Z

...astro/src/main/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryService.java

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


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

amitjoy · 2017-06-23T09:19:14Z

extensions/binding/org.eclipse.smarthome.binding.astro/README.md

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


English: Tip German: Tipp 😉

amitjoy · 2017-06-23T09:21:04Z

...astro/src/main/java/org/eclipse/smarthome/binding/astro/discovery/AstroDiscoveryService.java

+    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);


Capitalized variables are primarily used for constant literals.

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

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

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

Astro Background discovery for location changes

e49ddf0

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

maggu2810 reviewed Jun 22, 2017

View reviewed changes

Fix location comparison

38cea63

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

kaikreuzer suggested changes Jun 23, 2017

View reviewed changes

kaikreuzer reviewed Jun 23, 2017

View reviewed changes

triller-telekom added 3 commits June 23, 2017 10:11

Use Lamda expression instead of additional public class

6d0005c

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

Missing README changes

ef2e5fe

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

Improve debug logging

84bd4c4

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

amitjoy reviewed Jun 23, 2017

View reviewed changes

Use Objects.equals, final variables, better debug logs

36beb06

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

kaikreuzer approved these changes Jun 24, 2017

View reviewed changes

kaikreuzer merged commit 76f70a1 into eclipse-archived:master Jun 24, 2017

kaikreuzer added the Binding-Astro label Jun 24, 2017

triller-telekom deleted the astroLocationChanged branch June 26, 2017 05:51

kaikreuzer modified the milestone: 0.9.0 Jun 26, 2017

kaikreuzer added the enhancement label Dec 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Astro Background discovery for location changes #3725

Astro Background discovery for location changes #3725

triller-telekom commented Jun 22, 2017

maggu2810 Jun 22, 2017

maggu2810 Jun 22, 2017

triller-telekom Jun 23, 2017

maggu2810 Jun 22, 2017

kaikreuzer Jun 23, 2017

kaikreuzer Jun 23, 2017

kaikreuzer Jun 23, 2017

kaikreuzer Jun 23, 2017

kaikreuzer Jun 23, 2017

kaikreuzer Jun 23, 2017

triller-telekom Jun 23, 2017

kaikreuzer Jun 23, 2017

kaikreuzer Jun 23, 2017

kaikreuzer Jun 23, 2017

triller-telekom Jun 23, 2017

kaikreuzer Jun 23, 2017

triller-telekom commented Jun 23, 2017

amitjoy commented Jun 23, 2017

amitjoy Jun 23, 2017

amitjoy Jun 23, 2017

amitjoy Jun 23, 2017

amitjoy Jun 23, 2017 •

edited

sjsf Jun 23, 2017

amitjoy Jun 23, 2017

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


		##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.

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


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

		public class AstroDiscoveryLocationChangedTask implements Runnable {

Astro Background discovery for location changes #3725

Astro Background discovery for location changes #3725

Conversation

triller-telekom commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triller-telekom commented Jun 23, 2017

amitjoy commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitjoy Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitjoy Jun 23, 2017 •

edited