breaking a large program into functions, not acting as expected

[Zane217]

I am working on python code for a rotary encoder. By following others scripts I was able to get a working program but it skipped a lot of counts, by trial and error I was able to put together some code that counted almost all counts. You may notice my code below looks like 9th grade basic from the mid 1980's, that's when I learned to code so when helping assume I know very little. ( I have put together a robot car running python with my son since so I know a little something)

I Ultimately need two encoders running at the same time so I was going to make a custom function to calculate movement for each part of the program to call. In my mind (but not in reality apparently) breaking a working program into multiple parts should just work as nothing is really being changed. However it stopped working. I then made the entire program a function and had the main program call the function and again it ran. I have moved lines from the function to the main program running it each time to find the line that was causing a problem leaving as much in the function as possible.

principle of rotary encoder https://en.wikipedia.org/wiki/Rotary_enc...ncoder.gif

My principle
I am waiting for A_State to rise (1)
Then determining the direction of movement by looking at B_State (1) or (0)
waiting for A_State to drop (0)
then determining weather it dropped on the same side and removing the movement
Or the other side and continuing with the movement.

here is the program the runs correctly.

from RPi import GPIO
from time import sleep
import csv
from time import sleep, strftime, time

A = 17
B = 18
GPIO.setmode(GPIO.BCM)
GPIO.setup(A, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
GPIO.setup(B, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)

counter = 0
AState = GPIO.input(A)

while True:
    while AState == 0:
        AState = GPIO.input(A)
    BState = GPIO.input(B)
    if BState == 1:
            counter += .5
    if BState == 0:
            counter -= .5
    while AState == 1:
            AState = GPIO.input(A)
    BState = GPIO.input(B)
    if BState == 1:
            counter -= .5
    if BState == 0:
            counter += .5
    print (int(counter))

GPIO.cleanup()

The similar code calling a function that does not run correctly
it runs without errors but the output is incorrect

from RPi import GPIO
from time import sleep
import csv
from time import sleep, strftime, time

A = 17
B = 18
GPIO.setmode(GPIO.BCM)
GPIO.setup(A, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
GPIO.setup(B, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)

counter = 0
AState = GPIO.input(A)

def encoder(A_S, newcounter, A ,B):
    B_S = GPIO.input(B)
    if B_S == 1:
        newcounter += .5
    if B_S == 0:
        newcounter -= .5
    while A_S == 1:
        A_S = GPIO.input(A)
    B_S = GPIO.input(B)
    if B_S == 1:
        newcounter -= .5
    if B_S == 0:
        newcounter += .5
    return newcounter

while True:
    while AState == 0:
        AState = GPIO.input(A)
    Ncounter = encoder(AState, counter, A, B)
    counter = Ncounter
    print (counter)

GPIO.cleanup()

The output of the second code starts paused, appears to be waiting for AState to change from 0
with a small movement on the encoder.
It outputs numbers either increasing or decreasing even if the encoder is not changing.


clearly my logic is not correct, sorry this is hard to reproduce without an encoder, I am hopping there is a simple logic step I am missing.

Extra information
I am on a raspberry pi 3B+ (I think) running MU 1.0.2
Sorry there is a little extra slop in the code as I have been testing different things trying to get it to work.

As always thank you for taking the time to read and assist me in my learning experience.

Gary

[deanhystad]

Like this?

from RPi import GPIO
 
GPIO.setmode(GPIO.BCM)
 
class Encoder():
    '''I monitor two quadrature encoder signals, a and b'''
    def __init__(self, a, b):
        GPIO.setup(a, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
        GPIO.setup(b, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
        self.a = a
        self.b = b
        self.a_state = 0
        self.b_state = 0
        self.count = 0
        self.reset()

    def reset(self):
        '''Reset count to zero and reset state'''
        self.count = 0
        self.a_state = GPIO.input(self.a)
        self.b_state = GPIO.input(self.b)

    def udpate(self):
        '''Call frequenly to monitor inputs'''
        a_state = GPIO.input(self.a)
        if a_state == self.a_state:
            return  # A did not change.  Do not change count

        # A changed.  Update count,
        self.a_state = a_state
        b_state = GPIO.input(self.b)
        self.count += 0.5 if b_state != self.b_state else -0.5
        self.b_state = b_state

count = 0
encoder = Encoder(17, 18)
while True:
    encoder.update()
    if encoder.count != count:
        count = encoder.count
        print(count)
 
GPIO.cleanup()

This code has a little state machine that replaces your loops. update() only changes the count when the new value of A is different than the previous value of A, otherwise it does nothing. If you don't wait (looping is waiting) you don't have to worry about missing an edge while you are waiting.

[Zane217]

deanhystad

Thank you for taking the time to help me. your code looks nothing like Basic so I do not understand 90% of it. I of course quickly copied it over to run it and try to figure out how it all works. Upon running the program I get a

Error:
Traceback (most resent call last):
   File  "home/pi/mu_code/test 4.py" , line38, in <module>
      encoder.update()
AttributeError:  'Encoder"  object has no attribute 'update'

I have tried to google it to see if I could make a quick change and fix it but my lack of understanding of how this program works has smoke coming out of my ears. I will spend some time to look up the class command (line 5)
as well as all the variables with "self" in front. Do you know why I am getting the error? I would like to see how well this code counts. I found 2 or 3 versions of encoder code online and none of them counted very well. The code I had was working the best, although it was still missing counts when spun fast and I am sure it can be refined a lot. I am hoping your code is even better.

Thank you for your time and for writing the code above.

Gary

[deanhystad]

I don't have RPi, so the code is completely untested. I made a GPIO stub that doesn't do anything (A and B inputs never change), but at least it lets me run the code. There was a typo, def udpate() should be def update(). I also found an error in how I implemented your algorithm. This is what I think your code does:

old_state = AState
while old_state == AState:
    AState = GPIO.input(A)
BState = GPIO.input(B)
if BState == AState:
    counter += .5
else:
    counter -= .5

Here is a corrected and annotated version of the code:

from RPi import GPIO

# This is a class.  It combines data and functions.  This class implements your encoder logic
class Encoder():
    '''I monitor two quadrature encoder signals, a and b'''

    # This is a special method that is called when you make an Encoder instance.
    def __init__(self, a, b):
        '''Initialize instance of class.  a and b are the input ID's for the two quadrature inputs'''
        GPIO.setup(a, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)  # Setup input A
        GPIO.setup(b, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)  # Setup input B
        self.a = a  # Save input ID's for A and B so I can use it later
        self.b = b
        self.a_value = 0  # These are instance variables.  They are like regular variables but they live
        self.count = 0    # in special scope (object scope) and you need to precede them with self.
        self.reset()  # Reset the counter

    # You could call Encoder.reset() to reset the encoder count to zero
    def reset(self):
        '''Reset count to zero and reset state'''
        self.count = 0  # Reset the counter
        self.a_value = GPIO.input(self.a)  # Reset state to reflect current inputs

    # You should call this method frequently or it might miss counts
    def update(self):  # This had a typo.  Was udpate instead of update
        '''Call frequenly to monitor inputs'''
        a_value = GPIO.input(self.a)  # Get the value for input A
        if a_value == self.a_value:   # If the value is the same as last time return out of this function/method
            return None               # Return None to signify the count did not change

        # If we get here, A changed value
        self.a_value = a_value        # Save the value for input A
        b_value = GPIO.input(self.b)  # Get the value for input B
        if b_value == self.a_value:
            self.count += 0.5         # If A and B same, add 0.5, else subtract 0.5
        else:
            self.count -= 0.5
        return self.count             # Return the new count

GPIO.setmode(GPIO.BCM)

# This makes two encoders and puts them in a dictionary.
# Encoder One uses inputs 17 and 18.  Encoder Two uses inputs 19 and 20.
encoders = {'One': Encoder(17, 18), 'Two': Encoder(19, 20)}
while True:
    for id, encoder in encoders.items():  # Loop through all encoders in the dictionary
        count = encoder.update()
        if count is not None:
            print(id, count)  # Print count if it changed

GPIO.cleanup()

I modified the example to make two Encoder objects which I named "One" and "Two". "One" uses inputs 17 and 18 as the quadrature inputs. "Two" uses inputs 19 and 20 (Don't know if that is valid or not). I also modified the update method to return the encoder count if it changed since the last update, or None if it did not change.

[Zane217]

Holly cow, thank you!!

I had to pull the
GPIO = RPiGPIO() line out. it was giving an error and did not look like it was really doing anything. The code runs but is always increasing even when spinning backwards. let me look through it and try to understand what is going on, maybe I can figure it out.

I looked through the code before and should have seen the typo, I think i was overwelled by a lot of it.

It may take some time as this is for my job but not part of my job, (trying to better myself) If i can not figure out why it is always increasing I will get back to you.

It is definitely hard to figure it out if you do not have the pi with an encoder hooked up, you cannot try it to see if it is acting correctly. I appreciate the effort and the skill.

Gary

[deanhystad]

That created my stub. I just updated my post and removed that. Take a look at the my last post. I think I have the logic off on when the encoder counts up and when it counts down. See the last post to see if you agree with my interpretation of your working code.

[deanhystad]

If you find classes intimidating this is the same code written to use a dictionary to store all the encoder info.

from RPi import GPIO

def make_encoder(A, B):
    '''Create an encoder object, a dictionary that stores all the info for an encoder'''
    encoder = {'A':A, 'B':B, 'state':0, 'count':0}
    GPIO.setup(A, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
    GPIO.setup(B, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
    reset_encoder(encoder)
    return encoder

def reset_encoder(encoder):
    '''Reset encoder count to zero'''
    encoder['count'] = 0
    encoder['state'] = GPIO.input(encoder['A'])

def update_encoder(encoder):
    '''Update encoder count.  Return count if value changed, else return None'''
    a_value = GPIO.input(encoder['A'])
    if a_value == encoder['state']:
        return None  # Waiting for input A to change

    encoder['state'] = a_value
    if a_value == GPIO.input(encoder['B']):
        encoder['count'] += 0.5
    else:
        encoder['count'] -= 0.5
    return encoder['count']

GPIO.setmode(GPIO.BCM)
encoder = make_encoder(17, 18)
while True:
    count = update_encoder(encoder)
    if count is not None:
        print(count)
 
GPIO.cleanup()

[Zane217]

I have been working on the other code trying to understand it. I can follow the logic but can not really change much because it is over my head.
you new code using a dictionary works both in the up and down direction and looks easier for me to follow. I only have 1 encoder at this time but will order another soon to see if both will work. (there was no point in getting two if I could not get the first to work.

As I was thinking about how it was all going to work the problem with waiting during the While loops hit me, I would definitely miss counting one encoder while the other was waiting.

I have been on sites working with encoders, I would not do this without your permission, can I post your code to help others?

I will work on you new code. Just for extra information I took another stab at the code from a fresh start. this code works and there are no while loops. Not as elegant as your code.

#import libraries

from RPi import GPIO
from time import sleep

#----------------------------------  pin setup
#pin numbers
enc1_A = 17     
enc1_B = 18
enc2_A = 19
enc2_B = 20

GPIO.setmode(GPIO.BCM)
GPIO.setup(enc1_A, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
GPIO.setup(enc1_B, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
GPIO.setup(enc2_A, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)
GPIO.setup(enc2_B, GPIO.IN, pull_up_down=GPIO.PUD_DOWN)

#----------------------------declair variables

#encoder counters and states
enc1_counter = 0
enc1_AState1 = GPIO.input(enc1_A)
enc1_AState2 = enc1_AState1

enc2_counter = 0
enc2_AState1 = GPIO.input(enc2_A)
enc2_AState2 = enc2_AState1

#placeholder for printing only once
i = 0

#encoder logic fuction
#returns the increase in counts
#runs if AState1 has changed

def encoder_logic(B):
    change_counter = 0              #sets the change in counts to 0
    BState = GPIO.input(B)          #looks at BState
    if BState == 1:                 #Determines which side of pulse A the change in AState occured
        change_counter += .5        #changes the change_counter acordingly
    if BState == 0:                 #the negative is taken in the reverse direction
        change_counter -= .5
    return (change_counter)

while True:                                                 #for encoder 1
    enc1_AState2 = enc1_AState1                                 #passes the new AState to the Old AState
    enc1_AState1 = GPIO.input(enc1_A)                           #gets new AState
    if enc1_AState1 == 1 and enc1_AState2 == 0:                 #checks if AState changed up
        i = 0                                                   # resets i if there was a change so it can print
        enc1_counter = enc1_counter + encoder_logic(enc1_B)     #changes counts
    if enc1_AState1 == 0 and enc1_AState2 == 1:                 #checks if the AState whent down
        enc1_counter = enc1_counter - encoder_logic(enc1_B)     #changes counts
                                                            #for encoder 2
    enc2_AState2 = enc2_AState1                                 #passes the new AState to the Old AState
    enc2_AState1 = GPIO.input(enc2_A)                           #gets new AState  
    if enc2_AState1 == 1 and enc2_AState2 == 0:                 #checks if AState changed up
        enc2_counter = enc2_counter + encoder_logic(enc2_B)     #changes counts
    if enc2_AState1 == 0 and enc2_AState2 == 1:                 #checks if the AState whent down
        enc2_counter = enc2_counter - encoder_logic(enc2_B)     #changes counts
    
    if enc1_counter % 10 == 0 and i == 0:      #prints data if encoder 1 changed 100 counts and has not been prited already
        i = 1
        print ("encoder one ", int(enc1_counter), "    encoder two", int(enc2_counter))

GPIO.cleanup()

[deanhystad]

I don't understand why you incremenr by 0.5 instead of 1. It doesn't really matter since additional scaling is required to convert counts to engineering units. I am used to encoders counting by 1.

Feel free to do anything you want with the code. I fixed the counter logic in the class version so it should work now. Classes are a better choice and the dictionary version was only to help understand what the class does. The class combines data, the dictionary, with functions into one cohesive package.

[Zane217]

The reason for the .5 increment was there are 600 pulses per revolution, half up and half down gives 1 count per pulse, but it does not really matter. originally I was trying to see if counts were being skipped so it was easier to count 1 for each pulse. The early programs I tried missed a lot of counts.

The dictionary version looks easier to follow so I will start to alter that code and see if I can figure it all out. If I am ambitious Ill try the same with the class code. The best way for me to figure it all out is to change the code and see what works and what does not.

thanks again.