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] Provide alarm channel for smoke detectors #18194

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

david-pace
Copy link
Member

@david-pace david-pace commented Jan 26, 2025

  • add new channel definition for alarm channel in thing-types.xml
  • add alarm channel to Smoke Detector and Smoke Detector II
  • add update instruction sets in binding.xml
  • re-generate i18n properties file
  • add constant for new channel
  • implement alarm service
  • register service package in tests
  • extend abstract smoke detector handler
  • add unit tests
  • add documentation

closes #18161

@david-pace david-pace added enhancement An enhancement or new feature for an existing add-on work in progress A PR that is not yet ready to be merged additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Jan 26, 2025
@david-pace david-pace force-pushed the 18161-smoke-detector-alarm branch from a843afc to 5f7531c Compare January 28, 2025 20:45
@GerdZanker
Copy link
Contributor

GerdZanker commented Feb 1, 2025

Hi, I took this PR code compiled it and used the latest openhab 5.0.0-snapshot runtime from today and started with a new clean setup.
I added a SHC Bridge, my smoke-detector was auto-discovered and I added it too. The new thing appeared as "UNINITIALIZED" and this exception is logged:

2025-02-01 16:57:18.583 [ERROR] [ore.thing.internal.ThingRegistryImpl] - Could not inform the ThingTracker 'org.openhab.core.thing.internal.ThingManagerImpl@622e41f5' about the 'THING_ADDED' event!
java.lang.IllegalArgumentException: Duplicate channels boschshc:smoke-detector:192-168-1-XXX:hdm_HomeMaticIP_3014F711A00004DBB85CAXXX:alarm
	at org.openhab.core.thing.util.ThingHelper.ensureUniqueChannels(ThingHelper.java:135) ~[?:?]
	at org.openhab.core.thing.util.ThingHelper.ensureUniqueChannels(ThingHelper.java:127) ~[?:?]
	at org.openhab.core.thing.util.ThingHelper.ensureUniqueChannels(ThingHelper.java:123) ~[?:?]
	at org.openhab.core.thing.binding.builder.ThingBuilder.withChannel(ThingBuilder.java:123) ~[?:?]
	at org.openhab.core.thing.internal.update.UpdateChannelInstructionImpl.doChannel(UpdateChannelInstructionImpl.java:140) ~[?:?]
	at org.openhab.core.thing.internal.update.UpdateChannelInstructionImpl.perform(UpdateChannelInstructionImpl.java:99) ~[?:?]
	at org.openhab.core.thing.internal.ThingManagerImpl.lambda$17(ThingManagerImpl.java:1095) ~[?:?]
	at java.lang.Iterable.forEach(Iterable.java:75) ~[?:?]
	at org.openhab.core.thing.internal.ThingManagerImpl.checkAndPerformUpdate(ThingManagerImpl.java:1095) ~[?:?]
	at org.openhab.core.thing.internal.ThingManagerImpl.registerAndInitializeHandler(ThingManagerImpl.java:918) ~[?:?]
	at org.openhab.core.thing.internal.ThingManagerImpl.thingAdded(ThingManagerImpl.java:359) ~[?:?]
	at org.openhab.core.thing.internal.ThingRegistryImpl.notifyTrackers(ThingRegistryImpl.java:213) ~[?:?]
	at org.openhab.core.thing.internal.ThingRegistryImpl.notifyListenersAboutAddedElement(ThingRegistryImpl.java:132) ~[?:?]
	at org.openhab.core.thing.internal.ThingRegistryImpl.notifyListenersAboutAddedElement(ThingRegistryImpl.java:1) ~[?:?]
	at org.openhab.core.common.registry.AbstractRegistry.added(AbstractRegistry.java:175) ~[?:?]
	at org.openhab.core.common.registry.AbstractRegistry.added(AbstractRegistry.java:1) ~[?:?]
	at org.openhab.core.common.registry.AbstractProvider.notifyListeners(AbstractProvider.java:60) ~[?:?]
	at org.openhab.core.common.registry.AbstractProvider.notifyListeners(AbstractProvider.java:79) ~[?:?]
	at org.openhab.core.common.registry.AbstractProvider.notifyListenersAboutAddedElement(AbstractProvider.java:83) ~[?:?]
	at org.openhab.core.common.registry.AbstractManagedProvider.add(AbstractManagedProvider.java:66) ~[?:?]
	at org.openhab.core.common.registry.AbstractRegistry.add(AbstractRegistry.java:357) ~[?:?]
	at org.openhab.core.config.discovery.internal.PersistentInbox.addThingSafely(PersistentInbox.java:621) ~[?:?]
	at org.openhab.core.config.discovery.internal.PersistentInbox.approve(PersistentInbox.java:223) ~[?:?]
	at org.openhab.core.io.rest.core.internal.discovery.InboxResource.approve(InboxResource.java:123) ~[?:?]
	at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) ~[?:?]
	at java.lang.reflect.Method.invoke(Method.java:580) ~[?:?]
	at org.apache.cxf.service.invoker.AbstractInvoker.performInvocation(AbstractInvoker.java:179) ~[bundleFile:3.6.4]
	at org.apache.cxf.service.invoker.AbstractInvoker.invoke(AbstractInvoker.java:96) ~[bundleFile:3.6.4]
	at org.apache.cxf.jaxrs.JAXRSInvoker.invoke(JAXRSInvoker.java:201) ~[bundleFile:3.6.4]
	at org.apache.cxf.jaxrs.JAXRSInvoker.invoke(JAXRSInvoker.java:104) ~[bundleFile:3.6.4]
	at org.apache.cxf.interceptor.ServiceInvokerInterceptor$1.run(ServiceInvokerInterceptor.java:59) ~[bundleFile:3.6.4]
	at org.apache.cxf.interceptor.ServiceInvokerInterceptor.handleMessage(ServiceInvokerInterceptor.java:96) ~[bundleFile:3.6.4]
	at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:307) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.ChainInitiationObserver.onMessage(ChainInitiationObserver.java:121) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.http.AbstractHTTPDestination.invoke(AbstractHTTPDestination.java:265) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.servlet.ServletController.invokeDestination(ServletController.java:234) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:208) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:160) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.servlet.CXFNonSpringServlet.invoke(CXFNonSpringServlet.java:225) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.handleRequest(AbstractHTTPServlet.java:304) ~[bundleFile:3.6.4]
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.doPost(AbstractHTTPServlet.java:217) ~[bundleFile:3.6.4]
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:517) ~[bundleFile:4.0.4]
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.service(AbstractHTTPServlet.java:279) ~[bundleFile:3.6.4]
	at org.ops4j.pax.web.service.spi.servlet.OsgiInitializedServlet.service(OsgiInitializedServlet.java:102) ~[bundleFile:?]
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:799) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1656) ~[bundleFile:9.4.57.v20241219]
	at org.ops4j.pax.web.service.spi.servlet.OsgiFilterChain.doFilter(OsgiFilterChain.java:113) ~[bundleFile:?]
	at org.ops4j.pax.web.service.jetty.internal.PaxWebServletHandler.doHandle(PaxWebServletHandler.java:334) ~[bundleFile:?]
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:600) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:235) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1624) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1440) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:188) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:505) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1594) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:186) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1355) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:234) ~[bundleFile:9.4.57.v20241219]
	at org.ops4j.pax.web.service.jetty.internal.PrioritizedHandlerCollection.handle(PrioritizedHandlerCollection.java:96) ~[bundleFile:?]
	at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:722) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.Server.handle(Server.java:516) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:487) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:732) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:479) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:277) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:883) ~[bundleFile:9.4.57.v20241219]
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1034) ~[bundleFile:9.4.57.v20241219]
	at java.lang.Thread.run(Thread.java:1583) [?:?]

Can you @david-pace make use of it? I have no idea what could be wrong in the new code or in my test setup.

* add new channel definition for alarm channel in thing-types.xml
* add alarm channel to Smoke Detector and Smoke Detector II
* add update instruction sets in binding.xml
* re-generate i18n properties file
* add constant for new channel
* implement alarm service
* register service package in tests
* extend abstract smoke detector handler
* add unit tests
* add documentation

Signed-off-by: David Pace <[email protected]>
* introduce new subclass
* move alarm service implementation into new subclass
* introduce new abstract test class
* move unit tests into abstract test class

Signed-off-by: David Pace <[email protected]>
@david-pace david-pace force-pushed the 18161-smoke-detector-alarm branch from 5f7531c to 1d0b8aa Compare February 1, 2025 20:53
@david-pace
Copy link
Member Author

Hi @GerdZanker, thanks for your test attempt 👍

At first glance I cannot explain why the exception occurs. Maybe there is currently a bug on the openhab:main branch, or I have overlooked something.

I just rebased on the latest main upstream state, maybe we are lucky and something was fixed meanwhile, although I don't see any commits that could be connected.

Note that I also introduced a new subclass for the smoke detectors, because in my first version the Twinguard inherited the Alarm service, which is not correct. I realized that after our discussion in #18161 (comment)

Let me know if it works with my latest commit, if not I will dig deeper 🔍

@GerdZanker
Copy link
Contributor

Good morning @david-pace,

I have the same result with your updated code :(

Only if I remove the <thing-type uid="boschshc:smoke-detector"> section from "update/binding.xml" then I can get rid of the exception. Is this an issue in our binding code or in openhab snapshot code?

Afterwards I can see item values of the smoke-detector, but a change of the alarm state results in REST request but no alarm - I expected to hear something.
Example:

2025-02-02 11:12:57.844 [TRACE] [ernal.devices.bridge.BoschHttpClient] - create request for https://192.168.1.xxx:8444/smarthome/devices/hdm:HomeMaticIP:3014F711A00004DBB85Cxxx/services/Alarm/state and content {"value":"PRIMARY_ALARM","stateType":"alarmState","@type":"alarmState"}

@david-pace
Copy link
Member Author

Thanks for your feedback. I think I mitigated the channel update problem after reading the documentation about updating thing types. Only providing the part in binding.xml is not enough. In thing-types.xml, additional thingTypeVersion properties have to be added to prevent newly created things from being updated. This fix is pushed now.

In addition, I studied the Bosch Smart Home Local API again and found two new state enum values that I haven't encountered before. Unfortunatley the documentation is incomplete and I have to guess some of the values, but I added the following enum constants now:

  • PRIMARY_ALARM_ON_REQUESTED
  • PRIMARY_ALARM_OFF_REQUESTED
  • SECONDARY_ALARM_ON_REQUESTED
  • SECONDARY_ALARM_OFF_REQUESTED
  • INTRUSION_ALARM_ON_REQUESTED
  • INTRUSION_ALARM_OFF_REQUESTED

Please let me know if the channel issue is solved with my latest version and also if these alarm state values are usable to make some noise 😉

@GerdZanker
Copy link
Contributor

With your updated code I get no errors anymore.

  1. I'm able to create a new bridge and add a smoke detector with the alarm channel from a binding compiled from PR code
  2. I'm able to use the "main" branch binding to create bridge and smoke detector without the alarm. After an update to the last binding I see the alarm channel and therefore the channel update is working well.

I wasn't able to select any of the new AlarmStates, I patched my thing-types.xml and added the missing option.

Afterwards I was able to "make noise" with INTRUSION_ALARM_ON_REQUESTED. The other two *_ALARM_ON_REQUESTED states made no noise. With INTRUSION_ALARM_OFF_REQUESTED the noise can be stopped.

Do you know if this what we can/should expect from the Bosch SHC API?

@david-pace
Copy link
Member Author

david-pace commented Feb 3, 2025

Thanks for the update 👍

OK, the two values INTRUSION_ALARM_ON_REQUESTED and INTRUSION_ALARM_OFF_REQUESTED are the only values documented in the Bosch SHC Local API. But then I wonder how the primary and secondary alarm can be triggered. I guess it is not possible via the API, at least it is documented nowhere and your tests showed that there are no additional values accepted using the same scheme.

Since we have no more information, I suggest to just keep the two working values in the enum and to add them to thing-types.xml as well as you did. I will soon push a final commit.

Thank you very much for testing @GerdZanker ❤️ Is there anything more to do from your perspective or can this PR be considered complete? If you could test one more time with the latest commit that would be great, thank you!

* remove enum values from AlarmState
* add two valid options to alarm channel type definition
* adjust unit test
* fix abstract test class name
* adjust documentation and fix tables

Signed-off-by: David Pace <[email protected]>
@david-pace david-pace removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Feb 3, 2025
@jlaur
Copy link
Contributor

jlaur commented Feb 3, 2025

@david-pace - did you mean to also mark the PR as ready for review? 🙂

@david-pace
Copy link
Member Author

david-pace commented Feb 3, 2025

I wanted to wait for the final confirmation, but actually you can already start reviewing if you want 😎

@david-pace david-pace requested a review from jlaur February 3, 2025 22:47
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! LGTM.

Copy link
Contributor

@GerdZanker GerdZanker left a comment

Choose a reason for hiding this comment

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

Latest code looks good and works well in my test setup.

@david-pace david-pace marked this pull request as ready for review February 5, 2025 16:54
@david-pace david-pace removed the work in progress A PR that is not yet ready to be merged label Feb 5, 2025
@jlaur jlaur merged commit ea978fe into openhab:main Feb 6, 2025
2 checks passed
@jlaur jlaur added this to the 5.0 milestone Feb 6, 2025
@david-pace david-pace deleted the 18161-smoke-detector-alarm branch February 6, 2025 21:52
chilobo pushed a commit to chilobo/openhab-addons that referenced this pull request Feb 10, 2025
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.

[boschshc] Support for Smoke Alarm II channel Alarm
3 participants