Skip to content

Commit

Permalink
All review comments considered
Browse files Browse the repository at this point in the history 
Use constructor injector for the discovery service

Signed-off-by: Laurent Garnier <[email protected]>
  • Loading branch information
@lolodomo
lolodomo committed Jun 2, 2020
1 parent 04af195 commit 18f34f2
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 114 deletions.
9 changes: 9 additions & 0 deletions ...enhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/NtpBindingConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,14 @@ public class NtpBindingConstants {
public static final String CHANNEL_DATE_TIME = "dateTime";
public static final String CHANNEL_STRING = "string";

// Custom Properties
public static final String PROPERTY_NTP_SERVER_HOST = "hostname";
public static final String PROPERTY_REFRESH_INTERVAL = "refreshInterval";
public static final String PROPERTY_REFRESH_NTP = "refreshNtp";
public static final String PROPERTY_TIMEZONE = "timeZone";
public static final String PROPERTY_LOCALE = "locale";
public static final String PROPERTY_DATE_TIME_FORMAT = "DateTimeFormat";
public static final String PROPERTY_NTP_SERVER_PORT = "serverPort";

public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Collections.singleton(THING_TYPE_NTP);
}
5 changes: 1 addition & 4 deletions .../src/main/java/org/openhab/binding/ntp/internal/config/NtpStringChannelConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
package org.openhab.binding.ntp.internal.config;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;

/**
* The {@link NtpStringChannelConfiguration} is responsible for holding
Expand All @@ -24,7 +23,5 @@
@NonNullByDefault
public class NtpStringChannelConfiguration {

public static final String DATE_TIME_FORMAT = "DateTimeFormat";

public @Nullable String DateTimeFormat;
public String DateTimeFormat = "yyyy-MM-dd HH:mm:ss z";
}
43 changes: 4 additions & 39 deletions ...ding.ntp/src/main/java/org/openhab/binding/ntp/internal/config/NtpThingConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,44 +24,9 @@
@NonNullByDefault
public class NtpThingConfiguration {

public static final String HOSTNAME = "hostname";
public static final String REFRESH_INTERVAL = "refreshInterval";
public static final String REFRESH_NTP = "refreshNtp";
public static final String SERVER_PORT = "serverPort";
public static final String TIMEZONE = "timeZone";

private static final String DEFAULT_SERVER_HOSTNAME = "0.pool.ntp.org";
private static final int DEFAULT_REFRESH_INTERVAL = 60;
private static final int DEFAULT_REFRESH_NTP = 30;
private static final int DEFAULT_SERVER_PORT = 123;

public @Nullable String hostname;
public @Nullable Integer refreshInterval;
public @Nullable Integer refreshNtp;
public @Nullable Integer serverPort;
public String hostname = "0.pool.ntp.org";
public int refreshInterval = 60;
public int refreshNtp = 30;
public int serverPort = 123;
public @Nullable String timeZone;

public String getHostname() {
String name = hostname;
return name != null && !name.trim().isEmpty() ? name.trim() : DEFAULT_SERVER_HOSTNAME;
}

public int getRefreshInterval() {
Integer interval = refreshInterval;
return interval != null ? interval : DEFAULT_REFRESH_INTERVAL;
}

public int getRefreshNtp() {
Integer number = refreshNtp;
return number != null ? number : DEFAULT_REFRESH_NTP;
}

public int getServerPort() {
Integer port = serverPort;
return port != null ? port : DEFAULT_SERVER_PORT;
}

public @Nullable String getTimeZone() {
return timeZone;
}
}
45 changes: 12 additions & 33 deletions ...ab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/discovery/NtpDiscovery.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,18 @@

import java.util.HashMap;
import java.util.Map;
import java.util.TimeZone;
import java.util.concurrent.TimeUnit;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.smarthome.config.discovery.AbstractDiscoveryService;
import org.eclipse.smarthome.config.discovery.DiscoveryResult;
import org.eclipse.smarthome.config.discovery.DiscoveryResultBuilder;
import org.eclipse.smarthome.config.discovery.DiscoveryService;
import org.eclipse.smarthome.core.i18n.LocaleProvider;
import org.eclipse.smarthome.core.i18n.TimeZoneProvider;
import org.eclipse.smarthome.core.i18n.TranslationProvider;
import org.eclipse.smarthome.core.thing.ThingUID;
import org.openhab.binding.ntp.internal.config.NtpThingConfiguration;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;

Expand All @@ -43,18 +42,16 @@
@Component(service = DiscoveryService.class, immediate = true, configurationPid = "discovery.ntp")
public class NtpDiscovery extends AbstractDiscoveryService {

public NtpDiscovery() throws IllegalArgumentException {
super(SUPPORTED_THING_TYPES_UIDS, 2);
}

@Override
protected void activate(@Nullable Map<String, @Nullable Object> configProperties) {
super.activate(configProperties);
}
private final TimeZoneProvider timeZoneProvider;

@Override
protected void modified(@Nullable Map<String, @Nullable Object> configProperties) {
super.modified(configProperties);
@Activate
public NtpDiscovery(final @Reference LocaleProvider localeProvider,
final @Reference TranslationProvider i18nProvider, final @Reference TimeZoneProvider timeZoneProvider)
throws IllegalArgumentException {
super(SUPPORTED_THING_TYPES_UIDS, 2);
this.localeProvider = localeProvider;
this.i18nProvider = i18nProvider;
this.timeZoneProvider = timeZoneProvider;
}

@Override
Expand All @@ -74,28 +71,10 @@ protected void startScan() {
*/
private void discoverNtp() {
Map<String, Object> properties = new HashMap<>(4);
properties.put(NtpThingConfiguration.TIMEZONE, TimeZone.getDefault().getID());
properties.put(PROPERTY_TIMEZONE, timeZoneProvider.getTimeZone().getId());
ThingUID uid = new ThingUID(THING_TYPE_NTP, "local");
DiscoveryResult result = DiscoveryResultBuilder.create(uid).withProperties(properties).withLabel("Local Time")
.build();
thingDiscovered(result);
}

@Reference
protected void setLocaleProvider(final LocaleProvider localeProvider) {
this.localeProvider = localeProvider;
}

protected void unsetLocaleProvider(final LocaleProvider localeProvider) {
this.localeProvider = null;
}

@Reference
protected void setTranslationProvider(TranslationProvider i18nProvider) {
this.i18nProvider = i18nProvider;
}

protected void unsetTranslationProvider(TranslationProvider i18nProvider) {
this.i18nProvider = null;
}
}
43 changes: 18 additions & 25 deletions ...penhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/handler/NtpHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
import java.io.IOException;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.time.DateTimeException;
import java.time.Instant;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.util.TimeZone;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

Expand Down Expand Up @@ -54,7 +54,7 @@
* to one of the channels.
*
* @author Marcel Verpaalen - Initial contribution OH2 ntp binding
* @author Thomas.Eichstaedt - Engelen OH1 ntp binding (getTime routine)
* @author Thomas.Eichstaedt-Engelen - OH1 ntp binding (getTime routine)
* @author Markus Rathgeb - Add locale provider
* @author Erdoan Hadzhiyusein - Adapted the class to work with the new DateTimeType
* @author Laurent Garnier - null annotations, TimeZoneProvider, configuration settings cleanup
Expand All @@ -73,7 +73,7 @@ public class NtpHandler extends BaseThingHandler {
/** for publish purposes */
private DateTimeFormatter dateTimeFormat = DateTimeFormatter.ofPattern(DATE_PATTERN_WITH_TZ);

private @Nullable NtpThingConfiguration configuration;
private NtpThingConfiguration configuration = new NtpThingConfiguration();

private @Nullable ScheduledFuture<?> refreshJob;

Expand Down Expand Up @@ -103,33 +103,31 @@ public void handleCommand(ChannelUID channelUID, Command command) {
public void initialize() {
logger.debug("Initializing NTP handler for '{}'.", getThing().getUID());

NtpThingConfiguration config = getConfigAs(NtpThingConfiguration.class);
configuration = config;
configuration = getConfigAs(NtpThingConfiguration.class);

refreshNtpCount = 0;

String timeZoneConfigValue = config.getTimeZone();
if (timeZoneConfigValue != null) {
if (configuration.timeZone != null) {
logger.debug("{} with timezone '{}' set in configuration setting '{}'", getThing().getUID(),
timeZoneConfigValue, NtpThingConfiguration.TIMEZONE);
configuration.timeZone, PROPERTY_TIMEZONE);
try {
timeZoneId = TimeZone.getTimeZone(timeZoneConfigValue).toZoneId();
} catch (Exception e) {
timeZoneId = ZoneId.of(configuration.timeZone);
} catch (DateTimeException e) {
timeZoneId = defaultTimeZoneId;
logger.debug("{} using default timezone '{}' due to an occurred exception: ", getThing().getUID(),
timeZoneId, e);
logger.debug("{} using default timezone '{}', because configuration setting '{}' is invalid: {}",
getThing().getUID(), timeZoneId, PROPERTY_TIMEZONE, e.getMessage());
}
} else {
timeZoneId = defaultTimeZoneId;
logger.debug("{} using default timezone '{}', because configuration setting '{}' is null.",
getThing().getUID(), timeZoneId, NtpThingConfiguration.TIMEZONE);
getThing().getUID(), timeZoneId, PROPERTY_TIMEZONE);
}

Channel stringChannel = getThing().getChannel(CHANNEL_STRING);
if (stringChannel != null) {
String dateTimeFormatString = stringChannel.getConfiguration()
.as(NtpStringChannelConfiguration.class).DateTimeFormat;
if (dateTimeFormatString != null && !dateTimeFormatString.isEmpty()) {
if (!dateTimeFormatString.isEmpty()) {
logger.debug("Date format set in config for channel '{}': {}", CHANNEL_STRING, dateTimeFormatString);
try {
dateTimeFormat = DateTimeFormatter.ofPattern(dateTimeFormatString);
Expand All @@ -149,16 +147,16 @@ public void initialize() {

logger.debug(
"Initialized NTP handler '{}' with configuration: host '{}', port {}, refresh interval {}, refresh frequency {}, timezone {}.",
getThing().getUID(), config.getHostname(), config.getServerPort(), config.getRefreshInterval(),
config.getRefreshNtp(), timeZoneId);
getThing().getUID(), configuration.hostname, configuration.serverPort, configuration.refreshInterval,
configuration.refreshNtp, timeZoneId);

refreshJob = scheduler.scheduleWithFixedDelay(() -> {
try {
refreshTimeDate();
} catch (Exception e) {
logger.debug("Exception occurred during refresh: {}", e.getMessage(), e);
}
}, 0, config.getRefreshInterval(), TimeUnit.SECONDS);
}, 0, configuration.refreshInterval, TimeUnit.SECONDS);
}

@Override
Expand All @@ -173,17 +171,12 @@ public void dispose() {
}

private synchronized void refreshTimeDate() {
NtpThingConfiguration config = configuration;
if (config == null) {
return;
}

long networkTimeInMillis;
if (refreshNtpCount <= 0) {
networkTimeInMillis = getTime(config.getHostname(), config.getServerPort());
networkTimeInMillis = getTime(configuration.hostname, configuration.serverPort);
timeOffset = networkTimeInMillis - System.currentTimeMillis();
logger.debug("{} delta system time: {}", getThing().getUID(), timeOffset);
refreshNtpCount = config.getRefreshNtp();
refreshNtpCount = configuration.refreshNtp;
} else {
networkTimeInMillis = System.currentTimeMillis() + timeOffset;
refreshNtpCount--;
Expand Down Expand Up @@ -215,7 +208,7 @@ private long getTime(String hostname, int port) {
ZonedDateTime zoned = ZonedDateTime.ofInstant(Instant.ofEpochMilli(serverMillis), timeZoneId);
logger.debug("{} Got time update from host '{}': {}.", getThing().getUID(), hostname,
zoned.format(DATE_FORMATTER_WITH_TZ));
updateStatus(ThingStatus.ONLINE, ThingStatusDetail.NONE);
updateStatus(ThingStatus.ONLINE);
return serverMillis;
} catch (UnknownHostException uhe) {
logger.debug(
Expand Down
24 changes: 11 additions & 13 deletions ...org.openhab.binding.ntp.tests/src/main/java/org/openhab/binding/ntp/test/NtpOSGiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@
import org.junit.Test;
import org.mockito.ArgumentMatchers;
import org.openhab.binding.ntp.internal.NtpBindingConstants;
import org.openhab.binding.ntp.internal.config.NtpStringChannelConfiguration;
import org.openhab.binding.ntp.internal.config.NtpThingConfiguration;
import org.openhab.binding.ntp.internal.handler.NtpHandler;
import org.openhab.binding.ntp.server.SimpleNTPServer;

Expand Down Expand Up @@ -194,13 +192,13 @@ public void testStringChannelTimeZoneUpdate() {
final String expectedTimeZonePST = "PST";

Configuration configuration = new Configuration();
configuration.put(NtpThingConfiguration.TIMEZONE, TEST_TIME_ZONE_ID);
configuration.put(NtpBindingConstants.PROPERTY_TIMEZONE, TEST_TIME_ZONE_ID);
Configuration channelConfig = new Configuration();
/*
* Set the format of the date, so it is updated in the item registry in
* a format from which we can easily get the time zone.
*/
channelConfig.put(NtpStringChannelConfiguration.DATE_TIME_FORMAT, TEST_DATE_TIME_FORMAT);
channelConfig.put(NtpBindingConstants.PROPERTY_DATE_TIME_FORMAT, TEST_DATE_TIME_FORMAT);

initialize(configuration, NtpBindingConstants.CHANNEL_STRING, ACCEPTED_ITEM_TYPE_STRING, channelConfig, null);

Expand All @@ -212,7 +210,7 @@ public void testStringChannelTimeZoneUpdate() {
@Test
public void testDateTimeChannelTimeZoneUpdate() {
Configuration configuration = new Configuration();
configuration.put(NtpThingConfiguration.TIMEZONE, TEST_TIME_ZONE_ID);
configuration.put(NtpBindingConstants.PROPERTY_TIMEZONE, TEST_TIME_ZONE_ID);
initialize(configuration, NtpBindingConstants.CHANNEL_DATE_TIME, ACCEPTED_ITEM_TYPE_DATE_TIME, null, null);

String testItemState = getItemState(ACCEPTED_ITEM_TYPE_DATE_TIME).toString();
Expand All @@ -228,7 +226,7 @@ public void testDateTimeChannelTimeZoneUpdate() {
@Test
public void testDateTimeChannelCalendarTimeZoneUpdate() {
Configuration configuration = new Configuration();
configuration.put(NtpThingConfiguration.TIMEZONE, TEST_TIME_ZONE_ID);
configuration.put(NtpBindingConstants.PROPERTY_TIMEZONE, TEST_TIME_ZONE_ID);
initialize(configuration, NtpBindingConstants.CHANNEL_DATE_TIME, ACCEPTED_ITEM_TYPE_DATE_TIME, null, null);
ZonedDateTime timeZoneIdFromItemRegistry = ((DateTimeType) getItemState(ACCEPTED_ITEM_TYPE_DATE_TIME))
.getZonedDateTime();
Expand All @@ -249,7 +247,7 @@ public void testStringChannelDefaultTimeZoneUpdate() {
* Set the format of the date, so it is updated in the item registry in
* a format from which we can easily get the time zone.
*/
channelConfig.put(NtpStringChannelConfiguration.DATE_TIME_FORMAT, TEST_DATE_TIME_FORMAT);
channelConfig.put(NtpBindingConstants.PROPERTY_DATE_TIME_FORMAT, TEST_DATE_TIME_FORMAT);

// Initialize with configuration with no time zone property set.
initialize(configuration, NtpBindingConstants.CHANNEL_STRING, ACCEPTED_ITEM_TYPE_STRING, null, null);
Expand Down Expand Up @@ -295,7 +293,7 @@ public void testStringChannelFormatting() {

Configuration configuration = new Configuration();
Configuration channelConfig = new Configuration();
channelConfig.put(NtpStringChannelConfiguration.DATE_TIME_FORMAT, formatPattern);
channelConfig.put(NtpBindingConstants.PROPERTY_DATE_TIME_FORMAT, formatPattern);
initialize(configuration, NtpBindingConstants.CHANNEL_STRING, ACCEPTED_ITEM_TYPE_STRING, channelConfig, null);

String dateFromItemRegistry = getItemState(ACCEPTED_ITEM_TYPE_STRING).toString();
Expand All @@ -319,7 +317,7 @@ public void testEmptyStringPropertyFormatting() {
Configuration configuration = new Configuration();
Configuration channelConfig = new Configuration();
// Empty string
channelConfig.put(NtpStringChannelConfiguration.DATE_TIME_FORMAT, "");
channelConfig.put(NtpBindingConstants.PROPERTY_DATE_TIME_FORMAT, "");

initialize(configuration, NtpBindingConstants.CHANNEL_STRING, ACCEPTED_ITEM_TYPE_STRING, channelConfig, null);

Expand All @@ -332,7 +330,7 @@ public void testEmptyStringPropertyFormatting() {
public void testNullPropertyFormatting() {
Configuration configuration = new Configuration();
Configuration channelConfig = new Configuration();
channelConfig.put(NtpStringChannelConfiguration.DATE_TIME_FORMAT, null);
channelConfig.put(NtpBindingConstants.PROPERTY_DATE_TIME_FORMAT, null);

initialize(configuration, NtpBindingConstants.CHANNEL_STRING, ACCEPTED_ITEM_TYPE_STRING, channelConfig, null);

Expand Down Expand Up @@ -380,16 +378,16 @@ private void initialize(Configuration configuration, String channelID, String ac
// There are 2 tests which require wrong hostnames.
boolean isWrongHostNameTest = wrongHostname != null;
if (isWrongHostNameTest) {
configuration.put(NtpThingConfiguration.HOSTNAME, wrongHostname);
configuration.put(NtpBindingConstants.PROPERTY_NTP_SERVER_HOST, wrongHostname);
} else {
configuration.put(NtpThingConfiguration.HOSTNAME, TEST_HOSTNAME);
configuration.put(NtpBindingConstants.PROPERTY_NTP_SERVER_HOST, TEST_HOSTNAME);
}
initialize(configuration, channelID, acceptedItemType, channelConfiguration);
}

private void initialize(Configuration configuration, String channelID, String acceptedItemType,
Configuration channelConfiguration) {
configuration.put(NtpThingConfiguration.SERVER_PORT, TEST_PORT);
configuration.put(NtpBindingConstants.PROPERTY_NTP_SERVER_PORT, TEST_PORT);
ThingUID ntpUid = new ThingUID(NtpBindingConstants.THING_TYPE_NTP, TEST_THING_ID);

ChannelUID channelUID = new ChannelUID(ntpUid, channelID);
Expand Down

0 comments on commit 18f34f2

Please sign in to comment.