search

adafruit / Adafruit_CircuitPython_PM25

CircuitPython library for PM2.5 sensors
MIT License
28 stars 16 forks source link

UART Fixes and Featrues #13

Closed fivesixzero closed 2 years ago

fivesixzero commented 3 years ago

Rewrote PM25_UART class to add active/passive mode options (with passive mode default) as well as functions for controlling the PM25 device's mode and sleep status via both UART commands and pin toggles where possible.

UART class enhancements:

UART read enhancements:

I've been testing this using a QT Py device while checking on the commands/responses and timeframes using a logic analyzer, which helped a lot with determining response, reset, and wakeup timeframes.

dglaude commented 3 years ago

Thank you very much, that seems impressive. I am not from Adafruit (and a lot of them are in Thanksgiving aftermath), but I will try your code asap and give an informal opinion on the result.

Maybe @Gadgetoid from Pimoroni should also have a look as they plan to use this for the FeatherWing Enviro+.

dglaude commented 3 years ago

(1) Behaviour when the PMS5003 is not connected (or poorly like for me)

Inititialy I could not confirm nor deny that this code was functioning as I had (and still frequently have) a poor UART connection (I may have played and moved too much with the hardware).

This situation is easy to reproduce, you don't even need the hardware and I already helped a user that had the FeatherWing Enviro+ but not the PMS5003.

The only difference was that old code was displaying Unable to read from sensor, retrying... in loop from the following loop in my pm25_simpletest.py version:

    try:
        aqdata = pm25.read()
    except RuntimeError:
        print("Unable to read from sensor, retrying...")
        continue

Where your code is stuck in init and never return:

# For use with microcontroller board:
uart = busio.UART(board.TX, board.RX, baudrate=9600)
# Connect to a PM2.5 sensor over UART
from adafruit_pm25.uart import PM25_UART
pm25 = PM25_UART(uart, reset_pin)

I believe there should be a way to have an exception after a few error and of maybe simpletest code should catch it and give a message to the user.

At least it never reach this line: print("Found PM2.5 sensor, reading data...") witch is currently displayed with the old version.

dglaude commented 3 years ago

(2) When the connection is good and established.

In that case, both the old code and the new code show the same result (I did not compare the value as I cannot run both in parallel). So "it works for me".

However I don't know exactly what I am testing and how to benefit from the new feature. There should be a bit a documentation and better example specific for UART (I think we need to split for I2C as there are new feature.

From reading the code, I know I am in "passive" mode and it works: def __init__(self, uart, reset_pin=None, set_pin=None, mode="passive"):

I also changed your code to try the active mode with this code: def __init__(self, uart, reset_pin=None, set_pin=None, mode="active"):

And it also work, however I don't really know how to benefit from that feature.

I would like to be able to write something like that: uart = busio.UART(board.TX, board.RX, baudrate=9600, mode="active")

dglaude commented 3 years ago

(3) About the code review itself.

I am not super well positioned to give an opinion about the code style or decision. I am not a good coder and not from Adafruit... but here is some idea:

Rather than to use full string for the mode, you could use a boolean and have something like def __init__(self, uart, reset_pin=None, set_pin=None, active=False): rather than def __init__(self, uart, reset_pin=None, set_pin=None, mode="active"):

This could save a little bit of space/memory and simplify the code.

Another way to save a bit of space/memory could be to not use constant (not really a Python thing).

I believe you could remove those single use constant and use them directly in the code (maybe with comment to say what it mean):

PLANTOWER_CMD_MODE_PASSIVE = b"\xE1\x00\x00"
PLANTOWER_CMD_MODE_ACTIVE = b"\xE1\x00\x01"
PLANTOWER_CMD_READ = b"\xE2\x00\x00"
PLANTOWER_CMD_SLEEP = b"\xE4\x00\x00"
PLANTOWER_CMD_WAKEUP = b"\xE4\x00\x01"

I know this might not be a good practice in code readability, but it can save a few byte left and right and some platform are constrained. But once again, let's wait for another review.

fivesixzero commented 3 years ago

Thank you for testing this, @dglaude ! And thank you for the comments. :)

1) After all of the hours of testing and re-testing the active/passive UART buffer reads I totally forgot to test the negative case (when the device isn't connected or not responding). This should be an easy fix though, since the uart library has its own timeout and we can just give up on a read and throw an Error after a few read timeout cycles. Based on the design guide it looks like we should throw a RuntimeError if we don't find the sensor device within a reasonable timeframe. I'll get this added in the next commit.

2) If everything works reliably as it should then that's exactly how it should work! The basic idea is to set up the sensor in passive mode on init so that the sensor doesn't end up streaming data we don't need.

3) The use of a string to determine active vs passive state can easily be switched to a boolean if that would save a few bytes of memory and/or fit the CircuitPython pattern more cleanly.

You bring up some good points about the use of constants and memory considerations in general. After reviewing the design guide again I've realized that I haven't got my head around the const() feature yet, which I'll be reading up on. My tendency is to write readable code first but if this causes undue memory usage then doing it differently definitely makes sense.

For reference, when compiled to mpy using mpy_cross, the UART driver subclass after these changes is 3,676 bytes vs 996 bytes for the I2C driver subclass.

Reading your comments, I also realized that I didn't provide a clear explanation of the new methods within the UART driver subclass for controlling the device via UART or pins. Here's a list (with examples) of the new methods available with this change. I probably should've documented these in the top comment on the PR.

Constructor options:

The only thing really required to set this thing up is a uart. The reset_pin, set_pin, and mode arguments are optional.

# basic device setup without reset pin or set pin, init in passive mode
sensor = PM25_UART(uart)

# setup with reset pin
reset_pin = DigitalInOut(board.D1)
sensor = PM25_UART(uart, reset_pin=reset_pin)
# or
sensor = PM25_UART(uart, reset_pin)

# setup with set pin (for pin-based sleep/wake)
set_pin = DigitalInOut(board.D0)
sensor = PM25_UART(uart, set_pin=set_pin)

# setup in active mode (for cases where active mode may be more desirable)
sensor = PM25_UART(uart, mode='active')

# setup with all of the arguments
sensor = PM25_UART(uart, reset_pin, set_pin, 'active')
# or
sensor = PM25_UART(uart, reset_pin=DigitalInOut(board.D1), set_pin=DigitalInOut(board.D0), mode='active')

Active/Passive mode can be switched after init if its required. I wasn't sure if these belong in the superclass so they're just internal methods for now.

sensor = PM25_UART(uart)
sensor.cmd_mode_active()
# sensor in active mode
sensor.cmd_mode_passive()
# sensor in passive mode

There are also commands for both pin-based and UART-command-based sleep/wake. I included both since the device supports it and there may be use cases for each, depending on power usage requirements.

sensor = PM25_UART(uart)
sensor.cmd_sleep()
# sensor asleep, should be drawing minimum current
sensor.cmd_wakeup()
# sensor awake, should be drawing more current
# Requires "SET pin" from device to be connected via GPIO
sensor = PM25_UART(uart, set_pin=DigitalInOut(board.D1))
sensor.pin_awake()
# sensor asleep, should be drawing minimum current
sensor.pin_sleep()
# sensor awake, should be drawing more current

I won't be at my full workbench for a few weeks so I don't have the ability to measure power/current usage under "awake", "pin sleep", and "uart sleep" conditions though, which would be nice to document.

For good measure I also added a "reset" command that flips the reset pin off and on again, if that pin is assigned.

# Requires "RESET pin" from device to be connected to GPIO
sensor = PM25_UART(uart, reset_pin=DigitalInOut(board.D0)0
sensor.pin_reset()

I'm not sure what a 'reset' actually does for the device internally. The pin reset doesn't appear to change the device's mode or other behavior. If it was set to passive before the reset it'll continue to be in passive mode after the reset rather than reverting to its default of active mode. Power cycling the device does cause it to start in active mode, however. So the actual use case for this reset is a bit of a mystery, unless it could be used to clear some kind of confused-internal-state situation within the sensor itself.

TL;DR:

  1. I'll be adding a commit soon to address the "device not attached" scenario fixed in 1d8dbb2
  2. I'll be reading up on memory usage considerations to see if there are any impactful changes that can be made
  3. Any additional comments are welcome. :)

edit: Updated examples for cmd and pin methods to remove the preceding underscore, reflecting changes from e5e1324 and 23be73b.

dglaude commented 3 years ago

Great and thank you for the explanation.

If that sleep mode works well, I believe this would would great with stuff like FeatherS2 (ESP32S2) that can also be put to sleep for lower consumption. If this is use to monitor the air quality from time to time, waking up every 5 minute(?) starting the PM25 (and other sensor), then when awake gathering the value, uploading it to the cloud, closing everything and sleeping seems like a great plan, especially if solar powered or something similar.

kevinjwalters commented 3 years ago

@fivesixzero Did you intend the underscore prefix on _cmd_sleep _cmd_wakeup _pin_reset _pin_sleep? That would be used for Python's form of protected and indicate they should not be used by a typical program?

fivesixzero commented 3 years ago

@fivesixzero Did you intend the underscore prefix on _cmd_sleep _cmd_wakeup _pin_reset _pin_sleep? That would be used for Python's form of protected and indicate they should not be used by a typical program?

This was intentional, largely because the superclass didn't provide any of these options. I come from a world of primarily Java coding where the superclass ends up defining the limits of functionality for the subclass, in terms of methods exposed.

If convention here allows for exposing these as external (without the underscores) then that's an easy change I'd be happy to make. :)

Addressed this in e5e1324/23be73b. It should make it more clear that these methods are definitely useable externally. :)

kevinjwalters commented 3 years ago

I've just been doing some semi-manual testing around change mode commands on a PMS5003. It doesn't seem to like them immediately after a reset. Have you tested using the reset line?

It also doesn't like a passive read followed immediately by switch into active mode. None of this is specified AFAICS...

fivesixzero commented 3 years ago

I've just been doing some semi-manual testing around change mode commands on a PMS5003. It doesn't seem to like them immediately after a reset. Have you tested using the reset line?

It also doesn't like a passive read followed immediately by switch into active mode. None of this is specified AFAICS...

I don't have my bench nearby (in the process of moving, sadly) but I do recall during my earlier testing that the PMS5003 requires some time to 'boot up' after a reset. It wasn't something I had time to quantify during this development but it was on my short list of behaviors to study once my bench was set up again and my additional Plantower devices arrived.

kevinjwalters commented 3 years ago

pin_reset() presents a dilemma here. It sounds like it's supposed to reset via the pin but the PMS5003 will come back after a reset in active mode. If the PM25_UART was created in passive mode that's weird as it's not passive any more and there's a mismatch.

fivesixzero commented 3 years ago

pin_reset() presents a dilemma here. It sounds like it's supposed to reset via the pin but the PMS5003 will come back after a reset in active mode. If the PM25_UART was created in passive mode that's weird as it's not passive any more and there's a mismatch.

That's a good catch too. :) Thankfully its a pretty straight-forward fix - if our driver's mode is passive then we can just send the command after the reset-and-wait is done. I'll get that added in the next commit.

fixed in a5fd666

kevinjwalters commented 3 years ago

I've added this functionality to the Pimoroni library too partly based on your work. Have a look at https://github.com/kevinjwalters/pms5003-circuitpython/blob/newexceptionspollingread/library/pms5003/__init__.py if you're interested.

BTW, one thing I've seen from the device is it's possible to do a mode change and read the response but be very unlucky and get a data frame instead. I thought that would be rare but I've had one over last 2 days of coding and testing.

fivesixzero commented 3 years ago

Awesome, glad that this work helped out with the Pimeroni driver! I love their boards! 😍 I'll take a look when I get a chance this weekend.

And yeah, command responses during active mode are a crapshoot. That's why I basically ignored the responses in this implementation. Some of that behavior is weird enough that I thought about cracking open a PMS5003 to see if I could dump its firmware and dig around a bit to see what it's actually doing behind the scenes. Could be fun someday when I'm back at my bench.

kevinjwalters commented 3 years ago

BTW, title has a typo in it, Featrues, ur transposed.

jposada202020 commented 3 years ago

Hello folks, just wondering what would be the status of this PR, and what work needs to be done? let me know thanks :)

IkeRolfe commented 2 years ago

Is this pull request going to move forward?

fivesixzero commented 2 years ago

@IkeRolfe - Given the time since its submission and the fact that I haven't been able to get around to updating it, I'm going to close this PR.

My plan, once I finally get time on my bench, will be to review all of the changes to this driver and CircuitPython in the last year and implement the "set to active/passive" feature in uart.py.