Raspberry-Pi morse-code LED

Question

I am new to programming, python and raspberry-pi, but I am keen to learn more. After doing a few basic tutorials I though it might be best to give it a shot and just come up with a task in which I could practice my current skills. I prepared a csv file with morse-code and wrote a little script to access the csv and translate it into a blinking LED. It works! Now I would like to learn a bit more and improve my code. How would a simpler/faster/neater solution look like?

I would appreciate any kind of constructive criticism. (I made a few comments for myself. I hope it is not too annoying)

import RPi.GPIO as GPIO
import time
import csv

GPIO.setmode(GPIO.BCM)
GPIO.setup(18, GPIO.OUT)

#cleaning out the csv

with open('Morse.csv') as csvfile:
    readCSV = csv.reader(csvfile, delimiter=',')

    letters = [] #a new list with letters only
    codes = [] #a new list with morse code only

    next(readCSV) #jump over the column description line and an empty line
    next(readCSV)

    for row in readCSV: #fill up the new lists

        letter = row[0]
        code = row

        code.remove(row[0]) 

        letters.append(letter) #append letters only
        codes.append(code) #append everything but letters

LetterInput = input('what do you want to morse? ')

MorseCode = [] #a new list where the final code for the LED input is saved

for SingLetter in LetterInput: #iterate through the input

    SingLetUp = SingLetter.upper() #capitalize letters so they can be found in csv

    if SingLetUp not in letters: #jump over spaces and special characters 
        continue

    MorseDex = letters.index(SingLetUp) #find code that corresponds with letter by matching indices 
    SingCode = codes[MorseDex]
    MorseCode.append(SingCode) #fill up list


#iterate through list in list to seperate single words

for pos in MorseCode: 
    for sing in pos:
        print(sing)

        x = 'short'
        y = 'long'
        z = 'none'

#LED blinks while parsing through the list

        if sing == x:
            GPIO.output(18, GPIO.HIGH)
            time.sleep(0.2)
            GPIO.output(18, GPIO.LOW)
            time.sleep(0.5)

        if sing == y:
            GPIO.output(18, GPIO.HIGH)
            time.sleep(1)
            GPIO.output(18, GPIO.LOW)
            time.sleep(0.5)

        if sing == z:
            GPIO.output(18, GPIO.LOW)
            time.sleep(1)


GPIO.output(18, GPIO.LOW)
GPIO.cleanup()

And here is the csv file

letter,position 1,position 2,position 3,position 4,position 5
,,,,,
A,short,long,none,none,none
B,long,short,short,short,none
C,long,short,long,short,none
D,long,short,short,none,none
E,short,none,none,none,none
F,short,short,long,short,none
G,long,long,short,none,none
H,short,short,short,short,none
I,short,short,none,none,none
J,short,long,long,long,none
K,long,short,long,none,none
L,short,long,short,short,none
M,long,long,none,none,none
N,long,short,none,none,none
O,long,long,long,none,none
P,short,long,long,short,none
Q,long,long,short,long,none
R,short,long,short,none,none
S,short,short,short,none,none
T,long,none,none,none,none
U,short,short,long,none,none
V,short,short,short,long,none
W,short,long,long,none,none
X,long,short,short,long,none
Y,long,short,long,long,none
Z,long,long,short,short,none
1,short,long,long,long,long
2,short,short,long,long,long
3,short,short,short,long,long
4,short,short,short,short,long
5,short,short,short,short,short
6,long,short,short,short,short
7,long,long,short,short,short
8,long,long,long,short,short
9,long,long,long,long,short
0,long,long,long,long,long

Answer 1

I'm aware that you are a beginner. So I will try to improve your code starting from the most obvious problems and try advance step by step.

Variable names

Names like x, y, z are an absolute no-go (unless naming coordinates). This is like naming them guesswhat. Names should describe what the variable holds as close to natural language as possible. So legal names for x, y, z would be long, short, none. Aloso other names are far from perfect. for SingLetter in LetterInput: should read for letter in message:. You generate a lot of temporary variables which requires you to invent a lot of names. Try avoid some temporaries and/or name temporaries 'less important' than variables you want to use later on. SingLetUp = SingLetter.upper() could read letter = letter.upper(). When reading SingLetUp I do think of 'singing', 'letting' and 'up'. There is no need to save letters in names, if a long name is needed, let it be. But a letter is always a single one, a group of letters shall be named 'word' or 'message' anyway.

Temporaries

As already said, you tend to use too many of them. one case where it makes the code bloated and thus less readable is

x = 'short'
y = 'long'
z = 'none'

and the only later use

if sing == x:

this useless. if named correctly this would read

short = 'short'
# ...
if symbol == short:

which shows the redundancy. it could simply read

if symbol == 'short':

saving some lines without any negative implications.

Do not repeat yourself

This is one of the most important rules to consider. In

if sing == x:
    GPIO.output(18, GPIO.HIGH)
    time.sleep(0.2)
    GPIO.output(18, GPIO.LOW)
    time.sleep(0.5)

if sing == y:
    GPIO.output(18, GPIO.HIGH)
    time.sleep(1)
    GPIO.output(18, GPIO.LOW)
    time.sleep(0.5)

you have two code blocks differing by just one value. all the other parts are required(!) to be identical. this is asking for a function definition (I'm not sure if you already learned that). We do something like (still not perfect)

def gpio_out(length):
    GPIO.output(18, GPIO.HIGH)
    time.sleep(length)
    GPIO.output(18, GPIO.LOW)
    time.sleep(0.5)

and use it like

if symbol == 'short':
    gpio_out(0.2)

if symbol == 'long':
    gpio_out(1)

Do not use 'magic values' in the code but define named constants

The most obvious use cases is to change the GPIO pin. The value 18 is appearing 7 times in your source and must be maintained to match. forgetting to change one will lead to subtle errors. So we do

morse_gpio = 18

and use this name throughout the code. the same applies to all other constants, especially the time values.

About morse code and your implementation

the most frequent letters in western languages are 'e', 't', 'n', ... that is why Morse assigned short codes to these letters. you are outputting many 'none' instead of a single pause separating letters. we fix this by not outputting anything on 'none' but inserting a single pause.

for symbol in pos:
    if symbol == 'short':
        gpio_out(0.2)
    if symbol == 'long':
        gpio_out(1)
    if symbol == 'none':
        pass
time.sleep(1)

Reading the csv file

While this might have been part of your assignment - csv is a little heavy here. this is implicitely shown in your code as you never use the header names. Another hint is that you have to use fill values of none. However you could read such a simple file in python directly. Also we get rid of this superfluous 'none' immediately. Nowhere in the code we are expecting a fixed length code, quite contrary we have special handling for it.

letters = []    #a new list with letters only
codes = []      #a new list with morse code only
with open('Morse.csv') as codefile:
    next(codefile)  #jump over the column description line and an empty line
    next(codefile)
    for line in codefile:
        words = line.strip().split(',')
        letters.append(words[0])    # append letter
        code = [x for x in words[1:] if x != 'none']
        codes.append(code)          # append effective code only

here i used list [comprehension https://docs.python.org/3/tutorial/datastructures.html#list-comprehensions]

Use the right data structures

If you want to look up code for a letter you want to write code = codes[letter] which is clean and readable. so we use a dict for storing our codes

codes = dict()
with open('Morse.csv') as codefile:
    next(codefile)  #jump over the column description line and an empty line
    next(codefile)
    for line in codefile:
        words = line.strip().split(',')
        code = [x for x in words[1:] if x != 'none']
        codes[words[0]] = code          # append effective code only

and rewrite the access

MorseCode = [] #a new list where the final code for the LED input is saved

for SingLetter in LetterInput: #iterate through the input

    SingLetUp = SingLetter.upper() #capitalize letters so they can be found in csv

    if SingLetUp not in letters: #jump over spaces and special characters 
        continue

    MorseDex = letters.index(SingLetUp) #find code that corresponds with letter by matching indices 
    SingCode = codes[MorseDex]
    MorseCode.append(SingCode) #fill up list

to

morse_code = [] #a new list where the final code for the LED input is saved

for letter in message.upper(): #iterate through the input

    if letter in codes:     # ignore spaces and special characters 
        morse_code.append(codes[letter])    # fill up list

Structure your code

Avoid so called spaghetti code, that is many lines of code at the same name space. As already stated about your variable names, have good names and make clear what scope they have. when using functions local variables are not visible outside. this improves readability and avoids name collisions. so we define some functions of already identified code blocks

def read_codes():
    #[...]
    return codes

def encode(message, codes):
    #[...]
    return code

def output_code(code):
    #[...]

and use it them like

codes = read_codes()
message = input('what do you want to morse? ')
code = encode(message, codes)
output_code(code)

Thas limits visibility of name like code or letter. also it adds testability. we can now call encode from some testcases without needing interactive user input.

Further down on internal data representation

When doing debugging you most probably do not want to print [['short', 'long'], ['short'], ['long']] more likely you want to print .- . -. So why not represent the symbols as shorter characters througout the code? We improve our existing function

def read_codes():
    codes = dict()
    with open('Morse.csv') as codefile:
        next(codefile)  #jump over the column description line and an empty line
        next(codefile)
        for line in codefile:
            words = line.strip().split(',')
            code = [x for x in words[1:] if x != 'none']
            codes[words[0]] = code          # append effective code only
    return codes

to

for line in codefile:
    words = line.strip().split(',')
    shortcode = {'long':'-', 'short':'.'}
    code = [shortcode[x] for x in words[1:] if x != 'none']
    codes[words[0]] = ''.join(code)          # append effective code only

here we do not only replace longer strings by characters but also join this characters to a string. this string allows better logging. For print(encode("Hello world!", codes)) we get ['....', '.', '.-..', '.-..', '---', '.--', '---', '.-.', '.-..', '-..']. Wouldn't it be nice to have '.... . .-.. .-.. --- .-- --- .-. .-.. -..' where the blanks represent the pause between letters? this would make the output loop better as it would not do an additional pause at the end. So somewhere in the code we should do ' '.join(code), either just before outputting or in encoding. your choice. for debugging I'd prefer to have it in encoding.

def encode(message, codes):
    code = []    # a new list where the final code for the LED input is saved
    for letter in message.upper(): #iterate through the input
        if letter in codes: #jump over spaces and special characters 
            code.append(codes[letter]) #fill up list
    return ' '.join(code)

of course we now have to fix the output

def output_code(code):
    GPIO.setmode(GPIO.BCM)
    GPIO.setup(18, GPIO.OUT)

    for symbol in code:
        print(symbol, end='', flush=True) # debug output

        if symbol == '.':
            gpio_out(0.2)
        elif symbol == '-':
            gpio_out(1)
        else:
            time.sleep(1)

    GPIO.output(18, GPIO.LOW)
    GPIO.cleanup()

Note that we also encapsulated all GPIO stuff in this function. there is no neet to initialize GPIOs before outputting.

Main guard

In python modules may be run directly or they may be imported by some other module e. g. a unit test. So there is a pattern to avoid hassle on import

if __name__ == '__main__':
    # stuff only executed on direct execution but not on import

current state of our module now

import RPi.GPIO as GPIO
import time

symbol_long = 1
symbol_short = 0.2
symbol_pause = 0.5
letter_pause = 1
morse_gpio = 18

def read_codes():
    codes = dict()
    with open('Morse.csv') as codefile:
        next(codefile)  #jump over the column description line and an empty line
        next(codefile)
        for line in codefile:
            words = line.strip().split(',')
            shortcode = {'long':'-', 'short':'.'}
            code = [shortcode[x] for x in words[1:] if x != 'none']
            codes[words[0]] = ''.join(code)          # append effective code only
    return codes

def encode(message, codes):
    code = []    # a new list where the final code for the LED input is saved
    for letter in message.upper(): #iterate through the input
        if letter in codes: #jump over spaces and special characters 
            code.append(codes[letter]) #fill up list
    return ' '.join(code) # 

def gpio_out(length):
    GPIO.output(morse_gpio, GPIO.HIGH)
    time.sleep(length)
    GPIO.output(morse_gpio, GPIO.LOW)
    time.sleep(symbol_pause)

def output_code(code):
    GPIO.setmode(GPIO.BCM)
    GPIO.setup(morse_gpio, GPIO.OUT)
    for symbol in code:
        print(symbol, end='', flush=True) # debug output

        if symbol == '.':
            gpio_out(symbol_short)
        elif symbol == '-':
            gpio_out(symbol_long)
        else:
            time.sleep(letter_pause)
    GPIO.output(morse_gpio, GPIO.LOW)
    GPIO.cleanup()

if __name__ == '__main__':
    codes = read_codes()
    print(codes) # debug output
    message = input('what do you want to morse? ')
    code = encode(message, codes)
    print(code) # debug output
    output_code(code)

Still to do

• Better comments - comment what is not obvious.
• Docstrings - explain what your functions do. Better encapsulation - avoid globals and pass parameters to functions.
• unit testing - worth to look at.

Answer 2

I'm seeing 2 parts that could be improved:

for row in readCSV: #fill up the new lists

    letter = row[0]
    code = row

    code.remove(row[0]) 

you could get letter and code in a single line like this (using star unpacking, Python 3 only):

letter,*code = row

you could then create a dictionary letter => code (with letters_dict = {}) for further use:

letters_dict[letter] = code

(you can delete letters and codes lists as the dictionary is the database now)

now the letter test is faster (dict search vs list seach):

if SingLetUp not in letters_dict: #jump over spaces and special characters 
    continue

and that can be replaced by dict lookup as well:

MorseDex = letters.index(SingLetUp) #find code that corresponds with letter by matching indices 
SingCode = codes[MorseDex]

replaced by

SingCode = letters_dict[SingLetUp]

(using index is slow, using indices is soooo Fortran)

and in the second loop use elif to avoid useless testing:

    if sing == x:
        GPIO.output(18, GPIO.HIGH)
        time.sleep(0.2)
        ...
    elif sing == y:
        GPIO.output(18, GPIO.HIGH)
        time.sleep(1)
    elif sing == z:
        ...

Also, variables should respect PEP8: readCSV should be read_csv, MorseCode should be morse_code and so on...

Answer 3

a slight adaptation of stefan's excellent answer

I would implement the read_codes like this, using csv, filter. And keeping in mind these excellent talks (1, 2) by Brandon Rhodes about bringing out the IO

import csv
from itertools import islice

def read_codes(filehandle, **kwargs):
    content = csv.reader(filehandle, **kwargs)
    for letter, *words in islice(content, 2, None):
        code = filter(lambda x: x != 'none', words)
        yield letter, tuple(code)

Then you can also more easily test it with something like this:

with StringIO(morse) as filehandle:
    codes = dict(read_codes(filehandle))

The **kwargs is to pass on parameters of the csv-file like the separator to handle diverging formarts