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 #15550

Closed

Conversation

pat-git023
Copy link
Contributor

[boschshc] Receive triggered scenarios of the Bosch Smart Home Controller

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 September 5, 2023 16:48
@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Sep 5, 2023
@lsiepel lsiepel changed the title feat: revceive name of triggered scenario [boschshc] Add scenario channel Sep 5, 2023
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Beside the comments, thing upgrade instructions are also needed as a channel is added.

I have also edited to PR title, feel free to further adjust.

@pat-git023
Copy link
Contributor Author

@lsiepel
Thank you very much for your fast feedback.

During the documentation of the new channel I was wondering if the Bridge would be the right place for this channel.
A look at the Rest API of the SHC showed that there is also a Scenario resource: https://local.apidocs.bosch-smarthome.com/#/Scenarios
This got me thinking that I should maybe introduce the Scenario as a new thing type, add the channel there and maybe later also offer a second channel where you can execute a Scenario on the SHC (POST /scenarios/{scenarioId}/triggers).

May I ask what would be your opinion on this option?

Best regards,
Patrick

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/bosch-smart-home-enhance-with-triggers-scenario-messages-of-universal-switch-flex/149361/1

@lsiepel
Copy link
Contributor

lsiepel commented Sep 6, 2023

@lsiepel Thank you very much for your fast feedback.

During the documentation of the new channel I was wondering if the Bridge would be the right place for this channel. A look at the Rest API of the SHC showed that there is also a Scenario resource: https://local.apidocs.bosch-smarthome.com/#/Scenarios This got me thinking that I should maybe introduce the Scenario as a new thing type, add the channel there and maybe later also offer a second channel where you can execute a Scenario on the SHC (POST /scenarios/{scenarioId}/triggers).

May I ask what would be your opinion on this option?

Best regards, Patrick

Modeling the API-structure to the openHAB domain is something that you might want to discuss with the codeowner. I can understand your doubts to add it to the bridge, i can also understand that it is the best fit. As the channel does not seem to be specific for a room or device. (but that is just a quick look, i'm not a codeowner or maintainer)

@david-pace
Copy link
Member

Hi @pat-git023, thank you very much for this great enhancement, I recently thought that it would be nice to be able to get scenario notifications or even trigger scenarios 👍

Also thank you very much for your very quick review, @lsiepel 👍

The change as such looks good. But as already mentioned we should discuss whether the channel(s) should be added to the bridge directly, or rather added to scenarios that could be modeled as things.

Adding the channel(s) directly to the bridge has the following disadvantages:

  • when new scenario-related channels (or channels for other "global" concepts in the Bosch Smart Home system) are necessary, they would have to be added to the bridge as well
  • every time we add/change the channels on the bridge, the users have to delete and re-create their bridges (this would affect all users and is not very convenient for the users). I'm also not sure how openHAB behaves when there are lots of things referencing the bridge as parent. Will the deletion be possible in this case? If not, it would mean that the users have to delete and re-create all Bosch SHC things...

So from my current intuition I would propose to keep the bridge as stable as possible, and to model scenarios as a separate thing type. Scenarios could be identified via ID (does anyone know if we can create two scenarios with the same name, but different IDs?) and have the channels:

  • name (string, read-only)
  • last-time-triggered (timestamp, read-only, is updated when triggered from the Bosch Smart Home App or the Controller)
  • trigger-channel (writable, used to trigger the scenario from OpenHAB)

When modelling it this way the users do not have to re-create all their bridges and if the scenario features are extended, they would only have to re-create their scenarios.

But I'm not 100% sure about this approach. I would like to hear what @GerdZanker and maybe also @jlaur think about it. By the way: do we need to create a github issue for this feature/enhancement?

Already a big thank you to @pat-git023 for proposing this pull request 😎

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/bosch-smart-home-enhance-with-triggers-scenario-messages-of-universal-switch-flex/149361/2

@lsiepel
Copy link
Contributor

lsiepel commented Sep 7, 2023

  • when new scenario-related channels (or channels for other "global" concepts in the Bosch Smart Home system) are necessary, they would have to be added to the bridge as well

And why is that a disadvantage?

  • every time we add/change the channels on the bridge, the users have to delete and re-create their bridges (this would affect all users and is not very convenient for the users). I'm also not sure how openHAB behaves when there are lots of things referencing the bridge as parent. Will the deletion be possible in this case? If not, it would mean that the users have to delete and re-create all Bosch SHC things...

With the thing upgrade instructions since 4.0 that is no longer the case.

@pat-git023
Copy link
Contributor Author

@david-pace no problem. Was just solving a use-case of mine where I used the scenario trigger of the Bosch App to manipulate devices that are not in the Bosch system.. Thought that it might be also useful for others and give the changes back to the community.

After thinking a bit further (executing a scenario from OH would also be nice) and having a look over the Bosch Rest API I had my doubts if the current solution would be the best one.
Another proposal could be to model the different scenarios as separate things with their channels as you proposed.
Some advantages I see would be:

  • they could be auto-discovered ( GET /scenarios )
  • for the trigger channel the scenarioId is already present in OH (POST /scenarios/{scenarioId}/triggers)

But how can a user react on a triggered scenario?
-> create a rule for every scenario of interest that is triggered if the last-time-triggered channel is updated

For this use-case wouldn't it also make sense to have a separate channel that is updated with the current scenario name?
In this case one rule (triggered if the channel was updated) with a switch-case would be sufficient to react on different scenarios.

Maybe a solution with both approaches is needed (like it is also in the Bosch API):

  • Have a general channel that gets updated with the triggered scenario name
  • Have scenario things with a write channel to execute the scenario on the Bosch controller

Btw: You can create multiple scenarios with the same name in the Bosch App and they get different IDs (looks like a UUID).
In the Bosch App you can trigger each scenario and the event message contains the corresponding ID

Best regards,
Patrick

@david-pace
Copy link
Member

david-pace commented Sep 8, 2023

And why is that a disadvantage?

With the thing upgrade instructions since 4.0 that is no longer the case.

It would have been a major disadvantage if my assumption had been correct that the bridge has to be re-created by the users every time the channels are changed. But I was simply not aware of the thing upgrade instructions since 4.0, thanks for pointing that out.

In this case both approaches are viable and as @pat-git023 pointed out, they can also be combined. There is also the approach to have a generic scenario trigger channel in the bridge (see list below).

In general scenario names are more convenient for users (as opposed to scenario UUIDs), but if we use scenario names the special case with multiple scenarios with the same name can not be handled properly (it will we impossible to distinguish which scenario(s) were triggered if we get multiple events with the same scenario name). But probably it is a bad idea anyway to have multiple scenarios with the same name, so I guess it's ok if this is not supported; users should simply rename their scenarios in this case. Do you agree that user convenience is more important here than supporting this rather theoretical edge case?

So as an overview, we have the following approaches:

  • Read-only channel triggered-scenario or scenario-triggered in the bridge that gets updated with names (or alternatively UUIDs) of scenarios that were triggered in the App or by the Controller
  • Writable channel trigger-scenario in the bridge that takes a scenario name (or UUID) as triggers it. Question: what happens if there are multiple scenarios with the same name? Are all triggered, is only the first matching scenario triggered or do we cancel the operation? Alternative approach: use UUID instead of name, it resolves the question.
  • Model scenarios as separate thing types
    • Configuration uses UUID as scenario identifier
    • Can be auto-discovered
    • name channel (string, read-only)
    • last-time-triggered channel (timestamp, read-only, is updated when triggered from the Bosch Smart Home App or the Controller)
    • trigger-channel or trigger-scenario (writable, used to trigger the scenario from OpenHAB)

From the user perspective:

  • Modeling scenarios as things
    • con: is more complex regarding the data/thing/channel model
    • pro: but offers more flexibility (e.g. allows to retrieve the trigger time, not only a simple trigger command)
    • con: requires more thing configuration by the user (but we can help with auto-detection)
    • con: requires multiple items (one for each last-time-triggered channel) to react on different scenarios in rules
    • pro: auto-detection/discovery of scenarios possible
    • con: more implementation effort
  • Providing two channels in the bridge
    • pro: simpler data/thing/channel model
    • con: less flexibility (only strings representing scenario names or UUIDs)
    • pro: no additional thing configuration by users required
    • pro: only one item association required to react on different scenarios in rules
    • con: no auto-detection/discovery of scenarios possible (users have to look up the scenario names or UUIDs)
    • pro: easier to implement
  • UUIDs vs. names: names are more convenient but do not work is some special cases

My idea would be to go for the two bridge channels for now, and use names instead of UUIDs. This approach does not offer the full flexibility, but requires less configuration and has more pros than cons. And it is easier to implement. We can still add scenario things later if required.

What do the others think? Any more pros/cons from your perspective? Would you prefer a "hybrid" approach the mixes bridge and thing channels or would you prefer to keep the channels at the same thing? Do you know if other openHAB bindings offer functionality redundantly (same mechanisms controllable via different channels/things)?

Last question: is it reasonable to have two separate channels for listening to triggered scenarios and for actively triggering scenarios? Or could this even be combined in one channel?

@GerdZanker
Copy link
Contributor

Hello @pat-git023,
I also want to thank you very much for your pull request.

@david-pace prepared a good overview of the approaches and perspectives.

I, as a user, like to trigger a scenario from openhab and I have already unique scenario names and want to use name in openhab.
Because I create all my things and items with the UI, I favor auto-detection possibilities or fool prove channels with simple names, because I add/rename scenarios in bosch smart home and openhab needs to use/work with the scenarios.

My idea would be to go for the two bridge channels for now, and use names instead of UUIDs. This approach does not offer the full flexibility, but requires less configuration and has more pros than cons. And it is easier to implement. We can still add scenario things later if required.

I full agree to this and in general I also think step by step improvements are a good way! With every step we can learn and improve.
Therefore I'm happy with every nice boschshc feature and suggest to start with simple implementation.

@pat-git023
Copy link
Contributor Author

Hi,
in the last days I had lot of work to do and lost a little bit track of the PR.
Is there anything still open I should change?

Best regards,
Patrick

@david-pace
Copy link
Member

Hi @pat-git023, no problem, from my perspective the following points could be enhanced:

  • Add a second channel that allows triggering scenarios
  • Add thing upgrade instructions as documented here that the new channels are automatically added to existing things

Once these two things are available, we will perform final code reviews and tests.

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

Signed-off-by: Patrick Gell <[email protected]>
@pat-git023 pat-git023 force-pushed the feat/boschshc_triggered_scenario branch from 5ce68e8 to c643d58 Compare September 21, 2023 06:32
@pat-git023
Copy link
Contributor Author

Hi,
I tried to add the update instructions but there I am struggling a bit.
Have running empty OH Test system with org.openhab.binding.boschshc-4.1.0.M1.jar in the addons folder.
Bosch Controller is automatically discovered and added as Thing (state is Online)
When I shutdown the system and replace the M1.jar with my locally build one with the update instructions included I always get the following error in the logs:

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) ~[?:?]

My update instructions are:

<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>
<update:update-descriptions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xmlns:update="https://openhab.org/schemas/update-description/v1.0.0"
	xsi:schemaLocation="https://openhab.org/schemas/update-description/v1.0.0 https://openhab.org/schemas/update-description-1.0.0.xsd">

	<thing-type uid="boschshc:shc">
		<instruction-set targetVersion="1">
			<add-channel id="triggered-scenario">
				<type>boschshc:triggered-scenario</type>
			</add-channel>
			<add-channel id="execute-scenario">
				<type>boschshc:execute-scenario</type>
			</add-channel>
		</instruction-set>
	</thing-type>

</update:update-descriptions>

Do you have any idea what I am doing wrong?

Best regards,
Patrick

@lsiepel
Copy link
Contributor

lsiepel commented Sep 21, 2023

Did you also set the thingversion on the thing.xml itself?

<properties>
    <property name="thingTypeVersion">1</property>
  </properties>

@pat-git023
Copy link
Contributor Author

Yes I did

<bridge-type id="shc">
        <label>Smart Home Controller</label>
        <description>The Bosch Smart Home Bridge representing the Bosch Smart Home Controller.</description>
        
        <channels>
	        <channel id="triggered-scenario" typeId="triggered-scenario"/>
	        <channel id="execute-scenario" typeId="execute-scenario"/>
        </channels>
        
        <properties>
	        <property name="thingTypeVersion">1</property>
        </properties>
        
        <config-description-ref uri="thing-type:boschshc:bridge"/>
</bridge-type>

@pat-git023
Copy link
Contributor Author

Hi,
did anyone had time to look over my latest changes?
Is there still something missing?

Best regards,
Patrick

@david-pace
Copy link
Member

Hi @pat-git023, thanks for the reminder. Is the issue with the thing update instructions solved in the mean time or do you still need support? And are all changes requested by @lsiepel implemented? If so, please remove the corresponding review state.

Once both issues are solved I will test and review the code as soon as I find some time.

@pat-git023
Copy link
Contributor Author

@david-pace
I tried to do the rebase but I think there went something wrong.
I see now that a review from a lot of code owners is required.

You have an idea how to solve that?
Should I try to revert the commits?

@david-pace
Copy link
Member

david-pace commented Oct 13, 2023

Hi, I know that this happens quite often, but unfortunately I don't know the solution. Does someome know how to properly rebase and remove the reviewers again?

@jlaur
Copy link
Contributor

jlaur commented Oct 13, 2023

Does someome know how to properly rebase and remove the reviewers again?

Reviewers have to be removed manually as far as I know.

As for resolving the rebase, I can only provide this link:
https://community.openhab.org/t/rebase-your-code-or-how-to-fix-your-git-history-before-requesting-a-pull/129358

and maybe also this as backup: #13570 (comment)

@morph166955
Copy link
Contributor

Have you tried to click the sync fork button? I generally use that to rebase my branches when they need it.

sync

@david-pace
Copy link
Member

david-pace commented Oct 14, 2023

@pat-git023 you could try one of the following strategies if you're using Eclipse:

Strategy 1

  1. In your git fork (https://github.com/pat-git023/openhab-addons/), click Sync Fork, if button is visible, to sync the main branch (as far as I can see, your main branch is up to date so currently you don't have to do that again)
  2. In Eclipse, in the workspace where you have checked out your fork, right click any project of openhab-addons and switch to the main branch (Team | Switch To | main)
  3. Pull the main branch. You should now see the latest openhab-addons commit in the history, like this:
image
  1. Switch to your branch feat/boschshc_triggered_scenario. The HEAD marker should now be on your latest commit.

  2. Show all branches in your History view: image

  3. Scroll down until you see the main branch marked with main and origin/main. On this commit, right-click and choose Rebase HEAD on:

image
  1. If the rebase works without issues (merge conflicts etc.) you can force-push your branch again. Otherwise, abort the rebase and use the following strategy

Strategy 2

  1. Switch to your branch feat/boschshc_triggered_scenario
  2. Copy all the files you have modified during this pull request to some location outside the workspace
  3. Switch back to the main branch.
  4. Make sure your fork is synced on github (Step 1 of the previous instructions)
  5. Pull the main branch
  6. Create a new branch starting from the latest main commit
  7. Copy the files from step 2 back into the workspace
  8. Commit the files and push the the new branch, creating a new pull request
  9. We could then close this pull request

@pat-git023
Copy link
Contributor Author

@david-pace
With the first strategy I got some merge conflicts that's why I switched to a new branch and will create a new PR (#15752 ) and close this one.

Sorry for the inconvenience

Best regards,
Patrick

@pat-git023 pat-git023 closed this Oct 14, 2023
@pat-git023 pat-git023 deleted the feat/boschshc_triggered_scenario branch December 5, 2023 08:29
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.