core icon indicating copy to clipboard operation
core copied to clipboard

Add async_register_entity_service for linkplay http custom commands

Open daniel-simpson opened this issue 1 year ago • 10 comments

Proposed change

Adds a linkplay.send_command service which allows access to low-level HTTP API commands (such as rebooting a device, enabling/disabling voice prompts, setting WiFi SSID, etc)

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [ ] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

daniel-simpson avatar Sep 13 '24 06:09 daniel-simpson

Hey there @velleman, mind taking a look at this pull request as it has been labeled with an integration (linkplay) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of linkplay can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign linkplay Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Sep 13 '24 06:09 home-assistant[bot]

@Velleman trying to psyche myself up to implement media player join and unjoin services, and thought I'd start with a smaller service implementation. Let me know if you this this would be valuable or not, and I'll sort out the documentation side of things next. TIA!

daniel-simpson avatar Sep 13 '24 06:09 daniel-simpson

@Velleman trying to psyche myself up to implement media player join and unjoin services, and thought I'd start with a smaller service implementation. Let me know if you this this would be valuable or not, and I'll sort out the documentation side of things next. TIA!

I have somewhere a draft branch laying around that adds support, but I haven't got time to get it tested.

silamon avatar Sep 18 '24 12:09 silamon

Rebooting a device should be a button entity, enabling/disabling voice prompt a switch entity. We should not introduce a catch all service for things that can be put into entities

joostlek avatar Sep 18 '24 12:09 joostlek

I'm not sure if all these different commands deserve an entity in cases where this is possible. Is it a common thing to make an button entity whenever the device supports rebooting? Some commands are also more "advanced", whereas I would prefer to have it more hidden than an entity.

silamon avatar Sep 18 '24 13:09 silamon

We can always hide entities by default. Another way would be to create a service specific for it. But I think we have to check that case by case, but we should not create a catch-all

joostlek avatar Sep 18 '24 13:09 joostlek

I'm absolutely happy to add a button for rebooting, but that underlying command also allows a lot of other "stuff" that might be useful to advanced automations but won't be functionality most would use. I'll see if I can dig up the linkplay api spec for some examples.

Giving access to the low level commands also allows people with slightly different player specs (wiim pro VS arylic VS edifier all having slightly different responses to certain commands).

daniel-simpson avatar Sep 19 '24 09:09 daniel-simpson

allows people with slightly different player specs (wiim pro VS arylic VS edifier all having slightly different responses to certain commands).

Then we can still create a service for it and handle that correctly

joostlek avatar Sep 19 '24 09:09 joostlek

Arylic api documentation: https://developer.arylic.com/httpapi/#http-api

Other linkplay api spec (crowd sourced): https://github.com/AndersFluur/LinkPlayApi/blob/master/api.md

Obviously the plan is to implement as many media_player functions as possible into the core entity, but there's a lot in there that might be useful to power users (programmatically scanning and connecting to a certain wifi, or multiroom grouping, or playing notification sounds / from local disk that isn't supported by all players.

Alopogies for not giving enough context in the original PR.

I guess a secondary reason (for me, at least) would be to access functionality that hasn't been explicitly implemented yet: multiroom join/unjoin, etc

daniel-simpson avatar Sep 19 '24 09:09 daniel-simpson

I would personally prefer to make some basic stuff available through entities like proposed above and from as soon as it's more advanced (more specificially the network stuff) I would jump to specific services. The custom integration also had support through a service for this: https://github.com/nagyrobi/home-assistant-custom-components-linkplay/blob/master/custom_components/linkplay/media_player.py#L2403 You may see that some command require specific input.

It can be detected in the library what the model and brand is of the device: If some devices don't support specific commands, or some devices started to implement their own commands, we can check and don't add services or entities for these devices.

Multiroom join/unjoin is part of the mediaplayer entity and should be added there only. The custom integration added specific services because they added grouping/ungrouping before it was implemented in the mediaplayer entity.

silamon avatar Sep 19 '24 09:09 silamon

Closing.

As mentioned above this should really be separate service calls, which won't have much if any code in common with this attempt.

daniel-simpson avatar Sep 20 '24 03:09 daniel-simpson