I haven’t convinced myself why you don’t want that interrupt handler attached to both pins.
not a matter of don’t want, a matter of didn’t notice! I’m guessing the processor that ran that code originally didn’t have enough interrupts?
I’ll try it, and see if it helps.
1 Like
I wonder whether the author of the code you were referencing didn’t need it because both pins were on the same port so it was actually firing on every change on the port. I find that code hard to reason about.
attachInterrupt(digitalPinToInterrupt(pin), ISR, mode)(recommended)
attachInterrupt(interrupt, ISR, mode)(not recommended)
attachInterrupt(pin, ISR, mode)(Not recommended. Additionally, this syntax only works on Arduino SAMD Boards, Uno WiFi Rev2, Due, and 101.)
Note that it recommends digitalPinToInterrupt() because there are fewer interrupts than pins; interrupts are per port not per pin. You might print out (or look up) the values of digitalPinToInterrupt(encoderPin1) and digitalPinToInterrupt(encoderPin2) — if they are different, it would matter, but they might have been the same on the board being used by the author of the reference.
Also, that doEncoderMotor() handler won’t notice missed values. There is a general approach to gray code decoders that handle different lengths (here, it’s just length 2) that is clearer.
I have not tested this, but I’d expect something like this to be easier to reason about:
void doEncoderMotor() {
static int previous = 0;
int gray = (digitalRead(encoderPin2)<<1) | digitalRead(encoderPin1);
int cur = gray ^ gray >> 1; // convert to binary
if abs(cur-previous) > 1 {
Serial.println("Lost encoder step");
}
// cur-previous == 0 does not imply missing 4 steps; there could be other interrupts on the same port
encoderPos += cur-previous;
previous = cur
}
Pin reading vs. Port reading
Note that the two digital reads are separated in time. As long as the interrupt handler is fast enough relative to motor speed, you will be fine. If I were an embedded engineer working on this for production, I’d be ensuring that both pins for each encoder were on adjacent bits on the same processor IO port, and I’d be doing a single port read and masking out the pins. In pseudo-code, if they were the third and fourth pins on portB, it would be something like gray = portB & 0xc >> 2. This is something that Arduino’s abstraction away from ports can hide, by pretending that all the pins are individually addressable from code.
The ports are still accessible, but it does make code less portable across Arduino board implementations; you need to look up the documentation for the exact implementation you are using. Here’s an article covering original arduino hardware, that won’t apply directly to any particular teensy:
You’d have to look into how to do the same for the teensy.
If you eventually choose to use the port read method, you’ll need to look into the documentation the particular Teensy you are using to find out how to do whole port reads, and you’ll need to do that before deciding on the final pins to use.
2 Likes
Wow, McD, school me!
yikes! NOTHING there I understand 
I’ll dig in here. I never did get bit-shifting, I’ll try again…
THANKS!!
Edit: wow, Gray code is like binary on acid! I am seriously tripping out…
Edit: I said that BEFORE I read about it as a traversal of vertices of a tesseract, or the image of it on the number line. It always amazes me when a search for a practical solution yields something this beautiful.
Edit: Reading about ports: Well, I didn’t expect so little code portability, but the process of figuring this out for teensy will no doubt help with whatever processor I end up using. I liked the solution I pulled the code from, which uses the STM32, in that it is extremely compact. But I don’t necessarily need to be THAT compact…
1 Like
Well, you just saw where the Processing / Wiring abstraction breaks down. It’s amazing how well it does work, but sometimes you can see through the cracks that the hardware differs from one implementation to another.
I was wrong to try to use arithmetic. That was silly of me. But I do think that it would be more robust comparing old to new values instead of assuming that it owns the interrupts.
Again I’m not testing anything, just writing off the cuff without thinking through, but something like this?
void doEncoderMotor() {
static int prev_gray = 0;
int gray = (digitalRead(encoderPin2)<<1) | digitalRead(encoderPin1);
if gray == prev_gray {
// spurious interrupt?
return
}
if gray == (~prev_gray | 0x3) {
// off by two positions; missed a step or just booted and read the default
if encoderPos == 0 {
// probably happened to boot at this reading
// ignore possibility of a lost encoder step at exactly position 0
prev_gray = gray;
}
// Otherwise, if cur and previous are inverse, we have missed an intermediate step
Serial.println("Lost encoder step");
// here you have to decide whether you want an error
return
}
if ( !(prev_gray & 1) && (gray & 1) ) {
if (gray & 2) {
encoderPos--;
} else {
encoderPos++;
}
} else {
if (gray & 2) {
encoderPos++;
} else {
encoderPos--;
}
}
prev_gray = gray;
}
Probably won’t even compile. Sorry.
well, if the logic is solid I can probably get it to compile. But understanding the logic is a whole other thing… I was trying to find info on how to attach the interrupts on the teensy, since it seems like it’s dependent on the hardware.
I read the attach interrupt page on the teensy, and I really can’t follow it–the use example is too dissimilar, and I couldn’t figure out how to read the table. But then I found this:
https://www.pjrc.com/teensy/td_libs_Encoder.html
Using the example, I wrote this code:
#include <Encoder.h>
int pwmPin = 16;
int motor1pin1 = 17;
int motor1pin2 = 18;
int encoderPin1 = 8;
int encoderPin2 = 9;
Encoder enc(encoderPin1,encoderPin2);
void setup() {
// put your setup code here, to run once:
pinMode(motor1pin1, OUTPUT);
pinMode(motor1pin2, OUTPUT);
Serial.begin(9600); // Other baud rates can be used...
Serial.println("encoder test");
//attachInterrupt(encoderPin1, doEncoderMotor, CHANGE); // encoderA pin on interrupt
}
long encPosition = -999;
void loop() {
// put your main code here, to run repeatedly:
long newEnc;
newEnc = enc.read();
if (newEnc != encPosition) {
Serial.print("position = ");
Serial.print(newEnc);
Serial.println();
encPosition = newEnc;
}
//value = analogRead(wiperPin);
//Controlling speed (0 = off and 255 = max speed):
analogWrite(pwmPin, 255); //ENA pin
//Controlling spin direction of motors:
digitalWrite(motor1pin1, HIGH);
digitalWrite(motor1pin2, LOW);
delay(100);
digitalWrite(motor1pin1, LOW);
digitalWrite(motor1pin2, LOW);
delay(1000);
digitalWrite(motor1pin1, LOW);
digitalWrite(motor1pin2, HIGH);
delay(100);
digitalWrite(motor1pin1, LOW);
digitalWrite(motor1pin2, LOW);
delay(1000);
}
But I am only getting values of 0 or 1 in the serial output. I’m wondering if the encoder on this motor even works, or if it works the way anybody expects an encoder to work. Maybe I should buy a motor and encoder from polulu. I need to try their stronger motor anyway.
EDIT: it occurs to me I should test that the encoder is putting out square waves. Any thoughts about how to do that without an oscilloscope? I don’t seem to have frequency on my multi-meter.
EDIT: wondering if the documentation on the encoder is wrong. It sure seems strange that the black wire is +.
Oh, a working and tested library is a much better choice!
Well, if you turn the encoder by hand slowly, you should see voltage change on each of the signal wires, if you have encoder power connected to it.
I wouldn’t count on wire color coming from a connector like that mapping to specific lines.
Can you see the circuit on the encoder?
1 Like
okay, tried to measure the outputs of the encoder, and one of them broke off. I think these were the last two JST connectors I did before I learned not to squeeze the crimper all the way, but only the minimum until it releases…
Sucess!!! Wonderful values in the serial monitor. So now on to PID, I guess…
This is starting to feel like something that might eventually work
3 Likes
So I installed Brett’s PID library mentioned in this thread. The documentation for the constructor looks like this:
PID()
Description
Creates a PID controller linked to the specified Input, Output, and Setpoint. The PID algorithm is in parallel form.
Syntax
PID(&Input, &Output, &Setpoint, Kp, Ki, Kd, Direction)
PID(&Input, &Output, &Setpoint, Kp, Ki, Kd, POn, Direction)
Parameters
Input: The variable we're trying to control (double)
Output: The variable that will be adjusted by the pid (double)
Setpoint: The value we want to Input to maintain (double)
Kp, Ki, Kd: Tuning Parameters. these affect how the pid will change the output. (double>=0)
Direction: Either DIRECT or REVERSE. determines which direction the output will move when faced with a given error. DIRECT is most common.
POn: Either P_ON_E (Default) or P_ON_M. Allows Proportional on Measurement to be specified.
Returns
None
So if I understand correctly, Setpoint is my pedal position, input comes from the encoder, and output goes to the PWM pin for the motor.
Here’s some test code. The first time I upload it, the motor moves briefly in response to the first movement of the pot, but subsequently does nothing. The teensy is correctly showing values from the pot. However, if I move the motor a bit (to make sure the encoder is working) it suddenly moves for a quarter second or so… So basically nothing is happening without a change in encoder values… which makes me wonder if I have the variables assigned wrong.
Edit: on startup, the encoder is moving to a value around 1000…
#include <PID_v1.h>
#include <Encoder.h>
int wiperPin = 14;
int pwmPin = 16;
int motor1pin1 = 17;
int motor1pin2 = 18;
int encoderPin1 = 8;
int encoderPin2 = 9;
int value = 0;
//Define Variables we'll be connecting to
double Setpoint, Input, Output;
//Specify the links and initial tuning parameters
PID pid1(&Input, &Output, &Setpoint,2,5,1, P_ON_M, DIRECT);
Encoder enc(encoderPin1,encoderPin2);
void debug(String var, int val) {
Serial.print(var);
Serial.print(" = ");
Serial.println(val);
}
void drive(int vel) {
//Serial.print("vel = ");
//Serial.println(vel);
if (vel > 10) {
digitalWrite(motor1pin1, HIGH);
digitalWrite(motor1pin2, LOW);
analogWrite(pwmPin, vel); //ENA pin
}
else if (vel < -10) {
digitalWrite(motor1pin1, LOW);
digitalWrite(motor1pin2, HIGH);
analogWrite(pwmPin, vel); //ENA pin
}
else {
digitalWrite(motor1pin1, LOW);
digitalWrite(motor1pin2, LOW);
}
}
void setup() {
// put your setup code here, to run once:
pinMode(motor1pin1, OUTPUT);
pinMode(motor1pin2, OUTPUT);
Serial.begin(9600); // Other baud rates can be used...
Serial.println("encoder test");
Setpoint = 0;
pid1.SetOutputLimits(-255,255);
pid1.SetMode(AUTOMATIC);
}
long encPosition = 0;
void loop() {
long newEnc;
newEnc = enc.read();
if (newEnc != encPosition) {
encPosition = newEnc;
debug("encoder",encPosition);
}
Input = encPosition;
int wiper;
wiper = analogRead(wiperPin);
if (wiper != Setpoint) {
Setpoint = wiper;
debug("wiper",Setpoint); //this value is changing 0-1024
}
pid1.Compute();
drive(Output);
Beyond that, I’m trying to get my head around what I need the PID to do actually, never mind the values for tuning it… Here are some thoughts that I don’t yet have any idea how to approach in code…
The idea is to track as closely as possible the movement of the pedal. This movement might be fast, faster than the maximum speed of the motor, so there will be some lag. But sometimes the movement will be a good bit slower than the maximum rate that the motor can change the pitch, and the algorithm should be able to follow the movement more accurately. I’m concerned that if the movement is TOO slow, the motor may struggle to move slowly enough. With narrow PWM values, the motor sure does whine loudly! Doesn’t sound good for the motors… but go too slowly, and the motor doesn’t move at all.
Both end points of the travel will be known positions, (one of which will be the startup position, zero) and I’m wondering if this information could be useful to the PID algorithm, and if so, how that is expressed…
I think I do want P_ON_M, as that is used to address overshoot, and that seems desirable in this situation. Overshoot is not going to be musical at all.
The encoder has at least 10x the accuracy (possibly 100x?) I’ll need, so I want to make sure that it doesn’t oscillate around needlessly if the error is small.
Input will be the encoder-informed absolute position, setpoint will be the desired encoder-informed absolute position, and output will control PWM.
I haven’t read your code yet in detail, so just making sure it’s clear.
The inertia in the motor and screw is low. You want to move as fast as you can. This means that you want a large P term. This screw won’t back-drive, so I would expect a 0 D term. Then make the I term as small as you can and be happy with the resulting sound. Debug that by ear not by Serial.print 
Edit: I’d suggest starting with 0 I and D terms and seeing whether you get audible overshoot.
You can also establish a minimum speed at which you try to move. Map the outputs from the algorithm into that range. So if you have a slowest PWM value that works reliably to move, map that to 0. Arduino has a map function you can use to calculate this, or you can use a function if you want a non-linear mapping. Edit: note that map only works on integers; if you need to work in double then it’s just a simple linear equation. In any case, rather than ignoring vel values in the [-10, 10] range you should make it move at the minimum speed when you aren’t at the target. Ignoring low vel values will just make it inaccurate.
The whine isn’t bad for the motor. It is in a range that won’t be useful for your end goal, but you aren’t hurting the motor with experiments in that range.
Doing PWM on a negative number is not meaningful (See the analogWrite() documentation). Take a look at this forum post:
You’ll notice that you need to invert the sign of vel before passing it to analogWrite().
I don’t actually know what happens when you pass a negative number, but if it’s just using bit masks you might get a lot of sign-extended 1 bits in the negative number and go faster than you intend. (See Two's complement - Wikipedia for how this works if you don’t already know.)
okay, so the range of the PID is -255 to 255, but I have to convert that to a combination of motor control and pwm. Okay, I get that. Just invert negative velocity. But the -255 to 255 range does make sense then…
What is a ‘large’ P? 255? or more like 10?
Here’s some code with a P of 5. It is just oscillating constantly, which is better than nothing I guess. Goes from min to max about 4 times a second. At either extreme of the pot, the rate of oscillation doubles.
#include <PID_v1.h>
#include <Encoder.h>
int wiperPin = 20;
int pwmPin = 16;
int motor1pin1 = 17;
int motor1pin2 = 18;
int encoderPin1 = 8;
int encoderPin2 = 9;
int value = 0;
int minSpeed = 30;
int pwmVal = 0;
//Define Variables we'll be connecting to
double Setpoint, Input, Output;
//Specify the links and initial tuning parameters
PID pid1(&Input, &Output, &Setpoint,5,0,0,DIRECT);
Encoder enc(encoderPin1,encoderPin2);
void debug(String var, int val) {
Serial.print(var);
Serial.print(" = ");
Serial.println(val);
}
void drive(int vel) {
// oscillating from min to max constantly...
if (vel > 0) {
pwmVal = map(vel,0,255,minSpeed,255);
digitalWrite(motor1pin1, HIGH);
digitalWrite(motor1pin2, LOW);
analogWrite(pwmPin, pwmVal); //ENA pin
}
else if (vel < 0) {
pwmVal = 0 - map(vel,-255,0,-255,-minSpeed);
digitalWrite(motor1pin1, LOW);
digitalWrite(motor1pin2, HIGH);
analogWrite(pwmPin, pwmVal); //ENA pin
}
else {
digitalWrite(motor1pin1, LOW);
digitalWrite(motor1pin2, LOW);
}
}
void setup() {
// put your setup code here, to run once:
pinMode(motor1pin1, OUTPUT);
pinMode(motor1pin2, OUTPUT);
Serial.begin(9600); // Other baud rates can be used...
Serial.println("encoder test");
Setpoint = analogRead(wiperPin);
pid1.SetOutputLimits(-255,255);
pid1.SetMode(AUTOMATIC);
}
long encPosition = 0;
void loop() {
long newEnc;
newEnc = enc.read();
if (newEnc != encPosition) {
encPosition = newEnc;
debug("encoder",encPosition);
}
Input = encPosition;
int wiper;
wiper = analogRead(wiperPin);
if (wiper != Setpoint) {
Setpoint = wiper;
// debug("wiper",wiper); //this value is changing 0-1024
}
pid1.Compute();
drive(Output);
}
so I think I’ve got it set up wrong somehow.
Future thought:
Once we have a good enough prototype, we could help spread the word by setting it up in a music store in Nashville and letting steel players use it to demo new copedents. A single guitar could have a lot of value to a lot of musicians.
well, if someone is interested in manufacturing these, let me know. Not really concerned about spreading the word, I’m just looking to build one for myself
Why not:
pwmVal = map(vel, -255, 0, minSpeed, 255);
Does it do this regardless of the wiper setting? Have you tried printing both the encoder and wiper values and comparing what it is doing?
I suggest setting speed 115200 rather than 9600 for debugging.
would that work? Doesn’t it map -255 to the slowest speed?
Boy I already have thousands of entries to sort through in the serial monitor. Do I really want 10x that number?
I have looked at both, but as far as comparing? the wiper moves predictably as I move the pot, the encoder value oscillates continuously regardless, but if P is low enough, it will double the frequency of oscillation when wiper is below about 150 or above about 850. Maybe I could sort through thousands of lines of monitor to tell you those values more precisely, would that help??? If P is above 5, it just goes full tilt all the time. I’m at a bit of a loss to say more… nothing in the printout seems at odds with what the motor or what the pot are doing…
I found the google group for Brett’s PID, I’ll try asking there…
I just failed to finish inverting arguments… I meant
pwmVal = map(vel, 0, -255, minSpeed, 255);
You get the same number of console messages, just faster; it’s just making it less likely that the system will be waiting for I/O and affecting your results.
Makes sense to ask for PID help from folks with experience with the library! 
Wow, it’s 29 days later!
My work got busy, which is great, but it means I have not had much time to put into this…
I did install the new motor and set up some pots to control PID settings, and 3d printed a bracket to mount it all. I’ve had time to plug it in and ascertain that it does not work. So now I need to go through and check my connections.
I also have learned a great deal of fusion 360, and modeled the mechanism about as far as makes sense until I know how/whether the motor is going to work.
Meantime, I need to do my taxes, which I am putting off by responding to emails, such as the one about your post above
In short, I have not given up, but progress has definietly slowed! Hoping to get some more time early April, but may not be much happening before then. I will keep you posted!
2 Likes
There are way worse things than work being busy! I was just curious and really enjoyed the thread so far.
1 Like
Slightly off topic but related.
A 3 string steel guitar made from Harley Davidson motorcycle parts!
2 Likes