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

[boschshc] Add scenario channel #15752

Merged
merged 6 commits into from
Nov 11, 2023

Conversation

pat-git023
Copy link
Contributor

Add Support to receives and trigger scenarios on Bosch SHC

Description

Some devices (e.g. Universal Switch Flex) of the Bosch Smart Home Controller trigger a preconfigured scenario on the SHC. Do be able to react on such a trigger this PR enhances the current bridge implementation with a channel that get's updated with the name of the triggered scenario.

@pat-git023 pat-git023 marked this pull request as ready for review October 14, 2023 12:50
@jlaur jlaur changed the title [boschshc] feat: support Bosch SHC Scenarios [boschshc] Add scenario channel Oct 14, 2023
@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Oct 14, 2023
@pat-git023
Copy link
Contributor Author

This PR is a followup of #15550 . There I had some issues with git and that's why I started with a fresh branch.
Short summary of the discussions in the closed PR.

  • It was decided that we will introduce two new channels on the bridge (one for receiving triggered scenarios the other one to execute a scenario)
  • There is still a little problem with the update instructions. I always see the following error in the log file:
2023-09-21 21:01:32.001 [INFO ] [core.thing.internal.ThingManagerImpl] - Updating 'boschshc:shc:192-168-1-42' from version 0 to 1
2023-09-21 21:01:32.002 [ERROR] [core.thing.internal.ThingManagerImpl] - Checking/initializing thing 'boschshc:shc:192-168-1-42' failed unexpectedly.
java.lang.IllegalArgumentException: Provider for thing boschshc:shc:192-168-1-42 cannot be determined because it is not known to the registry
	at org.openhab.core.thing.internal.ThingManagerImpl.thingUpdated(ThingManagerImpl.java:243) ~[?:?]
	at org.openhab.core.thing.internal.ThingManagerImpl.checkAndPerformUpdate(ThingManagerImpl.java:1100) ~[?:?]
	at org.openhab.core.thing.internal.ThingManagerImpl.registerAndInitializeHandler(ThingManagerImpl.java:917) ~[?:?]
	at org.openhab.core.thing.internal.ThingManagerImpl.checkMissingPrerequisites(ThingManagerImpl.java:1122) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?]
	at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
	at java.lang.Thread.run(Thread.java:833) ~[?:?]

Big thanks to @david-pace who investigated the issue and found the following problem (#15550 (comment)):

I could reproduce the issue with the thing update. What happens is:
A ThingBuilder is created in org.openhab.core.thing.internal.ThingManagerImpl.checkAndPerformUpdate(Thing, ThingHandlerFactory) which is supposed to create a copy of the original thing (in this case the Bosch SHC bridge)

The update instructions applied using the thing builder
A new thing is created, which is not identical to the orignal thing (it is the "updated" version of the original thing)
I noticed that those things have different types: the original is a BridgeImpl, the updated version is a ThingImpl
org.openhab.core.thing.internal.ThingManagerImpl.thingUpdated(Thing) is called on the new thing
The provider of the thing is not found and the method fails with the exception mentioned previously

@jlaur and/or @lsiepel, do you have an idea what goes wrong here? Is the new thing supposed to have the same type as the original? Does it have to be registered first?

Best regards
Patrick

@david-pace
Copy link
Member

Thanks a lot for providing a new pull request, @pat-git023 👍 And nevermind the hassle with the rebase, it happens quite often.

@J-N-K, do you have any idea what might cause the issue described in #15550 (comment) ? As mentioned I analyzed what happens in #15550 (comment). Thank you 🙂

@david-pace
Copy link
Member

Could the issue be related to the following warnings from the sat-plugin?

[WARNING] OH-INF.thing.thing-types.xml:[0]
Missing configuration for the configuration reference with uri - thing-type:boschshc:bridge
[WARNING] OH-INF.thing.thing-types.xml:[0]
Missing configuration for the configuration reference with uri - thing-type:boschshc:device

@jlaur
Copy link
Contributor

jlaur commented Oct 29, 2023

See openhab/openhab-core#3858

@david-pace
Copy link
Member

@pat-git023 apparently we hit a bug that is fixed in the mean time. Could you please update your main branch again and rebase your changes? Then I can start with tests + reviews 👍

@pat-git023 pat-git023 force-pushed the feat/boschshc_scenario_channels branch from cd078d9 to 9855ced Compare October 31, 2023 06:03
@pat-git023
Copy link
Contributor Author

@david-pace
Did rebase my changes to the latestes main changes.
Hope it worked better this time ;-)

@pat-git023
Copy link
Contributor Author

Just a short update. With the latest 4.1.0-SNAPSHOT distribution from Jenknis I was able to successfully apply the update instructions to an existing Bosch Smart Home Bridge.

2023-10-31 12:33:10.694 [INFO ] [core.thing.internal.ThingManagerImpl] - Updating 'boschshc:shc:192-168-x-x' from version 0 to 1

@jlaur
@J-N-K
Thank you very much for the quick bug fix!!!

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, and sorry for having been a victim of a bug in the channel type migration for so long. I have added a few comments, and will be confidently leaving the rest in the hands of the binding maintainers.

Signed-off-by: Patrick Gell <[email protected]>
Signed-off-by: Patrick Gell <[email protected]>
Signed-off-by: Patrick Gell <[email protected]>
@GerdZanker
Copy link
Contributor

Hello @pat-git023,

I found the time to compile your code and test it: Receiving triggered Scenarios and triggering Scenarios by Name works well - good job! I really want to get your feature in a new update, because it enables many more smart home integration scenarios.

BTW: It took me some minutes more for testing, because one of my created scenario names contained a space at the end of its names, leading to a mismatch when triggering it and I never detected this before.

Because I joined this PR late, I will leave the code review to @jlaur and @david-pace.
Thanks a lot for your contribution.

@david-pace
Copy link
Member

Good news 🙂

  • All review comments are resolved
  • @GerdZanker tested the code successfully
  • I tested the code successfully
  • We even have good unit test coverage 🎉

@jlaur I also marked your review comments as resolved after verifying that your suggestions were implemented. But of course you can check again just to be sure, sometimes two pairs of eyes see more 😉

There are some strange things I noticed while testing but I think it is not related to this pull request: when starting openHAB, the bridge was in state NOT_YET_READY and stayed in this state for quite some time (2-3 minutes) until it became ONLINE. But now I cannot reproduce this anymore.

Also, I see many exceptions like this in the log:

javax.ws.rs.ClientErrorException: HTTP 404 Not Found
	at org.apache.cxf.jaxrs.utils.SpecExceptions.toHttpException(SpecExceptions.java:118) ~[cxf-rt-frontend-jaxrs-3.6.2.jar:3.6.2]
	at org.apache.cxf.jaxrs.utils.ExceptionUtils.toHttpException(ExceptionUtils.java:168) ~[cxf-rt-frontend-jaxrs-3.6.2.jar:3.6.2]
	at org.apache.cxf.jaxrs.utils.JAXRSUtils.findTargetMethod(JAXRSUtils.java:673) ~[cxf-rt-frontend-jaxrs-3.6.2.jar:3.6.2]
	at org.apache.cxf.jaxrs.interceptor.JAXRSInInterceptor.processRequest(JAXRSInInterceptor.java:182) ~[cxf-rt-frontend-jaxrs-3.6.2.jar:3.6.2]
	at org.apache.cxf.jaxrs.interceptor.JAXRSInInterceptor.handleMessage(JAXRSInInterceptor.java:79) ~[cxf-rt-frontend-jaxrs-3.6.2.jar:3.6.2]
	at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:307) ~[cxf-core-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.ChainInitiationObserver.onMessage(ChainInitiationObserver.java:121) ~[cxf-core-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.http.AbstractHTTPDestination.invoke(AbstractHTTPDestination.java:265) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.servlet.ServletController.invokeDestination(ServletController.java:234) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:208) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:160) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.servlet.CXFNonSpringServlet.invoke(CXFNonSpringServlet.java:225) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.handleRequest(AbstractHTTPServlet.java:304) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.doGet(AbstractHTTPServlet.java:222) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:655) ~[org.apache.felix.http.servlet-api-1.2.0.jar:?]
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.service(AbstractHTTPServlet.java:279) ~[cxf-rt-transports-http-3.6.2.jar:3.6.2]
	at org.ops4j.pax.web.service.spi.servlet.OsgiInitializedServlet.service(OsgiInitializedServlet.java:102) ~[pax-web-spi-8.0.22.jar:?]
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:799) ~[jetty-servlet-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1656) ~[jetty-servlet-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.ops4j.pax.web.service.spi.servlet.OsgiFilterChain.doFilter(OsgiFilterChain.java:100) ~[pax-web-spi-8.0.22.jar:?]
	at org.ops4j.pax.web.service.jetty.internal.PaxWebServletHandler.doHandle(PaxWebServletHandler.java:320) ~[pax-web-jetty-8.0.22.jar:?]
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:600) ~[jetty-security-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:235) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1624) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1440) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:188) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:505) ~[jetty-servlet-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1594) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:186) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1355) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:234) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.ops4j.pax.web.service.jetty.internal.PrioritizedHandlerCollection.handle(PrioritizedHandlerCollection.java:96) ~[pax-web-jetty-8.0.22.jar:?]
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.Server.handle(Server.java:516) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:487) ~[jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:732) [jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:479) [jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:277) [jetty-server-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311) [jetty-io-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105) [jetty-io-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104) [jetty-io-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338) [jetty-util-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315) [jetty-util-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173) [jetty-util-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131) [jetty-util-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409) [jetty-util-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:883) [jetty-util-9.4.52.v20230823.jar:9.4.52.v20230823]
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1034) [jetty-util-9.4.52.v20230823.jar:9.4.52.v20230823]
	at java.lang.Thread.run(Thread.java:833) [?:?]

But the Bosch SHC things are all online and working fine.

@GerdZanker @pat-git023 @jlaur have you noticed any of behavior and do you think it has to do something with this pull request? Does anyone know who triggers/queues these REST requests?

@jlaur
Copy link
Contributor

jlaur commented Nov 6, 2023

There are some strange things I noticed while testing but I think it is not related to this pull request: when starting openHAB, the bridge was in state NOT_YET_READY and stayed in this state for quite some time (2-3 minutes) until it became ONLINE. But now I cannot reproduce this anymore.

This can happen during thing update if a channel is missing (because it's not yet removed) - see #15774 (comment)

I don't know why you have experienced it for this PR though, since it only adds two new channels. Could it have been caused by switching back and forth between versions while testing? If your thing would already be upgraded with the new channels, and you would then go back to the previous version, this would probably happen.

@david-pace
Copy link
Member

OK, then I know why this happened: throughout the development of this PR I checked out the code multiple times, and the channel names changed a few times as well. But the thing type version always stayed the same. So I guess this puzzle is solved, thanks 👍

Do you have any idea why I get these REST-related exceptions?

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Very minor last findings, otherwise LGTM.

Signed-off-by: Patrick Gell <[email protected]>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur
Copy link
Contributor

jlaur commented Nov 7, 2023

Do you have any idea why I get these REST-related exceptions?

Unfortunately I don't. Did anyone else experience it? And you don't see it when running without the changes from this PR? I'll hold back the merge for a while until you have had a chance to comment/test. @pat-git023 - I assume you didn't experience this exception?

@pat-git023
Copy link
Contributor Author

Hi,
can not say anymore if I got this exception during dev and testing.
I did another clean test right now and there I didn't get any exceptions.
Test setup:

Test steps:

  1. start openhab with latest SHC addon from jenkins
  2. add SHC bridge (was ONLINE)
  3. stop openhab
  4. replace SHC addon with local build jar of the PR
  5. start openhab
  6. => existing log for updating existing bridge to version 1; NO exceptions
  7. stop openhab
  8. start again (without any changes)
  9. => NO exceptions in the log

@pat-git023
Copy link
Contributor Author

Hello @pat-git023,

I found the time to compile your code and test it: Receiving triggered Scenarios and triggering Scenarios by Name works well - good job! I really want to get your feature in a new update, because it enables many more smart home integration scenarios.

BTW: It took me some minutes more for testing, because one of my created scenario names contained a space at the end of its names, leading to a mismatch when triggering it and I never detected this before.

Because I joined this PR late, I will leave the code review to @jlaur and @david-pace. Thanks a lot for your contribution.

@GerdZanker Thanks a lot for the feedback. And also @david-pace and @jlaur for your great support!!!
I am happy if I could contribute some useful feature.

For you naming miss match with the trailing white space would you suggest to handle this case in the scenario filtering (execute a .trim() on the scenario names) or should there be a more detailed logging that makes it easier to troubleshoot such problems? E.g. if no scenario could be found that matches the name print out the following log:

Scenario 'Scenario 1' was not found in the list of available scenarios [
  Scenario{name='Scenario 1 ', id='d47dc88c-8153-4320-ba1d-d1e9540f3951', lastTimeTriggered='1699373054871'}
  Scenario{name='Scenario 2', id='c473d8d0-db25-4ea7-879f-0d0641a658c7', lastTimeTriggered='1699373054871'}]

@GerdZanker
Copy link
Contributor

more detailed logging that makes it easier to troubleshoot such problems

I like your logging idea, because it helps in case of other mistakes too, e.g. when names are not matching at all.

@pat-git023
Copy link
Contributor Author

pat-git023 commented Nov 7, 2023 via email

@david-pace
Copy link
Member

Meanwhile I tested again and could not reproduce the REST exceptions anymore. Probably a random hiccup not related to this PR.

@jlaur should we wait for the logging enhancement or should we create a new PR for that?

@jlaur
Copy link
Contributor

jlaur commented Nov 7, 2023

Meanwhile I tested again and could not reproduce the REST exceptions anymore. Probably a random hiccup not related to this PR.

👍 Excellent! So I'd say the PR is ready for being merged.

@jlaur should we wait for the logging enhancement or should we create a new PR for that?

I would leave that decision to @pat-git023. If the logging is to pinpoint as suspected problem within this PR (trailing space in a scene name?), it might be worth getting it in to potentially avoid a follow-up bugfix. If it will be logging only without any specific suspicion, that could also be added in another PR.

I'm fine to merge as is, but on the other hand, I guess there is no rush, so if @pat-git023 would like to include the logging here, we can wait until the weekend. Let me know how you'd prefer to proceed.

@david-pace
Copy link
Member

ok, let's wait until the weekend, then we can properly celebrate the merge 😉🎉🍻

Signed-off-by: Patrick Gell <[email protected]>
@jlaur jlaur merged commit 1b466fb into openhab:main Nov 11, 2023
3 checks passed
@jlaur jlaur added this to the 4.1 milestone Nov 11, 2023
@david-pace
Copy link
Member

david-pace commented Nov 11, 2023

Thank you very much for the valuable contribution @pat-git023 👍 And big thanks to @jlaur for the insightful reviews and help during this PR ❤️

@pat-git023
Copy link
Contributor Author

🎉
That's great so we can now start with the proper celebration 🍻 😁

Thank you all for your great support ❤️

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Nov 26, 2023
@pat-git023 pat-git023 deleted the feat/boschshc_scenario_channels branch December 5, 2023 08:29
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
Signed-off-by: Patrick Gell <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants