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

• G_M: I'm curious about what Morse.csv looks like, is there any way you could add it to your question (separate code section?) or paste it to something like bpaste and add a link to it?
• zomb23: Sure! I have added another code section that contains the csv.
• G_M: Thanks for adding it! I'm curious about a couple more things, 1) is the csv file mandatory? 2) if it is mandatory, can the structure can be changed at all? The reason I ask is it might be much simpler to just use a dictionary with the key/value pairs for the morse code.
• zomb23: I think it is not mandatory. I wanted to apply what I have learned and kind of "pretended" that I have a pre set csv that I have to manipulate with my code in order to reach my goal. I think it was partly due to the fact that I did a tutorial about reading a csv right before. Regarding the dictionary: I will definitely give it a try!

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.

• Maarten Fabré: another thing you can do is using generators and generator expressions . The last 3 lines of read_codes would become code = (shortcode[x] for x in words[1:] if x != 'none'); yield words[0], ''.join(code). Convention for global constants as symbol_long is also in ALL_CAPS
• stefan: @MaartenFabré - you are right. I tried to do the most important points, which took some time, and did not go for PEP 8 fully.
• Maarten Fabré: In your encode, by filtering out all other symbols, you also delete the spaces around words link
• zomb23: @stefan: Thanks for this elaborate review. A few follow up questions: you say that csv is a bit too much for this assignment. Is there a threshold that tells me "okay, now using csv is the right choice"? Inside the read_codes function. Does the .strip() have an effect here or is it just for "safety reasons"? Is codes = dict() equivalent to codes = {}? In the encode function, why did you introduce the list code = []? wouldn't it be possible to save the line and say code = codes[letter] instead of appending it to an empty list?
• stefan: csv is fine if you are familiar with it, but it is not necessary. Use csv when you want to read different data files and/or need some of the provided functions or configuration options. the file you read is actually a config file which is in a csv format. your script will never read a different file and you do not save a single line of code by using the csv module. In such cases I recommend to go without extra modules as there may be a another person reading/maintaining the code with little knowledge of this imported module.

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