Part 2

It was almost working so posted this message and got a working version.

I wrote:Using your improved version it usually prints 530 ticks but occasionally print a 512 count instead. I notice the motors seem to brake when the 512 occurs.

ChatGPT replied:

That symptom strongly suggests your encoder count is occasionally being corrupted while being read.

Because counter2 is a 32-bit long on an 8-bit AVR Arduino, reads/writes are not atomic.

Your ISR updates counter2 while the main loop is simultaneously reading it in:

while (counter2 < 530)

If the interrupt fires during the 4-byte read, you can occasionally get a corrupted value like 512.

That explains:

• sudden premature braking

• occasional incorrect counts

• mostly working otherwise

Proper fix

Copy the volatile counter into a local variable with interrupts disabled.

Instead of:

while (counter2 < 530) {
}

do:

long c2;

do {
  noInterrupts();
  c2 = counter2;
  interrupts();
} while (c2 < 530);

Full corrected approach

void loop()
{
  if (!digitalRead(btn0))
  {
    noInterrupts();
    counter1 = 0;
    counter2 = 0;
    interrupts();

    forwardA(240);
    forwardB(240);

    long c2;

    // Fast section
    do {
      noInterrupts();
      c2 = counter2;
      interrupts();
    } while (c2 < 500);

    // Slow section
    forwardA(120);
    forwardB(120);

    do {
      noInterrupts();
      c2 = counter2;
      interrupts();
    } while (c2 < 530);

    activeBrake();

    long c1;

    noInterrupts();
    c1 = counter1;
    c2 = counter2;
    interrupts();

    Serial.println("==========");
    Serial.print(c1);
    Serial.print("  ");
    Serial.println(c2);

    delay(1000);
  }
}

Why 512 specifically?

512 (0x0200) is a classic sign of a torn multi-byte read on AVR systems.

Example:

• actual value changes from 0x01FF to 0x0200

• main code reads some bytes before ISR update

• remaining bytes after ISR update

• resulting mixed value can become incorrect

Very common with:

• int32_t

• long

• float

• timer variables

• encoder counters

Additional improvements

Use ATOMIC_BLOCK

Cleaner and safer:

#include <util/atomic.h>

long c2;

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    c2 = counter2;
}

This is the preferred AVR method.

Avoid activeBrake() if the driver dislikes it

Some motor drivers momentarily brown out or inject noise when braking aggressively.

You can test:

turnOffMotorA();
turnOffMotorB();

instead of active braking.

If the 512 issue disappears, electrical noise may also be contributing.

Add encoder debouncing/noise filtering

Motor braking generates EMI.

Possible improvements:

• twisted encoder wires

• pullups closer to encoder

• shielded cable

• 0.01 µF capacitor to ground

• Schmitt trigger buffer

• hardware quadrature decoder

But the atomic read issue is the first thing to fix.

Although a brief suspension of interrupts may have a negligible effect, any interrupts during the suspension will be lost and eventually this may add up and you would need to consider the effect of whatever else is going on with your program not just the encoder counts.

So to tempt you to go down a rabbit hole you might like to consider using a rpi pico as your microcontroller and use its PIO state machines for the encoder counts.  

Try putting "how to code a rpi pico to use PIO with DMA for motor encoder counts" into your AI 

My AI says:

"

to code a Raspberry Pi Pico to use PIO with DMA for motor encoder counts, you can set up a PIO state machine to read the encoder signals and use DMA to transfer the data without CPU intervention. This allows for efficient counting of encoder pulses while minimizing processing delays.

Alternative methods to read motor encoder counts on the Raspberry Pi Pico include using a quadrature encoder with a hybrid approach that optimizes calculations for both high and low speeds, and implementing a method that calculates the time between impulses instead of counting them. These methods can improve accuracy and responsiveness in reading motor speeds.

"

And of course you can then ask the AI for example C++ code

@byron 

An important point here is I am using AI as an educational tool.

For these experiments I will be sticking to the old 2560 Arduino Board because I have it all wired up and working with the aim of getting precise encoder reading and motor control.

The next experiment on the todo list,

This is the code I copied from the video ... have yet to try it maybe tomorrow. Not sure if there are any copy errors yet.

#define ENCA 2 // yellow
#define ENCB 3 // WHITE
#define PWM 5;
#define IN2 6;
#define IN1 7;

int pos = 0;
long prevT = 0;
float eprev = 0;
float eintegral = 0;


void setup() {
  Serial.begin(9600);
  pinMode(ENCA,INPUT);
  pinMode(ENCB,INPUT);
  attachInterrupt(digitalPinTOInterrupt(ENCA),readEncoder,RISING);
}

//void loop(){
//  setMotor(1,25,PWM,IN1,IN2);
//  delay(200);
//  Serial.println(pos);
//  setMotor(-1,25,PWM,IN1,IN2);
//  delay(200);
//  Serial.println(pos);
//  setMotor(0,25,PWM,IN1,IN2);
//  delay(200);
//  Serial.println(pos);
//}

void loop(){
  // set target position
  int target = 1200;

  // PID constants
  float kp = 1;
  float kd = 0.025;
  float ki = 0;

  // time difference
  long currt = micros();

  float deltaT = ((float)(currT-prevT))/1.0e6;
  prevT = currT;

  // error
  int e = pos - target;

  // derivative
  float dedt = (e-eprev)/(deltaT);

  // integral
  eintegral = eintegral + e*deltaT;

  // control signal
  float u = kp*e + kd*dedt + ki*eintegral;

  // motor power
  float pwr = fabs(u);
  if (pwr>255){
    pwr = 255;
  }

  // motor direction
  int dir = 1;
  if (u<0){
    dir = -1;
  }

  // signal the motor
    setMotor(dir, pwr, PWM, IN1, IN2);

   // store previous error
   eprev = 0;

   Serial.print(target);
   Serial.print(" ");
   Serial.print(pos);
   Serial.println();

}


void setMotor(int dir, int pwmVal, int pwm, int in1, int in2){
  analogWrite(pwm,pwmVal);
  if (dir == 1){
    digitalWrite(in1,HIGH);
    digitalWrite(in2,LOW);
  }
  else if(dir == -1)
    digitalWrite(in1,LOW);
    digitalWrite(in2,HGIH);
  }
  else{
    digitalWrite(in1,LOW);
    digitalWrite(in2,LOW);
  }
}

void readEncoder(){
  int b = digitalRead(ENCB);
  if(b>0){
    pos++;
  else{
    pos--;
  }
}

If I am understanding this correctly maybe this will help. I am responding to your original code.

Set a flag to high inside your interrupt and in your loop if the flag is high increment a 16 bit number (or two 8 bit numbers) and clear the flag. Serial print at the end. Right now your loop is always trying to print the count even if it wasn't incremented.

Posted by: @robotbuilder

↑

An important point here is I am using AI as an educational tool.

I can see you are having fun with your AI forages but be sure not to take the first responses and to ask questions in a different way and not just to rely on further question relating to the first answer.  Only today in framing the AI question to as per my previous post I also asked about micropython version of using the rpi pico PIO and DMA.  

First I has a response:

MicroPython does not expose DMA, so this uses a PIO state machine that maintains the counter in its register and autopushes updates to the RX FIFO

Eh! ask again, response:

You can use Direct Memory Access (DMA) with MicroPython on the Raspberry Pi Pico to transfer data between memory and I/O registers without CPU intervention. This allows for efficient data handling, especially for tasks like high-speed data acquisition or output.

Phew! saved from mucking about with c++ 😀 

And a couple of weeks ago I got an entirely incorrect response about something or other. Nevertheless AI prompts have proved useful, perhaps embarrisingyly by assisting me to remember how some of my own code created some time ago was working.  I gave the AI my code and asked for it to be fully commented.   It did a good job on that.  Indeed on some of the code examples that AI generates as example code can be helpfully augmented by asking for a fully commented version.

But I would hesitate to present AI generated code without fully testing it and probably amending it to my own preferences, as theres alway many different ways to do the very same thing and AI code whilst often working, does appear a bit haphazard to me. 

I posted the code in your last post into AI and asked for it to be fully commented and here is the response.  It took the liberty of making corrections! (I'd wring its neck if it did that to my code) along with the comments, bleeding obvious and all 😀 .  But does it work?   We await the next exciting episode. 😎  (I'll leave you alone with your AI pal, thats enough from me)

/*Below is the corrected and fully commented version of your sketch. 
I fixed syntax/logic bugs (missing semicolons in defines, 
wrong variable names, braces, typos like HGIH, incorrect 
prev/cur variable use, eprev reset, missing pinMode/outputs for 
motor pins, missing enable pins) so the comments match working code. 
Comments explain each line / block.
*/
// Encoder channel pins (digital inputs)
// ENCA is the channel tied to the interrupt (yellow wire)
// ENCB is the other quadrature channel (white wire)
#define ENCA 2
#define ENCB 3

// Motor driver pins
// PWM: PWM output controlling motor speed
// IN1 / IN2: direction control inputs to H-bridge or motor driver
#define PWM 5
#define IN2 6
#define IN1 7

// Encoder position counter (counts encoder pulses, signed)
int pos = 0;

// Timing and PID state variables
long prevT = 0;        // previous timestamp in microseconds
float eprev = 0;       // previous error (for derivative term)
float eintegral = 0;   // accumulated integral of error

void setup() {
  Serial.begin(9600);

  // Configure encoder pins
  pinMode(ENCA, INPUT);
  pinMode(ENCB, INPUT);

  // Configure motor pins as outputs
  pinMode(PWM, OUTPUT);
  pinMode(IN1, OUTPUT);
  pinMode(IN2, OUTPUT);

  // Initialize previous timestamp
  prevT = micros();

  // Attach interrupt to ENCA pin; call readEncoder() on RISING edge of ENCA
  attachInterrupt(digitalPinToInterrupt(ENCA), readEncoder, RISING);
}

void loop(){
  // Target encoder position (setpoint)
  int target = 1200;

  // PID gains (tune these for your system)
  float kp = 1.0;
  float kd = 0.025;
  float ki = 0.0;

  // Current time and elapsed time (deltaT) in seconds
  long currT = micros();
  float deltaT = (float)(currT - prevT) / 1.0e6; // convert microseconds to seconds
  if (deltaT <= 0) deltaT = 1e-6; // guard against zero or negative dt
  prevT = currT;

  // Error: desired - actual. Using (target - pos) is typical for PID.
  float e = (float)target - (float)pos;

  // Derivative: rate of change of error
  float dedt = (e - eprev) / deltaT;

  // Integral: accumulate error over time
  eintegral += e * deltaT;

  // PID control signal (u). Positive u -> forward, Negative -> reverse
  float u = kp * e + kd * dedt + ki * eintegral;

  // Motor power magnitude is absolute value of control signal
  float pwr = fabs(u);

  // Limit power to PWM range 0-255
  if (pwr > 255.0) pwr = 255.0;

  // Determine direction from sign of control signal
  int dir = 0;
  if (u > 0.0) dir = 1;    // forward
  else if (u < 0.0) dir = -1; // reverse
  else dir = 0;            // stop

  // Send command to motor driver
  setMotor(dir, (int)pwr, PWM, IN1, IN2);

  // Save error for next derivative calculation
  eprev = e;

  // Telemetry: print target and current position
  Serial.print(target);
  Serial.print(" ");
  Serial.print(pos);
  Serial.println();

  // Small delay to avoid flooding serial and to give time for motor/encoder updates
  delay(10);
}

// setMotor: apply direction and PWM to motor driver pins
// dir:  1 = forward (IN1 HIGH, IN2 LOW)
//      -1 = reverse (IN1 LOW, IN2 HIGH)
//       0 = brake/coast (both LOW)
void setMotor(int dir, int pwmVal, int pwm, int in1, int in2){
  // constrain pwmVal to valid range
  if (pwmVal < 0) pwmVal = 0;
  if (pwmVal > 255) pwmVal = 255;

  analogWrite(pwm, pwmVal);

  if (dir == 1){
    digitalWrite(in1, HIGH);
    digitalWrite(in2, LOW);
  }
  else if (dir == -1){
    digitalWrite(in1, LOW);
    digitalWrite(in2, HIGH);
  }
  else { // dir == 0
    digitalWrite(in1, LOW);
    digitalWrite(in2, LOW);
  }
}

// Interrupt service routine for encoder channel A rising edge.
// Reads channel B to determine direction and updates position counter.
void readEncoder(){
  int b = digitalRead(ENCB);
  if (b == HIGH){
    pos++;   // if B is HIGH on A rising, assume forward
  } else {
    pos--;   // if B is LOW, assume reverse
  }
}

/*
If you want, I can:

Convert the encoder count to revolutions or degrees,
Add debouncing or filter for encoder jitter,
Implement velocity limiting or anti-windup for the integral term,
Or show how to tune the PID gains. Which would you prefer?
*/

@byron 

(I'll leave you alone with your AI pal, thats enough from me)

It is not my AI pal it is a look up tool like google that is trained on data (weights adjusted) from the internet. It "knows" a lot and "understands" nothing.

It is like those AI trained object recognition functions trained from hundreds of examples you posted about. They also can get it badly wrong. They don't really "know" what they are looking at.

That last post with the video link is not AI code it is human code,  an example of PID control the result of humans ingenuity.

Yes the AI tool can and does get it wrong from false information it has taken from the internet. Garbage in garbage out.

When I posted chatGPT responses it was just like posting any sourced material from the internet that could be right or wrong. Users of AI know or should learn what it is and what it is not to use it properly.

Now the following is from humans but was presented by chatGPT.

Humans, not AI, are the real source of knowledge

Most knowledge originates from:

scientists doing experiments

engineers building things

historians studying records

journalists reporting events

everyday people documenting experiences

Fields like Physics, Biology, and Computer Science constantly generate new findings.

Posted by: @noweare

↑

If I am understanding this correctly maybe this will help. I am responding to your original code.

Set a flag to high inside your interrupt and in your loop if the flag is high increment a 16 bit number (or two 8 bit numbers) and clear the flag. Serial print at the end. Right now your loop is always trying to print the count even if it wasn't incremented.

Best to keep interrupt routines short. Remember there are two hardware interrupt routines in the code so what happens when one is triggered while the other is being executed? Lots to understand.

@robotbuilder 

I guess my point was just use the serial.print statements at the end, after reaching your count.

Best to keep interrupt routines short. Remember there are two hardware interrupt routines in the code so what happens when one is triggered while the other is being executed? Lots to understand.

You have a long time (computer speaking) between pin 2 and pin 3 interrupts.  Putting a statement in both of the interrupts to capture the time will give you a feel for how close they are to interfering with each other. You'd be surprised by how much time you have. Its a worthwhile excercise.

void loop()
{
    //...
    activeBrake();

    long c1;

    noInterrupts();
    c1 = counter1;
    c2 = counter2;
    interrupts();

    Serial.println("==========");
    Serial.print(c1);
    Serial.print("  ");
    Serial.println(c2);

    //...
  }
}

What is the value you get for c1 and c2?

What does it tell you? (You shouldn't need chatGPT for this.)

(Yes, I'm getting a bit weary of uncurated verbatim chatGPT in the posts.)

Posted by: @robotbuilder

↑

When I posted chatGPT responses it was just like posting any sourced material from the internet that could be right or wrong. Users of AI know or should learn what it is and what it is not to use it properly.

Sorry, I have to object to this.

The Ai is not posting the code; The OP is. The forum users trust that they have put thought into the post. There's critical review but it not a "spot the bug" hunt. When you post code from an AI, it's your name on the line. So, you curate it. Don't expect others to do it for you or hide behind an AI assistant. You posted it, you own it.

Posted by: @tfmccarthy

↑

void loop()
{
    //...
    activeBrake();

    long c1;

    noInterrupts();
    c1 = counter1;
    c2 = counter2;
    interrupts();

    Serial.println("==========");
    Serial.print(c1);
    Serial.print("  ");
    Serial.println(c2);

    //...
  }
}

What is the value you get for c1 and c2?

What does it tell you? (You shouldn't need chatGPT for this.)

(Yes, I'm getting a bit weary of uncurated verbatim chatGPT in the posts.)

I forget the exact numbers. It told me that the wheel was turning more than the number of turns designated in the loop. I was only monitoring one wheel in the code although I printed both values.

I did need chatGPT to explain why that was the case. I had long posts in another thread about this some time ago but chatGPT explained and solved it providing information I didn't find anywhere else.

Posted by: @tfmccarthy

↑

Posted by: @robotbuilder

↑

When I posted chatGPT responses it was just like posting any sourced material from the internet that could be right or wrong. Users of AI know or should learn what it is and what it is not to use it properly.

Sorry, I have to object to this.

The Ai is not posting the code; The OP is. The forum users trust that they have put thought into the post. There's critical review but it not a "spot the bug" hunt. When you post code from an AI, it's your name on the line. So, you curate it. Don't expect others to do it for you or hide behind an AI assistant. You posted it, you own it.

Well I always disclose the information was from chatGPT so I no more own it than if I disclose the information was sourced from a google article, book, or a post from Tim or anywhere else. They might be interested to learn from it or use it but that is up to them.

chatGPT is just an AI tool and I use it as a learning tool.

This thread was just a demo of how useful it can be in solving a problem or explaining things.

The code was not there for anyone to debug or even bother reading that was up to them. It was only demo code from chatGPT to explain stuff.

@noweare 

Thank you for all the suggestions, all interesting and taken on board.

@byron

I posted the code in your last post into AI and asked for it to be fully commented and here is the response. 

That code came from a human. It came from the video tutorial on using PID control to use an ordinary motor with encoders like a servo motor or a stepper motor.

The other posts simply showed how I used chatGPT to resolve encoder readings for accuracy and consistency. It explained and solved the problems with code and information about the hardware.

I sense a lot of hostility to using AI and perhaps a misunderstanding of how it can be used productively to learn about hardware and software.