I am revisiting a project I was working on about a year ago. I gave up on it then since I needed to learn a lot more, lol. But now with "learning" under my belt I am back to work on it. This project measures the "Timing" of a sensored brushless motor by taking the difference in the falling edge of an rpm signal from a hall sensor and the falling edge of the corresponding phase bemf. The code uses the analog comparator on pin d6 and d7 to change the bemf into a square wave and an isr on pin d2 for the rpm falling edge. I would like to average the timing and created an averaging function to do that, but it doesn't work. I suspect this is because the timing comes from an isr and only calculates if the isr condition is true. So I am not quite sure where to put the averaging function.
My code is below.
// Test for reading zero cross and Timing
unsigned long RPMMicros;
unsigned long PHMicros;
unsigned long prevRPMMicros;
unsigned long prevPHMicros;
unsigned long RPMDuration;
unsigned long PHDuration;
unsigned long RPMCount;
unsigned long PHCount;
unsigned long prevDisplayMillis;
unsigned long displayInterval = 1000;
unsigned long FREQ;
float Timing;
float aTiming;
unsigned long int RPM;
float DEGREES;
// variables for the RPM ISR
volatile unsigned long RPMisrMicros;
volatile unsigned long RPMisrCount;
volatile bool RPMnewIsrMicros = false;
// variables for the PH ISR
volatile unsigned long PHisrMicros;
volatile unsigned long PHisrCount;
volatile bool PHnewIsrMicros = false;
void setup() {
Serial.begin(115200);
DDRD |= 0x08; // Configure pin 3 as output for scope or LED
PORTD = 0x00;
ACSR =
(0 << ACD) | // Analog Comparator: Enabled
(0 << ACBG) | // Analog Comparator Bandgap Select: AIN0 is applied to the positive input
(0 << ACO) | // Analog Comparator Output: Off
(1 << ACI) | // Analog Comparator Interrupt Flag: Clear Pending Interrupt
(1 << ACIE) | // Analog Comparator Interrupt: Enabled
(0 << ACIC) | // Analog Comparator Input Capture: Disabled
//(1 << ACIS1) | (0 << ACIS0); //interrupt on rising output edge
(1 << ACIS1) | (1 << ACIS0); //interrupt on falling output edge
attachInterrupt(digitalPinToInterrupt(2), RPMSensorISR, FALLING);
}
void loop() {
getIsrData();
aTiming = getTimingAVG(25);
if (millis() - prevDisplayMillis >= displayInterval) {
prevDisplayMillis += displayInterval;
showData();
}
if (ACSR & 0x20)
PORTD &= ~8; // D3 LOW
else
PORTD |= 8; // D3 HIGH
}
void getIsrData() {
if (RPMnewIsrMicros == true) {
prevRPMMicros = RPMMicros; // save the previous value
noInterrupts();
RPMMicros = RPMisrMicros;
RPMCount = RPMisrCount;
RPMnewIsrMicros = false;
interrupts();
RPMDuration = RPMMicros - prevRPMMicros;
FREQ = 1000000 / RPMDuration;
RPM = FREQ * 60;
DEGREES = (RPMDuration * 1000) / 360;
}
if (PHnewIsrMicros == true) {
prevPHMicros = PHMicros;
noInterrupts();
PHMicros = PHisrMicros;
PHCount = PHisrCount;
PHnewIsrMicros = false;
interrupts();
PHDuration = PHMicros - RPMMicros;
if (PHDuration > 50) { // debounce ---- This needs work!!!
Timing = ((PHDuration * 1000) / DEGREES) - 30;
}
}
}
float getTimingAVG(int samples) {
int i;
float avg;
for (int i = 0; i < samples; i++) {
getIsrData();
avg += Timing;
return avg / samples;
}
}
void showData() {
Serial.println();
Serial.println("===============");
Serial.print(" RPM Duration ");
Serial.print(RPMDuration);
Serial.print(" FREQ ");
Serial.print(FREQ);
Serial.print(" Hz");
Serial.print(" RPM ");
Serial.print(RPM);
Serial.print(" Timing ");
Serial.print(Timing);
Serial.print("*");
Serial.print(" aTiming ");
Serial.print(aTiming);
Serial.print("*");
Serial.print(" PHDuration ");
Serial.print(PHDuration);
Serial.println();
}
void RPMSensorISR() {
RPMisrMicros = micros();
RPMisrCount++;
RPMnewIsrMicros = true;
}
ISR(ANALOG_COMP_vect) {
PHisrMicros = micros();
PHisrCount++;
PHnewIsrMicros = true;
}
One option is to create an array with the last values. For example the last 10 values. Then calculate the average of the array. It is called a "walking average" or "moving window" average.
Another option is a low-pass filter. I prefer to use 'float' numbers for it.
// global variable with the filtered value
float filteredValue;
void loop()
{
...
// calculate the filtered value, but using 1% of the new value.
float newValue = ...
filteredValue = 0.99 * filteredValue + 0.01 * newValue;
...
}
Both these filters works best when the samples arrive at a fixed interval. I don't know what the influence of a RPM value is on those filters.
I assume you want to take an average because your RPM values are jumping all over the place.
Why don't you just measure the time between say 11 pulses rather than measuring the duration between two. RPMisrMicros will then be the sum of the previous 10 durations.
In your ISR set RPMnewIsrMicros = true when RPMisrCount == 10
You can eliminate the getTimingAVG function
Simple, no external libraries needed, no buffers or arrays needed.
What does that mean? What do you expect to happen and what happens instead?
This function is the correct basic idea to average 25 successive numbers, but as written will return after one loop iteration.
float getTimingAVG(int samples) {
int i;
float avg;
for (int i = 0; i < samples; i++) {
getIsrData();
avg += Timing;
return avg / samples;
}
}
return MUST NOT be in the loop. It is always a good idea to explicitly initialize all variables. Fix as follows:
float getTimingAVG(int samples) {
int i;
float avg = 0; //explicit
for (int i = 0; i < samples; i++) {
getIsrData(); //this must wait for a new sample
avg += Timing;
}
return avg / samples; //outside the loop
}
My first thought when the average function wasn't working correctly was an array. I will have to look into it.
I'll take a look at the library.
I wouldn't say all over the place, but yes, there is fluctuation in the RPM duration as well as the phase duration. Not sure if this is due to the microcontroller I am using not being fast enough. Currently prototyping with an Arduino Nano, I will be moving the project to either an STM32 or ESP32 at some point.
to make sure RPMDuration was essentially averaged.
I will look at this.
I'm looking for an average of the timing value. This value only happens with the isr flag returns true though, which doesn't happen every time through the loop, so it is averaging essentially the same value. If I average say 1000 times it does change the output of Timing vs aTiming, but I can't say how many of those values are actually from the ISR.
So essentially what I need is every time the ISR happens it adds to the average function, but only when the ISR happens and not just because there is a value in Timing.
I changed my function to return outside of the for loop.
float getTimingAVG(int samples) {
int i;
float avg = 0;
for (int i = 0; i < samples; i++) {
if (RPMnewIsrMicros == true) {
getIsrData();
}
avg += Timing;
}
return avg / samples;
}
Then I realized that the for loop happens and increments i regardless if RPMnewIsrMicros == true, so I tried it again but outside the for loop, but once RPMnewIsrMicros == true it goes through the loop again too fast and doesn't wait for the next time it is true.
That is required, and it is trivial to fix. Try this
float getTimingAVG(int samples) {
int i;
float avg = 0;
for (int i = 0; i < samples; i++) {
while (RPMnewIsrMicros == false); //wait for new sample
getIsrData();
RPMnewIsrMicros = false: //looks like this is unnecessary, but must be set somewhere
avg += Timing;
}
return avg / samples;
}
Then you did something wrong. getISR() should wait for a new value, not the averaging function.
You still appear to be struggling with the basic principles of coding, so when you run into a specific problem, post the code that is causing the problem.
You won't get anywhere on this forum by claiming "I tried that", and then failing to post the code.
My apologies, I copied the function you suggested exactly from your response except to change one : to a ;.
I assumed it wasn't necessary to reply with code that I copied from you. Also I am switching out all my hardware and working on putting this on an Nano ESP32, so sometimes I am purposefully quick on my replies. But... Here is the full code after your suggested change. As posted in my previous post, the output isn't correct.
// Test for reading zero cross and Timing
unsigned long RPMMicros;
unsigned long PHMicros;
unsigned long prevRPMMicros;
unsigned long prevPHMicros;
unsigned long RPMDuration;
unsigned long PHDuration;
unsigned long RPMCount;
unsigned long PHCount;
unsigned long prevDisplayMillis;
unsigned long displayInterval = 1000;
unsigned long FREQ;
float Timing;
float aTiming;
unsigned long int RPM;
float DEGREES;
// variables for the RPM ISR
volatile unsigned long RPMisrMicros;
volatile unsigned long RPMisrCount = 0;
volatile bool RPMnewIsrMicros = false;
// variables for the PH ISR
volatile unsigned long PHisrMicros;
volatile unsigned long PHisrCount;
volatile bool PHnewIsrMicros = false;
void setup() {
Serial.begin(115200);
DDRD |= 0x08; // Configure pin 3 as output for scope or LED
PORTD = 0x00;
ACSR =
(0 << ACD) | // Analog Comparator: Enabled
(0 << ACBG) | // Analog Comparator Bandgap Select: AIN0 is applied to the positive input
(0 << ACO) | // Analog Comparator Output: Off
(1 << ACI) | // Analog Comparator Interrupt Flag: Clear Pending Interrupt
(1 << ACIE) | // Analog Comparator Interrupt: Enabled
(0 << ACIC) | // Analog Comparator Input Capture: Disabled
//(1 << ACIS1) | (0 << ACIS0); //interrupt on rising output edge
(1 << ACIS1) | (1 << ACIS0); //interrupt on falling output edge
attachInterrupt(digitalPinToInterrupt(2), RPMSensorISR, FALLING);
}
void loop() {
getIsrData();
aTiming = getTimingAVG(25);
if (millis() - prevDisplayMillis >= displayInterval) {
prevDisplayMillis += displayInterval;
showData();
}
if (ACSR & 0x20)
PORTD &= ~8; // D3 LOW
else
PORTD |= 8; // D3 HIGH
}
void getIsrData() {
if (RPMnewIsrMicros == true) {
prevRPMMicros = RPMMicros; // save the previous value
noInterrupts();
RPMMicros = RPMisrMicros;
RPMCount = RPMisrCount;
RPMnewIsrMicros = false;
interrupts();
RPMDuration = (RPMMicros - prevRPMMicros);
FREQ = 1000000 / RPMDuration;
RPM = FREQ * 60;
DEGREES = (RPMDuration * 1000) / 360;
}
if (PHnewIsrMicros == true) {
prevPHMicros = PHMicros;
noInterrupts();
PHMicros = PHisrMicros;
PHCount = PHisrCount;
PHnewIsrMicros = false;
interrupts();
PHDuration = PHMicros - RPMMicros;
if (PHDuration > 50) { // debounce
Timing = ((PHDuration * 1000) / DEGREES) - 30;
}
}
}
float getTimingAVG(int samples) {
int i;
float avg = 0;
for (int i = 0; i < samples; i++) {
while (RPMnewIsrMicros == false)
; //wait for new sample
getIsrData();
RPMnewIsrMicros = false; //looks like this is unnecessary, but must be set somewhere
avg += Timing;
}
return avg / samples;
}
void showData() {
Serial.println();
Serial.println("===============");
Serial.print(" RPM Duration ");
Serial.print(RPMDuration);
Serial.print(" FREQ ");
Serial.print(FREQ);
Serial.print(" Hz");
Serial.print(" RPM ");
Serial.print(RPM);
Serial.print(" Timing ");
Serial.print(Timing);
Serial.print("*");
Serial.print(" aTiming ");
Serial.print(aTiming);
Serial.print("*");
Serial.print(" PHDuration ");
Serial.print(PHDuration);
Serial.println();
}
void RPMSensorISR() {
RPMisrMicros = micros();
RPMisrCount++;
RPMnewIsrMicros = true;
}
ISR(ANALOG_COMP_vect) {
PHisrMicros = micros();
PHisrCount++;
PHnewIsrMicros = true;
}
Also, this is an insult. Comments like this only make me defensive. I am trying to expand my horizon and learn. Please refrain from comments like this in the future, at least towards me. Thanks.
No, it is simply a comment, based on observation. Everyone has to start from the beginning.
I can't make much sense out of the code for getIsrData(), so I will leave you to continue with your efforts. However, I will point out that "averaging" is just one of the problems you have with the posted code.
Funny that. The code works perfectly fine with the exception of averaging. The code output lines up with an external comparator reading on my scope. Point of fact I can put the output of D3 on my scope and it lines up perfectly with an ic comparator output on my scope. Doing math manually gets the same results as my code. So I must be doing something right...
I rewrote my code to use the external comparator instead of the on board Nano analog comparator. This makes it easier to port to a different board in the Arduino environment. So it is using external interrupts on D2 and D3 now. Also this makes hardware adjustments for the comparator easier as I can choose different comparators or add hysteresis to the hardware if I want.
But I still want to be able to average the values and my guess is the array will be the way to do it. I'll look into that more. Here is the new code with the averaging function commented out.
// Test for reading zero cross and Timing
uint32_t rpmMicros{};
uint32_t phMicros{};
uint32_t prevRPMMicros{};
uint32_t prevPHMicros{};
uint32_t rpmDuration{};
uint32_t phDuration{};
uint32_t rpmCount{};
uint32_t phCount{};
uint32_t prevDisplayMillis{};
uint32_t displayInterval = 1000; // refresh rate of the serial monitor
uint32_t freq{};
uint32_t rpm{};
float timing{};
float aTiming{};
float degree{};
// variables for the RPM ISR
volatile unsigned long rpmIsrMicros{};
volatile unsigned long rpmIsrCount{};
volatile bool rpmNewIsrMicros = false;
// variables for the PH ISR
volatile unsigned long phIsrMicros{};
volatile unsigned long phIsrCount{};
volatile bool phNewIsrMicros = false;
void setup() {
Serial.begin(115200);
attachInterrupt(digitalPinToInterrupt(2), rpmSensorISR, FALLING);
attachInterrupt(digitalPinToInterrupt(3), phSensorISR, FALLING);
}
void loop() {
getIsrData();
// aTiming = getTimingAVG(25);
if (millis() - prevDisplayMillis >= displayInterval) {
prevDisplayMillis += displayInterval;
showData();
}
}
void getIsrData() {
if (rpmNewIsrMicros == true) {
prevRPMMicros = rpmMicros; // save the previous value
noInterrupts();
rpmMicros = rpmIsrMicros;
rpmCount = rpmIsrCount;
rpmNewIsrMicros = false;
interrupts();
rpmDuration = (rpmMicros - prevRPMMicros);
freq = 1000000 / rpmDuration;
rpm = freq * 60;
degree = (rpmDuration * 1000) / 360;
}
if (phNewIsrMicros == true) {
prevPHMicros = phMicros;
noInterrupts();
phMicros = phIsrMicros;
phCount = phIsrCount;
phNewIsrMicros = false;
interrupts();
phDuration = phMicros - rpmMicros;
if (phDuration > 50) { // debounce
timing = ((phDuration * 1000) / degree) - 30;
}
}
}
/*
float getTimingAVG(int samples) {
int i;
float avg = 0;
for (int i = 0; i < samples; i++) {
while (rpmNewIsrMicros == false)
; //wait for new sample
getIsrData();
rpmNewIsrMicros = false; //looks like this is unnecessary, but must be set somewhere
avg += timing;
}
return avg / samples;
}
*/
void showData() {
Serial.println();
Serial.println("===============");
Serial.print(" RPM Duration ");
Serial.print(rpmDuration);
Serial.print(" FREQ ");
Serial.print(freq);
Serial.print(" Hz");
Serial.print(" RPM ");
Serial.print(rpm);
Serial.print(" Timing ");
Serial.print(timing);
Serial.print("*");
//Serial.print(" aTiming ");
//Serial.print(aTiming);
//Serial.print("*");
Serial.print(" PHDuration ");
Serial.print(phDuration);
Serial.println();
}
void rpmSensorISR() {
rpmIsrMicros = micros();
rpmIsrCount++;
rpmNewIsrMicros = true;
}
void phSensorISR() {
phIsrMicros = micros();
phIsrCount++;
phNewIsrMicros = true;
}
EDIT: Instead of posting another post...
My goal with the array will be to add all the values in the array and divide by the array number. Then I think I will drop the earliest value and another value to get a running average. I will have to use a for loop to iterate through the elements of the array. I'll post back when I have come up with something.
Hmm, I've tried a few different approaches with arrays, still not getting anywhere. So I have a new idea... If I make the getIsrData() function into 2 separate functions, I could then make them return the durations I am looking for and do all the calculations inside another function. Since the function would be made to return values if the the isr flags are true, then I can average those results outside of the function.
Does that make sense?
Also, I could set these up as class functions since I will eventually have 3 phases and 3 rpm sensors. Need more practice at making classes anyway...
I decided to start with the easier of the 2 returns, the RPM... However it isn't returning the right value, lol. The function is returning a value, it is just a large value instead of what I am expecting. Updated code below: