HDMI CEC - support for devices and commands #4781

konikvranik · 2016-12-06T15:26:23Z

Description:

This pullrequest extends functionality of previous hdmi_cec platform so now detected HDMI devices are handled as switch entites, reports name, vendor, address and state, could be turned on/off.
Also adds services to pass arbitraty CEC command and control volume.

Pull request in home-assistant.github.io with documentation: home-assistant/home-assistant.io#1642

Example entry for configuration.yaml (if applicable):

hdmi_cec:

Checklist:

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

Documentation added/updated in home-assistant.github.io

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

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

mention-bot · 2016-12-06T15:26:25Z

@konikvranik, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @happyleavesaoc and @balloob to be potential reviewers.

houndci-bot · 2016-12-06T15:26:32Z

homeassistant/components/switch/hdmi_cec.py

+        _LOGGER.info("Creating %s switch %d", DOMAIN, logical)
+        self._state = False
+        CecDevice.__init__(self, hass, cec_client, logical)
+        self.entity_id = "%s.%s_%s" % (DOMAIN, 'hdmi', hex(self._logical_address)[2:])


line too long (86 > 79 characters)

houndci-bot · 2016-12-06T15:26:32Z

homeassistant/components/switch/hdmi_cec.py

+    if ATTR_NEW in discovery_info:
+        _LOGGER.info("Setting up devices %s", discovery_info[ATTR_NEW])
+        add_devices(CecSwitch(hass, CEC_CLIENT, device) for device in
+                    filter(lambda x: x not in DEVICE_PRESENCE or not DEVICE_PRESENCE[x], discovery_info[ATTR_NEW]))


line too long (115 > 79 characters)

Please don't use filter. It does not give readable code.

houndci-bot · 2016-12-06T15:26:33Z

homeassistant/components/switch/hdmi_cec.py

+"""
+import logging
+
+from homeassistant.components.hdmi_cec import CecDevice, CEC_CLIENT, ATTR_NEW, DEVICE_PRESENCE


line too long (94 > 79 characters)

houndci-bot · 2016-12-06T15:26:33Z

homeassistant/components/media_player/hdmi_cec.py

+"""
+import logging
+
+from homeassistant.components.hdmi_cec import CecDevice, CEC_DEVICES, CEC_CLIENT


line too long (80 > 79 characters)

houndci-bot · 2016-12-06T15:26:33Z

homeassistant/components/hdmi_cec.py

+    def cec_key_press_callback(self, key, duration):
+        """key press callback"""
+        _LOGGER.info("[key pressed] " + str(key))
+        self.hass.bus.fire(EVENT_CEC_KEYPRESS_RECEIVED, {ATTR_KEY: key, ATTR_DUR: duration})


line too long (92 > 79 characters)

houndci-bot · 2016-12-06T15:26:34Z

homeassistant/components/hdmi_cec.py

+        self.lib_cec = cec.ICECAdapter.Create(self.cecconfig)
+        # print libCEC version and compilation information
+        _LOGGER.info("libCEC version " + self.lib_cec.VersionToString(
+            self.cecconfig.serverVersion) + " loaded: " + self.lib_cec.GetLibInfo())


line too long (84 > 79 characters)

houndci-bot · 2016-12-06T15:26:34Z

homeassistant/components/hdmi_cec.py

+    def available(self):
+        return self._available
+
+    @property


redefinition of unused 'device_state_attributes' from line 342

houndci-bot · 2016-12-06T15:26:34Z

homeassistant/components/hdmi_cec.py

+
+    @property
+    def icon(self):
+        return icon_by_type(self._cec_type_id) if self._icon is None else self._icon


line too long (84 > 79 characters)

houndci-bot · 2016-12-06T15:26:34Z

homeassistant/components/hdmi_cec.py

+
+    @property
+    def vendor_name(self):
+        return VENDORS[self._vendor_id] if self._vendor_id in VENDORS else VENDORS[0]


line too long (85 > 79 characters)

houndci-bot · 2016-12-06T15:26:34Z

homeassistant/components/hdmi_cec.py

+                                                           and self.vendor_name != 'Unknown' \
+            else "%s %d" % (DEVICE_TYPE_NAMES[self._cec_type_id], self._logical_address) if self._name is None \
+            else "%s %d (%s)" % (
+            DEVICE_TYPE_NAMES[self._cec_type_id], self._logical_address, self._name)


line too long (84 > 79 characters)

houndci-bot · 2016-12-06T15:26:35Z

homeassistant/components/hdmi_cec.py

+    def name(self):
+        """Return the name of the device."""
+        return "%s %s" % (self.vendor_name, self._name) if self._name is not None and self.vendor_name is not None \
+                                                           and self.vendor_name != 'Unknown' \


continuation line over-indented for visual indent

houndci-bot · 2016-12-06T15:26:35Z

homeassistant/components/hdmi_cec.py

+                self._physical_address = "%d.%d.%d.%d" % (
+                    cmd_chain[0] / 0x10, cmd_chain[0] % 0x10, cmd_chain[1] / 0x10, cmd_chain[1] % 0x10)
+                self._cec_type_id = cmd_chain[2]
+                _LOGGER.info("Got physical address and type for device %x -> %x, %s, %s",


line too long (89 > 79 characters)

houndci-bot · 2016-12-06T15:26:35Z

homeassistant/components/hdmi_cec.py

+                _LOGGER.info("Got vendor for device %x -> %x, %s: %s", src, dst, self.vendor_id, self.vendor_name)
+            elif cmd == 0x84:
+                self._physical_address = "%d.%d.%d.%d" % (
+                    cmd_chain[0] / 0x10, cmd_chain[0] % 0x10, cmd_chain[1] / 0x10, cmd_chain[1] % 0x10)


line too long (103 > 79 characters)

houndci-bot · 2016-12-06T15:26:35Z

homeassistant/components/hdmi_cec.py

+                for i in cmd_chain:
+                    self._vendor_id *= 0x100
+                    self._vendor_id += i
+                _LOGGER.info("Got vendor for device %x -> %x, %s: %s", src, dst, self.vendor_id, self.vendor_name)


line too long (114 > 79 characters)

houndci-bot · 2016-12-06T15:26:35Z

homeassistant/components/hdmi_cec.py

+                self._name = ''
+                for c in cmd_chain:
+                    self._name += chr(c)
+                _LOGGER.info("Got name for device %x -> %x, %s", src, dst, self._name)


line too long (86 > 79 characters)

houndci-bot · 2016-12-06T15:26:36Z

homeassistant/components/hdmi_cec.py

+                    self._state = STATE_OFF
+                else:
+                    self._state = STATE_UNKNOWN
+                _LOGGER.info("Got status for device %x -> %x, %s", src, dst, self._state)


line too long (89 > 79 characters)

balloob

A lot of changes needed. Biggest one is that all the CEC protocol/vendor specific information will have to be extracted into a third party lib.

balloob · 2016-12-07T05:10:19Z

homeassistant/components/hdmi_cec.py

+    _LOGGER.debug("CEC setup")
+    config = base_config.get(DOMAIN)
+
+    global CEC_CLIENT


Don't use a global but use an entry in the dictionary hass.data instead.

balloob · 2016-12-07T05:11:00Z

homeassistant/components/hdmi_cec.py

+        self._available = False
+        self._hidden = False
+        self._name = None
+        DEVICE_PRESENCE[logical] = True


Please use hass.data

balloob · 2016-12-07T05:14:22Z

homeassistant/components/hdmi_cec.py


-MAX_DEPTH = 4
+VENDORS = {0x000039: 'Toshiba',


There is too much protocol specific logic in this component. This should be moved to a third party lib on which Home Assistant can depend.

balloob · 2016-12-07T05:15:58Z

homeassistant/components/hdmi_cec.py

+            dev_type = 'switch'
+
+            while True:
+                new_devices = filter(lambda x: CEC_CLIENT.lib_cec.PollDevice(x),


Please rewrite this using for loops to keep this code understandable.

balloob · 2016-12-07T05:16:43Z

homeassistant/components/hdmi_cec.py

+    global CEC_CLIENT
+    CEC_CLIENT = CecClient(hass)
+    exclude = config.get(CONF_EXCLUDE)
+    stop = threading.Event()

    def _start_cec(event):


You cannot have an event handler that never returns, you are then stealing a thread from HASS. If you want to run in a thread, start your own.

balloob · 2016-12-07T05:29:12Z

homeassistant/components/switch/hdmi_cec.py

+
+    def turn_on(self, **kwargs) -> None:
+        """Turn device on."""
+        _LOGGER.info("***************** turn_on *****************")


Please remove these logging

balloob · 2016-12-07T05:29:26Z

homeassistant/components/switch/hdmi_cec.py

+        _LOGGER.info("***************** turn_on *****************")
+        self.cec_client.lib_cec.PowerOnDevices(self._logical_address)
+        self._state = STATE_ON
+        self.async_update()


You can't call async_update from outside the event loop

balloob · 2016-12-07T05:29:56Z

homeassistant/components/switch/hdmi_cec.py

+
+    def turn_off(self, **kwargs) -> None:
+        """Turn device off."""
+        _LOGGER.info("***************** turn_off *****************")


Remove comment

balloob · 2016-12-07T05:30:07Z

homeassistant/components/switch/hdmi_cec.py

+        _LOGGER.info("***************** turn_off *****************")
+        self.cec_client.lib_cec.StandbyDevices(self._logical_address)
+        self._state = STATE_STANDBY
+        self.async_update()


No async_update from outside event loop

balloob · 2016-12-07T05:30:20Z

homeassistant/components/switch/hdmi_cec.py

+        self.async_update()
+        self.schedule_update_ha_state()
+
+    def __init__(self, hass, cec_client, logical):


Please put init methods on the top of a class definition.

houndci-bot · 2016-12-08T15:05:36Z

homeassistant/components/hdmi_cec.py

+    def available(self):
+        return self._available
+
+    @property


redefinition of unused 'device_state_attributes' from line 339

houndci-bot · 2016-12-19T13:03:57Z

homeassistant/components/hdmi_cec.py

+        return "%s %s" % (
+            self.vendor_name, self._device.osd_name) \
+            if self._device.osd_name is not None and \
+               self.vendor_name is not None and self.vendor_name != 'Unknown' \


continuation line over-indented for visual indent

konikvranik · 2016-12-19T13:07:17Z

As you asked, I moved CEC specific stuff into separate library https://github.com/konikvranik/pycec

rytilahti · 2016-12-19T21:10:36Z

Thanks a lot for your work, I'm eagerly waiting for this to get merged into dev. It'd be nice if you'd merge related commits (even for this published PR, if it's okay for HASS developers) or at least for the future ones. :)

konikvranik · 2016-12-19T21:23:05Z

@rytilahti Thank you. I created PR to master. So should I create PR into dev branch?

kellerza · 2016-12-20T07:36:16Z

We can squash before we merge, but you should really be running local tests! Try the shortcut:

$ script/lint --changed

If you add new files without tests, you should add them to coveragerc

kellerza · 2016-12-20T07:38:50Z

The PR is against homeassistant:dev so no worries. You can have it in any branch in your repo (but makes your life easier if you work in a new branch in your repo)

konikvranik · 2016-12-20T09:37:24Z

@kellerza So I added also switch/hdmi_cec into .coveragerc.

$ script/lint --changed

Now passing without error, but I have had to modify it to check python files only. It failed on "syntax error" in services.yaml.

I also run whole tox lint and also passed.

Only I suspect to miss now is to slightly update documentation on home-assistant.github.io

kellerza

I have not looked at your custom library, but you should make sure that the component you created can function (and most importantly that you can test it!) outside of HASS

kellerza · 2016-12-20T09:54:43Z

homeassistant/components/hdmi_cec.py


-CONFIG_SCHEMA = vol.Schema({


You should always have a CONFIG_SCHEMA in the component to validate your config.

CONFIG_SCHEMA allows extra keys

kellerza · 2016-12-20T09:54:50Z

.coveragerc

@@ -172,6 +172,7 @@ omit =
    homeassistant/components/feedreader.py
    homeassistant/components/foursquare.py
    homeassistant/components/hdmi_cec.py
+    homeassistant/components/switch/hdmi_cec.py


Will you please sort 😄

kellerza · 2016-12-20T09:56:30Z

homeassistant/components/hdmi_cec.py

+        vol.Optional(CMD_UNMUTE): None,
+        vol.Optional(CMD_MUTE_TOGGLE): None
+    })
+}, extra=vol.REMOVE_EXTRA)


For services SCHEMAS you probably want to fail on additional keys?
Not sure if you really need the DOMAIN

kellerza · 2016-12-20T09:57:01Z

homeassistant/components/hdmi_cec.py

+    DOMAIN: vol.Schema({
+        vol.Optional(ATTR_DST): vol.Coerce(int),
+    })
+}, extra=vol.REMOVE_EXTRA)


Same as the Volume schema

kellerza · 2016-12-20T09:58:48Z

homeassistant/components/hdmi_cec.py

+    def _new_device(device):
+        """Called when new device is detected by HDMI network."""
+        discovery.load_platform(hass, "switch", DOMAIN,
+                                discovered={ATTR_NEW: [device]},


Anything sent over the hass bus, like device must be serializable (i.e. a str/int/dict). It does not seem so from here

So should I put device into hasss.data with my unique key and then retrieve it by this key which will be send over load_platform?

Yes, you can make your DOMAIN part of the key, then you know it should be unique

kellerza · 2016-12-20T10:00:15Z

homeassistant/components/hdmi_cec.py

+        hass.services.register(DOMAIN, SERVICE_VOLUME, _volume,
+                               descriptions[SERVICE_VOLUME])
+        hass.services.register(DOMAIN, SERVICE_UPDATE_DEVICES, _update,
+                               descriptions[SERVICE_UPDATE_DEVICES])


You should add the schemas you defined when registering these services

kellerza · 2016-12-20T10:03:08Z

homeassistant/components/hdmi_cec.py

+        self._state = STATE_UNKNOWN
+        self._logical_address = logical
+        self.entity_id = "%s.%d" % (DOMAIN, self._logical_address)
+        device.set_update_callback(self._update)


This also indicates the data you sent over the buss is not serializable.

Rather send non-serializable data in the hass.data dictionary.

kellerza · 2016-12-20T10:05:19Z

script/lint

@@ -4,7 +4,7 @@
 # performs roughly what this test did in the past.

 if [ "$1" = "--changed" ]; then
-  export files="`git diff upstream/dev --name-only | grep -v requirements_all.txt`"
+  export files="`git diff upstream/dev --name-only | grep -e '\.py$'`"


👍 I like this, but do you mind doing it in a separate PR?

OK, in #5018

kellerza · 2016-12-20T10:06:54Z

homeassistant/components/hdmi_cec.py

+    def _volume(call):
+        """Increase/decrease volume and mute/unmute system."""
+        for cmd, att in call.data.items():
+            att = int(att)


Once you have the SCHEMAs validating your input, this type of coercing variables should not be needed

* newer version of pyCEC * updated services.yaml * fixed lint scrpt to operate only on python files * pycec version up * update services * no coverage report * exclude non python files from lint * lint only on python files

* reordered * sending nonserialized data through hass.data * code formatting * code formatting * import order

* newer version of pyCEC * updated services.yaml * fixed lint scrpt to operate only on python files * pycec version up * update services * no coverage report * exclude non python files from lint * lint only on python files * reordered * sending nonserialized data through hass.data * import order * fixed object handling * code formatting

* services: power_on standby active_source

* newer version of pyCEC

* getting device name from config

* correct call on shutdown

* Preparation for merge to upstream (#5) * newer version of pyCEC * updated services.yaml * reordered * sending nonserialized data through hass.data * services: power_on standby active_source * code formatting * getting device name from config * correct call on shutdown

* newer version of pyCEC * updated services.yaml * sending nonserialized data through hass.data * services: ** power_on ** standby ** active_source * getting device name from config * correct call on shutdown * fork new thread on multicore machines * support both config schemas: original and new (#16) * volume press and release support (#17)

* accept hexadecimal format of commands * support for media player * platform customization * type constants

* accept hexadecimal format of commands * support for media player * platform customization

* accept hexadecimal format of commands * support for media player * platform customization * preparing tcp support

* cleanup imports * cleanup and enhance services description * removed unwanted file

* pyCEC v0.4.6 * pined dependency version * tighten service schemas

konikvranik · 2017-01-20T12:04:59Z

@rytilahti: pycec dependency is present in version 0.4.6. Otherwise checks fail.

kellerza · 2017-01-21T20:40:25Z

@konikvranik Thanks for the PR, it took a while to get it in, but you added many features and worth it in the end! 🐬 !!

michaelhenningersrb · 2017-01-24T07:19:07Z

Hey could you tell me when this giant feature is available in a new release.... Great... thank you guys

kellerza · 2017-01-24T19:29:14Z

It should be the coming weekend

houndci-bot reviewed Dec 6, 2016

View reviewed changes

balloob changed the base branch from master to dev December 7, 2016 05:08

balloob reviewed Dec 7, 2016

View reviewed changes

balloob self-assigned this Dec 7, 2016

houndci-bot reviewed Dec 8, 2016

View reviewed changes

houndci-bot reviewed Dec 19, 2016

View reviewed changes

konikvranik mentioned this pull request Dec 20, 2016

Satisfy requirements to merge into home-assistant dev branch konikvranik/pyCEC#3

Closed

kellerza requested changes Dec 20, 2016

View reviewed changes

vranikp and others added 23 commits January 20, 2017 12:04

fixed overindented

603fa1e

fixed overindented

0e408ef

order of imports

4cb2d10

PEP8

821e317

keep signature of overriding

241f098

removed redundant blank line

15049f2

fixed update call method (#4)

06399d1

Preparation for merge to upstream (#5)

c96783b

* newer version of pyCEC * updated services.yaml * fixed lint scrpt to operate only on python files * pycec version up * update services * no coverage report * exclude non python files from lint * lint only on python files

Dev (#6)

d7227c4

* reordered * sending nonserialized data through hass.data * code formatting * code formatting * import order

Backwards compatibility of hdmi_cec (#10)

647c8ee

* services: power_on standby active_source

new version of pyCEC (#12)

b10bfe6

* newer version of pyCEC

devices config (#13)

6557f7b

* getting device name from config

shutdown fix (#14)

88f099c

* correct call on shutdown

support for media_player (#21)

78b83eb

* accept hexadecimal format of commands * support for media player * platform customization * type constants

Dev (#23)

f5ffb1c

* accept hexadecimal format of commands * support for media player * platform customization

TCP CEC support (#24)

e8de958

* accept hexadecimal format of commands * support for media player * platform customization * preparing tcp support

volume handling (#25)

254eebe

Incorporated CR remarks (#26)

cfd0c12

* cleanup imports * cleanup and enhance services description * removed unwanted file

implemented CR remarks (#27)

7c9c359

* pyCEC v0.4.6 * pined dependency version * tighten service schemas

requirements (#28)

ea80eeb

balloob merged commit 067e11e into home-assistant:dev Jan 20, 2017

home-assistant locked and limited conversation to collaborators Apr 30, 2017


		CONFIG_SCHEMA = vol.Schema({

HDMI CEC - support for devices and commands #4781

HDMI CEC - support for devices and commands #4781

Conversation

konikvranik commented Dec 6, 2016 • edited Loading

mention-bot commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konikvranik commented Dec 19, 2016

rytilahti commented Dec 19, 2016

konikvranik commented Dec 19, 2016

kellerza commented Dec 20, 2016

kellerza commented Dec 20, 2016

konikvranik commented Dec 20, 2016

kellerza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konikvranik commented Jan 20, 2017

kellerza commented Jan 21, 2017

michaelhenningersrb commented Jan 24, 2017

kellerza commented Jan 24, 2017

konikvranik commented Dec 6, 2016 •

edited

Loading