Frankencode, help understanding and is there a better way?

Using Arduino Programming Questions
1

So this is part of a program I'm working on. Trying to get the individual sections working before attempting to merge them into one.

For this part, I have built an electronic circuit that will clean the dirty 12V pulse from a distributor on a car and reduce it down to a 5V signal to be read by the Arduino and then be converted to RPMs displayed on a screen,

I have it kind of working but as the code was modified from a much larger program, I wondered if I had missed anything significant or if anything could be done to improve what I currently have. I was reading up on interrupts and got a bit confused about if i should include a detach interrupt.
Looking for any tips or constructive criticism. TIA.

/*
  Arduino 2x16 LCD RPM meter
  Modified code found at https://github.com/seanauff/classic-car-sensor-interface/blob/master/sensor_interface_2560.ino
  Trying to read a cleaned pulse from a distributor and convert it to a digital readout of RPM
*/

const byte tachPin = 3; // tach signal on digital pin 3 (interrupt 1)
const byte tachInterrupt = 1; // tach signal on interrupt 1 (digital pin 3)
byte engineCylinders = 8; // for tach calculation (pulses per revolution = 2 * cylinders / cycles)
byte engineCycles = 4; // for tach calculation
int refreshInterval = 750; // milliseconds between sensor updates
unsigned long previousMillis = 0;
volatile int RPMpulses = 0;

#include <LiquidCrystal.h>
//LCD pin to Arduino
const int pin_RS = 8;
const int pin_EN = 9;
const int pin_d4 = 4;
const int pin_d5 = 5;
const int pin_d6 = 6;
const int pin_d7 = 7;
const int pin_BL = 10;
LiquidCrystal lcd( pin_RS,  pin_EN,  pin_d4,  pin_d5,  pin_d6,  pin_d7);


void setup()
{
  lcd.begin(16, 2);                                     // Initialize LCD
  pinMode(tachPin, INPUT);                              // Set tachPin to read +V input pulses
  attachInterrupt(tachInterrupt, countRPM, RISING);     // starts counting when pulse received from distributor
  Serial.begin(9600);
}

void loop()
{


  if (millis() - previousMillis > refreshInterval)
  {
    previousMillis = millis();

    lcd.setCursor(0, 0);
    lcd.print("RPM ");
    lcd.print(getRPM());
    lcd.print("    ");
  }
}

// counts tach pulses
void countRPM()
{
  RPMpulses++;
}

// checks accumulated tach signal pulses and calculates engine speed
// returns engine speed in RPM
// Resolution: 30000 * engineCycles / refreshInterval / engineCylinders RPM (for default values = 20 RPM)
int getRPM()
{
  int RPM = int(RPMpulses * (60000.0 / float(refreshInterval)) * engineCycles / engineCylinders / 2.0 ); // calculate RPM
  RPMpulses = 0; // reset pulse count to 0
  RPM = min(9999, RPM); // don't return value larger than 9999
  return RPM;
}
2

the only comment i have is that the following calculation should be made just once in setup() so that there is a single multiplication made in getRPM()

60000.0 / float(refreshInterval)) * engineCycles / engineCylinders / 2.0 )
3

+1, Looks good.

Why use a refresh Interval of 750mS instead of 1000mS?

4

The pattern for interrupts is that the interrupt pings a volatile variable, and the non-interrupt code makes a copy of that variable in a noInterrupts block, and then does what it needs to do by working with that copy.

volatile int _RPMpulses = 0;
int RPMpulses = 0;

void countRPM()
{
  _RPMpulses++;
}

void loop() {
  nointerrupts();
  RPMPulses=_RPMPulses;
  _RPMPulses=0;
  interrupts();

  // continue on with stuff.
}

Your code will probably work ok regardless, because it isn't doing anything complicated with RPMPulses - it's just reading it once and setting it to zero.

I'd be inclined to move some of your calculations into derived const variables named 'pulsesPerRev' and 'samplesPerMinute'. Use const. This means the compiler inserts the value inline into the machine code rather than fetching it from memory.

const byte engineCylinders = 8; // for tach calculation (pulses per revolution = 2 * cylinders / cycles)
const byte engineCycles = 4; // for tach calculation
const int refreshInterval = 750; // milliseconds between sensor updates

const double pulsesPerRev = 2.0 *  (double)engineCylinders / (double)engineCycles ;
const double samplesPerMinute = 60000.0 / refreshInterval;

RPM then becomes pulses / pulsesPerRev * samplesPerMinute. You could condense this expression further using a const pulsedsPerRPM, but it's no necessary. Because everything is const, the compiler will precalculate it. That is, you don't really need pulsesPerRev and samplesPerMinute either - but I think that's a nice level of granularity.

5

gcjr:
the only comment i have is that the following calculation should be made just once in setup() so that there is a single multiplication made in getRPM()

60000.0 / float(refreshInterval)) * engineCycles / engineCylinders / 2.0 )

Noted, thank you. I will amend the code when I get home from work.

Idahowalker:
+1, Looks good.

Why use a refresh Interval of 750mS instead of 1000mS?

Thank you, purely because the code was modified from another piece of code and so I left that value as was. I could certainly change it to see what effect that would have.

PaulMurrayCbr:
The pattern for interrupts is that the interrupt pings a volatile variable, and the non-interrupt code makes a copy of that variable in a noInterrupts block, and then does what it needs to do by working with that copy.

volatile int _RPMpulses = 0;

int RPMpulses = 0;

void countRPM()
{
 _RPMpulses++;
}

void loop() {
 nointerrupts();
 RPMPulses=_RPMPulses;
 _RPMPulses=0;
 interrupts();

// continue on with stuff.
}




Your code will probably work ok regardless, because it isn't doing anything complicated with RPMPulses - it's just reading it once and setting it to zero.

I'd be inclined to move some of your calculations into derived const variables named 'pulsesPerRev' and 'samplesPerMinute'. Use const. This means the compiler inserts the value inline into the machine code rather than fetching it from memory.

const byte engineCylinders = 8; // for tach calculation (pulses per revolution = 2 * cylinders / cycles)
const byte engineCycles = 4; // for tach calculation
const int refreshInterval = 750; // milliseconds between sensor updates

const double pulsesPerRev = 2.0 *  (double)engineCylinders / (double)engineCycles ;
const double samplesPerMinute = 60000.0 / refreshInterval;




RPM then becomes pulses / pulsesPerRev * samplesPerMinute. You could condense this expression further using a const pulsedsPerRPM, but it's no necessary. Because everything is const, the compiler will precalculate it. That is, you don't really need pulsesPerRev and samplesPerMinute either - but I think that's a nice level of granularity.

Thank you for the input, I will re read and digest what you have written as it is quite complicated for a n00b like me to understand.
I want to simplify the code as much as possible as it will become part of a four sensor setup.
Having done some more reading, for every revolution of the cars crank, the distributor sends out 4 pulses. It is a V8 engine and the crank rotates twice to complete the 4 stroke cycle. The first part of the code was copied from another program and was designed for another engine so perhaps some of my variables and constants are not required.
Thank you all for your input, it is truly appreciated. I will update later when I have had a chance to go through it again.

6 
const byte tachPin = 3; // tach signal on digital pin 3 (interrupt 1)
const byte tachInterrupt = 1; // tach signal on interrupt 1 (digital pin 3)

void setup()
{
  pinMode(tachPin, INPUT);                              // Set tachPin to read +V input pulses
  attachInterrupt(tachInterrupt, countRPM, RISING);
  Serial.begin(9600);
}

It's recommended that you instead use digitalPinToInterrupt():

const byte tachPin = 3; // tach signal on digital pin 3

void setup()
{
  pinMode(tachPin, INPUT);                              // Set tachPin to read +V input pulses
  attachInterrupt(digitalPinToInterrupt(tachPin), countRPM, RISING);
  Serial.begin(9600);
}

That way it doesn't matter if you run the sketch on an UNO (Pins 2,3 -> Interrupts 0,1) or a Leonardo (Pins 2,3 -> Interrupts 1,0).

7

I managed to have a bit of a play and must admit I struggled. I really do need to go back to basics.
I programmed a spare nano to output pulses to check my code and hardware was working as expected and luckily it was.
I then went back to the code to see if I could make the adjustments mentioned above but fell flat on my face!
The only part I could get to work was johnwasser's change to make the code more universal.

@gcjr, I tried moving the calculation to the setup section but although it compiled, the output stayed at 0 rpm.
I obviously did something wrong.

@PaulMurrayCbr I tried the 2nd section of code you posted but again being new, I obviously did something wrong so it didn't work.

I have it working but would like to simplify the code in anyway possible not least to make it easier for me to understand.

As the original code was borrowed from another sketch, is there a simpler way for me to read the pulse inputs, carry out a single simple calculation and display the results of that calculation on the screen.
Just trying to get my head around it. I'll go back to the tutorial sections to see what I can learn there.
Apologies for all the questions that probably have you more knowledgeable folks banging your heads against the wall!!

8

Please post your current version.

9

wildbill:
Please post your current version.

It's pretty much the same as in my first post with the exception of a change of the interval from 750ms to 250 and the code attachInterrupt(digitalPinToInterrupt(tachPin), countRPM, RISING); to make it more universal.
Thank you for your input,

10

I'm going to make an assumption that you really want to use interrupts, and this is how I would implement it

Basic idea.

  • You still have your pulse counter that increments a counter every time a RISING edge is detected. That is covered by the ISR(INT0_vect) in the snapshot above
  • You have another interrupt that triggers at a certain interval (lets say every second). You can use one of the timers. In my example, it uses TIMER0
  • Inside that TIMER0 interrupt, all it does is get the accumulated value of your pulseCounter and save it to another variable pulseTotalperSecond, zero out the pulseCounter, and set a flag.
  • At this point, we know that pulseTotalperSecond is the number of pulses we detected for the last 1 second and we have prepped our instantaneous counter back to zero, making it ready for counting pulses for the next second until TIMER0 is triggered again.
  • Inside your main loop, all it does is check whether the flag is true or not. Since we know that the flag becomes true every second, we can go ahead and do the Math here and update the LCD.
  • Of course there's the added complexity of setting up the interrupts but hopefully this is enough to give you an idea how interrupts would be useful in a use case like yours.

ddd.png
ddd.png649×542 11.5 KB

ddd.png
ddd.png649×542 11.5 KB

11

hzrnbgy:
I'm going to make an assumption that you really want to use interrupts, and this is how I would implement it

Thanks for your input. The only reason I wanted to use interrupts was because I believed it to be the best way of doing it. Do you have another suggestion?
My completed sketch will be very simplistic in that it will read the input of 4 sensors and display them finally all together on a TFT screen. There will be minimal calculations required on the inputs to display the desired results. One input may need the use of an array but that is something I will look into once this part of the sketch is worked out.

12

You know, if people were comfortable using github, then code suggestions and collaborations could be done on that. I wonder if this forum would consider hosting a repo? It would make sense, especially if support for it were integrated into the IDE.

13