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

Adds Orange Pi GPIO platform #22541

Merged
merged 7 commits into from Apr 18, 2019

Conversation

@pascallj
Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Mar 29, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Successor of #19732.

@rohankapoorcom
Copy link
Member

left a comment

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

Please see the comments with detailed changes requested.

@pascallj

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

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

@awarecan

This comment has been minimized.

Copy link
Contributor

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 added some commits Mar 29, 2019

@pascallj

This comment has been minimized.

Copy link
Contributor Author

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.

@pascallj

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Any other changes I have to make to this PR?

@rohankapoorcom

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

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

@rohankapoorcom rohankapoorcom merged commit df475cb into home-assistant:dev Apr 18, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 5e7fdb4...9925917
Details
codecov/project 93.83% (target 90%)
Details

@OverloadUT OverloadUT removed the in progress label Apr 18, 2019

@MartinHjelmare
Copy link
Member

left a comment

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

self._state = orangepi_gpio.read_input(self._port)
self.schedule_update_ha_state()

orangepi_gpio.edge_detect(self._port, read_gpio)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 18, 2019

Member

State update callbacks should be registered in async_added_to_hass entity coroutine method.


_LOGGER = logging.getLogger(__name__)

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(PORT_SCHEMA)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 18, 2019

Member

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.

"""Support for binary sensor using Orange Pi GPIO."""
import logging

from homeassistant.components import orangepi_gpio

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 18, 2019

Member

Integration package imports should always be relative.

from . import setup_input, setup_mode, etc
self._invert_logic = invert_logic
self._state = None

orangepi_gpio.setup_input(self._port)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 18, 2019

Member

We try to avoid side effects in init method.

This comment has been minimized.

Copy link
@pascallj

pascallj Apr 19, 2019

Author Contributor

What side effects are you referring to?

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 19, 2019

Member

We're setting the port as an input channel, correct? So we're doing something besides returning a value.

This comment has been minimized.

Copy link
@pascallj

pascallj Apr 19, 2019

Author Contributor

True, but in my opinion this still belongs to the term initialization. But I will compare against other components to see how it should be done instead.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 19, 2019

Member

Either move it to async_added_to_hass and schedule the call on the executor thread pool, or move it to setup_platform before creating the entity. The former won't work at the moment though since we're asking update to be called during entity addition. update will be called before async_added_to_hass is called.

@balloob balloob referenced 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
7 participants
You can’t perform that action at this time.