[smartthings] Fix discovery service bug and enhancement to SmartApp #9889
Conversation
Signed-off-by: Bob Raker <rjraker@gmail.com>
bundles/org.openhab.binding.smartthings/contrib/smartthings/SmartApps/OpenHabAppV2.groovy
Show resolved
Hide resolved
| @@ -889,6 +899,14 @@ def actionColor(device, attribute, value) { | |||
| device.setColor(colormap) | |||
| device.setLevel(value[2] as int) | |||
| break | |||
| case "off": | |||
| // log.debug "actionColor: Setting device \"${device}\" with attribute \"${attribute}\" to off" | |||
There was a problem hiding this comment.
I'm not sure what you want here. Is it to remove the commented out log.debug statement? If so I find it handy to leave those as written in case debugging is needed in the future. The Smartthings IDE does not have anything like an integrated debugger with nice things like breakpoints and variable examination.
There was a problem hiding this comment.
yes it's to remove commented out code. Can't you just leave it in, but not commented out?
| @@ -55,9 +55,10 @@ | |||
| * @author Bob Raker - Initial contribution | |||
| */ | |||
| @NonNullByDefault | |||
| @Component(service = { ThingHandlerFactory.class, | |||
| @Component(service = { ThingHandlerFactory.class, SmartthingsHubCommand.class, | |||
There was a problem hiding this comment.
If you're fixing this. Why not fix it properly and move all code that should be in the bridge to the brigde. So it becomes much simpler as you don't need to keep track of the things here as they're already known by the bridge and also don't have to pass bridge information around here.
There was a problem hiding this comment.
Sorry, I don’t understand what would be proper here. Let me explain why it is the way it is and maybe you can explain what is wrong.
SmartthingsHandlerFactory;sendDeviceCommand() is dependent on the httpClient service. To my knowledge I have to get access to this in a component. Therefore sendDeviceCommand has to be in a component or at least be somewhere with the httpClient service can be passed to it. I could move that to the bridge but I don’t see how that makes things simpler.
SmartthingsHandlerFactory;sendDeviceCommand() is used from SmartthingsDiscoveryService and SmartthingsThingHandler. In SmartthingsDiscoveryService how do I get a reference to the Bridge?
Thanks
There was a problem hiding this comment.
@Hilbrand
I'm still waiting for feedback and more direction on what you think the proper solution is.
There was a problem hiding this comment.
Because discovery depends on a bridge. It means it should be handled differently than just making it a component. This can be implemented by let the discovery implements ThingHandlerService See this prefix documentation on how that can be implemented: https://deploy-preview-1262--openhab-docs-preview.netlify.app/docs/developer/bindings/#discovery-that-is-bound-to-a-thing That implementation gives you access to the bridge in the discovery class and then you can call methods from the bridge. This also means the methods in the factory that do something with the bridge can be moved to the bridge class.
There was a problem hiding this comment.
@Hilbrand
I've implemented the changes in the document that you provided but now I am getting compile errors that I don't know how to fix. Can you point me to a working binding that uses this approach?
Here is what I've done and the resulting errors:
Discovery Service
@NonNullByDefault
@Component(service = { DiscoveryService.class,
EventHandler.class }, configurationPid = "discovery.smartthings", property = "event.topics=org/openhab/binding/smartthings/discovery")
public class SmartthingsDiscoveryService extends AbstractDiscoveryService implements DiscoveryService, ThingHandlerService, EventHandler {
... existing code ...
@Override
public void setThingHandler(@Nullable ThingHandler handler) {
if (handler instanceof SmartthingsBridgeHandler) {
bridgeHandler = (SmartthingsBridgeHandler) handler;
}
}
@Override
public @Nullable ThingHandler getThingHandler() {
return bridgeHandler;
}
}Compiler messages:
The inherited method AbstractDiscoveryService.deactivate() cannot hide the public abstract method in ThingHandlerService Redundant superinterface DiscoveryService for the type SmartthingsDiscoveryService, already defined by AbstractDiscoveryService
Bridge Handler
@NonNullByDefault
public class SmartthingsBridgeHandler extends ConfigStatusBridgeHandler implements ThingHandlerService {
... existing code ...
@Override
public Collection<Class<? extends ThingHandlerService>> getServices() {
return Collections.singleton(SmartthingsDiscoveryService.class);
}Compiler messages
The type SmartthingsBridgeHandler must implement the inherited abstract method ThingHandlerService.getThingHandler()
The type SmartthingsBridgeHandler must implement the inherited abstract method ThingHandlerService.setThingHandler(ThingHandler)
Please advise on how I can move forward. At this point I am answering questions from users about when the discovery service will work. I would like to move this forward. Soon.
There was a problem hiding this comment.
In DiscoveryService you should override the deactivate method:
@Override
public void deactivate() {
super.deactivate();
}The class SmartthingsBridgeHandler should not implements ThingHandlerService.
Ps. I've edited your comment to use code blocks.
Hilbrand
changed the title
|
@Hilbrand First of all public class <your binding bridge DiscoveryService> extends AbstractDiscoveryService
implements DiscoveryService, ThingHandlerService {This generates the compiler error: Second Most importantly @NonNullByDefault
@Component(service = { DiscoveryService.class,
EventHandler.class }, configurationPid = "discovery.smartthings", property = "event.topics=org/openhab/binding/smartthings/discovery")
public class SmartthingsDiscoveryService extends AbstractDiscoveryService
implements DiscoveryService, ThingHandlerService, EventHandler {
private static final int DISCOVERY_TIMEOUT_SEC = 30;
private static final int INITIAL_DELAY_SEC = 10; // Delay 10 sec to give time for bridge and things to be created
private static final int SCAN_INTERVAL_SEC = 600;
private final Pattern findIllegalChars = Pattern.compile("[^A-Za-z0-9_-]");
private final Logger logger = LoggerFactory.getLogger(SmartthingsDiscoveryService.class);
private final Gson gson;
private @Nullable SmartthingsBridgeHandler bridgeHandler;
private @Nullable ScheduledFuture<?> scanningJob;
/*
* default constructor
*/
public SmartthingsDiscoveryService() {
super(SmartthingsBindingConstants.SUPPORTED_THING_TYPES_UIDS, DISCOVERY_TIMEOUT_SEC);
gson = new Gson();
logger.debug("Created new Discovery Service instance {}", System.identityHashCode(this));
}
@Override
public void deactivate() {
super.deactivate();
}
@Override
public void activate() {
super.activate(Collections.emptyMap());
}
…
private void sendSmartthingsDiscoveryRequest() {
if (bridgeHandler != null) {
try {
String discoveryMsg = "{\"discovery\": \"yes\"}";
bridgeHandler.sendDeviceCommand("/discovery", 5, discoveryMsg);
// Smartthings will not return a response to this message but will send it's response message
// which will get picked up by the SmartthingBridgeHandler.receivedPushMessage handler
} catch (InterruptedException | TimeoutException | ExecutionException e) {
logger.warn("Attempt to send command to the Smartthings hub failed with: {}", e.getMessage());
}
} else {
logger.debug("Discovery.sendSmartthingsDiscoveryRequest instance {} trying to use a null bridgeHandler",
System.identityHashCode(this));
}
}
/**
* Handle discovery data returned from the Smartthings hub.
* The data is delivered into the SmartthingServlet. From there it is sent here via the Event service
*/
@Override
public void handleEvent(@Nullable Event event) {
if (event == null) {
logger.info("SmartthingsDiscoveryService.handleEvent: event is uexpectedly null");
return;
}
if (bridgeHandler == null) {
logger.debug("Discovery.handleEvent instance {} trying to use a null bridgeHandler",
System.identityHashCode(this));
return;
}
String topic = event.getTopic();
String data = (String) event.getProperty("data");
if (data == null) {
logger.debug("Event received on topic: {} but the data field is null", topic);
return;
} else {
logger.trace("Event received on topic: {}", topic);
}
// The data returned from the Smartthings hub is a list of strings where each
// element is the data for one device. That device string is another json object
List<String> devices = new ArrayList<>();
devices = gson.fromJson(data, devices.getClass());
logger.debug("Discovery received data for {} devices from the Smartthings hub.", devices.size());
for (String device : devices) {
SmartthingsDeviceData deviceData = gson.fromJson(device, SmartthingsDeviceData.class);
createDevice(Objects.requireNonNull(deviceData));
}
}
/**
* Create a device with the data from the Smartthings hub
*
* @param deviceData Device data from the hub
*/
private void createDevice(SmartthingsDeviceData deviceData) {
logger.trace("Discovery: Creating device: ThingType {} with name {}", deviceData.capability, deviceData.name);
// Build the UID as a string smartthings:{ThingType}:{BridgeName}:{DeviceName}
String name = deviceData.name; // Note: this is necessary for null analysis to work
if (name == null) {
logger.info(
"Unexpectedly received data for a device with no name. Check the Smartthings hub devices and make sure every device has a name");
return;
}
String deviceNameNoSpaces = name.replaceAll("\\s", "_");
String smartthingsDeviceName = findIllegalChars.matcher(deviceNameNoSpaces).replaceAll("");
ThingUID bridgeUid = bridgeHandler.getThing().getBridgeUID();
String bridgeId = bridgeUid.getId();
String uidStr = String.format("smartthings:%s:%s:%s", deviceData.capability, bridgeId, smartthingsDeviceName);
Map<String, Object> properties = new HashMap<>();
properties.put("smartthingsName", name);
properties.put("deviceId", deviceData.id);
DiscoveryResult discoveryResult = DiscoveryResultBuilder.create(new ThingUID(uidStr)).withProperties(properties)
.withRepresentationProperty("deviceId").withBridge(bridgeUid).withLabel(name).build();
thingDiscovered(discoveryResult);
}Log file excerpt: I do like the fact that the sendDeviceCommandt can be moved to the Bridge. Unfortunately the httpClient has to be acquired in the HandlerFactory because it is a service and then has to be passed to the BridgeHandler during creation which is another coupling which isn’t ideal. Do you agree? Is there a better way? Smartthings background: Commands sent to the Smartthings hub are immediately answered with an http 202. Then later the hub will send the response openHAB using a separate message. I handle this using the SmartthingsServlet class. I then use the OSGI EventBus to send those messages to either the SmartthingsDiscoveryService (for discovery responses) or the SmartthingsHandlerFactory (for state responses). Logically it seems that the state responses should be sent to either the Bridge or Thing but that isn’t possible since they aren’t services. Is they are good way around that? I finished this binding a couple of years ago and submitted it for review. At that time the platform was being converted to using BND. And, during the code review I was told the discovery service should be implemented as a Component. After spending countless hours unsuccessfully trying to make it work I just walked away from it for a year. The instructions at that time seem very similar to the approach in this unpublished document, Now it feels this is happening all over. But, this time there are users of the binding. I hope in short order you can get me pointed in the right direction or can decide to accept the solution I came up with. |
There was a problem hiding this comment.
Thanks for the detailed explanation. The eventhandler does make it a bit more complex. I also tried your code to see how I could work out how this can be done. But it would require more work than would be a smart thing to do. Currently having the eventhandler interface in the discovery class doesn't work smoothly with the ThingHandlerService as it's now implemented in openHAB-core. So I would say to just leave it for now as is. If you are happy with what is in this pr we can merge this.
|
@Hilbrand Yes I am happy with merging as is. I know there are users waiting for this to be resolved. If I need to make any changes in the future that would be a good time to rearchitect parts if that would be better. |
…r OH3 (openhab#9889) Signed-off-by: Bob Raker <rjraker@gmail.com> Signed-off-by: John Marshall <john.marshall.au@gmail.com>
wborn
changed the title
…r OH3 (openhab#9889) Signed-off-by: Bob Raker <rjraker@gmail.com>
…r OH3 (openhab#9889) Signed-off-by: Bob Raker <rjraker@gmail.com>
Signed-off-by: Bob Raker rjraker@gmail.com
Most importantly this fixes a bug that caused the discovery service to fail.
The problem was that I needed a reference to the SmartthingsThingHandlerFactory and I incorrectly created an @reference to ThingHandlerFactory. At runtime frequently the ThingHandlerFactory from a different bundle would be passed in and I would ignore it. I didn't quite understand how that OSGI mechanism worked. I corrected the problem by creating a new Service interface (named SmartthingsHubCommand that included the methods needed by the discovery service.
Secondly a user opened an issue in my Smartthings github repo #102 in BobRak/OpenHAB-Smartthings stating the the SmartApp that needs to be loaded onto the Smartthings Hub didn't correctly handle changing the brightness to 0. I was able to reproduce this in OH 3.0 and applied the changes the user suggested.