hap-java / HAP-Java

Java implementation of the HomeKit Accessory Protocol
MIT License
153 stars 82 forks source link

add support for linked services #110

Closed dfrommi closed 3 years ago

dfrommi commented 3 years ago

Some services require linked services to work, for example, the Television service which requires each input to be a linked service. With this change, support to link services is added, though it's not used in any existing service, as they don't need linking. But I verified the functionality using my own Television service, which is not yet production-ready.

This change also centralizes the generation of interface ids. They are now only generated in the registry. We were previously also generating them in the accessory controller, relying on an identical processing sequence.

This is implementing the issue https://github.com/hap-java/HAP-Java/issues/104 and replacing PR https://github.com/hap-java/HAP-Java/pull/105. The author seems to have abandoned the PR and I personally also think that linking accessories rather than services is not the right level.

Pull Request Checklist

Please confirm that you've done the following when opening a new pull request:

yfre commented 3 years ago

@dfrommi thank you for your pull request. this is really most wanted/missing feature. quick question: how to add a linked service to accessory? via getServices().add(..) or missed something? maybe you can share you TV example

dfrommi commented 3 years ago

You have a base service (just made this name up) and a linked service. For example, the Television service is the base service and the input source service would be the linked service.

And you just have to make sure that baseService.getLinkedServices() contains the linked service. So you don't add the linked service directly to the accessory services list, but just add it to the base service.

I don't have a strong opinion on how it should be added to the accessory interface, but you will probably somewhere have code like baseService.getLinkedServices().add(linkedService).

Sharing my TV-accessory doesn't help much. I have a small facade around HAP-Java to better integrate into my Project-Reactor and Kotlin environment. There's not this one small piece of code that I could share as an example.

dfrommi commented 3 years ago

@yfre I decided to make my wrapper public. Here's a sample for a TV service

yfre commented 3 years ago

@dfrommi i was trying to get it working with test client (here in sample branch) or with openhab but without success. probably the getLinkedService needs to be adapted as it always returns a new array and forget all services added via "getLinkedServices().add()".

  @Override
  public List<Service> getLinkedServices() {
    return new ArrayList<>();
  }

the way how you initialise HomkitAccessory in kotlin is not the way how it is typically done in java.

would it be ok for you, if I take your PR, add some additional java API and submit as a new PR?

yfre commented 3 years ago

@dfrommi just checked and indeed, getLinkedServices was the issue. so, only minor changes needed to make it work in java as well. i will propose them as part of PR review here, maybe you could incorporate

dfrommi commented 3 years ago

@yfre Thank you so much for reviewing my changes. I've done the changes you suggested and now the linked services are treated just the same as characteristics on the Service interface and abstract service class. To have it properly aligned, I did not add addLinkedServices to the interface, though. Hope you agree with that.

yfre commented 3 years ago

@yfre Thank you so much for reviewing my changes. I've done the changes you suggested and now the linked services are treated just the same as characteristics on the Service interface and abstract service class. To have it properly aligned, I did not add addLinkedServices to the interface, though. Hope you agree with that.

@dfrommi you are right. linkedService should be done in the same way as characteristics. thank you for your contribution. this feature will enable a number of new services & service combinations.