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

Fix Raspi GPIO binary_sensor produces unreliable responses #31788

Closed
wants to merge 2 commits into from

Conversation

greg2001
Copy link

@greg2001 greg2001 commented Feb 13, 2020

Home Assistant's rpi_gpio binary sensor captures the pin state directly in the first egde interrupt.
At first glance, it looks logical and, on a real-time platform like Arduino, it would work properly.
On RPi/Linux, however, the timing is unpredictable, so that it might (actually, it often does) happen
that the signal bounces back before the edge handler has been called, and the handler captures a wrong pin
state. To fix it, we just wait until the signal stabilizes itself ("bouncetime" parameter) and capture
the pin state after that.

NOTE: PRi.GPIO's "bouncetime" parameter doesn't debounce input but simply disables additional egde interrupts
for the specified period of time.

Breaking change

Proposed change

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

…unreliable responses ("Doorbell Scenario")

Home Assistant's rpi_gpio binary sensor captures the pin state directly in the first egde interrupt.
At first glance, it looks logical and, on a real-time platform like Arduino, it would work properly.
On RPi/Linux, however, the timing is unpredictable, so that it might (actually, it often does) happen
that the signal bounces back before the edge handler has been called, and the handler captures a wrong pin
state. To fix it, we just wait until the signal stabilizes itself ("bouncetime" parameter) and capture
the pin state after that.

NOTE: PRi.GPIO's "bouncetime" parameter doesn't debounce input but simply disables additional egde interrupts
for the specified period of time.
@homeassistant
Copy link
Contributor

Hi @greg2001,

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!

@project-bot project-bot bot added this to Needs review in Dev Feb 13, 2020
@probot-home-assistant probot-home-assistant bot added integration: rpi_gpio small-pr PRs with less than 30 lines. labels Feb 13, 2020
@MartinHjelmare MartinHjelmare changed the title Fix for issue #10498 Raspi GPIO binary_sensor produces unreliable res… Fix Raspi GPIO binary_sensor produces unreliable responses Feb 14, 2020
@stale
Copy link

stale bot commented Mar 21, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Mar 21, 2020
@frenck
Copy link
Member

frenck commented Mar 21, 2020

Thanks, stalebot, but this PR awaits an initial review from us.
Please keep open for now, thanks 👍

@stale stale bot removed the stale label Mar 21, 2020
@frenck
Copy link
Member

frenck commented Mar 21, 2020

Meanwhile, @greg2001, the builds fail, which need to be addressed in order for this PR to be reviewed/merged. Could you take a look?

@greg2001
Copy link
Author

Meanwhile, @greg2001, the builds fail, which need to be addressed in order for this PR to be reviewed/merged. Could you take a look?

@frenck I would, but unfortunately, I have absolutely no idea about your build system. How do I get a detailed build log instead of just "Bash exited with code '1'"?

@frenck
Copy link
Member

frenck commented Mar 25, 2020

The logs have been removed since it was quite a while back. I'll retrigger them.

@frenck
Copy link
Member

frenck commented Mar 25, 2020

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…unreliable responses ("Doorbell Scenario")

Home Assistant's rpi_gpio binary sensor captures the pin state directly in the first egde interrupt.
At first glance, it looks logical and, on a real-time platform like Arduino, it would work properly.
On RPi/Linux, however, the timing is unpredictable, so that it might (actually, it often does) happen
that the signal bounces back before the edge handler has been called, and the handler captures a wrong pin
state. To fix it, we just wait until the signal stabilizes itself ("bouncetime" parameter) and capture
the pin state after that.

NOTE: PRi.GPIO's "bouncetime" parameter doesn't debounce input but simply disables additional egde interrupts
for the specified period of time.
@greg2001
Copy link
Author

@frenck Thanks, got it! Should be OK now.

"""Edge detection handler."""
threading.Timer(float(self._bouncetime) / 1000, read_gpio).start()

rpi_gpio.edge_detect(self._port, edge_detected, self._bouncetime)
Copy link
Member

Choose a reason for hiding this comment

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

How often is an edge detected ?

Comment on lines +71 to +73
def edge_detected(port):
"""Edge detection handler."""
threading.Timer(float(self._bouncetime) / 1000, read_gpio).start()
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use a thread but instead schedule it on the event loop.

Suggested change
def edge_detected(port):
"""Edge detection handler."""
threading.Timer(float(self._bouncetime) / 1000, read_gpio).start()
async def async_read_gpio(self):
"""Read state from GPIO."""
await asyncio.sleep(float(self._bouncetime) / 1000)
self._state = await self.hass.async_add_executor_job(rpio_gpio.read_input, self._port)
self.async_write_ha_state()
def edge_detected(port):
"""Edge detection handler."""
self.hass.add_job(self.async_read_gpio)

@MartinHjelmare MartinHjelmare moved this from Needs review to Review in progress in Dev Apr 16, 2020
@stale
Copy link

stale bot commented Apr 25, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Apr 25, 2020
@Misiu
Copy link
Contributor

Misiu commented Apr 26, 2020

Not stale

@stale stale bot removed the stale label Apr 26, 2020
@frenck
Copy link
Member

frenck commented Apr 26, 2020

@Misiu I'm sorry, can I ask why you wrote not stale?
You have not been involved in this PR, and as a matter of fact, it is stale.

@Misiu
Copy link
Contributor

Misiu commented Apr 26, 2020

@frenck I was observing this PR from the beginning and I saw that @balloob give some reviews.
The is no comment that this shouldn't get merged.
Please give the author some more time to address the comments.
My last PI3 is broken, I'm waiting for a new one and I want to get involved in fixing this.

@frenck
Copy link
Member

frenck commented Apr 26, 2020

@Misiu The author did not respond in a month. That is why it is marked stale. You've just messed with that process, which isn't appreciated. Please don't do that again.

I'm sorry to hear you have problems, but the stale bot is here for a reason.

@pvizeli pvizeli added the stale label Apr 26, 2020
@balloob balloob closed this Apr 26, 2020
Dev automation moved this from Review in progress to Cancelled Apr 26, 2020
@greg2001
Copy link
Author

@frenck @balloob
Just saw it closed. No interest in fixing this old problem?

@frenck
Copy link
Member

frenck commented Apr 27, 2020

@greg2001 Sure we are!

However, this PR has been stale for over a month, without response. Hence it was closed. Feel free to re-open a PR when ready to work on it again and address the comments raised!

👍

@greg2001
Copy link
Author

@frenck Would you kindly re-open this PR or shall I create a new one?

@TheFax
Copy link

TheFax commented Apr 27, 2020

I would like to remember that issue #10498 concerns the same problem and is still open.
I worked on it 25 days ago.

@lock lock bot locked and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

Raspi GPIO binary_sensor produces unreliable responses ("Doorbell Scenario")
7 participants