Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Sonos binding: fix bug with notification timeout handling #3883

Merged
merged 1 commit into from Jul 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -9,11 +9,11 @@
<description>The UDN identifies the Zone Player.</description>
<required>true</required>
</parameter>
<parameter name="notificationTimeout" type="integer">
<parameter name="notificationTimeout" type="integer" unit="s">
<label>Notification Timeout</label>
<description>Specifies the amount of time for which the notification sound will be played</description>
<default>40</default>
<required>true</required>
<description>Specifies the amount of time in seconds for which the notification sound will be played</description>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using the unit atrribute and the unitLabel element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I don't know this ability ;) If it is described somewhere, I will try to change that.

Copy link
Contributor

@cweitkamp cweitkamp Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to know. I found PR #889 but no directly hint in the docs.)

EDIT: I am wrong. One can find it in the https://github.com/eclipse/smarthome/blob/091a79696626da2298968cf4ffbe82bdadab6a66/docs/documentation/development/bindings/xml-reference.md section.

<unitLabel>second</unitLabel>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't "s" the normal label? "5 s" looks better to me than "5 second"...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be free to change it if you think it is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, so this line should be removed completely!

<default>20</default>
</parameter>
<parameter name="refresh" type="integer">
<label>Refresh interval</label>
Expand Down
Expand Up @@ -37,7 +37,6 @@
import org.eclipse.smarthome.binding.sonos.internal.SonosXMLParser;
import org.eclipse.smarthome.binding.sonos.internal.SonosZoneGroup;
import org.eclipse.smarthome.binding.sonos.internal.SonosZonePlayerState;
import org.eclipse.smarthome.config.core.Configuration;
import org.eclipse.smarthome.config.discovery.DiscoveryServiceRegistry;
import org.eclipse.smarthome.core.library.types.DecimalType;
import org.eclipse.smarthome.core.library.types.IncreaseDecreaseType;
Expand Down Expand Up @@ -101,12 +100,12 @@ public class ZonePlayerHandler extends BaseThingHandler implements UpnpIOPartici
private static final int SOCKET_TIMEOUT = 5000;

/**
* Default notification timeout
* Default notification timeout (in seconds)
*/
private static final Integer DEFAULT_NOTIFICATION_TIMEOUT = 40000;
private static final Integer DEFAULT_NOTIFICATION_TIMEOUT = 20;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config-description XML is using a default of 40 seconds, you change it here to 20 seconds.
I don't know what is better 20 or 40.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the XML value after your comment.


/**
* configurable notification timeout
* configurable notification timeout (in seconds)
*/
private Integer notificationTimeout = null;

Expand Down Expand Up @@ -212,15 +211,12 @@ public void initialize() {

if (getUDN() != null) {
onUpdate();

if (getConfigAs(ZonePlayerConfiguration.class).notificationTimeout == null) {
Configuration c = editConfiguration();
c.put(ZonePlayerConfiguration.NOTIFICATION_TIMEOUT, DEFAULT_NOTIFICATION_TIMEOUT);
updateConfiguration(c);
}


this.notificationTimeout = getConfigAs(ZonePlayerConfiguration.class).notificationTimeout;

if (this.notificationTimeout == null) {
this.notificationTimeout = DEFAULT_NOTIFICATION_TIMEOUT;
}

super.initialize();
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
Expand Down Expand Up @@ -2388,7 +2384,7 @@ private void waitForFinishedNotification() {
// check Sonos state events to determine the end of the notification sound
String notificationTitle = stateMap.get("CurrentTitle");
long playstart = System.currentTimeMillis();
while (System.currentTimeMillis() - playstart < this.notificationTimeout.longValue()) {
while (System.currentTimeMillis() - playstart < this.notificationTimeout.longValue() * 1000) {
try {
Thread.sleep(50);
if (!notificationTitle.equals(stateMap.get("CurrentTitle"))
Expand All @@ -2407,7 +2403,7 @@ private void waitForTransportState(String state) {
while (!stateMap.get("TransportState").equals(state)) {
try {
Thread.sleep(50);
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue()) {
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue() * 1000) {
break;
}
} catch (InterruptedException e) {
Expand All @@ -2423,7 +2419,7 @@ private void waitForNotTransportState(String state) {
while (stateMap.get("TransportState").equals(state)) {
try {
Thread.sleep(50);
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue()) {
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue() * 1000) {
break;
}
} catch (InterruptedException e) {
Expand Down