Astro Background discovery for location changes #3725
Changes from 2 commits
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. English: Tip German: Tipp 😉 |
||
|
||
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,27 @@ | ||
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.i18n.LocationProvider; | ||
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; | ||
private LocationProvider locationProvider; | ||
|
||
public AstroDiscoveryLocationChangedTask(AstroDiscoveryService astroDiscoveryService, | ||
LocationProvider locationProvider) { | ||
this.astroDiscoveryService = astroDiscoveryService; | ||
this.locationProvider = locationProvider; | ||
} | ||
|
||
@Override | ||
public void run() { | ||
PointType currentLocation = locationProvider.getLocation(); | ||
if ((currentLocation != null) && !currentLocation.equals(previousLocation)) { | ||
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,39 +36,70 @@ | |
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, locationProvider); | ||
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()); | ||
} | ||
|
||
|
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.