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

Simplify UI for devices that only have a power state #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asherkin
Copy link

Only add the TelevisionSpeaker service if we have a device with mute state implemented (which is required for TelevisionSpeaker), which simplifies the UI displayed in HomeKit for things that only have a power state.

Additionally, the empty array checks in the Homebridge callbacks weren't working as it turns out [] is truthy - which was causing Homebridge to complain that the callback was never called. This check shouldn't actually be needed any more with the first change, but I fixed that before realising the service could be removed completely.

I ran into this troubleshooting a warning from Homebridge:

[homebridge-television-universal-control] This plugin slows down Homebridge. The read handler for the characteristic 'Mute' didn't respond at all!. Please check that you properly call the callback! See https://git.io/JtMGR for more info.

with a very basic config for my projector:

{
  "groups": [
    {
      "name": "Projector",
      "devices": {
        "serial": [
          {
            "name": "Projector",
            "path": "/dev/ttyUSB0",
            "requestTimeout": 150,
            "delimiter": "\u0003",
            "getStatus": {
              "power": {
                "command": "\u0002QPW\u0003",
                "onResponse": "\u0002001",
                "offResponse": "\u0002000"
              }
            }
          }
        ]
      },
      "power": {
        "on": {
          "commands": [
            {
              "serial": [
                {
                  "commands": ["\u0002PON\u0003"],
                  "device": "Projector"
                }
              ]
            }
          ]
        },
        "off": {
          "commands": [
            {
              "serial": [
                {
                  "commands": ["\u0002POF\u0003"],
                  "device": "Projector"
                }
              ]
            }
          ]
        }
      },
      "inputs": [],
      "manufacturer": "Panasonic",
      "model": "AT6000E"
    }
  ],
  "platform": "TelevisionUniversalControl"
}

Only add the TelevisionSpeaker service if we have a device with mute state implemented (which is required for TelevisionSpeaker), which simplifies the UI displayed in HomeKit for things that only have a power state.

Additionally, the empty array checks in the Homebridge callbacks weren't working as it turns out `[]` is truthy - which was causing Homebridge to complain that the callback was never called. This check shouldn't actually be needed any more with the first change, but I fixed that before realising the service could be removed completely.
@asherkin
Copy link
Author

asherkin commented Mar 5, 2023

@pkmnct it looks like you were prodding some of this in #17, any chance this could get reviewed / merged?

@pkmnct
Copy link
Owner

pkmnct commented Mar 23, 2023

@pkmnct it looks like you were prodding some of this in #17, any chance this could get reviewed / merged?

Hey! Sorry for the delay, I commented some suggestions but this overall is looking sensible!

@pkmnct
Copy link
Owner

pkmnct commented Mar 23, 2023

This fixes #8

@asherkin
Copy link
Author

@pkmnct it looks like you were prodding some of this in #17, any chance this could get reviewed / merged?

Hey! Sorry for the delay, I commented some suggestions but this overall is looking sensible!

Heya - I can't see them I'm afraid!

If they're in a review, double-check the whole review was submitted at the top of the page 😄

);

// Mute is a required characteristic for TelevisionSpeaker, if we don't have any mutable devices don't register the service.
if (this.devicesToQueryForMute?.length) {
Copy link
Owner

Choose a reason for hiding this comment

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

We should instead check if the volume and/or mute command(s) are configured. It is possible for the volume/mute commands to be configured via only LIRC, at which point there is no devicesToQueryForMute since LIRC can't be queried. The state falls back to this.states.mute in that case.


// register inputs
this.device.inputs.forEach(
Copy link
Owner

Choose a reason for hiding this comment

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

We should update the type on inputs to be optional

@@ -136,7 +136,7 @@ interface UniversalControlDevice {
*/
export class Television {
private tvService: Service;
private tvSpeakerService: Service;
private tvSpeakerService: Service | undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of | undefined, use tvSpeakerService?: Service to be consistent with other possibly undefined type definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants