Expand the rpi_gpio component to support orangepi as well. by mikalstill · Pull Request #19732 · home-assistant/core

mikalstill · 2019-01-03T11:06:10Z

Description:

Instead of adding a new component for orangepi (which is a raspberry pi clone), let's just extend the existing component to support more than one family of boards.

I have tested this patch on a raspberry pi 3 and an orangepi prime, in both cases with a LED on a GPIO controlled switch. Note that the orangepi GPIO library requires access to /dev/mem, which I have resolved for now by running Home Assistant as root.

I'm holding off on a home-assistant.io pull request to match this until I get a feel for if this approach is acceptable to you.

This code was implemented to support a Home Assistant tutorial running at linux.conf.au 2019 in a couple of weeks, it would be super cool to be able to have this merged by then so that attendees don't need to carry private patches.

Example entry for `configuration.yaml` (if applicable):

rpi_gpio:
  board_family: orange_pi
  board: prime

switch:
 - platform: rpi_gpio
   ports:
     29: LED

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

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

Documentation added/updated in home-assistant.io (see PR Update documentation for rpi_gpio home-assistant.io#8185)

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

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

homeassistant · 2019-01-03T11:06:11Z

Hi @mikalstill,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

mikalstill · 2019-01-03T11:09:10Z

Ummm, I'm happy to sign the CLA, but https://www.home-assistant.io/developers/cla/ implies there should be a link above for me to do that thing, and I can't see it...

rohankapoorcom · 2019-01-03T15:33:19Z

@mikalstill, it looks like your link is https://home-assistant.io/developers/cla_sign_start/?pr=home-assistant/home-assistant#19732

The message from the home assistant bot had it (the word here was hyperlinked to that url).

mikalstill · 2019-01-03T19:53:31Z

I swear that comment wasn't there before. Regardless, I'll sign the CLA now.

houndci-bot · 2019-01-03T20:03:01Z

+
+
+class UnknownOrangePiBoard(Exception):
+    """'board' config item not set""" 


trailing whitespace

fabaff · 2019-01-03T22:38:01Z

Note that the orangepi GPIO library requires access to /dev/mem, which I have resolved for now by running Home Assistant as root.

To be honest, I not a big fan of suggesting to run HA as root. How was this issue solved for RPi.GPIO?

fabaff · 2019-01-03T22:40:14Z

+
+def setup(hass, base_config):
+    """Set up the GPIO component."""
+    global GPIO_LIBRARY


Don't use global, we have hass.data.

Ok, I'll go have a look at examples.

fabaff · 2019-01-03T22:40:45Z

+        board = ORANGEPI_BOARDS.get(board_name)
+        _LOGGER.info('OrangePi %s board specified', board_name)
+
+        if not board_name:


All validation should be done with the Schema.

board_name is only required if you specify an Orange Pi, there is no equivalent concept with Raspberry Pi GPIOs. Is there an existing example of how to conditionally validate that a config value is set dependant on another config value?

mikalstill · 2019-01-03T22:42:10Z

Note that the orangepi GPIO library requires access to /dev/mem, which I have resolved for now by running Home Assistant as root.

To be honest, I not a big fan of suggesting to run HA as root. How was this issue solved for RPi.GPIO?

Totally agreed. That said, its not an as-root requirement, its a with-permissions-for-/dev/mem requirement. That can be done with filesystem permissions if required.

My understanding is that Raspberry Pi deals with this by having a kernel driver which exposes the GPIOs in a more friendly manner. I'm not opposed to doing that for OrangePi, but it will take a little time to do.

Per review comments on PR home-assistant#19732.

deece · 2019-01-04T21:41:05Z

The Rpi.GPIO library first tries /dev/gpiomem (which is Broadcom specific, and has perms granted via udev), then falls back to /dev/mem (which will require perms).

We can solve the access issue for /dev/mem with some udev rules.

Having said that, after some discussion around the office at Ozlabs, /dev/mem is quite dangerous, any process that can access that can effectively access the memory of any process on the system, including the kernel, and library devs who access that should be admonished^H^H^H^H taken aside and taught how to abstract access via kernel driver.

In this case, we should have access to /sys/class/gpio, which in provided by the pinctrl subsystem on the Orange Pi. This library access GPIO via that: https://pypi.org/project/OPi.GPIO/ It will need a patch to support the Orange Pi Prime, but that seems pretty straightforward.

deece · 2019-01-09T10:10:23Z

It's worth noting that HassOS currently runs HomeAssistant as root, and passes /dev/mem through to the docker container (not that I recommend we take this path). I'll log this as an issue.

mikalstill · 2019-01-10T05:36:28Z

Ok, so this latest patch does a few things:

Moves to a new GPIO library for OrangePi that does not require root (it uses sysfs not /dev/mem)
Removes the requirement to configure the OrangePi board, as it turns out they all seem to have the same GPIO pinout

I think that resolves all of fabaff's concerns?

mikalstill · 2019-01-14T10:05:45Z

Hey @pvizeli, it was suggested I ping you about two things around this PR:

the CLA-Error label is now incorrect, but I don't know how to remove it. How do we do that thing?
also, the pin numbering scheme for OrangePi that makes the most sense is something called SUNXI. However, its not integers. How would we feel about relaxing the schema for pin numbers in components dependant on this one such as rpi_switch? I'd prefer to do that than to fork out a separate OrangePi switch component if possible.

Thanks!

Instead of adding a new component for orangepi (which is a raspberry pi clone), let's just extend the existing component to support more than one family of boards. I have tested this patch on a raspberry pi 3 and an orangepi prime, in both cases with a LED on a GPIO controlled switch. Note that the orangepi GPIO library requires access to /dev/mem, which I have resolved for now by running Home Assistant as root.

Per review comments on PR home-assistant#19732.

This library doesn't require root or access to /dev/mem because it uses the sysfs interface for GPIOs instead. It also doesn't need to know what OrangePi board you're using, so the configuration is simpler.

Its much less confusing, given there are two competing numbering schemes.

This update brings the documentation inline with what is proposed in Home Assistant PR: home-assistant/core#19732

MartinHjelmare · 2019-01-27T22:26:02Z

+    """Set up the GPIO component."""
+    hass.data[DOMAIN] = {}
+
+    config = base_config.get(DOMAIN)


Use dict[key] for required config keys and keys with default config schema values.

MartinHjelmare · 2019-01-27T22:27:28Z

+    family_name = config.get(CONF_BOARD_FAMILY, DEFAULT_FAMILY)
+    lib_name = FAMILY_LIBRARIES.get(family_name)
+    _LOGGER.info('Configured to use board family %s', family_name)
+    _LOGGER.info('Will use %s as GPIO library', lib_name)


Merge these logging messages to one.

MartinHjelmare · 2019-01-27T22:28:19Z

+    if not lib_name:
+        raise UnknownBoardFamily('Unknown board family: %s'
+                                 % config.get(CONF_BOARD_FAMILY))
+    hass.data[DOMAIN][LIBRARY] = importlib.import_module(lib_name)


Cache the imported lib in a local variable that we can use below.

MartinHjelmare · 2019-01-27T22:30:51Z

        self._state = None

-        rpi_gpio.setup_input(self._port, self._pull_mode)
+        rpi_gpio.setup_input(self._hass, self._port, self._pull_mode)


Move this to entity coroutine method async_added_to_hass.

MartinHjelmare · 2019-01-27T22:31:12Z

    """Represent a binary sensor that uses Raspberry Pi GPIO."""

-    def __init__(self, name, port, pull_mode, bouncetime, invert_logic):
+    def __init__(self, hass, name, port, pull_mode, bouncetime, invert_logic):


Don't pass in hass. It will be set on the entity when it has been added to home assistant.

MartinHjelmare · 2019-01-27T22:32:15Z

    """Representation of a Raspberry GPIO cover."""

-    def __init__(self, name, relay_pin, state_pin, state_pull_mode,
+    def __init__(self, hass, name, relay_pin, state_pin, state_pull_mode,


MartinHjelmare · 2019-01-27T22:32:24Z

-    def __init__(self, name, port, pull_mode, bouncetime, invert_logic):
+    def __init__(self, hass, name, port, pull_mode, bouncetime, invert_logic):
        """Initialize the RPi binary sensor."""
+        self._hass = hass


Remove this.

MartinHjelmare · 2019-01-27T22:32:53Z

-        rpi_gpio.setup_output(self._relay_pin)
-        rpi_gpio.setup_input(self._state_pin, self._state_pull_mode)
-        rpi_gpio.write_output(self._relay_pin, 0 if self._invert_relay else 1)
+        rpi_gpio.setup_output(self._hass, self._relay_pin)


Move to async_added_to_hass.

MartinHjelmare · 2019-01-27T22:33:39Z

    """Representation of a  Raspberry Pi GPIO."""

-    def __init__(self, name, port, invert_logic):
+    def __init__(self, hass, name, port, invert_logic):


MartinHjelmare · 2019-01-27T22:34:01Z

        self._state = False
-        rpi_gpio.setup_output(self._port)
-        rpi_gpio.write_output(self._port, 1 if self._invert_logic else 0)
+        rpi_gpio.setup_output(self._hass, self._port)


Move to async_added_to_hass.

balloob · 2019-02-21T19:17:46Z

This should not be part of the Raspberry Pi integration but instead be a new one. Over time, the integrations will diverge, and we will end up with a bunch of if…else statements. We don't want that.

balloob · 2019-02-26T23:12:48Z

This PR seems to have gone stale. Closing it.

homeassistant added cla-needed integration: rpi_gpio labels Jan 3, 2019

ghost added the in progress label Jan 3, 2019

homeassistant added cla-signed cla-error and removed cla-needed labels Jan 3, 2019

houndci-bot reviewed Jan 3, 2019

View reviewed changes

fabaff reviewed Jan 3, 2019

View reviewed changes

mikalstill added a commit to mikalstill/home-assistant that referenced this pull request Jan 4, 2019

Remove globals and use hass.data instead.

2bd9758

Per review comments on PR home-assistant#19732.

deece mentioned this pull request Jan 9, 2019

Opi prime updates home-assistant/operating-system#315

Merged

mikalstill and others added 7 commits January 15, 2019 14:07

Attempt to fix linter style nags

cf5939e

Fix trailing whitespace error.

4f871ae

Remove globals and use hass.data instead.

05067f7

Per review comments on PR home-assistant#19732.

Move to a different OrangePi GPIO library

a8c320b

This library doesn't require root or access to /dev/mem because it uses the sysfs interface for GPIOs instead. It also doesn't need to know what OrangePi board you're using, so the configuration is simpler.

Move to SUNXI pin naming for OrangePI.

d259d91

Its much less confusing, given there are two competing numbering schemes.

Update requirements with new OrangePi GPIO library

8fcfeb2

mikalstill force-pushed the rpi_gpio branch from 23b2d6b to 8fcfeb2 Compare January 15, 2019 03:08

mikalstill added a commit to mikalstill/home-assistant.io that referenced this pull request Jan 16, 2019

Update documentation for rpi_gpio

2f04827

This update brings the documentation inline with what is proposed in Home Assistant PR: home-assistant/core#19732

mikalstill mentioned this pull request Jan 16, 2019

Update documentation for rpi_gpio home-assistant/home-assistant.io#8185

Closed

2 tasks

MartinHjelmare added cla-recheck and removed cla-error cla-signed labels Jan 27, 2019

homeassistant added cla-signed and removed cla-recheck labels Jan 27, 2019

home-assistant deleted a comment from homeassistant Jan 27, 2019

MartinHjelmare requested changes Jan 27, 2019

View reviewed changes

balloob closed this Feb 26, 2019

ghost removed the in progress label Feb 26, 2019

fabaff mentioned this pull request Mar 29, 2019

Adds Orange Pi GPIO platform #22541

Merged

9 tasks



		class UnknownOrangePiBoard(Exception):
		"""'board' config item not set"""

Uh oh!

Conversation

mikalstill commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

homeassistant commented Jan 3, 2019

Uh oh!

mikalstill commented Jan 3, 2019

Uh oh!

rohankapoorcom commented Jan 3, 2019

Uh oh!

mikalstill commented Jan 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabaff commented Jan 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikalstill commented Jan 3, 2019

Uh oh!

deece commented Jan 4, 2019

Uh oh!

deece commented Jan 9, 2019

Uh oh!

mikalstill commented Jan 10, 2019

Uh oh!

mikalstill commented Jan 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Feb 21, 2019

Uh oh!

balloob commented Feb 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mikalstill commented Jan 3, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):