OneWireDirect crash if senor disconected

bmsec commented 4 years ago

The problem

If DS18S20 is disconnected class OneWireDirect(OneWire) throws exception

Environment

Home Assistant Core release with the issue: 0.115.3.
Last working Home Assistant Core release (if known):
Operating environment (OS/Container/Supervised/Core): RPI2, Docker
Integration causing this issue: OneWire with Direct RPI-IO
Link to integration documentation on our website: https://www.home-assistant.io/integrations/onewire/

Problem-relevant `configuration.yaml`

sensor:
  - platform: onewire

Traceback/Error logs

2020-09-29 21:53:12 ERROR (MainThread) [homeassistant.components.sensor] onewire: Error on device update!
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 346, in _async_add_entity
    await entity.async_device_update(warning=False)
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 471, in async_device_update
    await self.hass.async_add_executor_job(self.update)  # type: ignore
  File "/usr/local/lib/python3.8/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/src/homeassistant/homeassistant/components/onewire/sensor.py", line 296, in update
    while lines[0].strip()[-3:] != "YES":
IndexError: list index out of range

Additional information

If the DS18S20 is disconnected reading the /sys/bus/w1/devices/28-xxxxxxxxxxxx/w1_slave returns an empty list object. Placing a "if len(lines) > 1:" condition before "while lines[0].strip()[-3:] != "YES":" fixes the problem.

Actually I came across the problem on startup inside Docker on RPI2 with a fully featured Raspbian. It seams that there is an initialization problem when starting the container, so the first read to w1_slave is always empty (as if sensor would be disconnected). If adding a few second sleep before doing the first reed also fixes the issue.

probot-home-assistant[bot] commented 4 years ago

Hey there @garbled1, mind taking a look at this issue as its been labeled with an integration (onewire) you are listed as a codeowner for? Thanks! _{^{(message by CodeOwnersMention)}}

epenet commented 4 years ago

@springstan you can assign this to me

epenet commented 4 years ago

Hi @bmsec,

Following discussions with Martin Hjelmare, the PR as it is currently implemented has been rejected.

In order to fix this issue, we would need to update the integration to use a 3rdParty library instead of using direct file access. I am willing to update the integration (for example with this library : https://pypi.org/project/pi1wire/) but my own onewire setup is using OWServer so I will not be able to test it locally.

If I make these changes, would you be ready to test the PR to make sure you are happy with it?

@Misiu, I saw on PR 39321 that you were using also the SysBus implementation. How would you feel about helping with this (either as a dev, or as a tester)

bmsec commented 4 years ago

Hello, I could help in testing, though I do have to learn first how to install HA directly on Raspbian. I only used Docker image so far.

Misiu commented 4 years ago

@epenet yes, I use DS18B20 sensors directly connected to Pi3 and Pi4. I can help with testing and I can try to help with development. I saw, that your PR (the one with config flow) is blocked because it is missing a 3tdParty library ☹

epenet commented 4 years ago

New branch available for testing on https://github.com/epenet/home-assistant/tree/onewire-sysbus I look forward to your feedback...

epenet commented 4 years ago

Did you get a chance to test the branch before I open the PR?

Misiu commented 4 years ago

@epenet sadly no. My son is ill and after returning home I'm taking care of him, so I won't have time for the next couple of days. Sorry for that 😕

epenet commented 4 years ago

No worries - there are more important things in life than PRs. Take good care of him.

bmsec commented 4 years ago

It seams to be working better....but not perfect.

The test details: -I did the test with one single DS18S20 senor... do not have any more available. If it is necessary I can buy a few more sensors, just let me know what type should it be.

I made the test just by replacing the onewire directory in an existing docker container and removing the .storage and home-assistant_v2.db. So it is not a full up t o date Home assistant installation, but as I understand, it should be the same for this test.

The remaining Issue: -If the sensor is disconnected when Home assistant starts, it will remain "disconnected" permanently. If it is started with attached senor, then disconnecting and reconnecting works properly.

epenet commented 4 years ago

SysBus only works with "temperature" sensors currently, so DS18S20 is fine.

Regarding the fact that sensors remain disconnected permanently:

once the ConfigFlow PR is merged in, it will have a button to "reload integration" which will clear the sensors and reload everything.
if the above is not sufficient, then we can look at adding a scheduled scan of the bus, or maybe a manual "rescan" service Please open a separate issue after the ConfigFlow is merged and it is still required.

I'll get the PR request finalised.

bmsec commented 4 years ago

It seams to be a problem that I did not spot at the first test. If the sensor is disconnected for just a minute or so, that it is OK, but if it is disconnected for more time, that periodic exceptions appear.

2020-10-09 13:02:22 ERROR (SyncWorker_11) [homeassistant.components.onewire.sensor] Cannot read from sensor /sys/bus/w1/devices/28-0416604cbbff/w1_slave: Unsupport response []
2020-10-09 13:02:52 ERROR (SyncWorker_31) [homeassistant.components.onewire.sensor] Cannot read from sensor /sys/bus/w1/devices/28-0416604cbbff/w1_slave: Unsupport response []
2020-10-09 13:03:22 ERROR (SyncWorker_0) [homeassistant.components.onewire.sensor] Cannot read from sensor /sys/bus/w1/devices/28-0416604cbbff/w1_slave: Unsupport response []
2020-10-09 13:03:52 ERROR (MainThread) [homeassistant.helpers.entity] Update for sensor.28_0416604cbbff_temperature fails
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 278, in async_update_ha_state
    await self.async_device_update()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 471, in async_device_update
    await self.hass.async_add_executor_job(self.update)  # type: ignore
  File "/usr/local/lib/python3.8/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/src/homeassistant/homeassistant/components/onewire/sensor.py", line 331, in update
    self._value_raw = self._owsensor.get_temperature()
  File "/usr/local/lib/python3.8/site-packages/pi1wire/_sensor.py", line 23, in get_temperature
    r = self._driver.read_w1_data(self._mac_address)
  File "/usr/local/lib/python3.8/site-packages/pi1wire/_driver.py", line 15, in read_w1_data
    with open(p) as f:
FileNotFoundError: [Errno 2] No such file or directory: '/sys/bus/w1/devices/28-0416604cbbff/w1_slave'
2020-10-09 13:04:22 ERROR (MainThread) [homeassistant.helpers.entity] Update for sensor.28_0416604cbbff_temperature fails
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 278, in async_update_ha_state
    await self.async_device_update()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 471, in async_device_update
    await self.hass.async_add_executor_job(self.update)  # type: ignore
  File "/usr/local/lib/python3.8/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/src/homeassistant/homeassistant/components/onewire/sensor.py", line 331, in update
    self._value_raw = self._owsensor.get_temperature()
  File "/usr/local/lib/python3.8/site-packages/pi1wire/_sensor.py", line 23, in get_temperature
    r = self._driver.read_w1_data(self._mac_address)
  File "/usr/local/lib/python3.8/site-packages/pi1wire/_driver.py", line 15, in read_w1_data
    with open(p) as f:
FileNotFoundError: [Errno 2] No such file or directory: '/sys/bus/w1/devices/28-0416604cbbff/w1_slave'

epenet commented 4 years ago

I've just now updated the PR 40943 to add a handler for FileNotFoundError, so it appears as a "handled" error and not as an "unhandled" error.

I don't think we should hide the issue.

bmsec commented 4 years ago

Can you please point me to the update. I can't find it.

epenet commented 4 years ago

This is the branch for the PR: https://github.com/epenet/home-assistant/tree/onewire-patch-40760

epenet commented 4 years ago

You can also add comments directly on the PR #40943

bmsec commented 4 years ago

Thanks, it is better. It appears as "handled" error. I agree that the problem should appear in the log, but I do consider that an entry in every 30seconds is a bit to much.

home-assistant / core