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

[dirigera] Initial contribution #17719

Open
wants to merge 155 commits into
base: main
Choose a base branch
from
Open

[dirigera] Initial contribution #17719

wants to merge 155 commits into from

Conversation

weymann
Copy link
Contributor

@weymann weymann commented Nov 7, 2024

Binding for IKEA DIRIGERA hub for smart products.

The work ist still in progress but I would like to start the review to find breaking changes right now and not at the end, e.g. thing and channel names.

The binding is published on Marketplace and Community Discussion is active.

@weymann weymann requested a review from a team as a code owner November 7, 2024 21:42
@lsiepel lsiepel added the new binding If someone has started to work on a binding. For a new binding PR. label Nov 8, 2024
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.

Only looked at the thing-structure.xml files as you wanted early feedback.

I did not repeat the same comments for multiple Things. As this seems a well strucutered binding, i think it would be easy for you to project my comments on the other Things.

Extra note, please user system state channels where possible:
https://www.openhab.org/docs/developer/bindings/thing-xml.html#system-state-channel-types

<label>Custom Name</label>
<description>Name given from IKEA home smart</description>
</channel>
<channel id="ota-status" typeId="ota-status">
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between state and status in this case?
Applies to multiple Things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also needed to Google this :)

otaStatus and otaState derived from working repository

@weymann
Copy link
Contributor Author

weymann commented Nov 13, 2024

@lsiepel
Thanks for the fast reply - I'll start now working on this.

@weymann
Copy link
Contributor Author

weymann commented Dec 1, 2024

@lsiepel
All channel adaptions done which shall be now in a stable status.
Binding is running for several community members who reported positive results for several devices. Stability improved with watchdog running.

I don't forsee changes in architecture, class structure and general behavior. Only in specific device handlers if bugfixes are necessary. So I would ask to extend the review on code. Leave logging out of scope, this is still messy and I'm still working on that.

@weymann
Copy link
Contributor Author

weymann commented Dec 11, 2024

@lsiepel @jlaur
What's wrong with the DateTimeType? Didn't changed anything but now it's throwing an error!

First log: String got from device json - ok
Second log: ZonedDateTime after conversion with defined TimeZone +02:00 - ok
Third log: Created DateTimeType

[main] WARN  o.o.b.d.i.handler.scene.SceneHandler - LastTrigger Instant       2024-10-16T00:21:15.977Z
[main] WARN  o.o.b.d.i.handler.scene.SceneHandler - LastTrigger ZonedDateTime 2024-10-16T02:21:15.977+02:00[Europe/Berlin]
[main] WARN  o.o.b.d.i.handler.scene.SceneHandler - LastTrigger DateTimeType  2024-10-16T00:21:15.977+0000
[main] WARN  o.o.b.d.internal.mock.CallbackMock - Update dirigera:scene:test-device:last-trigger : 2024-10-16T00:21:15.977+0000

Code here

@jlaur
Copy link
Contributor

jlaur commented Dec 11, 2024

What's wrong with the DateTimeType?

Hopefully nothing is wrong - see #17719 (comment)

Didn't changed anything but now it's throwing an error!

I assume it's your test throwing an error? Otherwise please be more specific. See #17719 (comment) regarding the test.

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.

So many files to look at, so have to break this up into smaller review rounds. This is a first pass. Besides the comments, want to point to recent changes:
color-temperate channels: #17754 for recent DatetimeType channels: #17725

Edit: lol. nice timing

bundles/pom.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.dirigera/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.dirigera/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.dirigera/doc/follow-sun.png Outdated Show resolved Hide resolved
Comment on lines +133 to +149
**OTA Mappings**

Mappings for `ota-status`

- 0 : Up to date
- 1 : Update available

Mappings for `ota-state`

- 0 : Ready to check
- 1 : Check in progress
- 2 : Ready to download
- 3 : Download in progress
- 4 : Update in progress
- 5 : Update failed
- 6 : Ready to update
- 7 : Check failed
- 8 : Download failed
- 9 : Update complete
- 10 : Battery check failed
Copy link
Contributor

Choose a reason for hiding this comment

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

You could provide the channels with option lists in the xml, this would prevent any mapping, make the UI able to provide presets and would make translation easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already in
For usage in UI these mappings are not needed.
For rules it's useful to have these mappings to check state updates and sending commands.

Comment on lines +193 to +195
Mappings for `startup`

- 0 : Previous
- 1 : On
- 2 : Off
- 3 : Switch
Copy link
Contributor

Choose a reason for hiding this comment

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

See earlier comment, better to have option lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Think you accidently moved this file instead of copying. Needs to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've no idea what's going on here.
Removing exceptions temporary, thenBaseDeviceConfiguration is guilty of moving this file.
image
Created Excpetion classes from scratch, now ApiException is the one marked moving. I'm clueless.

Fact: There's no file moved from anywhere
image

@weymann
Copy link
Contributor Author

weymann commented Dec 11, 2024

So many files to look at, so have to break this up into smaller review rounds. This is a first pass. Besides the comments, want to point to recent changes: color-temperate channels: #17754 for recent DatetimeType channels: #17725

Edit: lol. nice timing

For color temperature #17754 since the beginning it was no option to set kelvin or system:color-temperature-abs directly - this should be fine!
For DateTimeType commit is already in place

@weymann weymann changed the title [WIP] [dirigera] Initial contribution [dirigera] Initial contribution Dec 13, 2024
@weymann weymann requested a review from lsiepel December 14, 2024 16:48
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Signed-off-by: Bernd Weymann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants