not related function mess up with ISR?

I at the moment trying solve my issue of why an non related function mess up the output my ISR routine were supposed to give me.

The code i am executing is this.

main.ino

#include "speed_profile.h"
    void setup() {
      // put your setup code here, to run once:
      cli();
      output_pin_setup();
      timer1_setup();
      sei();
    }
    
    void loop() 
    {
          int motor_steps = -23400;//-9600; //23400 -  100%
          // Accelration to use.
          int motor_acceleration = 500;//500;
          // Deceleration to use.
          int motor_deceleration = 500;//500;
          // Speed to use.
          int motor_speed = 1000; // 1000
          init_tipper_position();
          compute_speed_profile(motor_steps, motor_acceleration, motor_deceleration, motor_speed); 
    }

Problem is that when i choose to execute init_tipper_position() before compute_speed_profile() it somehow mess up my ISR, and don’t properly end it as it was supposed to.

My ISR is coded as such:

    ISR(TIMER1_COMPA_vect)
    {
      // Holds next delay period.
      unsigned int new_first_step_delay;
    
      // Remember the last step delay used when accelrating.
      static int last_accel_delay;
    
      // Counting steps when moving.
      static unsigned int step_count = 0;
    
      // Keep track of remainder from new_step-delay calculation to incrase accurancy
      static unsigned int rest = 0;
      OCR1A = profile.first_step_delay;
      switch (profile.run_state)
      {
    
        case STOP:
          step_count = 0;
          global_state = false;
          rest = 0;
          TCCR1B &= ~((1 << CS12) | (1 << CS11) | (1 << CS10)); // Stop the timer,  No clock source
          break;
    
        case ACCEL:
          digitalWrite(step_pin, !digitalRead(step_pin));
          step_count++;
          
          if (profile.dir == CCW)
          {
            profile.current_step_position++;
          }
          else
          {
            profile.current_step_position--;
          }  
          
          profile.accel_count++;
          new_first_step_delay = profile.first_step_delay - (((2 * (long)profile.first_step_delay) + rest) / (4 * profile.accel_count + 1));
          rest = ((2 * (long)profile.first_step_delay) + rest) % (4 * profile.accel_count + 1);
    
          // Chech if we should start decelration.
          if (step_count >= profile.decel_start)
          {
            profile.accel_count = profile.decel_length;
            profile.run_state = DECEL;
          }
    
          // Chech if we hitted max speed.
          else if (new_first_step_delay <= profile.min_time_delay)
          {
            last_accel_delay = new_first_step_delay;
            new_first_step_delay = profile.min_time_delay;
            rest = 0;
            profile.run_state = RUN;
          }
          break;
        case RUN:
          digitalWrite(step_pin, !digitalRead(step_pin));
          step_count++;
          
          if (profile.dir == CCW)
          {
            profile.current_step_position++;
          }
          else
          {
            profile.current_step_position--;
          }  
          new_first_step_delay = profile.min_time_delay;
          // Chech if we should start decelration.
          if (step_count >= profile.decel_start)
          {
            profile.accel_count = profile.decel_length;
            // Start decelration with same delay as accel ended with.
            new_first_step_delay = last_accel_delay;
            profile.run_state = DECEL;
          }
          break;
        case DECEL:
          digitalWrite(step_pin, !digitalRead(step_pin));
          step_count++;
          if (profile.dir == CCW)
          {
             profile.current_step_position++;
          }
          else
          {
            profile.current_step_position--;
          }  
          profile.accel_count++;
          new_first_step_delay = profile.first_step_delay - (((2 * (long)profile.first_step_delay) + rest) / (4 * profile.accel_count + 1));
          rest = ((2 * (long)profile.first_step_delay) + rest) % (4 * profile.accel_count + 1);
          // Check if we at last step
              
          if (profile.accel_count >= 0)
          {
            digitalWrite(en_pin, !digitalRead(en_pin));
            profile.run_state = STOP;
          }
          break;
    
      }
       
      profile.first_step_delay = new_first_step_delay;
      
    }

The ISR is started by the function compute_speed_profile(motor_steps, motor_acceleration, motor_deceleration, motor_speed); and is always initialised with the same value. Usually would the ISR go through all the states and end at STOP, but here stops it at DECEL, and never gets a change to change the bool global_state = false, such that the ISR will be turned of. The timer stalls at DECEL, and never continues.

The init_tipper_position() looks like this

    void init_tipper_position()
    {
      digitalWrite(en_pin, HIGH);
      delay(1);
      
      digitalWrite(dir_pin, LOW);
      delay(1);
      int sensor_value = digitalRead(slot_sensor_pin);
      
      while(sensor_value == LOW)
      {
        sensor_value = digitalRead(slot_sensor_pin);
        digitalWrite(step_pin, HIGH);
        delay(1);
        digitalWrite(step_pin, LOW);
        delay(1);
      }
    
      digitalWrite(en_pin, LOW);
        
    }

Both functions are basically non-related which make me more confused what is going wrong.

I tried by some debugging to narrow the search space, and the error seem to occur due to the executing of the while(digitalRead(slot_sensor_pin) == LOW). Executing this before causes the ISR to perform incorrectly, removing it and it performs correctly.

it should be noted that slot_sensor_pin is initialized as and INPUT_PULLUP.

Most of the variable in the ISR are constant, except those that are being increment or calculated inside.

The values that are used for comparison are constants.

profile is basically a struct that keeps track of the values.

any clue why this is a problem?

is global_state declared as volatile?

No...?

should it be? the ISR is only one who is changing it, and the variable is initialised globally..

The C standard states that the volatile keyword should be used in the definition of a variable when there's a chance that the variable's value could change outside the normal flow of execution of the program.

It's not about changing it - reading it means there is no guarantee you are reading the right value. I have not checked your full code (hint - we can't really help with what you shared) but just the name of the variable seemed to imply it was used elsewhere. So if you use it for decision making outside the ISR then make it volatile. Same for any other variable or structure used both sides

Changing it to volatile doesn’t not work.
compute_speed_profile() calculated the constants in the ISR and starts the timer1.

void compute_speed_profile(signed int motor_steps, unsigned int motor_accel, unsigned int motor_decel, unsigned int motor_speed)
{
  while (global_state == false)
  {
    status_step_count.data = ((float)(profile.current_step_position / 23400.0)) * 100.0; 
    status_tipper_availability.data = 0;
  }

  digitalWrite(en_pin, HIGH);
  delay(1);

  if (motor_steps < 0)
  {
    profile.dir = CCW;
    motor_steps = -motor_steps;
    digitalWrite(dir_pin, LOW);
  }
  else
  {
    profile.dir = CW;
    digitalWrite(dir_pin, HIGH);
  }

  delay(1);
  // received_data = percentage converted to a step number received from tipper_control topic
  
  computation_of_speed_profile(received_data, motor_accel, motor_decel, motor_speed); // just computing constant value for the ISR

  OCR1A = 10; 

  // Set Timer/Counter to divide clock by 8 -  ISR started
  TCCR1B |= ((0 << CS12) | (1 << CS11) | (0 << CS10));


  while (global_state == true)
  { 
    status_step_count.data = ((float)(profile.current_step_position / 23400.0)) * 100.0; 
    status_tipper_availability.data = 1;

    //delay(100);
  }
}

speed_profile.h (2.25 KB)

speed_profile.cpp (9.06 KB)

main.ino (552 Bytes)

Your ISR is pretty long, how often do you call it? (digitalWrite etc)

Not sure if this is a pb (EDIT = problem), did not check how those evolve, but you should ensure your types match what's expected

compute_speed_profile(signed int motor_steps, [color=red]unsigned int[/color] motor_accel, [color=red]unsigned int[/color] motor_decel, [color=red]unsigned int[/color] motor_speed)
      [color=red]int[/color] motor_acceleration = 500;//500;
      // Deceleration to use.
      [color=red]int[/color] motor_deceleration = 500;//500;
      // Speed to use.
      [color=red]int[/color] motor_speed = 1000; // 1000

i am not sure what pb means.

I am not sure whether the mismatch between using signed vs usigned could be the cause of the things occurring. I guess i could try changing it.

About the length of the ISR. I don't think its that long. The ISR is basically stolen from a Atmel application note http://www.atmel.com/images/doc8017.pdf. It states that Accel and Decel uses around 200 µs and Run around 35µs . It code doesn't execute all the state, but only one of the State pr. ISR call, which does make it less heavy.

  cli();
  output_pin_setup();
  timer1_setup();
  sei();

Why do you disable and enable interrupts in setup()? They should be disabled/enabled only where needed - in the timer1_setup() function.

With interrupts disabled, it's really stupid to call delay() (which for some bizarre reason output_pin_setup() does).

I agree with J-M-L that you REALLY need to pay attention to types. Wherever you use a cast is a clue that you really need to evaluate whether the variable being cast IS the proper type. If it were, then the cast probably wouldn't be necessary.

i am not sure what pb means.

pb is the chemical symbol for lead. It's also text-speak for "problem".

It’s also short for “personal best”

acronymfinder : PB

first entry was what I meant indeed. "problem"

The problem seem to be resolved by used direct port access, instead of digitalRead in the while loop.

Guess i could the same thing for the digitalWrites in the ISR... Seems like it consumes alot of time.