-
Notifications
You must be signed in to change notification settings - Fork 783
Astro Background discovery for location changes #3725
Changes from 1 commit
e49ddf0
38cea63
6d0005c
ef2e5fe
84bd4c4
36beb06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
If you change your system location the background discovery detects this change and applies it to the "Local Sun" and a "Local Moon" items automatically. | ||
|
||
## 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comma should be optional as well. And as commented on the PR, I would prefer the syntax
especially as we now have two levels of brackets. |
||
Optionally, a refresh interval (in seconds) can be defined to also calculate positional data like azimuth and elevation. | ||
An complementary altitude (optional) configuration item can also be specified to sharpen results provided by Radiation group. | ||
The complementary altitude part of the geolocation parameter is optional and sharpens results provided by the Radiation group. | ||
|
||
## Channels | ||
|
||
|
@@ -123,26 +125,26 @@ sunrise is 22:10 but `latest` is set to 20:00 so the event/datetime value is mov | |
Things: | ||
|
||
``` | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx", altitude=100, interval=60 ] | ||
astro:moon:home [ geolocation="xx.xxxxxx,xx.xxxxxx", interval=60 ] | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx,xx.xxx", interval=60 ] | ||
astro:moon:home [ geolocation="xx.xxxxxx,xx.xxxxxx,xx.xxx", interval=60 ] | ||
``` | ||
|
||
or optionally with an event offset | ||
|
||
``` | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx", altitude=100, interval=60 ] { | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx,xx.xxx", interval=60 ] { | ||
Channels: | ||
Type rangeEvent : rise#event [ | ||
offset=-30 | ||
] | ||
} | ||
astro:moon:home [ geolocation="xx.xxxxxx,xx.xxxxxx", interval=60 ] | ||
astro:moon:home [ geolocation="xx.xxxxxx,xx.xxxxxx,xx.xxx", interval=60 ] | ||
``` | ||
|
||
or a datetime offset | ||
|
||
``` | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx", altitude=100, interval=60 ] { | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx,xx.xxx", interval=60 ] { | ||
Channels: | ||
Type start : rise#start [ | ||
offset=5 | ||
|
@@ -156,7 +158,7 @@ astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx", altitude=100, interval=60 ] | |
or a offset and latest | ||
|
||
``` | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx", altitude=100, interval=60 ] { | ||
astro:sun:home [ geolocation="xx.xxxxxx,xx.xxxxxx,xx.xxx", interval=60 ] { | ||
Channels: | ||
Type rangeEvent : rise#event [ | ||
offset=-10, | ||
|
@@ -187,3 +189,7 @@ then | |
... | ||
end | ||
``` | ||
|
||
##Tipps | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't there be a space between |
||
|
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do every sentence on a new line for easier diffing. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package org.eclipse.smarthome.binding.astro.discovery; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. license? |
||
|
||
import org.eclipse.smarthome.core.library.types.PointType; | ||
|
||
public class AstroDiscoveryLocationChangedTask implements Runnable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't know why we would need this logic in its own public (and exported) class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am now using a lambda expression instead |
||
|
||
private AstroDiscoveryService astroDiscoveryService; | ||
private PointType previousLocation; | ||
|
||
public AstroDiscoveryLocationChangedTask(AstroDiscoveryService astroDiscoveryService) { | ||
this.astroDiscoveryService = astroDiscoveryService; | ||
} | ||
|
||
@Override | ||
public void run() { | ||
PointType currentLocation = astroDiscoveryService.getLocationProvider().getLocation(); | ||
if (currentLocation != previousLocation) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really want to compare the reference, regardless if another point type variable points to the same location? Shouldn't we use equals? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh damn... It was late and warm in the office yesterday... O_o |
||
astroDiscoveryService.createResults(currentLocation); | ||
previousLocation = currentLocation; | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,14 +9,19 @@ | |
|
||
import static org.eclipse.smarthome.binding.astro.AstroBindingConstants.*; | ||
|
||
import java.text.ParseException; | ||
import java.util.Arrays; | ||
import java.util.HashSet; | ||
|
||
import org.eclipse.smarthome.binding.astro.AstroBindingConstants; | ||
import org.eclipse.smarthome.config.discovery.AbstractDiscoveryService; | ||
import org.eclipse.smarthome.config.discovery.DiscoveryResultBuilder; | ||
import org.eclipse.smarthome.core.i18n.LocationProvider; | ||
import org.eclipse.smarthome.core.library.types.PointType; | ||
import org.eclipse.smarthome.core.scheduler.CronExpression; | ||
import org.eclipse.smarthome.core.scheduler.CronHelper; | ||
import org.eclipse.smarthome.core.scheduler.Expression; | ||
import org.eclipse.smarthome.core.scheduler.ExpressionThreadPoolManager; | ||
import org.eclipse.smarthome.core.scheduler.ExpressionThreadPoolManager.ExpressionThreadPoolExecutor; | ||
import org.eclipse.smarthome.core.thing.ThingTypeUID; | ||
import org.eclipse.smarthome.core.thing.ThingUID; | ||
import org.slf4j.Logger; | ||
|
@@ -31,42 +36,77 @@ | |
public class AstroDiscoveryService extends AbstractDiscoveryService { | ||
private final Logger logger = LoggerFactory.getLogger(AstroDiscoveryService.class); | ||
private static final int DISCOVER_TIMEOUT_SECONDS = 30; | ||
private static final int LOCATION_CHANGED_CHECK_INTERVAL = 60; | ||
private LocationProvider locationProvider; | ||
private final ExpressionThreadPoolExecutor scheduledExecutor; | ||
private Runnable backgroundJob; | ||
|
||
private static ThingUID SUN_THING = new ThingUID(THING_TYPE_SUN, LOCAL); | ||
private static ThingUID MOON_THING = new ThingUID(THING_TYPE_MOON, LOCAL); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Capitalized variables are primarily used for constant literals. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...so that okay - let's worry why they're not declared There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. That's also my concern 😉. |
||
|
||
/** | ||
* Creates a AstroDiscoveryService with disabled autostart. | ||
* Creates a AstroDiscoveryService with enabled autostart. | ||
*/ | ||
public AstroDiscoveryService() { | ||
super(new HashSet<>(Arrays.asList(new ThingTypeUID(BINDING_ID, "-"))), DISCOVER_TIMEOUT_SECONDS, false); | ||
super(new HashSet<>(Arrays.asList(new ThingTypeUID(BINDING_ID, "-"))), DISCOVER_TIMEOUT_SECONDS, true); | ||
|
||
scheduledExecutor = ExpressionThreadPoolManager.getExpressionScheduledPool("astro"); | ||
} | ||
|
||
@Override | ||
protected void startScan() { | ||
logger.debug("Starting Astro discovery scan"); | ||
|
||
PointType location = locationProvider.getLocation(); | ||
|
||
if (location == null) { | ||
logger.debug("LocationProvider.getLocation() is not set -> Will not provide any discovery results"); | ||
return; | ||
} | ||
createResults(location); | ||
} | ||
|
||
@Override | ||
protected void startBackgroundDiscovery() { | ||
Expression expression = null; | ||
try { | ||
expression = new CronExpression( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the complexity of a cron job here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SJKA suggested that we can make use of our scheduler. I thought its a good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I doubt that this is what he meant. |
||
CronHelper.createCronForRepeatEverySeconds(LOCATION_CHANGED_CHECK_INTERVAL)); | ||
} catch (ParseException e) { | ||
return; | ||
} | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. debug is enough |
||
backgroundJob = job; | ||
} | ||
} | ||
|
||
ThingUID sunThing = new ThingUID(AstroBindingConstants.THING_TYPE_SUN, LOCAL); | ||
ThingUID moonThing = new ThingUID(AstroBindingConstants.THING_TYPE_MOON, LOCAL); | ||
@Override | ||
protected void stopBackgroundDiscovery() { | ||
if (backgroundJob != null) { | ||
scheduledExecutor.remove(backgroundJob); | ||
backgroundJob = null; | ||
} | ||
} | ||
|
||
public void createResults(PointType location) { | ||
String propGeolocation; | ||
if (location.getAltitude() != null) { | ||
propGeolocation = String.format("%s,%s,%s", location.getLatitude(), location.getLongitude(), | ||
location.getAltitude()); | ||
} else { | ||
propGeolocation = String.format("%s,%s", location.getLatitude(), location.getLongitude()); | ||
} | ||
thingDiscovered(DiscoveryResultBuilder.create(sunThing).withLabel("Local Sun") | ||
thingDiscovered(DiscoveryResultBuilder.create(SUN_THING).withLabel("Local Sun") | ||
.withProperty("geolocation", propGeolocation).build()); | ||
thingDiscovered(DiscoveryResultBuilder.create(moonThing).withLabel("Local Moon") | ||
thingDiscovered(DiscoveryResultBuilder.create(MOON_THING).withLabel("Local Moon") | ||
.withProperty("geolocation", propGeolocation).build()); | ||
} | ||
|
||
public LocationProvider getLocationProvider() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also thing about adding the location provider to the constructor argument of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just completely remove |
||
return locationProvider; | ||
} | ||
|
||
protected void setLocationProvider(LocationProvider locationProvider) { | ||
this.locationProvider = locationProvider; | ||
} | ||
|
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.
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.