Need help understanding this code

Hi all,

I’m a Mech. Eng trying to dabble in coding. For a project I’m working on I need to read the time between the start of two signals going high. (signal A goes high…some time later signal B goes high). I have found code that does what I need it to do but there are many lines and commands that I do not understand. I will comment next to each line of code and hopefully someone will be available and willing to shed some light.

#include <TimerOne.h>
const int pinA = 2; //Why does this include “const” typically I see just “int”
const int pinB = 3;// //Why does this include “const” typically I see just “int”
volatile bool //what is volatile and what is bool?
bevtB; //what is bevtB??
volatile unsigned long //what is volatile? and unsigned long?
timeA,
timeB;

void setup()
{
Serial.begin(9600);
pinMode (12, OUTPUT); //[ASSIGNS DIGITAL PIN 12 AS AN OUTPUT]
pinMode( pinA, INPUT );
pinMode( pinB, INPUT );
attachInterrupt(digitalPinToInterrupt(pinA), ISR_pinA, RISING); //please explain this section and formatting
attachInterrupt(digitalPinToInterrupt(pinB), ISR_pinB, RISING); //please explain this section and formatting
bevtB = false; //why false?

}

void loop()
{
digitalWrite (12, HIGH); //[DRIVES DIGITAL PIN 12 HIGH]
delay (30); //[Waits 0.30 Second]
digitalWrite (12, LOW); //[DRIVES DIGITAL PIN 12 LOW]
delay (970); //[Waits 0.970 Second]
unsigned long // again, what is an unsigned long?
timeDiff; //is this a variable or a library command?
if( bevtB )
{
if( timeB >= timeA )
{
Serial.print( "Time between pulses was: " );
timeDiff = timeB - timeA;
Serial.print( timeDiff );
Serial.println( “uS” );
}
noInterrupts(); //why no interrupts?
bevtB = false; //why false?
interrupts();
}
}// End Void loop

void ISR_pinA( void ) //please explain this section as well
{
timeA = micros();

}//ISR_pinA

void ISR_pinB( void )
{
timeB = micros();
bevtB = true;

}//ISR_pinB

bool is a Boolean datatype. A variable of type bool can hold the values true or false.
volatile is a qualifier, and is a hint to the compiler that the value of the variable may change unexpectedly, and special care must be taken to mitigate against the effects of code optimisation.

unsigned long is another datatype, capable of holding integer values in the range 0 to 232-1

Const is another modifier. It tells the compiler that the value of the int will never change. And a compiler error will be genarated if the code tries to change the value.

Why does this include “const” typically I see just “int”

By making it const, short for constant, the compiler will flag up an error if you ever try and change the variable.

what is volatile and what is bool?

Volatile means he variable can be used in an interrupt service routine and also outside it. Often the compiler takes variable in an ISR and does not make them available outside them.

bool is a Boolean variable that is one either true or false it can have no other values.

bevtB; //what is bevtB??

Given that the line above doesn’t end in a semicolon it is an extension of the line above and declares bevB to be the name of a volitile Boolean variable.

unsigned long // again, what is an unsigned long?
timeDiff; //is this a variable or a library command?

These two lines declare an unsigned long variable, that is one 32 bits long and is called timeDiff

void ISR_pinA( void ) //please explain this section as well

This is an interrupt service routine it is called when pinA makes a transition as set up in the setup function.

In general if you don’t know what a statement means the look it up in the reference page you get from the help menu of the Arduino IDE.

By the way, that is crap code why are you having to read it?

The Arduino language reference page For the Arduino functions.

The Arduino is programmed using the C++ language. Here is a good C++ site with a language reference and tutorials.

AWOL:
bool is a Boolean datatype. A variable of type bool can hold the values true or false.
volatile is a qualifier, and is a hint to the compiler that the value of the variable may change unexpectedly, and special care must be taken to mitigate against the effects of code optimisation.

unsigned long is another datatype, capable of holding integer values in the range 0 to 232-1

Grumpy_Mike:
By making it const, short for constant, the compiler will flag up an error if you ever try and change the variable.
Volatile means he variable can be used in an interrupt service routine and also outside it. Often the compiler takes variable in an ISR and does not make them available outside them.

bool is a Boolean variable that is one either true or false it can have no other values.
Given that the line above doesn’t end in a semicolon it is an extension of the line above and declares bevB to be the name of a volitile Boolean variable.
These two lines declare an unsigned long variable, that is one 32 bits long and is called timeDiff
This is an interrupt service routine it is called when pinA makes a transition as set up in the setup function.

In general if you don’t know what a statement means the look it up in the reference page you get from the help menu of the Arduino IDE.

By the way, that is crap code why are you having to read it?

Thanks for all the explanations. I will try using the reference page in the future as well. I found this code and after verifying it worked for what I’m trying to do, I wanted to understand the sections that I didn’t know. For future reference, what makes this code crap? formatting or the logic? both?

I have stepping switches that are driven off of a signal until an interrupt signal is received. I am trying to make sure all the stepping switches have semi-uniform timing. Ultimately I am trying to put together a test fixture to isolate suspect switches.

For future reference, what makes this code crap? formatting or the logic? both?

Mainly the method of using interrupts in the first place.

There is no need to use interrupts at all for this problem.

As mentioned earlier, volatile is a keyword modifier usually associated with variables used in interrupt service routines (ISR). Optimizing compilers often store frequently-used variables in a register (cache) to improve performance by avoiding a reload of the variable from memory. The volatile keyword forces the compiler to fetch the variable (i.e., don't use a cached value), thus avoiding a potential out of sync error for that variable when the interrupt triggers

Grumpy_Mike:
Mainly the method of using interrupts in the first place.

There is no need to use interrupts at all for this problem.

Pin 12 produces a PWM signal that is amplified to run the stepping switch. This output from pin 12 is connected to the stepping switch and to pin 2 ( which receives it as an input). Lastly, the interrupt signal is an input on pin 3.

In the loop- I am generating the PWM signal.

My question:
How else could I time the difference between the start of the signals, while simultaneously generating the required PWM signal without using an interrupt? I don't control when the interrupt signal is sent.

Thanks again for replies

Use the blink without delay technique, sometimes known as a state machine. Get rid of those delays they are a waste of CPU cycles.

You have added more detail about what your project is going to have to do but it is still insufficient to give detailed advice. What is this stepping switch, what does it do? And where is the signals that you want to measure coming from. Over all what on Earth are you trying to do.

At the moment you are presenting an x-y problem.

void ISR_pinA( void )                            //please explain this section as well
{
     timeA = micros();
    
}//ISR_pinA

  
void ISR_pinB( void )
{
     timeB = micros();
     bevtB = true;      
    
}//ISR_pinB

pinA starts the timing cycle. timeA = micros(); makes a record of the current time.

pinB ends the timing cycle and timeB records current micros(). micros(); increments continually in the background. So, timeB micros will naturally be greater than timeA micros. Simple subtraction of timeB - timeA yields elapsed time.

wvpolymath:
Pin 12 produces a PWM signal that is amplified to run the stepping switch.

Are you saying that

digitalWrite (12, HIGH); //[DRIVES DIGITAL PIN 12 HIGH]
delay (30); //[Waits 0.30 Second]
digitalWrite (12, LOW); //[DRIVES DIGITAL PIN 12 LOW]
delay (970); //[Waits 0.970 Second]

is your PWM signal? Have you confirmed this generates the waveform you are expecting with an external meter?
The UNO has another method to do PWM on pin 12. The resolution is 490Hz (roughly 2 milliseconds). in a single line of code, you can set the PWM without using delay().

analogWrite(12,79);// 79 is 79 of 255, the on time of the 2 milliseconds

Grumpy_Mike:
Use the blink without delay technique, sometimes known as a state machine. Get rid of those delays they are a waste of CPU cycles.

You have added more detail about what your project is going to have to do but it is still insufficient to give detailed advice. What is this stepping switch, what does it do? And where is the signals that you want to measure coming from. Over all what on Earth are you trying to do.

At the moment you are presenting an x-y problem.

The delays are part of the PWM signal but I will look into "blink without delay".

I don't need detailed advise. I only wanted clarification on some of the code (which you and others have provided). A stepping switch is a type of mechanical rotary switch. (Think old telephone systems). Google stepping switch. I explained where the signals are coming from (off the switch). I explained what I'm trying to do. (Measure the time between two signals going HIGH while simultaneously generating a PWM signal). It is not an X-Y problem because I know what I want to do and it already does it. I only wanted to understand the "crap" code. Thanks again for help.

p.s. I have confirmed results with O-scope.--accurate to 3 digits

delay (30); //[Waits 0.30 Second]Nope.

A stepping switch is a type of mechanical rotary switch. (Think old telephone systems).

The proper name is a uniselector, and back in the 60's I used them for switching lights on an off in a pattern for discos. In fact I once got a job where they wanted me to manually switch lights on and off in a disco. I simply showed up, wired my uniselector into the lighting desk and let the thing run all night.

(Measure the time between two signals going HIGH while simultaneously generating a PWM signal)

And the method that the writer of the code choose to do it was crap. That is all I am saying. The main reason is that it fails to consider the time the rest of the code takes to run.

To do it properly you need to implement it like this:-
http://www.thebox.myzen.co.uk/Tutorial/State_Machine.html
Or Robin2's several things at once

AWOL:
delay (30); //[Waits 0.30 Second]Nope.

Yeah, my bad. 0.03 Sec

Grumpy_Mike:
The proper name is a uniselector, and back in the 60's I used them for switching lights on an off in a pattern for discos. In fact I once got a job where they wanted me to manually switch lights on and off in a disco. I simply showed up, wired my uniselector into the lighting desk and let the thing run all night.
And the method that the writer of the code choose to do it was crap. That is all I am saying. The main reason is that it fails to consider the time the rest of the code takes to run.

To do it properly you need to implement it like this:-
State Machine
Or Robin2's several things at once
http://forum.arduino.cc/index.php?topic=223286.0

Haha. That was a smart solution for the dico lights.

Discos---the original "blinky light"

Thanks for feedback & help Mike

Grumpy_Mike:
The proper name is a uniselector, and back in the 60’s I used them for switching lights on an off in a pattern for discos. In fact I once got a job where they wanted me to manually switch lights on and off in a disco. I simply showed up, wired my uniselector into the lighting desk and let the thing run all night.
And the method that the writer of the code choose to do it was crap. That is all I am saying. The main reason is that it fails to consider the time the rest of the code takes to run.

To do it properly you need to implement it like this:-
State Machine
Or Robin2’s several things at once
http://forum.arduino.cc/index.php?topic=223286.0

Mike,

I followed your advise and changed how the code is written.

Still having an issue trying to get time between when pin A goes high and when pin B goes high.
(time B- time A).

I believe its in the logic (or lack of);
Does anything obvious jump out to you ?

// -------------------------------------------LIBRARIES-------------------------------------------------------------------------------

#include <TimerOne.h>

#include <LiquidCrystal.h>
// initialize the library by associating any needed LCD interface pin
// with the arduino pin number it is connected to
const int rs = 12, en = 11, d4 = 5, d5 = 4, d6 = 10, d7 = 9;
LiquidCrystal lcd(rs, en, d4, d5, d6, d7);

// -----------------------------------------CONSTANTS (won’t change)------------------------------------------------------------------

const int pwmPin = 22; // the pin numbers for the LEDs
const int PulseLowTime = 970; // number of millisecs that the PWM signal is LOW
const int PulseHighTime = 30; // number of millisecs that the PWM signal is HIGH
const int Signal_A_Pin = 2;
const int Signal_B_Pin = 3;
//byte ReadA= digitalRead(Signal_A_Pin);
//byte ReadB= digitalRead(Signal_B_Pin);

// --------------------------------------VARIABLES (will changes)---------------------------------------------------------------------

unsigned long currentMillis = 0; // stores the value of millis() in each iteration of loop()
unsigned long previousPWM_Millis = 0; // will store last time the PWM signal was updated

byte pwmState = LOW; // LOW = initally LOW
byte Signal_A_State = LOW; // LOW = initally LOW
byte Signal_B_State = LOW; // LOW = initally LOW

unsigned long Signal_A_Time =0;
unsigned long Signal_B_Time =0;
unsigned long Delta_T =0;

// ------------------------------------------VOID SETUP--------------------------------------------------------------------------

void setup() {

Serial.begin(9600);
Serial.println(“The program is executing tasks”); // so we know what sketch is running

// set the digital pins as an input or output:
pinMode(pwmPin, OUTPUT);
pinMode(Signal_A_Pin, INPUT_PULLUP);
pinMode(Signal_B_Pin, INPUT_PULLUP);

}

// ------------------------------------------VOID LOOP--------------------------------------------------------------------------------

void loop() {

// none of the actions happen in loop() apart from reading millis()
// it just calls the functions that have the action code

currentMillis = millis(); // capture the latest value of millis()
// this is equivalent to noting the time from a clock
// use the same time for all LED flashes to keep them synchronized

update_PWM_State();
update_Signal_A_State();
update_Signal_B_State();
update_time();

}

// ------------------------------------------------FUNCTIONS--------------------------------------------------------------------------

// ----------update_PWM_State FUNCTION----------------

void update_PWM_State() {

if (pwmState == LOW) {
// if the PWM is LOW, we must wait for the interval to expire before turning it HIGH
if (currentMillis - previousPWM_Millis >= PulseLowTime) {
// time is up, so change the state to HIGH
pwmState = HIGH;
digitalWrite (pwmPin, HIGH);
// and save the time when we made the change
previousPWM_Millis += PulseLowTime;
// NOTE: The previous line could alternatively be
//previousPWM_Millis = currentMillis
//Which is the style used in the BlinkWithoutDelay example sketch
//Adding on the interval is a better way to ensure that succesive periods are identical
//Serial.println (pwmState);
}
}
else { // i.e. if pwmState is HIGH

// if the PWM is HIGH, we must wait for the duration to expire before turning it LOW
if (currentMillis - previousPWM_Millis >= PulseHighTime) {
// time is up, so change the state to LOW
pwmState = LOW;
digitalWrite (pwmPin, LOW);
// and save the time when we made the change
previousPWM_Millis += PulseHighTime;
//Serial.println (pwmState);
}
}
}

// ----------update_Signal_A_State FUNCTION----------------

void update_Signal_A_State() {

if (digitalRead(Signal_A_Pin) != Signal_A_State) {
// if current reading (ReadA) does not equal old reading (Signal_A_State)
if (digitalRead(Signal_A_Pin) ==HIGH) {
// if current reading is greater than zero then Signal has went from low to high

Signal_A_Time = millis();

// Serial.println (“A high”);
}
}
else { // i.e. if ReadA is LOW

if (digitalRead(Signal_A_Pin)==LOW){

Signal_A_State = LOW;
// Serial.println (“A low”);

}
}
}

// ----------update_Signal_B_State FUNCTION----------------

void update_Signal_B_State() {

if (digitalRead(Signal_B_Pin) != Signal_B_State) {
// if current reading does not equal old reading (Signal_A_State)
if (digitalRead(Signal_B_Pin) ==HIGH) {
// if current reading is greater than zero then Signal has went from low to high

Signal_B_Time = millis();

//Serial.println (“B high”);
}
}
else { // i.e. if ReadA is LOW

if (digitalRead(Signal_B_Pin)==LOW){

Signal_B_State = LOW;
//Serial.println (“B low”);

}
}
}

// ----------update_time FUNCTION----------------

void update_time() {

Delta_T=((Signal_B_Time) - (Signal_A_Time));
Serial.println (Delta_T);
Delta_T=0;
Signal_A_Time=0;
Signal_B_Time=0;
}

Does anything obvious jump out to you ?

Well your use of blank lines is driving me a bit mad. You should have a blank line between functions but only one. You should not have multiple blank lines in functions. This is only cosmetic but removing the unnecessary ones do make it much easier to read the code.

In the update_Signal_A_State() function you read the input several times, have you considered what happens if the input changes during the execution of that function? Basically you will screw things up and it will not work like you expect. So read once and put it into a variable and after that use that variable. Some evidence of you doing this is commented out in the code. The same goes for the update_Signal_B_State function.

Also in the same two functions the ‘else’ part seems a bit superfluous:-

else {  // i.e. if ReadA is LOW
    if (digitalRead(Signal_B_Pin) == LOW) {
      Signal_B_State = LOW;
      //Serial.println ("B low");
    }
  }

Why not simply use the Signal_B_State variable to hold the results of the digital read in the first place?

You are calling the update_time function each time through the loop, which is doing a print. Printing takes time but you are also zeroing the signal time variables, so the code is wiping them out before they have a chance to complete a cycle. Use a boolean variable to indicate you have a complete cycle of measurement, and only call the update_time function when you have. Also use the same function to reset this variable.

Finally the PWM code is a bit clunky, You should have one variable that you check to see if it is time to change anything, if not the exit the function. When you do have to change the state of the output you simply set the time when you need to set it again. So when you set it high, the time to set it again is the high time, and when you set it low the time to change again is the low time.

You seem to have lost the synchronisation between the PWM signal and the reading of the inputs. Is that important or are the outputs not going to change?

You have an #include <TimerOne.h> but you never use this library.

Finally use code tags not quotes. The code tag is the one on the far left of the reply window and looks like this </>