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

[deconz] Add Pairing/Scene actions, new devices and improve code #14622

Merged
merged 10 commits into from Mar 18, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Mar 17, 2023

Closes #14124
Fixes #13368
Fixes #13132
Fixes #12703
Fixes #12082
Fixes #11031
Closes #10997
Fixes #10858
Fixes #8260
Fixes #12228

This includes all changes from the SmartHome/J version of the binding and some more improvements.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
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 a lot for this massive collection of refactoring and other improvements. I have provided some initial comments after glancing over first half of the changed files.

@J-N-K
Copy link
Member Author

J-N-K commented Mar 17, 2023

This is NOT ready for review. I know that there are still reverts that need to be checked but it's much easier for me to self-review on GitHub.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K changed the title Deconz [deconz] Add Pairing/Scene actions, new devices and improve code Mar 17, 2023
@J-N-K J-N-K added the enhancement An enhancement or new feature for an existing add-on label Mar 17, 2023
@J-N-K
Copy link
Member Author

J-N-K commented Mar 17, 2023

There is one thing that I could either include here or in a later PR: the absolute color temperature channel is plain Number even though it is using K. We could change to the system-channeltype.

@J-N-K J-N-K marked this pull request as ready for review March 17, 2023 19:55
@J-N-K J-N-K requested a review from a team as a code owner March 17, 2023 19:55
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.

A few additional comments.

@@ -28,19 +32,28 @@
<item-type>Switch</item-type>
<label>All On</label>
<description>"On" if all lights in this group are "On", otherwise "Off".</description>
<tags>
<tag>Lighting</tag>
Copy link
Contributor

Choose a reason for hiding this comment

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

@lolodomo - is this a correct single tag for a Switch? I can't determine this from the rules established in #12262, and no other bindings have it.

@jlaur
Copy link
Contributor

jlaur commented Mar 18, 2023

There is one thing that I could either include here or in a later PR: the absolute color temperature channel is plain Number even though it is using K. We could change to the system-channeltype.

I agree. Up to you if you want to include it in this PR - it would certainly fit in. 🙂

@J-N-K
Copy link
Member Author

J-N-K commented Mar 18, 2023

After thinking about it I prefer another PR for that. The reason is that all of the changes here are "safe", while changing the channel type might cause issues and it's easier to debug if these changes are separated.

Signed-off-by: Jan N. Klug <github@klug.nrw>

<representation-property>uid</representation-property>

<config-description-ref uri="thing-type:deconz:sensor"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this new thing to the textual configuration example? This would particularly useful since it has channel configuration for which the syntax might be unfamiliar or hard to remember.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Someone" can sure do it. Whoever wants to use figure out how to do it can do so, but I won't waste my time to make it easier for people who deliberately chose the hard way. Use discovery and managed things and you'll have no issues.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@jlaur
Copy link
Contributor

jlaur commented Mar 18, 2023

@J-N-K - if you see any issues from the filter "is:issue is:open in:title deconz" that you can quickly assess will be fixed by this PR, please link them. I think the PR is ready for being merged, and we can get back to the tag question with @lolodomo later.

@J-N-K
Copy link
Member Author

J-N-K commented Mar 18, 2023

I'm not sure about #12228, that should be checked.

@jlaur
Copy link
Contributor

jlaur commented Mar 18, 2023

I'm not sure about #12228, that should be checked.

I have just tested it, it's fixed also. 👍 I have added it to the list in the PR description.

@J-N-K
Copy link
Member Author

J-N-K commented Mar 18, 2023

I guess that makes deconz drop out of the "top 10 bindings with open issues" list :-)

Signed-off-by: Jan N. Klug <github@klug.nrw>
@jlaur
Copy link
Contributor

jlaur commented Mar 18, 2023

I guess that makes deconz drop out of the "top 10 bindings with open issues" list :-)

Certainly! 😄 Thanks for all the work done over the years and now in this backport.

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 jlaur merged commit ee1de11 into openhab:main Mar 18, 2023
2 checks passed
@jlaur jlaur added this to the 4.0 milestone Mar 18, 2023
@J-N-K J-N-K deleted the deconz branch March 18, 2023 16:27
@Redsandro
Copy link

I haven't tested it yet but this may also close #13740.

<thing-type id="airqualitysensor">
<supported-bridge-type-refs>
<bridge-type-ref id="deconz"/>
</supported-bridge-type-refs>
<label>Air quality Sensor</label>
<description>An air quality sensor</description>
<label>Carbon-monoxide Sensor</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

@J-N-K - late finding: Was this change intended? This is the same label as for thing type carbonmonoxidesensor.

Copy link
Member Author

@J-N-K J-N-K Mar 19, 2023

Choose a reason for hiding this comment

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

Yes, when the description does not provide additional information it can be omitted. I also remove the article because it is not needed.

Edit: Uh. I See what you mean. I'll fix that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #14636

renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
…nhab#14622)

* port changes
* update instructions
* Incorporate review comments from openhab#14134
* new improvements (mostly Java 17 changes)
* further improvements

Signed-off-by: Jan N. Klug <github@klug.nrw>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
…nhab#14622)

* port changes
* update instructions
* Incorporate review comments from openhab#14134
* new improvements (mostly Java 17 changes)
* further improvements

Signed-off-by: Jan N. Klug <github@klug.nrw>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment