Skip to content

Adds Orange Pi GPIO platform#22541

Merged
rohankapoorcom merged 7 commits intohome-assistant:devfrom
pascallj:orangepi_gpio
Apr 18, 2019
Merged

Adds Orange Pi GPIO platform#22541
rohankapoorcom merged 7 commits intohome-assistant:devfrom
pascallj:orangepi_gpio

Conversation

@pascallj
Copy link
Copy Markdown
Contributor

@pascallj pascallj commented Mar 29, 2019

Description:

This component adds support for GPIO on Orange Pi boards. It uses rpi_gpio as base without support for port debouncing or pull-up resistors. These features are (currently) not supported by the OPi.GPIO library. This library uses SYSFS to access the GPIOs and therefore need some additional steps to get working in HASS (see documentation).

As there exist different pinouts for different boards, an additional pinmode attribute has been added to support all boards supported by the library (see documentation).

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#9072

Example entry for configuration.yaml (if applicable):

# Example configuration.yaml entry
binary_sensor:
  - platform: orangepi_gpio
    pinmode: pc
    ports:
      11: PIR Office
      12: PIR Bedroom

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
Copy link
Copy Markdown
Contributor

Hi @pascallj,

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!

@fabaff
Copy link
Copy Markdown
Member

fabaff commented Mar 29, 2019

Successor of #19732.

Comment thread homeassistant/components/orangepi_gpio/binary_sensor.py Outdated
Comment thread homeassistant/components/orangepi_gpio/binary_sensor.py Outdated
Comment thread homeassistant/components/orangepi_gpio/cover.py Outdated
Comment thread homeassistant/components/orangepi_gpio/cover.py Outdated
Comment thread homeassistant/components/orangepi_gpio/cover.py Outdated
Comment thread homeassistant/components/orangepi_gpio/cover.py Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
Copy link
Copy Markdown
Member

@rohankapoorcom rohankapoorcom left a comment

Choose a reason for hiding this comment

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

I've finished a first pass review. It's a great start :)

Please see the comments with detailed changes requested.

@pascallj
Copy link
Copy Markdown
Contributor Author

pascallj commented Apr 2, 2019

Thanks for your first review, I appreciate your optimisim :).

@awarecan
Copy link
Copy Markdown
Contributor

awarecan commented Apr 4, 2019

It is harsh time for new integration as we are doing refactoring, we are now requesting all components has to have a manifest file.

Please rebase you branch against current dev branch, and create a manifest.json file under your component's folder.

You may need to do more rebase in the next few days as we are still in the progress of refactoring.

@pascallj
Copy link
Copy Markdown
Contributor Author

pascallj commented Apr 9, 2019

As I could not work out why the output is written in the initialization stage of the cover platform, I removed the platform for now so we can get this through. Platform can be added at a later stage.

Also I rebased a couple days ago but as far as I can see it is still up-to-date with the refactoring process.

Comment thread homeassistant/components/orangepi_gpio/__init__.py Outdated
Comment thread homeassistant/components/orangepi_gpio/binary_sensor.py Outdated
Comment thread homeassistant/components/orangepi_gpio/manifest.json Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
Comment thread homeassistant/components/orangepi_gpio/switch.py Outdated
@pascallj
Copy link
Copy Markdown
Contributor Author

The changes you requested for the switch platform have the same issue as the cover platform. I assume the fix is simple, but needs further testing before I can verify that. Therefore I will limit this PR to just one platform, the binary_sensor, for now (as instructed by the component checklist).

@pascallj
Copy link
Copy Markdown
Contributor Author

Any other changes I have to make to this PR?

Comment thread homeassistant/components/orangepi_gpio/__init__.py Outdated
Comment thread homeassistant/components/orangepi_gpio/binary_sensor.py Outdated
@rohankapoorcom
Copy link
Copy Markdown
Member

rohankapoorcom commented Apr 18, 2019

Hi @pascallj - thanks for your patience as we went through a major refactor during the process of you writing your PR. I’ve marked two lines that need to be removed, but otherwise this looks good to me!

I did try to commit the change myself and merge this in, but your fork does not allow HA members to push to it. Once you get those lines removed and the build passes, we’ll be all set here 🎉 🎉 🎉 .

@pascallj
Copy link
Copy Markdown
Contributor Author

Thank you, I removed those lines. Waiting for the tests to pass :)

@rohankapoorcom rohankapoorcom merged commit df475cb into home-assistant:dev Apr 18, 2019
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please open a new PR where we can address the comments.

Comment thread homeassistant/components/orangepi_gpio/binary_sensor.py

_LOGGER = logging.getLogger(__name__)

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(PORT_SCHEMA)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we know that more platforms are likely incoming, it would be better to have all config under the integrating component section, and set up platforms via discovery helper. See the netgear_lte component for example.

Comment thread homeassistant/components/orangepi_gpio/binary_sensor.py
Comment thread homeassistant/components/orangepi_gpio/binary_sensor.py
Comment thread homeassistant/components/orangepi_gpio/__init__.py
@balloob balloob mentioned this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants