43
\$\begingroup\$

I've been trying to show my 6-year old daughter a way to build and create a simple toy car. We've got a 4-motor chassis kit, Raspberry PI and a motozero shield that can operate these 4 motors. We've put a power bank and a separate battery for the motors:

                                      enter image description here

                                              (The green light on the right is the traffic light)

Now, as far as coding goes, I've created a simple Car class with forward, backward, forward_left, forward_right, backward_left and backward_right actions. For the demonstration purposes mostly, I've tried to be explicit and violated the DRY principle a lot.

I've also initialized an endless loop listening for keyboard inputs - F would be for forward, B for backwards and S for stop (have not decided which keys would go for those "left-ish" and "right-ish" actions yet).

from RPi import GPIO
from gpiozero import Motor, OutputDevice


class Car:
    def __init__(self):
        self.front_right = Motor(forward=24, backward=27)
        self.front_left = Motor(forward=6, backward=22)

        self.rear_right = Motor(forward=13, backward=18)
        self.rear_left = Motor(forward=23, backward=16)

        OutputDevice(5).on()
        OutputDevice(17).on()
        OutputDevice(25).on()
        OutputDevice(12).on()

    def forward(self):
        self.front_right.forward()
        self.front_left.backward()

        self.rear_right.backward()
        self.rear_left.backward()

    def backward(self):
        self.front_right.backward()
        self.front_left.forward()

        self.rear_right.forward()
        self.rear_left.forward()

    def backward_right(self):
        self.front_right.backward(1)
        self.front_left.forward(1)

        self.rear_right.forward(1)
        self.rear_left.forward(0.2)

    def backward_left(self):
        self.front_right.backward(1)
        self.front_left.forward(1)

        self.rear_right.forward(0.2)
        self.rear_left.forward(1)

    def forward_left(self):
        self.front_right.forward()
        self.front_left.backward(0.2)

        self.rear_right.backward(1)
        self.rear_left.backward(1)

    def forward_right(self):
        self.front_right.forward(0.2)
        self.front_left.backward(1)

        self.rear_right.backward(1)
        self.rear_left.backward(1)

    def stop(self):
        self.front_right.stop()
        self.front_left.stop()

        self.rear_right.stop()
        self.rear_left.stop()


if __name__ == '__main__':
    GPIO.setwarnings(False)
    GPIO.cleanup()

    commands = {
        "f": "forward",
        "b": "backward",
        "s": "stop"
    }
    try:
        car = Car()

        while True:
            command = raw_input()
            getattr(car, commands[command])()
    finally:
        try:
            GPIO.cleanup()
        except:  # ignore cleanup errors
            pass

(I've also mixed up some motors and pins on the board which led to some motors accepting "backwards" as "forward")

It is, of course, difficult to explain this kind of code to a 6-year old, since she still expects a printer to print something when I show her the Python's print() command, but, how would you change this code to make it more understandable for a kid?

I would also appreciate any points about the code organization and quality since the next step would be to control the car "via Bluetooth".

\$\endgroup\$
  • 27
    \$\begingroup\$ Have you considered using WASD controls as many PC games do? That would seem more natural than F, B and S in my opinion. Kudos for the project though, that sounds like a great way to teach a child to build something! \$\endgroup\$ – Phrancis Dec 6 '17 at 2:48
  • 3
    \$\begingroup\$ @Phrancis thanks! wasd would be better..or may be even arrow up, down, left, right - a bit more straightforward for a kid.. \$\endgroup\$ – alecxe Dec 6 '17 at 3:44
  • 10
    \$\begingroup\$ You could make both, I do agree arrows would be simpler for a kid. Maybe space bar for brakes. Cheers! \$\endgroup\$ – Phrancis Dec 6 '17 at 4:34
  • 7
    \$\begingroup\$ Couldn't you just swap the pin numbers in the program rather than changing the actual wiring and still end up with forward and backward actually going in the right direction? \$\endgroup\$ – IllusiveBrian Dec 6 '17 at 19:29
  • 1
    \$\begingroup\$ I know it would probably be a much more of an effort, but you could consider trying to make the program for the car using Google Blockly. Kids love colors and puzzles and this is awesome tool for children, and plus it compiles directly to Python. To dig deeper into Blockly, check out this: developers.google.com/blockly/guides/create-custom-blocks/… \$\endgroup\$ – Avram Dec 6 '17 at 23:58
34
+100
\$\begingroup\$

The implementation of forward is horrible. The only correct way of expressing it is to make all four motors go forward. As an analogy, how would you react if your keyboard manufacturer told you "oh sorry, we made a mistake, you have to type every second letter in uppercase to get a lowercase word"? That would be non-intuitive.

The API you provide must be as clean and simple as possible. You don't want your daughter to learn that "programmers make mistakes and are lazy about fixing them, so we have to live with that".

In which ways is your daughter supposed to change the current program? You should probably add some example code for waiting some time, to allow for composed commands like "forward, wait 1s, right, 1s, backward".

Your code seems to crash with an exception when an unexpected key is pressed. That's unacceptable for a robot, since it might harm people or pets when running uncontrolled (see Asimov's laws).

Using raw_input looks as if the user has to press the Enter key to confirm each action. That's too much of a burden. Controlling the car must be simple and immediate, to prevent accidents. How would you feel if your grown-up street car would ask you for confirmation for each step on the gas or brake?

\$\endgroup\$
  • 1
    \$\begingroup\$ Wow, super good points. It was our start and we are absolutely ready to learn from our mistakes. About the composed commands - nice idea, I was thinking to create a path/road for her to try to get through by making a combination of actions. Interesting take about safety as well. Thanks so much. \$\endgroup\$ – alecxe Dec 6 '17 at 12:43
  • 4
    \$\begingroup\$ I'm not sure a toy car needs to implement Asimov's laws. \$\endgroup\$ – jwg Dec 7 '17 at 13:54
  • 2
    \$\begingroup\$ @jwg I'm definitely blaming you when super intelligent toy cars enslave us all. \$\endgroup\$ – David Dec 10 '17 at 4:54
  • 1
    \$\begingroup\$ Asimov's Laws while very clever are not entirely relevant to actual day to day life (and coding) but other than that, great points \$\endgroup\$ – 13ros27 Dec 15 '17 at 17:57
26
\$\begingroup\$
  • From my experience, kids understand DRY without explanation, and in fact they are very bored by the repetition.

  • which led to some motors accepting "backwards" as "forward"

    This is I am afraid a dire mistake. Fix it as soon as you can, because it may ruin kid's intuition.Even I can't grok why forward method is so asymmetric.

  • Do not rely on magic numbers and default argument values. Defaults are really hard to grasp.

All that said, I believe the question is better suited for CSE exchange.

\$\endgroup\$
  • 4
    \$\begingroup\$ or CodeReview ? \$\endgroup\$ – Walfrat Dec 6 '17 at 12:52
  • 12
    \$\begingroup\$ Children love repetition. It's the number one tool for education. The Teletubbies would almost constantly repeat the same process (taking a certain action) four times, one for each Teletubby. Dora the Explorer keeps repeating the same plan over and over again during an episode. As a kid, I rewatched my favorite movies over and over again, and never once got bored rewatching it (and I paid attention, it wasn't just background noise). I still know most of The Nightmare Before Christmas' script by heart ;) \$\endgroup\$ – Flater Dec 6 '17 at 13:27
  • 1
    \$\begingroup\$ @Flater some do, some don't. I, for one, was extremely quickly bored by anything even remotely repetitious. \$\endgroup\$ – vaxquis Dec 7 '17 at 2:21
  • 2
    \$\begingroup\$ Two year olds like the core Teletubbies audience are a lot more into repetition than six year olds like the OP's daughter. \$\endgroup\$ – jwg Dec 7 '17 at 13:57
5
\$\begingroup\$

I wrote a Pi+python control program so I had a quick look at what I did. Interestingly I noticed that I seem to have had the same problem with forwards and backwards. I found this comment:

# Forwards and backwards are the same for left and right
# The trick is to reverse the motor connections for one  side :-) 

Thus I solved it by swapping the connections for both motors on one side. I could tell you where the code it but it is on a website which has my product and I don't want to advertise. It uses a different motor controller but the principle is the same. Here is the core code:

# Main program

# Get the curses screen
screen = curses.initscr()

..... <omitted code to set up the I/O>

# Tell user what to expect
# in curses print does not work anymore. use addstr
screen.addstr("<prouduct name> example program for Python\n")
screen.addstr("Use numeric keypad keys control\n")
screen.addstr("Left  Both  Right\n")
screen.addstr("     Forward     \n")
screen.addstr("  7     8     9  \n")
screen.addstr("                 \n")
screen.addstr("  4    Stop   6  \n")
screen.addstr("                 \n")
screen.addstr("  1     2     3  \n")
screen.addstr("     Reverse     \n")
screen.addstr("Left  Both  Right\n")
screen.addstr("Don't forget to set numlock on!!!!\n")
screen.addstr("Use Q or q to quit\n")
screen.addstr("\n")


run = 1
while run==1 :
  key = screen.getch() # Key?
  if key==ord('q') :
    run = 0 # stop running

  if key==ord('1') : 
     gb.move_brushed(BOARD,LEFT,BACKW) # Left backwards 

  if key==ord('2') : 
     gb.move_brushed(BOARD,LEFT,BACKW)  # Left backwards 
     gb.move_brushed(BOARD,RIGHT,BACKW) # Right backwards 

  if key==ord('3') : 
     gb.move_brushed(BOARD,RIGHT,BACKW) # Right backwards 

  if key==ord('4') :
     gb.move_brushed(BOARD,LEFT,STOP) # Left stop 

  if key==ord('5') : 
     gb.move_brushed(BOARD,LEFT,STOP)  # Left stop 
     gb.move_brushed(BOARD,RIGHT,STOP) # Right stop 

  if key==ord('6') :
     gb.move_brushed(BOARD,RIGHT,STOP) # Right stop 

  if key==ord('7') :
     gb.move_brushed(BOARD,LEFT,FORWD) # Left forwards 

  if key==ord('8') :
     gb.move_brushed(BOARD,LEFT,FORWD)  # Left forwards 
     gb.move_brushed(BOARD,RIGHT,FORWD) # Right forwards 

  if key==ord('9') :
     gb.move_brushed(BOARD,RIGHT,FORWD) # Right forwards 

# on exist stop everything
gb.emerg_stop()
# Set terminal behaviour normal again
curses.endwin()
\$\endgroup\$
  • \$\begingroup\$ Ah, nice one. I've stumbled upon curses for reading the user inputs - I think I should finally give it a try for this problem. And thanks for the tip to swap the connections. \$\endgroup\$ – alecxe Dec 7 '17 at 14:51
2
\$\begingroup\$

On a different note, in order to not have to press enter after every command you could add the following modification to the main part of the code to use pygame although this would make it less understandable so I would definitely keep both versions.

if __name__ == '__main__':
    GPIO.setwarnings(False)
    GPIO.cleanup()

    try:
        car = Car()
        import pygame

        while True:
            events = pygame.event.get()
            for event in events:
                if event.type == pygame.KEYDOWN:
                    if event.key == pygame.K_UP or event.key == pygame.K_w:
                        car.forward()
                    elif event.key == pygame.K_LEFT or event.key == pygame.K_a:
                        car.forward_left()
                    elif event.key == pygame.K_RIGHT or event.key == pygame.K_d:
                        car.forward_right()
                    elif event.key == pygame.K_DOWN or event.key == pygame.K_s:
                        car.backward()
                    elif event.key == pygame.K_SPACE:
                        car.stop()
    finally:
        try:
            GPIO.cleanup()
        except:  # ignore cleanup errors
            pass

Where wasd or arrow keys controls its movement and space stops it.

With thanks to @Haz on https://stackoverflow.com/questions/16044229/how-to-get-keyboard-input-in-pygame

\$\endgroup\$
  • 1
    \$\begingroup\$ Using pygame is such a nice idea - out-of-the-box thinking, thanks! \$\endgroup\$ – alecxe Dec 15 '17 at 17:55
2
\$\begingroup\$

I think the advice from import this will help you here, especially "Explicit is better than implicit", "Simple is better than complex", "Flat is better than nested" and "Readability counts." In particular, I would note that you are creating a Car class which can only be used as a singleton. But Python modules are already perfectly valid singleton classes, so you can eliminate a level of abstraction and nesting by implementing everything directly at the module level.

I would also agree with others that you must fix the forward/backward problems, and it's a good idea to always specify the motor speed rather than relying on defaults. I like your event stack approach, but I would reference functions directly (not by name) and I would add an extra step to make it easier to see what's happening.

This gives simpler code, like this:

from RPi import GPIO
from gpiozero import Motor, OutputDevice

# setup GPIO
GPIO.setwarnings(False)
GPIO.cleanup()
OutputDevice(5).on()
OutputDevice(17).on()
OutputDevice(25).on()
OutputDevice(12).on()

# setup motor references (with correct forward/backward settings)
front_right = Motor(forward=24, backward=27)
front_left = Motor(forward=22, backward=6)
rear_right = Motor(forward=18, backward=13)
rear_left = Motor(forward=16, backward=23)

# define available commands
def forward(self):
    front_right.forward(1)
    front_left.forward(1)
    rear_right.forward(1)
    rear_left.forward(1)

def backward(self):
    front_right.backward(1)
    front_left.backward(1)
    rear_right.backward(1)
    rear_left.backward(1)

def backward_right(self):
    front_right.backward(1)
    front_left.backward(1)
    rear_right.backward(1)
    rear_left.backward(0.2)

def backward_left(self):
    front_right.backward(1)
    front_left.backward(1)
    rear_right.backward(0.2)
    rear_left.backward(1)

def forward_left(self):
    front_right.forward(1)
    front_left.forward(0.2)
    rear_right.forward(1)
    rear_left.forward(1)

def forward_right(self):
    front_right.forward(0.2)
    front_left.forward(1)
    rear_right.forward(1)
    rear_left.forward(1)

def stop(self):
    front_right.stop()
    front_left.stop()
    rear_right.stop()
    rear_left.stop()

commands = {
    "f": forward,
    "b": backward,
    "s": stop
}

try:
    while True:
        # better to use another library that can use arrow keys and maybe spacebar for stop
        command = raw_input() 
        func = commands[command]
        func()
finally:
    try:
        GPIO.cleanup()
    except:  # ignore cleanup errors
        pass

If you want a DRYer approach, you could do this (at the cost of more abstraction):

from RPi import GPIO
from gpiozero import Motor, OutputDevice

# setup GPIO
GPIO.setwarnings(False)
GPIO.cleanup()
OutputDevice(5).on()
OutputDevice(17).on()
OutputDevice(25).on()
OutputDevice(12).on()

# setup motor references (with correct forward/backward settings)
motors = dict(
    front_right=Motor(forward=24, backward=27),
    front_left=Motor(forward=22, backward=6),
    rear_right=Motor(forward=18, backward=13),
    rear_left=Motor(forward=16, backward=23)
)

# define available commands (dicts of speeds for each motor)
commands = {
    "f": dict(front_right=1, front_left=1, rear_right=1, rear_left=1),
    "b": dict(front_right=-1, front_left=-1, rear_right=-1, rear_left=-1),
    "s": dict(front_right=0, front_left=0, rear_right=0, rear_left=0),
}

def set_motor_speed(motor, speed):
    if speed < 0:
        motor.backward(-speed)
    elif speed == 0:
        motor.stop()
    else:
        motor.forward(speed)

def run_command(command):
    settings=commands[command]
    for motor_name, speed in settings.items():
        set_motor_speed(motors[motor_name], speed)

try:
    while True:
        # better to use another library that can use arrow keys and maybe spacebar for stop
        command = raw_input() 
        run_command(command)
finally:
    try:
        GPIO.cleanup()
    except:  # ignore cleanup errors
        pass
\$\endgroup\$
  • \$\begingroup\$ What do you mean by import this \$\endgroup\$ – 13ros27 Dec 15 '17 at 18:47
  • 1
    \$\begingroup\$ @13ros27, try it! Or see Python PEP 20: python.org/dev/peps/pep-0020 \$\endgroup\$ – Matthias Fripp Dec 15 '17 at 19:01
  • \$\begingroup\$ PEP20 is just my life manifesto! I really like the additional abstractions you’ve presented. Thank you, Matthias. \$\endgroup\$ – alecxe Dec 15 '17 at 19:04
  • \$\begingroup\$ Oh yeah, cool, I never knew that \$\endgroup\$ – 13ros27 Dec 15 '17 at 19:46
  • \$\begingroup\$ @alexce, glad to help! But I think most six-year-olds will have trouble with dicts of dicts and negative-negative numbers (might help to use abs(speed) instead of -speed). Mine certainly isn't ready for those! But at least the 'API' should be pretty easy to use. \$\endgroup\$ – Matthias Fripp Dec 15 '17 at 20:06
1
\$\begingroup\$
class Car:
    def __init__(self):
        self.front_right = Motor(forward=24, backward=27)
        self.front_left = Motor(forward=22, backward=6)

        self.rear_right = Motor(forward=18, backward=13)
        self.rear_left = Motor(forward=16, backward=23)

This modification to the __init__ method should mean that it doesn't matter that you muddled the pins up because I just reversed the ones which are muddled up. NOTE: It still needs the bit of code to switch them on. Also, it may be beneficial if this is to try and help someone to learn coding to add comments to detail what everything does.

\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.