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

Expand the rpi_gpio component to support orangepi as well. #19732

Open
wants to merge 7 commits into
base: dev
from

Conversation

Projects
None yet
7 participants
@mikalstill
Copy link

mikalstill commented Jan 3, 2019

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:

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

This comment has been minimized.

Copy link

homeassistant commented Jan 3, 2019

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

This comment has been minimized.

Copy link

mikalstill commented Jan 3, 2019

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

This comment has been minimized.

Copy link
Member

rohankapoorcom commented Jan 3, 2019

@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

This comment has been minimized.

Copy link

mikalstill commented Jan 3, 2019

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

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

@homeassistant

This comment has been minimized.

Copy link

homeassistant commented Jan 3, 2019

Hello @mikalstill,

When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).

The commits that are missing a linked GitHub account are the following:

Unfortunately, we are unable to accept this pull request until this situation is corrected.

Here are your options:

  1. If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.

  2. If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.

    • If you only made a single commit you should be able to run
      git commit --amend --author="Author Name <email@address.com>"
      
      (substituting Author Name and email@address.com for your actual information) to set the authorship information.
    • If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
      1. You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
      2. You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
    • Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.

We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.

Thanks, I look forward to checking this PR again soon! ❤️



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

This comment has been minimized.

@houndci-bot

houndci-bot Jan 3, 2019

trailing whitespace

@fabaff

This comment has been minimized.

Copy link
Member

fabaff commented Jan 3, 2019

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?


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

This comment has been minimized.

@fabaff

fabaff Jan 3, 2019

Member

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

This comment has been minimized.

@mikalstill

mikalstill Jan 3, 2019

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

This comment has been minimized.

@mikalstill
board = ORANGEPI_BOARDS.get(board_name)
_LOGGER.info('OrangePi %s board specified', board_name)

if not board_name:

This comment has been minimized.

@fabaff

fabaff Jan 3, 2019

Member

All validation should be done with the Schema.

This comment has been minimized.

@mikalstill

mikalstill Jan 3, 2019

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

This comment has been minimized.

Copy link

mikalstill commented Jan 3, 2019

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.

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

@deece

This comment has been minimized.

Copy link

deece commented Jan 4, 2019

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 deece referenced this pull request Jan 9, 2019

Merged

Opi prime updates #315

@deece

This comment has been minimized.

Copy link

deece commented Jan 9, 2019

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

This comment has been minimized.

Copy link

mikalstill commented Jan 10, 2019

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

This comment has been minimized.

Copy link

mikalstill commented Jan 14, 2019

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!

mikalstill and others added some commits Jan 2, 2019

Expand the rpi_gpio library to support orangepi as well.
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.
Remove globals and use hass.data instead.
Per review comments on PR #19732.
Move to a different OrangePi GPIO library
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.
Its much less confusing, given there are two competing numbering
schemes.

@mikalstill mikalstill force-pushed the mikalstill:rpi_gpio branch from 23b2d6b to 8fcfeb2 Jan 15, 2019

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

Update documentation for rpi_gpio
This update brings the documentation inline with what is proposed
in Home Assistant PR:

    home-assistant/home-assistant#19732

@mikalstill mikalstill referenced this pull request Jan 16, 2019

Open

Update documentation for rpi_gpio #8185

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment