Requesting a code review: MQTT, Adafruit IO, Scroll bot, et al. (Python)

(Daniel Hollands) #1

Howdy folks, I’m hoping someone here might be able to give some code I’ve written a once over, please?

I picked up a scroll bot kit last week and decided I wanted to use it in collaboration with the MQTT features of Adafruit IO. Before I go any further, here’s what I’ve ended up with:

# Example of using the MQTT client class to subscribe to and publish feed values.
# Author: Tony DiCola

# Import standard python modules.
import random
import sys
import time
import threading
import os
# import signal

# Import Adafruit IO MQTT client.
from Adafruit_IO import MQTTClient

# Import scrollphathd
import scrollphathd
from scrollphathd.fonts import font5x7

import settings


messages = []

def add_message(string):

def display_feed():
    while True:
        if len(messages) > 0:
            print('There are {0} messages waiting for display'.format(len(messages)))

def display_string(string):
    string += '      '
    buffer = scrollphathd.write_string(string, x=17, y=0, font=font5x7, brightness=0.5)

    for i in range(buffer):

    scrollphathd.scroll_to(0, 0)

# Define callback functions which will be called when certain events happen.
def connected(client):
    # Connected function will be called when the client is connected to Adafruit IO.
    # This is a good place to subscribe to feed changes.  The client parameter
    # passed to this function is the Adafruit IO MQTT client so you can make
    # calls against it easily.
    print('Connected to Adafruit IO!  Listening for Message changes...')
    # Subscribe to changes on a feed named Message.

def disconnected(client):
    # Disconnected function will be called when the client disconnects.
    print('Disconnected from Adafruit IO!')

def message(client, feed_id, payload):
    # Message function will be called when a subscribed feed has a new value.
    # The feed_id parameter identifies the feed, and the payload parameter has
    # the new value.
    print('Feed {0} received new value: {1}'.format(feed_id, payload))

# Create an MQTT client instance.

# Setup the callback functions defined above.
client.on_connect = connected
client.on_disconnect = disconnected
client.on_message = message

# Connect to the Adafruit IO server.

# Now the program needs to use a client loop function to ensure messages are
# sent and received.  There are a few options for driving the message loop,
# depending on what your program needs to do.

# The first option is to run a thread in the background so you can continue
# doing things in your program.


This is the end result of three of four different source examples all copy/pasted together into something which works, but can, no doubt, be improved upon.

In fact, just looking at it now, it’s become clear that I don’t need to put the display_feed function into its own thread, as I think it can quite happily sit at the bottom of the script, looping to its heart’s content. (I’ve ended up with this threaded approach because, in my original take, the feed values were being populated by the same script, as you can see here).

Anyway, the script above does two things. First is registers itself via MQTT to the Adafruit IO broker service as a subscriber to the Message feed. Every time the feed has a new payload (which, for testing purposes, is currently be populated with tweets via IFTTT), this gets added to a messages list.

The second thing it does is check the messages list every second for a new element, popping the top element off when it’s found one (never used pop before, that was exciting), and sending it for display on the on the scrollphathd.

The process for displaying the message on the scrollphathd, itself, is a bit weird, but that’s a side effect of how it works, which I’ve discussed over on the Pimoroni forums.

Anyway, getting to the point, I’d welcome any feedback any Python folk might have. At the moment this feels really clunky, and I’m sure it can be massively improved upon. For example, I’m sure it’s not right that the global scoped messages array is accessible from inside the add_message and display_feed functions.

Ideally, I’d like to try and separate out the concerns and make the code more self-descriptive, but despite my couple years experience of Ruby (and several years of PHP before that), I’m a total n00b at Python. I’m also a total n00b at real-time stuff, as my entire coding background is in stateless web apps, so a lot of these concepts (such as running threads, etc…) is very foreign to me.

Thanks :slight_smile:

Family Boxes (a project I'm working on)
Amazon Dash is in the UK
(Stuart Langridge) #2

Honestly? This is not that bad at all. There are a few tweaks I’d make, which I’ll outline, but it’s reasonable stuff. Simple is better than complex; the Zen of Python (do import this to read it) specifically says that.

You call out the global messages variable, but that’s not actually a problem in a short script like this. I mean, you could wrap up your functions in a class and then make that an instance variable if you wanted, but there’s not a great deal of point in doing so right now. You may need to later on if this becomes part of a much more complicated system, but right now it’s not really worth it.

Similarly, I tend to not do things on script run; instead, I say if __name__ == "__main__": main() and then put the stuff I want to run into a def main() function, because this means that my script can be imported by another module without running that stuff but, again, you don’t need that until this becomes part of something larger, at which point you can refactor.

It is more Pythonic to to “try this thing and catch the error” than “see if there’ll be an error and if not go ahead”, so my display_feed would look more like:

def display_feed():
    while True:
            next_message = messages.pop(0)
        except IndexError:
            time.sleep (1)

because then you’re not worrying about what happens if someone inserts a message in between the check and the use, although it doesn’t really matter in this situation. More importantly, though, do you really need to poll every second? That’s pretty horrid. I would restructure things so that your app’s asleep and using no power and CPU while there are no messages, and have the adafruit listener start the message displayer, personally. But that’s not a Python issue. :slight_smile:

(Daniel Hollands) #3

TBH, I was more surprised that I was even able to do this. I’m pretty sure this isn’t possible in PHP or Ruby, and had it in there as a placeholder more than anything, followed by shock when I discovered it actually worked.

I saw some other example scripts doing this, but wasn’t entirely sure what was going on. This looks like a convention, which I’d be more than happy to adopt.

That feels weird to me. In the Ruby world, Exceptions should be reserved for exceptional things. While I’m sure that an Error in Python and an Exception in Ruby are not the same, this still feels unusual.

My question, in this case, is how do I do that? I’m maybe missing the obvious, but as the messages in the feed could be coming in at any point, possibly while a previous message is being displayed, how do I get it to add the message to the list instead of just showing it directly?

Again, I’m new to this real-time thing, so there are no doubt a lot of lessons I need to learn.

(Stuart Langridge) #4


import queue #

def startQueueProcessor():
    queueProcessorRunning = True
    while True:
        if queue.empty:
            queueProcessorRunning = False

def receiveMessageFromAdafruit(message):
    if not queueProcessorRunning: startQueueProcessor()

basically, if you’ve finished processing the queue, shut down the worker; if you get a message and the worker isn’t running, start the worker up.

(Andy Wootton) #5

FYI That code-box didn’t have scroll bars on Firefox/Android but does on Firefox/Ubuntu.

(Daniel Hollands) #6

It doesn’t have them on Chrome on MacOS either, until you start to scroll, that is. It’s not the best, I admit, but it’s what Discourse provides.

(Daniel Hollands) #7

@sil I’ve played around with this some more, and while I’ve had some successes, I’m still failing with a few bits.

I made some initial changes based on your feedback:

Then I updated the list into a queue:

But now I’m trying to implement something like the psuedo code you provided - and while it appears to work, as intended I don’t think it is.

First, the code:

Running this works, at least, it appears to work - messages are coming in, and being shown on the scrollphathd. But looking at the output in the console suggests something else:

ssh://pi@helium.local:22/usr/bin/python3 -u /home/pi/Code/scroll-bot/
Connected to Adafruit IO!  Listening for Message changes...
Feed Message received new value: @natalie_davies9 tweeted: I know we're on a health kick but a micro cauliflower is too far @AldiUK. Wow. I don't need all this foliage.
Starting up queue
Shutting down queue
Feed Message received new value: @zeth0 tweeted: @holdergn @PHammondMP when they find themselves left of the public, you have to wonder if they have lost touch a little.
Starting up queue
Shutting down queue
Feed Message received new value: @WANIMINAJ tweeted: Debie seems like she can do this ex thing in real life #BBNaija
Starting up queue
Shutting down queue
Feed Message received new value: @kwakbiker tweeted: @AdamJamesJoyce you haven't?😀
Starting up queue
Shutting down queue
Feed Message received new value: @WANIMINAJ tweeted: Bisola and Bassey should have performed your love is upon me and slowly slowly #BBNaija
Starting up queue
Shutting down queue
Feed Message received new value: @LOCO_CORONA tweeted: As if it's rah been a year since @tweet_mist had man doing "karla's out now skank"
Starting up queue
Shutting down queue

The fact that it’s shutting down the queue, then starting it up again, suggests that the act of running start_queue_processor() is causing blocking, meaning nothing else is being added to the messages queue in the background.

I tried adopting the trick of running various parts of this in their own threads, but that just screwed up the display of messages across the scrollphathd - so right now I’m at a bit of a loss.

I’d welcome any input you might have.

(Stuart Langridge) #8

Ah, I see your problem. Let me gen up a thing. The issue is that you’re starting up the queue processor inside the mqtt thread. Example code shortly.

(Stuart Langridge) #9

See here for details.

We start the queue processor in a background thread. It basically does this:

block until there is a message in the queue
print the message
wait a bit
start again

Then in the foreground thread we run the MQTT client. It blocks and waits for MQTT messages. When it finds one, it calls our on_message, which drops the new message onto the end of the queue and does nothing else.

Does that make sense? Threading is a bit of a headpecker.

(Andy Wootton) #10

Oh year, I can scroll left-right but if I try up-down the whole page moves.

(Steve Jalim) #11

On mobile, the preformatted code blocks DO scroll if you finger-drag them. It’s pretty neat

(Daniel Hollands) #12

That’s perfect, thank you @sil.

(Daniel Hollands) #13

Me again. That code is working fine, so far as I can tell, but after an amount of time, it seems to time out. I used the time command on it, and after 5 minutes the first time, 8 minutes the second, and then 12 minutes the third, it just disconnects from Adafruit IO and quits.

Now, I have no idea if this is Adafruit killing the connection (although, if it is, that’s a bit rubbish), or if maybe I’m having some internet issues, but ideally, I’d like for the script to detect that it’s disconnected, and reconnect again.

How would I go about doing something like this?

Family Boxes (a project I'm working on)
(Stuart Langridge) #14

Note that you run, as the last command in the script, client.loop_blocking(), which blocks until disconnect and then ends. Since it’s the last command, once that finishes, the script has no more to do and will exit. So,

while True: client.loop_blocking()

should call it again when it exits, forever. (And your on_connect and on_disconnect handlers will presumably be called again as well.)

(Daniel Hollands) #15

I tried this, but it didn’t work. I’m not too concerned, however, as I’ve since played with the python MQTT client directly, and that seems far more fit for purpose.

I do have some new questions, however - such as, what’s the best way to make this modular? While I’m sure there are lots of arguments for keeping it simple, this is a learning exercise for me, so I want to use this simple example to learn some more advanced stuff.

In the first instance, rather by accident, I seem to have figured out that using import filename seems to treat said file somewhat like an object, with any variables and functions it containing becoming available for use externally.

With this newfound knowledge, because I wanted to separate out the message scrolling from the rest of the code, I’ve since created and popped all the logic for that in there, calling the display_string() function thusly:


Now, this works - but is that by design, or by accident? Would I also be able to do the same with the queue, and even the base MQTT stuff?

I did try extracting the message queue stuff out into its own file, but this didn’t work too well (I think something to to with the threaded processing). But I’m not too concerned with this right now, as I’d rather some the basics working first.


(Stuart Langridge) #16

That’s by design. If you create and put def myfn(): pass in it, you can import ahaha; ahaha.myfn() or from ahaha import myfn; myfn() in other files. You can also create a whatever folder and drop an file in it, and then import whatever gives you access to the functions/classes/vars in whatever/, and import whatever.something gives you access to the stuff in whatever/ See for more details; the Python docs and tutorial are good, and a readthough will help you out a lot I think.