-
Notifications
You must be signed in to change notification settings - Fork 25
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
[amazonechocontrol] fix cycling of UNKOWN/ONLINE for smarthome devices #124
Conversation
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
for (HandlerBase handlerBase : handlers.values()) { | ||
UpdateChannelResult result = new UpdateChannelResult(); | ||
for (String interfaceName : handlerBase.getSupportedInterface()) { | ||
List<JsonObject> stateList = mapInterfaceToStates.get(interfaceName); | ||
if (stateList != null) { | ||
try { | ||
handlerBase.updateChannels(interfaceName, stateList, result); | ||
} catch (Exception e) { | ||
} catch (RuntimeException e) { | ||
// We catch all exceptions, otherwise all other things are not updated! |
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.
Not sure if it is a good idea to change to RuntimeException here. If anything else happens which is not expected, all other things will not be updated. This was the reason for catching Exception.
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.
But all exceptions that do not inherit from RuntimeException are checked exceptions and thus the compiler makes sure that they are catched.
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.
Ok, I see, this feature does not exist in C#, so I'am not familiar with it.
@@ -255,11 +252,7 @@ public void updateChannelStates(List<SmartHomeBaseDevice> allDevices, | |||
} | |||
} | |||
|
|||
if (stateFound) { | |||
updateStatus(ThingStatus.ONLINE); |
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.
maybe we a debug trace instead of the updateStatus would be better. Removing this "if" complete would make diagnostic much harder.
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.
The logging in l. 228 is exactly that. It just exists early and skips the processing the lookup of applianceIds and interfaces if the state array is empty.
#124) Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
The code did not properly handle the case where the capability is present but not the individual device and thus cycled between UNKNOWNand ONLINE.
Signed-off-by: Jan N. Klug jan.n.klug@rub.de