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.

Sign up for GitHub

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

Jump to bottom

[huesync] Hue Play HDMI Sync Box Binding - Initial contribution #16516

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

[huesync] Hue Play HDMI Sync Box Binding - Initial contribution #16516

wants to merge 128 commits into from

Conversation

pgfeller
Copy link
Contributor

@pgfeller pgfeller commented Mar 12, 2024 •
edited
Loading

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

  • Marco Kawon: The code is based on his work - but the binding is a re-implementation. The refactoring is done to add functionality step-by-step, and that I understand the code and in the hope to simplify it a bit to improve maintainability (as a learning exercise for me).
  • @lsiepel for major support during the implementation and quality assurance by performing detailed code review
  • Andrew Fiddian-Green: Code review and technical support/advice
  • April_Wexler (Kai): Testing & Documentation

Tasks

  • ☠️ Binding skeleton created for org.openhab.binding.huesync
  • 🔎 skeleton mDNS discovery implemented
  • communication infrastructure
    • 🔎 mDNS device discovery - use API to get device information
    • 🔐 SSL Handshake & 🔎 Discovery
    • 🤝 Device registration
    • 👋 Device unregistration (remove thing)
    • ⛔ solve timing problems during shutdown
    • 📜 improve handling of configuration update(s)
  • CI/CD & Code maintainability
    • ⛔ Fix CI validation errors
    • ⚠️ Fix PR CI validation warnings
    • 🗨️ check feasibility of re-using resources for log & exception messages ✅
    • 🗨️ check feasibility of using AuthenticationStore ✅
  • Implementation
    • 🔌 device status polling
      • 🐞Solve handling of invalid tokens (automatic re-registration)
      • 🐞Investigate and resolve "Bad Request" warning
      • 🐞 Investigate and resolve partial device information during "poll"
    • 📦 Create device state DTO
      • Device status
      • HDMI input/output status
      • Commands
    • Channels
      • 📜 update/revert icons to use basic icon set
      • Create .channel xml declaration/poc for device information
      • Create .channel xml declaration prototype for HDMI input/output status
      • ➕ HDMI input/output channels (read only)
      • ➕ "execution" API infrastructure added ...
        • ➕ Add support to switch ambilight on / off
        • ➕ Add support to select mode
        • ➕ Add support to select HDMI source added
        • ➕ Add support for HDMI active channel
        • ➕ Add support to adjust brightness
        • ➕ Add support for intensity channel
    • 📜 Documentation
    • 🔎 Code & static code analysis
      • 🔎 Review
      • ⚙️ CI static code analysis (warnings)

ℹ️: 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.

@pgfeller pgfeller added new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged labels Mar 12, 2024
@pgfeller pgfeller requested a review from andrewfg March 12, 2024 23:12
@pgfeller pgfeller self-assigned this Mar 12, 2024
@pgfeller pgfeller requested a review from a team as a code owner March 12, 2024 23:12
@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/philips-hdmi-sync-api/111679/39

@pgfeller pgfeller force-pushed the 10218-hue-add-hue-sync-box branch 2 times, most recently from edf4eb7 to 565fc72 Compare March 12, 2024 23:41
@andrewfg
Copy link
Contributor

@pgfeller I will start to look at this tomorrow.

@pgfeller pgfeller removed the request for review from maniac103 March 12, 2024 23:58
@pgfeller
Copy link
Contributor Author

@andrewfg Thanks! Take your time - progress will be slow unfortunately; as free time is a rare commodity ...

@pgfeller pgfeller force-pushed the 10218-hue-add-hue-sync-box branch 3 times, most recently from 566ee5d to a42500f Compare March 16, 2024 22:52
@jlaur jlaur added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 17, 2024
andrewfg
andrewfg reviewed Mar 18, 2024
View reviewed changes
CODEOWNERS Outdated Show resolved Hide resolved
@pgfeller pgfeller force-pushed the 10218-hue-add-hue-sync-box branch 2 times, most recently from 186bd73 to 0549ac6 Compare May 7, 2024 19:55
@pgfeller

This comment was marked as outdated.

Sign in to view
@andrewfg

This comment was marked as outdated.

Sign in to view
@pgfeller

This comment was marked as outdated.

Sign in to view
@pgfeller

This comment was marked as outdated.

Sign in to view
@pgfeller
🔧 post merge clean up (fix)
2ad011b 
Signed-off-by: Patrik Gfeller <[email protected]>
lsiepel
lsiepel requested changes Nov 10, 2024
View reviewed changes
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.

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

bundles/org.openhab.binding.huesync/README.md Outdated Show resolved Hide resolved
lsiepel
lsiepel requested changes Nov 20, 2024
View reviewed changes
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.

Found these suggestions.

bundles/org.openhab.binding.huesync/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.huesync/README.md Outdated Show resolved Hide resolved
...sync/src/main/java/org/openhab/binding/huesync/internal/handler/tasks/HueSyncUpdateTask.java Outdated Show resolved Hide resolved
...rc/main/java/org/openhab/binding/huesync/internal/handler/tasks/HueSyncRegistrationTask.java Outdated Show resolved Hide resolved
...rc/main/java/org/openhab/binding/huesync/internal/discovery/HueSyncDiscoveryParticipant.java Outdated Show resolved Hide resolved
...c/src/main/java/org/openhab/binding/huesync/internal/connection/HueSyncDeviceConnection.java Outdated Show resolved Hide resolved
...c/src/main/java/org/openhab/binding/huesync/internal/connection/HueSyncDeviceConnection.java

if (command instanceof QuantityType) {
value = Integer.toString(((QuantityType<?>) command).intValue());
} else if (command instanceof OnOffType) {
Copy link
Contributor

@lsiepel lsiepel Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is marked as resolved, but it is not (i now notice this comment is on the wrong line, not sure what happend, but it applies to line 99-100)
My example might have missed the generic/wildcard operator, if needed you can add it.

pgfeller and others added 6 commits November 20, 2024 22:57
@pgfeller @lsiepel
Update bundles/org.openhab.binding.huesync/README.md
818f6f4 
Co-authored-by: lsiepel <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller @lsiepel
Update bundles/org.openhab.binding.huesync/src/main/java/org/openhab/…
4befab5 
…binding/huesync/internal/handler/tasks/HueSyncUpdateTask.java

Co-authored-by: lsiepel <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller @lsiepel
Update bundles/org.openhab.binding.huesync/src/main/java/org/openhab/…
7a788af 
…binding/huesync/internal/handler/tasks/HueSyncRegistrationTask.java

Co-authored-by: lsiepel <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller @lsiepel
Update bundles/org.openhab.binding.huesync/src/main/java/org/openhab/…
1293f64 
…binding/huesync/internal/discovery/HueSyncDiscoveryParticipant.java

Co-authored-by: lsiepel <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller @lsiepel
Update bundles/org.openhab.binding.huesync/src/main/java/org/openhab/…
dc699a1 
…binding/huesync/internal/connection/HueSyncDeviceConnection.java

Co-authored-by: lsiepel <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller
refactor(project): 🔎 Resolve code review finding
d49584d 
Signed-off-by: Patrik Gfeller <[email protected]>
@lsiepel
Copy link
Contributor

lsiepel commented Nov 21, 2024

Four comments are left open, otherwise LGTM.
#16516 (comment)
#16516 (comment)
#16516 (comment)
#16516 (comment)

@pgfeller
docs(readme): 🔎 Resolve code review finding
546c703 
Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller
refactor(project): 🔎 Resolve code review finding
aeb8f76 
Seems to be a false positive reported by the IDE - seems to build ok - let's check with CI ...

Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller
refactor(project): 🔎 Resolve code review finding
649d765 
Signed-off-by: Patrik Gfeller <[email protected]>
@lsiepel
Copy link
Contributor

lsiepel commented Nov 22, 2024

The last few commits fixed some comment,s but introduced some new SAT warnings regarding naming conventions.

@pgfeller
Copy link
Contributor Author

The last few commits fixed some comment,s but introduced some new SAT warnings regarding naming conventions.

Unfortunately I was not yet able to configure my locale IDE to use the correct format rules ... (following up on that & once I understand it, I'll update the docu how to setup vscode).

Once I've addressed all your findings I'll consult the static code analysis report of the CI (https://ci.openhab.org/job/PR-openHAB-Addons/24380/Static_20Code_20Analysis_20Report/) and solve the naming convention issues and all of the warnings that I can.

I propose to ignore the violations as long as they are in the report. As those I'll address once logic and general style is ok. If all this is green I'll need a couple of days to test the binding .jar to ensure that all still works as expected. Let me know once you think I can concentrate on the static code analysis report.

When you're happy, the report is green and the binding performed stable for a few days we can probably merge 🙂. If there's a milestone planed we can also skip the real life test and do that in parallel with the milestone users (to get more use cases covered).

@lsiepel
Copy link
Contributor

lsiepel commented Nov 22, 2024

Two remaining points: @SuppressWarnings("null") / compiler warnings and SAT warnings

@pgfeller
Copy link
Contributor Author

Hi @lsiepel,

I hope you're doing fine! We've construction workers in the house - thus I can not test the changes myself (2+ weeks unfortunately). But the changes should not affect functionality. Unfortunetly I did not think about the testing and the device is now under a pile of things that is sealed with plasic (to protect it from the dust).

I've addressed the static code analysis findings - unfortunately the last commit CI did fail (I think that's a known issue). So I'm not sure if I missed a tab/space indentation ... do you have a local configuration that can do the analysis locally in the IDE (I was not able to configure vscode to match the pipeline - as you use it as well you might have a working config).

➡️ Can you close the comments that are properly addressed and let me know if there's something left that prevents the merge? Thank you for the time and help; I'm aware of the other contributions you do - it's highly appreciated that you invest so much of your time into the project (I'm more selfish: if something is missing that I need, I try to contribute - next Jellyfin is on my plate once this thingy is completed ...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsiepel lsiepel lsiepel requested changes

@wborn wborn wborn left review comments

@andrewfg andrewfg andrewfg left review comments

@GiviMAD GiviMAD Awaiting requested review from GiviMAD

Requested changes must be addressed to merge this pull request.

Assignees

@pgfeller pgfeller

Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
Release Planning
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[hue] Add Hue Sync Box
7 participants
@pgfeller @openhab-bot @andrewfg @wborn @lsiepel @florian-h05 @jlaur