pimoroni / automation-hat

Python library and examples for the Pimoroni Automation HAT, pHAT and HAT Mini
https://shop.pimoroni.com/products/automation-hat
MIT License
121 stars 42 forks source link

Fix race condition with ADC reads. #22

Closed avian2 closed 5 years ago

avian2 commented 5 years ago

I've noticed that sometimes a read() from one ADC will return a value that seemed to originate from another ADC.

To reproduce, using AutomationHat with 12V DC connected to ADC 1 and 5V DC connected to ADC 2, run the following script. It will randomly print out values around 12V (which it shouldn't, since all read values should be close to 5 V):

import automationhat

while True:
    u = automationhat.analog.two.read()
    if u > 5.05:
        print(u)

I believe this happens because of a race condition between ADC reads from the _update_lights() function, which runs in a separate thread, and ADC reads in the main thread. The ads1015.read() is not thread-safe, since only one thread at a time can talk on the i2c bus.

This pull request adds a lock on read() that synchronizes accesses to the ADC. I believe this change should fix the problem. With the patch applied the test script above no longer prints out wrong readings.

Gadgetoid commented 5 years ago

I was aware of the race conditions, but my attempts to fix it were inelegant and unsuccessful compared to your locking approach, see: https://github.com/pimoroni/automation-hat/issues/16

I've merged your change, and reverted my ham-fisted fix in favour of a straight delay since I was experiencing invalid values and suspect the status register was always returning 1 and making my code pointless.

Thank you!