Using a tachometer program and Servo library makes servo slow

I am trying to write an engine governor program where a servo controls the throttle but first I need to get the servo to operate somewhat smoothly while displaying rpms. The tachometer program displays the rpms quite well but the servo is really slow and jumpy. When used with another sketch using the servo library without the “tachometer” , the servo movement is pretty smooth. Am I just expecting too much from a UNO? Thank you. Norm

#include <Servo.h> 
//Define Tach Variables
int ticsPerRev = 8;       // define number of tics per rev of code wheel
float rpm = 0.0;  // I prefer float for rpm
volatile int rpmcount =0;  // volitile because the variable is manipulated during the interrupt
unsigned long timeold = 0; // used to calculate d_t= millis()-timeold;
int d_t;

//Define Servo Variables
int PrintRate =5; // # of cycles before printing data
int potpin = 1;  // analog pin used to connect the potentiometer
int val;    // variable to read the value from the analog pin
Servo myservo;  // create servo object to control a servo

void setup()
{
  Serial.begin(56700);
  myservo.attach(9);
  attachInterrupt(0, rpm_fun, FALLING);   
}

void loop()
{

  //Update RPM every second, don't process interrupts during calculations
  detachInterrupt(0);
  d_t=millis()-timeold;  
  if (d_t >= 1000)
  {
    rpm = float(60.0*1000.0)/float((d_t))*float(rpmcount)/ticsPerRev;     
    timeold = millis();
    d_t=0; //reset d_t

    Serial.print("Time(ms) ");
    Serial.print(timeold); //time at end of interval (hence timeold=millis(); above )
    Serial.print(" TicsPerInterval ");
    Serial.print(rpmcount);
    Serial.print(" RPM ");
    Serial.println(rpm);    
    Serial.println(val);
    rpmcount = 0; //reset rpmcount
    val = analogRead(potpin);            // reads the value of the potentiometer (value between 0 and 1023) 
    val = map(val, 0, 1023, 0,90);     // degree scale it to use it with the servo (value between 0 and 180)     
    myservo.write(val);
  }
  //Restart the interrupt processing
  attachInterrupt(0, rpm_fun, FALLING);
}

void rpm_fun()
{

  rpmcount++; //update rpmcount    

}

Couple of things - firstly don't call detachInterrupt and attachInterrupt repeatedly
in loop(), that's crazy. You will lose interrupts and thus counts and all sorts of
problems.

What you should do is sample rpmcount (with interrupts disabled, since its more
than one byte) every 1000ms:

volatile int rpmcount = 0 ;
int sampled_rpmcount ;

void loop ()
{
  ....
  if (millis () - timeold >= 1000)
  {
    byte sreg = SREG ;   // save interrupt flags
    cli () ;  // disable interrupts
    sampled_rpmcount = rpmcount ; // copy safely
    SREG = sreg ;   // restore interrupt flags

    ....  // now used the sampled_rpmcount variable for calculations
  }
}

Or if you know interrupts should always be enabled then:

    cli () ;  // disable interrupts
    sampled_rpmcount = rpmcount ; // copy safely
    sei () ;   // restore interrupt flags

The second thing is the code to test for every 1000ms. Its got two
problems, one is that it will go wrong at rollover (only an issue after 49 days
running continuously for millis(), but really important if you every use micros().

Then you fail to count milliseconds properly so that you will overrun the 1000ms
(because you reset timeold after doing some floating point calculations.

Try a structure like this:

  if (millis () - timeold >= 1000)  // test for next tick
  {
    timeold += 1000 ; // setup for next tick, accounting is not sensitive to code delays
    ....
  }

Note the subtraction before the comparison - that's what makes it work whether or
not millis() rolls over - the result of subtraction is always correct.

Note the imcrement of timeold - don't set it to the current time, set it to 1000ms later
than what the current time is meant to be... this way any delays in the code are
finessed and it will always be waiting for the correct next timeslot.

Thank you for your help. I applied the changes you suggested and it runs without any errors but I am still getting poor servo response but I may not have done it correctly.

include <Servo.h> 
int ticsPerRev = 8; 
float rpm = 0.0;  
volatile int rpmcount =0;  
unsigned long timeold = 0; // used to calculate d_t= millis()-timeold;
int sampled_rpmcount;
int d_t;
//int PrintRate =5; // # of cycles before printing data
int potpin = 1;  // analog pin used to connect the potentiometer
int val;    // variable to read the value from the analog pin
Servo myservo;  // create servo object to control a servo

void setup()
{
  Serial.begin(56700);
  myservo.attach(9);
  attachInterrupt(0, rpm_fun, FALLING);   
}

void loop()
{
  if (millis () - timeold >= 1000) 
  {
    byte sreg = SREG ;   // save interrupt flags
    cli () ;  // disable interrupts
    sampled_rpmcount = rpmcount ; // copy safely
    SREG = sreg ;   // restore interrupt flags

    d_t=millis()-timeold;  
    {
      rpm = float(60.0*1000.0)/float((d_t))*float(sampled_rpmcount)/ticsPerRev;     

    }  
    if (millis () - timeold >= 1000)  // test for next tick
    {
    timeold += 1000 ; // setup for next tick, accounting is not sensitive to code delays
    Serial.print("Time(ms) ");
    Serial.print(timeold); //time at end of interval (hence timeold=millis(); above )
    Serial.print(" TicsPerInterval ");
    Serial.print(rpmcount);
    Serial.print(" RPM ");
    Serial.println(rpm);    
    Serial.println(val);
    rpmcount = 0; //reset rpmcount
    val = analogRead(potpin);            // reads the value of the potentiometer (value between 0 and 1023) 
    val = map(val, 0, 1023, 0,90);     // degree scale it to use it with the servo (value between 0 and 180)     
    myservo.write(val);
    d_t=0; //reset d_t 
  }      
  } 
}
void rpm_fun()
{
  rpmcount++; //update rpmcount    

}
]

It seems to be related to these lines creating a delay that is necessary for the serial.print to work. If I remove them, the servo works correctly but the serial.print scrolls really fast:

 if (millis () - timeold >= 1000)  // test for next tick
    {
 timeold += 1000 ; // setup for next tick, accounting is not sensitive to code delays

Would it make sense to create a separate loop for the the serial.print functions including the delay? Since there are multiple serial.print lines, I don’t know how to accomplish this without using a Goto which appears to be frowned upon. Thank you for helping newbs like me. Norm

    cli () ;  // disable interrupts

It's generally a good idea to then re-enable them.

Thank you for the tip regarding enabling the interrupt Pauls. My biggest problem is that the servo is still slow to respond to turning the pot. I assume it is because the program is waiting for milis to reach 1000 before it proceeds. Do you have any suggestions as to how address this? Thank you for your time. Norm

I finally figured it out. I needed to put the commands to read the pot and write to the servo at the beginning of the Loop, that is before the if (millis () - timeold >= 1000). Probably seems obvious to all of you but I am still learning. Thank you again and I wish you well. Norm