-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[huesync] Hue Play HDMI Sync Box Binding - Initial contribution #16516
base: main
Are you sure you want to change the base?
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/philips-hdmi-sync-api/111679/39 |
edf4eb7
to
565fc72
Compare
@pgfeller I will start to look at this tomorrow. |
565fc72
to
ba801a2
Compare
@andrewfg Thanks! Take your time - progress will be slow unfortunately; as free time is a rare commodity ... |
566ee5d
to
a42500f
Compare
.../java/org/openhab/binding/huesync/internal/api/dto/device/HueSyncDeteiledDeviceInfoWifi.java
Outdated
Show resolved
Hide resolved
...huesync/src/main/java/org/openhab/binding/huesync/internal/connection/HueSyncConnection.java
Show resolved
Hide resolved
...huesync/src/main/java/org/openhab/binding/huesync/internal/connection/HueSyncConnection.java
Outdated
Show resolved
Hide resolved
...huesync/src/main/java/org/openhab/binding/huesync/internal/connection/HueSyncConnection.java
Outdated
Show resolved
Hide resolved
186bd73
to
0549ac6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed the review. Many relative small changes are required. When all comments are addressed (please do so by thumbs up or writing a comment, not by resolving comments), i'll look over it again and we are ready to merge this.
bundles/org.openhab.binding.jellyfin/src/main/resources/OH-INF/i18n/jellyfin.properties
Outdated
Show resolved
Hide resolved
logMessage = HueSyncLocalizer.getResourceString(message); | ||
} | ||
|
||
logger.error("{}", logMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't log from the exception itself, better to log when it occurs and base it on how it is handles.
try { | ||
return new HueSyncHandler(thing, this.httpClientFactory); | ||
} catch (Exception e) { | ||
// TODO: Implementation ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle the TODO
...rc/main/java/org/openhab/binding/huesync/internal/handler/tasks/HueSyncRegistrationTask.java
Outdated
Show resolved
Hide resolved
...inding.huesync/src/main/java/org/openhab/binding/huesync/internal/i18n/HueSyncLocalizer.java
Outdated
Show resolved
Hide resolved
...inding.huesync/src/main/java/org/openhab/binding/huesync/internal/i18n/HueSyncLocalizer.java
Outdated
Show resolved
Hide resolved
...inding.huesync/src/main/java/org/openhab/binding/huesync/internal/i18n/HueSyncLocalizer.java
Outdated
Show resolved
Hide resolved
...inding.huesync/src/main/java/org/openhab/binding/huesync/internal/i18n/HueSyncLocalizer.java
Outdated
Show resolved
Hide resolved
@lsiepel Thank you for the fast review! I'll work on the findings as soon as possible. I'll split the work into multime commits - as I can sometimes not allocate a lot of time to work on the project. Once I've addressed all feedback I'll send you a message. I'll not mark the comments as resolved, but add a comment (e.g. commit id). |
Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/philips-hdmi-sync-api/111679/52 |
…binding/huesync/internal/handler/tasks/HueSyncRegistrationTask.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/handler/HueSyncHandler.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/connection/HueSyncConnection.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/connection/HueSyncConnection.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/connection/HueSyncDeviceConnection.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/handler/tasks/HueSyncUpdateTask.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
Update names to follow coding convention. Signed-off-by: Patrik Gfeller <[email protected]>
Obsolete image file removed. Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/connection/HueSyncDeviceConnection.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/connection/HueSyncConnection.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/connection/HueSyncDeviceConnection.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/connection/HueSyncDeviceConnection.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
…binding/huesync/internal/discovery/HueSyncDiscoveryParticipant.java Co-authored-by: lsiepel <[email protected]> Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When looking at some resolved comments, i noticed some new and wanted to share them right away.
Besides these i want to point you to the coding guidelines regarding logging:
https://www.openhab.org/docs/developer/guidelines.html#f-logging
|
||
### Registration | ||
|
||
To communicate with the sync box, you need to couple the thing with the hardware (registration). The thing will start this process automatically. To complete the registration you just press the "coupling" button on the sync box for 3 seconds.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each sentence on its own line
To communicate with the sync box, you need to couple the thing with the hardware (registration). The thing will start this process automatically. To complete the registration you just press the "coupling" button on the sync box for 3 seconds.: | |
To communicate with the sync box, you need to couple the thing with the hardware (registration). | |
The thing will start this process automatically. | |
To complete the registration you just press the "coupling" button on the sync box for 3 seconds.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The logging is still no my todo -... but I'll have a look at the guidelines. The markdown mistake I make every time. I wonder if the linter could mark them as error as well - so that you and other reviewers do not need to waste time on such trivialities. Same goes for naming conventions - as every project I'm working on uses different conventions and sometimes I do not work on one of them for a while it is easy to do it wrong (and if I read the coventions every time again, the timeslot is already over ...).
The CI/CD and linter of the project already has a very high quality. I'm not familiar with the stack used - but maybe the investment in some more linter rules would increase productivity and review overhead. Do you know who is the maintainer of the pipeline? Then I could ask if that's easy to achieve - or a bigger project.
Something else the README: I had a look at the Hue binding README as a reference and noticed, that it used some images for visualization. We removed them from the README; but honestly if possible I would like to re-introduce them - as it seems other bindings do so as well and I realy think the documentation looks better without them. Would it be acceptable to re-introduce them?
https://www.openhab.org/addons/bindings/hue/#philips-hue-binding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confectrician did a lot of work on the documentation/linter. I use vscode and it markes all the issue by default (atleast i'm not aware that i added an extension). Maybe the static code analysis can be extended to cehck the thing-structure.xml files for naming convention. If you can make the life easier for contributers and maintainers, your are very welcome!
Regarding the images i prefer that you leave them out.
Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
This binding integrates the Play HDMI Sync Box into openHAB. The integration happens directly through the Hue HDMI Sync Box API.
The binding is using mDNS to discover HDMI Sync devices in the local network. The LED on the Sync Box must be white or red. This indicates that the device is connected to the Network. If the LED is blinking blue, you need to setup the device using the official Hue Sync App.
Community discussion thread: Philips HDMI Sync API
Closes #10218
Credits
Tasks
AuthenticationStore
✅on
/off
mode
ℹ️: The 1st version will not support all possible functions - as the basic setup can be done via the official App ➡️ - I'll focus on status information and commands that are most relevant for automation in this PR.