UART Fixes and Featrues

[fivesixzero]

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:

• Added functions for sending all of the supported UART commands I could find documentation for
  • Active/passive mode change via UART command
  • Data read request via UART command
  • Sleep/wake via UART command
• Added functions for handling both SET and RESET pins
  • Reset via pin
  • Sleep/wake via pin

UART read enhancements:

• If the device is in passive mode, it sends a "read" command prior to reading in a frame
  • In active mode it skips sending the command and simply reads in the next expected frame
• Handling for variable frame lengths using bytes 3/4 of the incoming frame
  • Allows for proper reading of command responses (which are 8 bytes)
• More resilient handling of cases reads may start mid-frame
• Error handling to avoid infinite loop scenarios
  • If we read in more than 32 bytes without seeing a header, we'll RuntimeError. If we get more than 3 malformed headers, invalid frame lengths, empty data frames, or failed checksums we'll also throw a RuntimeError

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]

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]

(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]

(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]

(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]

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]

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]

@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?

[kevinjwalters]

The keyword argument of mode="passive" is going to change the behaviour for existing users of this library. It seems undesirable for them to get this change without requesting it explicitly by setting mode from the program?

[fivesixzero]

That's a good point. I spent a bit of time mulling over it and looking for guidance from docs before settling on this. Here's my general train of thought.

At a high level, behabior should be unchanged. The end result should be identical operation, based on the driver's design. When a request is made for a read() the user will get a reading as expected.

The only difference for a user would be at a lower level beyond the driver's abstraction. Rather than having a stream of frames coming in every 900-1000ms over the UART, the UART would only see incoming data frames in response to requests (triggered by read().

With this constructor arg set up the user could request an "active" mode configuration if their use case required it. For example, if their implmentation was reading the UART frames outside of the CircuitPython program/MCU, or using lower-level UART reads outside of our driver.

Additionally, I noticed that active-mode reads are generally unreliable, leading to errors from if frames are picked up in the middle and/or truncated before the checksum comes in. That was the quirk that led me down this rabbit hole in the first place. 😄

That was my original intent and internal argument though. I can see the case for making the default "active", if retaining that convention for UART devices in this driver is paramount.

[fivesixzero]

I didn't address this in my commits this morning, but I'd be happy to make any changes suggested by maintainers here.

Its my first time working on a proper CircuitPython driver so I'm very open to guidance and discussion. :)

[fivesixzero]

@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]

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]

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]

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]

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]

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]

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]

I just tested a timeout at REPL. I've unplugged PMS5003, set timeout to 4 on UART and read(1) (and read()) give me b'' whereas this code is expecteding None. RX line would be floating here, I think for my particular test.

[fivesixzero]

Another good catch. I still don't have my bench set up again unfortunately.

Do you have a suggestion for a better way to handle the initial "did we get any actaul data" check?

I wasn't aware that the uart driver could send us an empty byte array in these scenarios. Maybe we could just check for both is not None (to cover normal, non-floating RX scenarios) as well as the content (or lack thereof) of the byte array we get back from uart.read(1) (for device-disconnect/floating RX scenarios)?

I feel like this should be a problem for many Circuitpython drivers like this that need to do incremental per-byte reads to find frame starts. Or is this the only one? 😄

[kevinjwalters]

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

[jposada202020]

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

[IkeRolfe]

Is this pull request going to move forward?

[fivesixzero]

@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.