Thanks to all of you for your very helpful comments.
Therev are often many sometimes conflicting factors involved in deciding what is “good” code.
It may to be correct, bug free, robust, efficient, maintainable ….
In this case the main requirements were for it to meet the needs of the end user; and for me to be able to understand it if it later needs amending.
While @PerryBebbington state machine solution isnt compact it is comprehensible and non-blocking, which has enabled me to do filtering on the data from the interrupt.
Thanks also @paulpaulson for your sketch which will give me a lot to think about. Its always interesting to see an alternative way of thinking about a problem.
However - a puzzle:
at a tSpeed above 60 the result of this line falls to zero. If I Serial.print milesPulsePeriod after this line, it does show as zero yet oneHour = 3,600,000; tSpeed = 61; milesCal=541
so the value SHOULD be 109. not zero.
at that speed the incoming pulses are at atound 900usec so I dont see that interrupts should be causing the issue. Any ideas?
milesPulsePeriod = oneHour / (tSpeed * milesCal); //milliseconds
Just for context here is my “final” (hopefully) sketch
/*
speedoFinal_state: drives speedo and odometer at a rate controlled by pulse input frequency
ALL WORKING using micros timing for input pulses on interrupt.
calculation: 0.1 mile = 55 wheel rotations at 3m rolling circumference
= 330 shaft rotations with a 6:1 final drive
and assume 20 sensor pulses per shaft rotation = 6600 pulses per 0.1 mile
on test the mileometer required 541 pulses = 1 mile. Due to mechanics likely true ratio is 540 as that factorises to match drive motor and gears
*/
//pin assignments
const byte speedoPin = 5; //pwm out to speedo MUST BE PWM PIN 5 or 6
const byte senderPin = 2; // pulse interrupt input MUST BE INTERRUPT PIN
const int phase1 = 6;
const int phase2 = 7;
// calibration constants: nb may need adjusting if not enough resolution wtih single digit integer values
// change these values to match the actual constant factors for the bus.
const int K = 9; // wheel rotations a minute at 1mph
const int D = 6; //final drive ratio
const int S = 20; //sender drive ratio (a guess for now)
int F; // F = K * D * S speed factor is pulses / min at 1mph so mph = ppm / F :
//with these values F is 1080
float iSpeedCal = 3.2; // 255 = 80mph so 1mph = 255/80
int milesCal = 541; // calibrated to true speed: mileometer registers 1 mile for 541 pulses.
volatile boolean iFlag = false;
volatile unsigned long tNow, tDiff = 100000, tPrev; //millis timing on interrupt to measure pulse period
unsigned long ppMin, ppMinOld, oneMin = 60000000; //60 sec 60000msec, 60,000,000 microseconds
int tSpeed, iSpeed; //true speed mph and indicated speed pwm value to galvo
unsigned long oneHour = 3600000; //in milliseconds, for mileometer pulse period
unsigned long tStep, tStep1, milesPulsePeriod; //managing the mileometer - unsigned long because using millis timing
int state = 2; //mileometer output state
boolean stepOn = false;
unsigned long tPrint, tPrintOld, tPrintWait = 1000; //only print once a second
char buffer[120];
void freqP() { //interrupt service routine
tNow = micros();
tDiff = tNow - tPrev; //tDiff is the pulse period in usec
tPrev = tNow;
iFlag = true;
}
void setup() {
Serial.begin(115200); // note sensible baud rate
Serial.println("comms up");
pinMode(phase1, OUTPUT);
pinMode(phase2, OUTPUT);
pinMode(speedoPin, OUTPUT); // pwm
attachInterrupt(digitalPinToInterrupt(senderPin), freqP, RISING);
tNow = millis();
ppMinOld = 1;
F = K * D * S;
Serial.print("F is ");
Serial.println(F);
}
void loop() {
if (iFlag) {
ppMin = oneMin / tDiff; //pulses per minute
//filter
ppMin = (ppMin + (63 * ppMinOld)) / 64; //careful not to exceed size of long unsigned integer
ppMinOld = ppMin;
iFlag = false;
}
//calculate true speed
tSpeed = ppMin / F; // in mph based on F = K*D*S
//operate the speedo
iSpeed = int(tSpeed * iSpeedCal);
analogWrite(speedoPin, iSpeed); //max value for speed is 255: max iSpeed is 60
milesPulsePeriod = oneHour / (tSpeed * milesCal); //milliseconds
//operate the mileometer
tStep = millis();
if ((tStep - tStep1) >= milesPulsePeriod) { //if itstime to start a new pulse ..
tStep1 = tStep;
//do a pulse
stepOn = true;
}
doStep();
//if its time to print
tPrint = millis();
if ((tPrint - tPrintOld) > tPrintWait) {
sprintf(buffer, "sender pulse interval is : %lu usec: sender pulses per minute : %lu Speed is %d mph: milesPulsePeriod is: %lu usec\n", tDiff, ppMin, tSpeed, milesPulsePeriod);
Serial.print(buffer);
tPrintOld = tPrint;
}
}
void doStep() {
uint32_t currentMillis = millis();
static uint32_t lastMillis;
constexpr uint32_t pulseLength = 30; //a 30msec phase pulse seems to work well with the mechanical mileometer
switch (state) {
case 0:
if (stepOn) {
stepOn = false;
lastMillis = currentMillis;
digitalWrite(phase1, HIGH);
digitalWrite(phase2, LOW);
state = 1;
}
break;
case 1:
if (currentMillis - lastMillis >= pulseLength) {
lastMillis = currentMillis;
digitalWrite(phase1, LOW);
digitalWrite(phase2, HIGH);
state = 2;
}
break;
default:
if (currentMillis - lastMillis >= pulseLength) {
digitalWrite(phase2, LOW);
state = 0; //ready for next step
}
break;
}
}
