oweidner / i2crelay

A Python library and command line tool to control PCF8574 I2C relay boards
GNU General Public License v2.0
8 stars 3 forks source link

Multithread access #3

Open Misiu opened 4 years ago

Misiu commented 4 years ago

Hi there, I'd like to use your library with Home Assistant. After searching their GitHub I found one issue that describes multithreaded errors with MCP23017 chip - https://github.com/home-assistant/core/issues/31867 I'd like to ask if your library is thread-safe? Home Assistant will show each relay as a separate switch, that the user can turn on and off. If the actions will be done manually (by user) then everything should work fine, but the user can create an action (script) that will turn 6 relays to off. In the linked issue the author is adding mutex to ensure only single write at a time. Do you think the same should be added to your library?

I'm not an expert with Python, so before I start creating a module for Home Assistant I'd like to be sure I won't run into any potential errors.

oweidner commented 4 years ago

Hi @Misiu -

The i2crelay library is really just a very thin wrapper around the Python i2c library, and unless that's thread safe (I doubt it), my library is not thread-safe either. I haven never used the library in a multi-threaded context, so I have never tested it.

If you end up adding thread-safety to it, I'd be more than happy to include it!

jpcornil-git commented 3 years ago

For information, I just created a pull request addressing the threading issue for mcp23017 and using smbus2 i2c library as well, see https://github.com/home-assistant/core/pull/42928

=> It should be pretty easy to add a new pcf8574 integration to home assistant using the same framework:

Cheers jpc

Misiu commented 3 years ago

@jpcornil-git thanks for the info. Your PR looks awesome 🚀 I'll wait until your PR is merged and then I'll try to create something. I'll ask you for a review when I'm ready. BTW is your PR supporting interrupts? I'm sure it's possible to do in Python (like here: https://github.com/wryan67/mcp23x17_rpi_lib)

jpcornil-git commented 3 years ago

Handling of interrupt is platform specific and therefore not implemented here.

You could implement a RPi-specific version of this component quite easily (e.g. rpi_mcp23017) by leveraging the rpi_gpio HA package and modify mcp23017/__init__.py;

You can keep both interrupt and polling implementations as well:

Now, interrupt is probably not important unless you need very short reaction time; for push-button applications, (multiple of) 100ms polling period like it is the case with this implementation gives you an extremely reactive system already and not loading the system/bus at all (very different from being polled from HA every second as it is the case today)

We could also make a component that is generic for polling but platform-specific when interrupt is required of course (I could add that once the 1st PR is over)

Cheers jpc

Misiu commented 3 years ago

@jpcornil-git let's wait for your PR to be merged. My scenario is a simple doorbell example. I've spent hours pulling my hair out, because with a simple python script I got instant notifications (correct readings), but In HA I got a lot of false-positives. I want to connect max 8 sensors, so PCF8574 was my first choice, but MCP23017 will be just fine.

jpcornil-git commented 3 years ago

You can easily use the i2c/mcp23017 code supplied in the PR without HA to test reactivity:

import homeassistant.components.i2c as i2c
import homeassistant.components.mcp23017 as mcp23017

# Create an I2C bus instance managing /dev/i2c-1 (the one available on RPi GPIOs) and a 100ms polling time
bus = i2c.I2cDeviceManager(1,100)
bus.start_polling()

# Add a MCP23017 device at i2c address 0x27 and a scan_multiple of 1 (1x100ms => 100ms)
d=bus.register_device(mcp23017.MCP23017, 0x27, 1)

# Configure MCP23017 pin 8 (pin0 of bank B) as input with pullup
d.set_input(8, True)
d.set_pullup(8, True)

# Create a simple call back
def callback(value):
    print(value)

# Register above callback to MCP pin 8 
d.register_input_callback(8, callback)

Play with pin 8

bus.stop_polling()
exit()