Skip to content
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

Astro Background discovery for location changes #3725

Merged
merged 6 commits into from Jun 24, 2017

Conversation

Projects
None yet
5 participants
@triller-telekom
Copy link
Contributor

commented Jun 22, 2017

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

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

##Tipps

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 22, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 22, 2017

Contributor

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

This comment has been minimized.

Copy link
@triller-telekom

triller-telekom Jun 23, 2017

Author Contributor

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

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

public LocationProvider getLocationProvider() {

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jun 22, 2017

Contributor

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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

Fix location comparison
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.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

license?


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

public class AstroDiscoveryLocationChangedTask implements Runnable {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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

This comment has been minimized.

Copy link
@triller-telekom

triller-telekom Jun 23, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

debug is enough

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

public LocationProvider getLocationProvider() {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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(

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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

This comment has been minimized.

Copy link
@triller-telekom

triller-telekom Jun 23, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 23, 2017

Member

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!).

triller-telekom added some commits Jun 23, 2017

Use Lamda expression instead of additional public class
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Missing README changes
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@amitjoy

This comment has been minimized.

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

This comment has been minimized.

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.

This comment has been minimized.

Copy link
Contributor

replied Jun 23, 2017

What about

if (!Objects.equals(currentLocation, previousLocation)) {
...
}
Improve debug logging
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@triller-telekom

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 23, 2017

Member

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)) {

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 23, 2017

Member

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

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

## Tipps

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 23, 2017

Member

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

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 23, 2017

Member

Capitalized variables are primarily used for constant literals.

This comment has been minimized.

Copy link
@sjka

sjka Jun 23, 2017

Contributor

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

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 23, 2017

Member

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

Use Objects.equals, final variables, better debug logs
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@triller-telekom triller-telekom deleted the triller-telekom:astroLocationChanged branch Jun 26, 2017

@kaikreuzer kaikreuzer modified the milestone: 0.9.0 Jun 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.