Hello!
Long time lurker, new contributor. I'm somewhat new to Arduino. I was a big Raspberry Pi fan but realized I was basically using RPi for things that Arduino would be much better suited for simply because I didn't know C++. But I'm really getting into it now. I am a software developer (mostly JS), but most of my languages are high level interpreted languages. Never anything like C++, so just keep that in mind.
Basic project summary: Trying to keep track of a diagnostic signal sent from a compressor pump, it will send a HIGH or LOW signal, and the duration and amount of HIGH signals and the time between them can be used to determine what the fault on the pump is.
In my very limited C++ experience, I decided that the best way to do this was to use the CHANGE interrupt, and keep track of the HIGH/LOW's in an object. And since there will only be one pump and there can only be 1 fault, it seems logical to use a singleton. I've simplified that singleton with the below code that has the same issue.
When the object is created, I give it the pin to use as the interrupt, then in the setup() I call the objects init() method to attach the function to the interrupt trigger.
Code of the whole thing is below. I know you usually want a schematic or diagram of the Arduino setup as well but im not sure the best way to do that, and honestly, it's just an Arduino mega with a jumper attached to digital pin 2 with a signal that switches between HIGH and LOW every 250ms.
class Singleton {
protected:
volatile int _diagnosticPin;
Singleton() = default;
public:
int data;
Singleton(int pin)
{
_diagnosticPin = pin;
Serial.print("Singleton was called with pin: ");
Serial.println(pin);
this->_diagnosticPin = pin; // Doesn't work
}
static Singleton &getInstance() {
static Singleton instance;
return instance;
}
static void interruptFn()
{
Singleton &existingInstance = Singleton::getInstance();
existingInstance.updateStatus();
}
void init()
{
pinMode(this->_diagnosticPin, INPUT_PULLUP);
attachInterrupt(digitalPinToInterrupt(this->_diagnosticPin), Singleton::interruptFn, CHANGE);
}
void updateStatus()
{
// this->_diagnosticPin should be 2
Serial.print("this->_diagnosticPin: "); Serial.println(this->_diagnosticPin);
// This is always ON because this->_diagnosticPin is always 0..
int currentStatus = digitalRead(this->_diagnosticPin);
// Was the fault signal just turned on?
if ( currentStatus == HIGH ){
Serial.println("Interrupt pin was just turned ON");
}
else {
Serial.println("Interrupt pin was just turned OFF");
}
}
};
const int interruptPin = 2;
// Initialize the Singleton instance with a pin to monitor
Singleton SomeInstance(interruptPin);
void setup() {
Serial.begin(9600);
// Initialize the monitoring
SomeInstance.init();
}
void loop() {
// put your main code here, to run repeatedly:
}
The issue seems to be that Im not storing the _diagnosticPin properly, so when the getInstance is called, the Singleton() returns an instance that had no diagnosticPin defined, instead of the singleton with the pin I wanted defined. I think that if the variable were static then it may work, but apparently you can't have static and volatile, and from what I understand, since it's attached to an interrupt, it needs to be set as volatile.
Any input is greatly appreciated. I welcome any and all constructive criticism
Thanks,
-J
P.S. The output of the above code is as below:
What I expect to see is:
If I move the whole thing out of a Singleton then I get that output, or if I change the line
int currentStatus = digitalRead(this->_diagnosticPin);
to
int currentStatus = digitalRead(2);
then it also works. So I'm certain its either a matter of how I'm storing/accessing the _diagnosticPin variable, or maybe even the getInstance() method.
Despite the name, your class is not a singleton. There are two different instances of it: the static variable instance in the getInstance() function, and the global variable SomeInstance.
note that whilst the singleton design pattern is a good design practice in many case, C++ does not force you to make everything a class and you can just use a simple non OOP approach to your problem:
your code would become
const uint8_t interruptPin = 2;
void interruptFn() {
if ( digitalRead(interruptPin) == HIGH ) {
Serial.println(F("PUMP: ON ⇒ OFF")); // not a great idea to print in an ISR but for demo purpose
} else {
Serial.println(F("PUMP: OFF ⇒ ON")); // not a great idea to print in an ISR but for demo purpose
}
}
void setup() {
pinMode(interruptPin, INPUT_PULLUP);
Serial.begin(115200); Serial.println();
attachInterrupt(digitalPinToInterrupt(interruptPin), interruptFn, CHANGE);
}
void loop() {}
you can test that here:
(I made the pump represented by a non bouncing button — assuming your PUMP will have a clean trigger)
alternatively, as you wrote
➜ basically your pump acts as a button.
if you want an OOP approach, you could use a button library like Button in easyRun or OneButton or Toggle and instantiate just one button to represent your pump. It would also work if you had more than one Pump.
Thanks for the reply.
The code snippet I gave above isn't the actual code I'm using, it was just a small and simplified bit of code I wrote to replicate the problem. The task at hand isn't really as simple as counting some interrupt signals. I actually need to keep track of how many times and how long the signal went HIGH as well as the duration of time between the peaks.
The compressor I'm working with is a QDZH35G, which is just a refrigerator/air-conditioning compressor with a very neat control unit attached which lets me run it on a ~12V DC power source. It also allows me to control the speed/RPMs, some safety parameters, monitor the diagnostic signal, and a few other things. I'm using this unit as the compressor for a nitrogen generator system that will feed into a cryocooler I just got.
Per the compressors manual:
And I agree that it's possible to use a non-OOP approach for this - and I actually have. This was the previous version, but it's not in a format that would make it easy for me to implement in the project(s) where I need it, and I wrote it before I learned about interrupts and how they would be a much more efficient way of doing this.
So while the following code works just fine for reporting the errors accurately, I still need to redo it. And I didn't clean it up for posting it here since it's not the code I'm using or needed help with in my post. But here it is:
/**
* Compressor fault codes
* Between the terminals (+) and (D)
* Key = number of flashes
* Each flash will last 1/5 of a second, then after the number of flashes it will
* pause (no flashes) then resume again in 3 minutes.
*/
typedef struct {
uint8_t code;
char* fault;
char* descriptor;
} operationalFaults;
const operationalFaults compressorFaultArr [] {
{0, "Healthy", "No fault"},
{1, "Battery protection cut-out", "the voltage is outside the cut-out setting"},
{2, "Fan over-current cut-out", "the fan loads the electronic unit with more than 1 amp"},
{3, "Motor start error", "The rotor is blocked or the differential pressure in the refrigeration system exceedes the high threshold of 6 kg/cm3"},
{4, "Minimum motor speed error", "The motor cannot maintain minimum speed of 1850 rpm or controller cannot find the rotor position"},
{5, "Thermal cut-out of electronic unit", "Aambient temperature exceedes the high temperature threshold of 75° C"},
{6, "Controller hardware failure", "controller detects abnormal parameters"}
};
/*
Operational errors will cause the LED to flash a number of times. The
number of flashes depends on what kind of operational error was
recorded. Each flash will last 1/4 of a second. After the actual number
of flashes there will be a delay with no flashes, so that the sequence
for each error recording is repeated every 4 seconds.
*/
// Pin that the diagnostic lead is attached to
#define DIAGNOSTIC_PIN 2
// constants won't change: - PROBABLY WON't NEED THIS
//const long checkInterval = 1000; // interval at which to check for blinks
// Times used for determining what state the fault report from the compressor is in
const unsigned int blinkIntervalMs = 250; // Milliseconds between flashes of the current sequence (1/4 of a second)
const unsigned int faultInterimMs = 4000; // Miliseconds between when the last blink of the error ends and the
// first blink of the next error.
const unsigned int fudgeFactorMs = 100; // Just some extra MS to add to the checks to account for slight variations,
// (which there shouldn't actually be any)
// States of the fault analysis loop iterations
const unsigned int FAULT_CYCLE_STATE_OK = 0; // No error found
const unsigned int FAULT_CYCLE_STATE_MID_REPORT = 1; // In process of reporting (signals detected but not yet completed sending)
const unsigned int FAULT_CYCLE_STATE_REPORTED = 2; // Fault report cycle completed (ie: the beep has stopped for ~4 seconds)
const unsigned int FAULT_CYCLE_STATE_ERROR = 3; // When an unexpected state is encountered (eg: LED signal on for too long)
/*
const unsigned int FAULT_CYCLE_STATE_MID_REPORT = 1;
const unsigned int FAULT_CYCLE_STATE_REPORTED = 2;
const unsigned int FAULT_CYCLE_STATE_ERROR = 3;
*/
// How many times does a fault need to be detected before reporting it?
// This is because if the code starts running in the middle of a fault report,
// then it may not get all of the beeps, resulting in an incorrect fault
// report. Thus requiring 2 of the same faults to be detected before reporting
// it ensures that were accurately detecting the fault.
// Note: It may be better to check if the LED has been off for for more than
// blinkIntervalMs. This would allow reporting on the first full set.
//const int minimumFaultCount = 2;
// Variables will change:
//int ledState = LOW; // ledState used to set the LED
long previousMillis, lastLedToggleMs; // will store last time LED was updated
// Total number of blinks (gets reset after a blink interval lasts longer
// than blinkIntervalMs)
unsigned int sequenceBlinkCount = 0,
lastConfirmedFault = 0,
faultCycleState = FAULT_CYCLE_STATE_OK;
// What the last error was (gets determined after the current series of
// blinks is completed)
//unsigned int lastConfirmedFault = 0;
// Set the initial faultCycleState to OK
//unsigned int faultCycleState = FAULT_CYCLE_STATE_OK;
// Variables used in the loop
unsigned int currentLEDState, lastFaultCycleState, lastLEDState;
//unsigned int currentLEDState;
//unsigned int lastFaultCycleState;
// If the LED was on or not in the previous iteration
//unsigned int lastLEDState;
void setup() {
pinMode(DIAGNOSTIC_PIN, INPUT);
// set the digital pin as output:
//pinMode(ledPin, OUTPUT);
Serial.begin(115200);
Serial.print("init previousMillis:");
Serial.println(previousMillis);
for(uint8_t i = 0; i < sizeof(compressorFaultArr)/sizeof(operationalFaults); ++i) {
Serial.print("Fault #");
Serial.print(i);
Serial.print(": ");
Serial.print(compressorFaultArr[i].fault);
Serial.print(" (");
Serial.print(compressorFaultArr[i].descriptor);
Serial.println(")");
//Serial.println(compressorFaultArr[i].fault); //Prints the values: "Settings", "Ajustes" and "Paramètres"
}
//Serial.println(compressorFaultArr[0].fault);
}
void loop() {
// store the current fault cycle state to compare the new one with
//lastFaultCycleState = faultCycleState;
// If the LED was on or not in the previous iteration
//lastLEDState = currentLEDState;
unsigned long currentMillis = millis();
int msSinceLastToggle = (currentMillis - lastLedToggleMs);
//Serial.println("["+ String(currentMillis) +"] sequenceBlinkCount value: " + String(sequenceBlinkCount));
//int currentLEDState = analogRead(DIAGNOSTIC_PIN);
currentLEDState = digitalRead(DIAGNOSTIC_PIN);
//Serial.println("Loop START - lastFaultCycleState: "+String(lastFaultCycleState)+"; lastLEDState:"+String(lastLEDState) + "; currentLEDState:"+String(currentLEDState));
// Is the LEDs current value different than the last value? If so then the status must have just changed
if ( lastLEDState != currentLEDState ){
Serial.println("LED State just toggled (from " + String(lastLEDState) + " to " + String(currentLEDState) + ")");
// Is it currently HIGH? If so then it just got set to HIGH
if ( currentLEDState == HIGH ){
//
// At this point, the LED has just turned back on (from being off in the last loop). This means
// were either starting a new fault report, or it's another blink in an ongoing fault report..
//
Serial.println("\tLED State is " + String(currentLEDState) + ", changing sequenceBlinkCount from " + String(sequenceBlinkCount) + " to " + String(sequenceBlinkCount+1));
sequenceBlinkCount++;
//
// If the LED was just set to HIGH, then regardless of the last state, we can assume were reporting a fault
//
faultCycleState = FAULT_CYCLE_STATE_MID_REPORT;
// LED was started a new blink
Serial.println("\tThe LED just now toggled ON... (sequenceBlinkCount is set to "+ String(sequenceBlinkCount) +")");
// If the fault cycle state is not FAULT_CYCLE_STATE_MID_REPORT, then that means this is the
// first signal from either a new error or another signal group
if ( lastFaultCycleState != FAULT_CYCLE_STATE_MID_REPORT ){
// If the state _was_ OK, then we can assume the compressor has just gone into a fault mode
if ( lastFaultCycleState == FAULT_CYCLE_STATE_OK ){
Serial.println("\tCompressor has just gone into fault mode - Analyzing fault signal");
}
// If the state _was_ REPORTED, then we can assume that this is another report of a fault (likely
// the same one as was reported in the last fault cycle)
else if ( lastFaultCycleState == FAULT_CYCLE_STATE_REPORTED ){
Serial.println("\tNew fault detection initiated..");
}
}
}
else {
Serial.println("The LED just now toggled OFF...");
Serial.println("");
}
// Since the LED was just toggled, set the lastLedToggleMs
lastLedToggleMs = millis();
}
// If not, then is the current state OFF?
else if (currentLEDState == LOW ){
// If so, then it's been off for multiple checks in a row..
// See how long since the last blink
unsigned long intervalDiff = millis() - lastLedToggleMs;
//
// At this point, the diagnostic signal is low (LED off) for at least the 2nd consecutive loop, which
// means we need to do some analysis to see just how long it's been off (intervalDiff), which we can
// use to determine what state were in.
//
// - LED has been off for more than 250ms = Since the HIGH signals sent during a fault reports are separated
// by 250ms, we can assume that any fault report that was being received has now concluded, and we can
// count the signals and determine what the fault was.
//
// - LED has been off for more than 4000ms = Since the fault reports are separated by 4 seconds, and it's
// been more than 4 seconds since we've seen a HIGH signal, we can conclude that whatever fault state
// the compressor was in has now been cleared.
//
// Is the interval difference more than 4.1 seconds?...
if ( intervalDiff > (faultInterimMs + fudgeFactorMs) ){
//
// At this point, the LED has been off for more than 4000ms. And since the fault reports are separated by 4
// seconds, and it's been more than 4 seconds since we've seen a HIGH signal, we can conclude that whatever
// fault state the compressor was in has now been cleared.
//
// We can assume the compressor is no longer in a fault status and clear the fault code
faultCycleState = FAULT_CYCLE_STATE_OK;
if ( lastFaultCycleState != FAULT_CYCLE_STATE_OK ){
Serial.println("Compressor has gone more than 4 seconds ("+ String(intervalDiff)+"ms) without sending a fault signal - Fault state cleared");
}
}
// ... Or is it just between beeps of the current report set?
else if ( intervalDiff >= (blinkIntervalMs + fudgeFactorMs) ){
//
// At this point, the LED has been off for more than 250ms. The compressor signal pattern for a fault is
// LED flashes separated by 250ms, and since it's been more than that without a new signal/flash, we can
// assume that the compressor is finished sending a fault report.
//
//
faultCycleState = FAULT_CYCLE_STATE_REPORTED;
// If the lastFaultCycleState is not REPORTED, then this was the first loop where it was finished reporting
if ( lastFaultCycleState != faultCycleState ){
//
//
//
Serial.println("Compressor has gone more than 250ms ("+ String(intervalDiff)+"ms) without sending a fault signal - Fault signal concluded");
Serial.println("\tFault state changed to REPORTED");
// If there is no last confirmed fault, then that measnt we just finished the first fault report
if ( lastConfirmedFault == 0 ){
//
// FIRST FAULT REPORT
//
Serial.println("\t\t\tInitial fault report completed: " + String(sequenceBlinkCount));
}
else if ( lastConfirmedFault != sequenceBlinkCount ){
//
// FIRST FAULT REPORT OF A DIFFERENT FAULT FAULT
//
// If the last confirmed fault and the current sequence blink count are different, then that likely
// means that the fault reported is different than the one from the last report
Serial.println("\t\t\tNew fault detected: " + String(sequenceBlinkCount));
}
else {
//
// FAULT REPORT WAS SAME AS LAST COMPLETED ANALYSIS
//
// The lastConfirmedFault was the same as the fault seen this loop, then..
Serial.println("\t\t\tThis fault was the same as the last report: " + String(sequenceBlinkCount));
}
// Reset the sequenceBlinkCount so it can start over soon.
lastConfirmedFault = sequenceBlinkCount;
Serial.println("");
}
sequenceBlinkCount = 0;
}
else {
// LED is off but has been off for less than .25 of a second
}
// blinkIntervalMs reportIntervalMs
}
// Has the LED been on for longer than it should have been?..
else if ( msSinceLastToggle > (blinkIntervalMs + fudgeFactorMs ) ){
// unsigned long intervalDiff = millis() - lastLedToggleMs;
//
// The LED is on and has been for likely less than 250ms
//
Serial.println("ERROR - LED has been on for "+ String(msSinceLastToggle) +"ms (longer than the expected limit of "+String(blinkIntervalMs)+"ms)");
}
else {
//
// The LED was on in the last iteration, and is still on in this iteration..
//
}
lastFaultCycleState = faultCycleState;
lastLEDState = currentLEDState;
// Set the "previous" variable values for the next check
previousMillis = millis();
}
Changes I'm making in the V2 for the above module are:
Using interrupts instead of checking the status every single iteration
Not using the String class any more (I didn't realize how bad it was until I started running into memory issues using an OLED in a different project)
Moving it to its own library that I can import whenever I use this type of compressor (and so I can upload it to my Github for anyone else that needs it)
Thanks again,
-J
P.S. @PieterP - Your snippets for the InterruptCounter in the thread you linked to are excellent. Thank you very much. That helps me quite a bit.
An eternity, sure, but it's what I have to work with, lol. And it's actually more like 1/4th a second, so even worse. The documentation also states theres like 3 minutes between fault reports, but it's actually more like 4 seconds. I have some videos in an imgur album here, but here's some of the peaks shown in my multimeter:
Note: Don't mind the weird single peak in the 2nd photo, that was just because I tried to restart the compressor.
And ignore the charts in the Imgur album that show the analog vs digital pin values, that was when I was trying to decide which would be best to use for the fault monitoring but was running into issues reading the values (which was just user error). Using Analog would allow me to see the voltages in the pump as well, but I decided to use an optocoupler to isolate the Arduino circuitry from the compressor, so I'll just be using an interrupt on a digital pin.
And I don't understand why it wouldn't be ideal to use an interrupt for this? Keep in mind that fault monitoring functionality is just one aspect of what the Arduino project will be handling. Ideally the fault interrupt will rarely get triggered. But if/when it does it would be nice to catch exactly what fault was being reported. So in my mind, this is the perfect situation for an interrupt.
The only thing that I have to work out is how best to do the followup checks. If a fault is done reporting, then the LOW signal will last for more than .25 seconds, so I need to run a function manually and see how long its been since the end of the last HIGH peak...
If it's been more than 250ms since the last HIGH signal then I need to count the peaks and report the fault, then reset the peak count.
If its been more than 4000ms then I can assume the fault has been fixed, and I can clear the fault in the Arduino.
I'm thinking I can have a function run every 100ms to check the status of the fault monitoring cycle, if it's OK, then do nothing, if its not in OK, then run an update/followup function to handle the above logic.
So keeping the above in mind, do you still think using interrupts is a bad idea? If so, what would you recommend?...
@PieterP Regarding your InterruptCounter code that you linked to earlier. Most of it I understand and can use, but I do have a few questions about the sections that I don't understand, if you don't mind..
At the top of your InterruptCounter header file, you have this bit of code:
// Use IRAM_ATTR for functions called from ISRs to prevent ESP8266 resets.
#if defined(ESP8266) || defined(ESP32)
#define IC_ISR_ATTR IRAM_ATTR
#else
#define IC_ISR_ATTR
#endif
Then for the private class constructor, you use:
IC_ISR_ATTR void incrementCount();
And in the cpp file, the logic for the InterruptCounter::get_isr method is:
// SECTION A
template <unsigned NumISR>
auto InterruptCounter::get_isr(unsigned interrupt) -> isr_func_t {
return interrupt == NumISR - 1
? []() IC_ISR_ATTR { instance_table[NumISR - 1]->incrementCount(); }
: get_isr<NumISR - 1>(interrupt); // Compile-time tail recursion
}
// SECTION 2
template <>
inline auto InterruptCounter::get_isr<0>(unsigned) -> isr_func_t {
return nullptr;
}
void InterruptCounter::attachInterruptCtx(int interrupt) {
if (attached) {
FATAL_ERROR(F("This instance was attached already"));
return;
}
if (interrupt == NOT_AN_INTERRUPT) {
FATAL_ERROR(F("Not an interrupt-capable pin"));
return;
}
if (instance_table[interrupt] != nullptr) {
FATAL_ERROR(F("Multiple instances on the same pin"));
return;
}
instance_table[interrupt] = this;
attached = true;
// SECTION 3 - Where you attach the get_isr function output to the interrupt trigger on the pin..
attachInterrupt(interrupt, get_isr(interrupt), FALLING);
}
Questions are:
After consulting with the Google gods, I know ISR stands for Interrupt Service Routine, What exactly is the purpose of IC_ISR_ATTR?
In the header file you check to see if ESP8266 or ESP32 are defined, and if so, you set IC_ISR_ATTR to IRAM_ATTR. Since I know I'm going to be running this on an Arduino Mega, can I get rid of this bit of code?
I'm not entirely clear on what the two InterruptCounter::get_isr (in Section A and B) methods do. I can see the get_isr method gets called (in Section 3) when attaching the function to the interrupt pin, but the rest of it is a bit confusing. The interrupt pin is handed to the class constructor when the obj is initiated, does this verify that there aren't more than [max interrupts allowed on pin] attached to that interrupt?
In your two InterruptCounter::get_isr methods, what is the purpose of -> isr_func_t or <0> in the 2nd one?
When you create an instance of the InterruptCounter class, you use the syntax: InterruptCounter ic2 {2};
Whereas when I do it, I use: CompressorFaultMonitor faultMonitor(interruptPin); What's the difference and what are the curly brackets for?
Since I know Ill be running this on an Arduino Mega, and will only be attaching the function to 1 interrupt pin, I think I can remove some of the logic in the get_isr methods.
Sorry for the barrage of questions. If you don't have the time to detail all of it then I totally understand. I'm sure i'll learn it eventually. I just don't want to blindly copy/paste code without learning it first (after all, half the point of this project is to learn C++ and Arduino )
It is a macro that conditionally adds the IRAM attribute to functions that are called from an ISR, for Espressif boards. On other platforms, the macro expands to the empty string, and has no effect whatsoever.
The IRAM attribute makes sure that the function it is applied to is placed in instruction RAM rather than (slow) flash memory, to minimize latency during interrupts.
Indeed, the macro has no effect on non-Espressif boards, so if you don't need to support ESP32/8266 boards, you can safely remove it.
get_isr has two main duties: at compile time, it instantiates many unique functions, one for each available interrupt, so max_num_interrupts functions in total. At run time, it can be used to retrieve the address of the function corresponding to the given interrupt (identified by its index in the range [0, max_num_interrupts)). This is important: get_isr returns a function, and that function is later passed to attachInterrupt.
Each of the generated functions calls the incrementCount() member function for a different instance of the class.
Consider this:
Q: When an interrupt fires, how do we know which pin triggered it?
A: We can tell based on which ISR was called. This is why it is important that get_isr returns unique handler functions.
Q: How does the ISR for a specific pin know which instance to call the incrementCount member function for?
A: It looks up a pointer to the right instance in the global instance_table. The index into that table is hard-coded into the unique ISR, this is done by get_isr.
As a side effect of storing the instance associated with each pin in a global table, we can verify that only one instance is attached to each pin, but this is not the main purpose of the table.
Initialization in C++ is quite a complex topic. See e.g. Initialization - cppreference.com and the links therein. In short, initialization using {} (also called uniform initialization) prevents narrowing conversions of the arguments, and avoids the most vexing parse.
Indeed, if you only need to support a single pin at a time, you don't need the full instance table, you can have a single instance pointer, and a single ISR.
Just make sure that you don't accidentally copy or move your one instance, that would leave a dangling pointer.
And I appreciate the more detailed answers! Very clear and concise, thank you for that.
I think I get what you mean. To avoid that (if I should need to hold the object instance in a separate variable), I would have to use the & to denote a reference instead of a pointer, right?
Which looks like a callback function in a way (but I think in C++ it's referred to as a lambda). When the interrupt is fired then the incrementCount() method for the value in the instance_table is executed. But since I'm not using the instance table, would I just change the instance_table to a single pointer, and populate it with this in the attachInterruptCtx method?
That's what I'm trying now and it's throwing some fatal errors.
In the hpp file I have:
private:
/// Single pointer to the active instance
static DiagnosticInterruptHandler *instance_handler;
And in the cpp file I've changed the attachInterruptCtx to:
void DiagnosticInterruptHandler::attachInterruptCtx(int interrupt) {
if (attached) {
//FATAL_ERROR(F("This instance was attached already"));
//Serial.println("This instance was attached already");
return;
}
instance_handler = this;
attached = true;
attachInterrupt(digitalPinToInterrupt(interrupt), DiagnosticInterruptHandler::interrupt_handler->incrementCount, CHANGE);
}
The exception being thrown is:
I'm pretty sure that I'm populating interrupt_handler correctly, but I think handing the method off to attachInterrupt like that is an issue.
You went about this by handing it the lambda function, right?
Edit: I think I got it working. This seems to work nicely
// Tells compiler this file is to be included only once in a single compilation
#pragma once
#include "DiagnosticInterruptHandler.hpp"
DiagnosticInterruptHandler::DiagnosticInterruptHandler(uint8_t pin) : pin {pin} {}
DiagnosticInterruptHandler::~DiagnosticInterruptHandler() { end(); }
void DiagnosticInterruptHandler::init() {
Serial.print("Attaching interrupt to: ");
Serial.println(pin);
pinMode(pin, INPUT_PULLUP);
//delayMicroseconds(2000);
attachInterruptCtx(digitalPinToInterrupt(pin));
}
void DiagnosticInterruptHandler::end() {
if (attached)
detachInterruptCtx(digitalPinToInterrupt(pin));
}
void DiagnosticInterruptHandler::incrementCount() {
Serial.println("Testing that I got to incrementCount");
++counter;
}
void DiagnosticInterruptHandler::attachInterruptCtx(int interrupt) {
if (attached) {
//FATAL_ERROR(F("This instance was attached already"));
//Serial.println("This instance was attached already");
return;
}
DiagnosticInterruptHandler::instance_handler = this;
attached = true;
attachInterrupt(interrupt, []{
Serial.println("In Lambda");
DiagnosticInterruptHandler::instance_handler->incrementCount();
}, CHANGE);
}
void DiagnosticInterruptHandler::detachInterruptCtx(int interrupt) {
detachInterrupt(interrupt);
attached = false;
}
DiagnosticInterruptHandler *DiagnosticInterruptHandler::instance_handler {};
Wow, it's working. I wasn't expecting to have a working POC so quickly after deciding to redo it.
Using most of the input and code from @PieterP absolutely helped a great deal.
If anyone's curious (or wants to make themselves feel better about their own C++ skills), here's the current revision, which I'm plenty happy with:
File: DiagnosticInterruptHandler.hpp
/**
* Some of the core OOP related code shared by PieterP:
* https://forum.arduino.cc/t/interrupt-in-class-esp32/1039326/52
*/
// Tells compiler this file is to be included only once in a single compilation
#pragma once
#ifndef DIAGNOSTIC_INTERRUPT_HANDLER_H
#define DIAGNOSTIC_INTERRUPT_HANDLER_H
#ifndef DIAGNOSTIC_INTERRUPT_HANDLER_SETTINGS_H
#include "Settings.hpp"
#endif
#include <Arduino.h>
class DiagnosticInterruptHandler {
public:
/// Constructor: doesn't do anything until init() is called.
DiagnosticInterruptHandler(uint8_t pin);
/// Destructor: calls end().
~DiagnosticInterruptHandler();
/// Copy constructor: meaningless, so it has been deleted.
DiagnosticInterruptHandler(const DiagnosticInterruptHandler &) = delete;
/// Copy assignment: meaningless, so it has been deleted.
DiagnosticInterruptHandler &operator=(const DiagnosticInterruptHandler &) = delete;
/// Move constructor: omitted for brevity.
DiagnosticInterruptHandler(DiagnosticInterruptHandler &&other) = delete;
/// Move assignment: omitted for brevity.
DiagnosticInterruptHandler &operator=(DiagnosticInterruptHandler &&other) = delete;
//void followup();
/// Attach the interrupt.
void init();
/// Detach the interrupt.
void end();
// Brief explanation of what each error code means (0 - 6)
static const char* faultExceptions[];
/// Get the actual count. Use with interrupts enabled only.
unsigned read() const;
// Taken out of private for now, though I may leave it here
void _processInterrupt();
private:
/// Single pointer to the active instance
static DiagnosticInterruptHandler *instance_handler;
/// Private handler function that is called from the ISR.
//void _processInterrupt();
/// Register the interrupt handler for this instance.
void attachInterruptCtx(int interrupt);
/// Un-register the interrupt handler for this instance.
void detachInterruptCtx(int interrupt);
private:
uint8_t _pin;
uint8_t _lastPinValue = LOW;
uint8_t _attached = INTERRUPT_HANDLER_DETACHED;
volatile unsigned _counter = 0;
unsigned long _lastChangeMs = millis();
public:
// Status of a fault cycle
// 0 = FAULT_CYCLE_STATE_OK
// 1 = FAULT_CYCLE_STATE_MID_REPORT
// 2 = FAULT_CYCLE_STATE_REPORTED
// 3 = FAULT_CYCLE_STATE_ERROR
uint8_t faultCycleStatus = FAULT_CYCLE_STATE_OK;
// Last confirmed fault 0 - 6
uint8_t confirmedFault = FAULT_CYCLE_STATE_OK;
// The time the last fault was reported (the ms the last HIGH signal ended)
unsigned long lastFaultReportMs;
};
inline unsigned DiagnosticInterruptHandler::read() const {
noInterrupts();
auto tmp = _counter;
interrupts();
return tmp;
}
#endif
File: DiagnosticInterruptHandler.cpp
// Tells compiler this file is to be included only once in a single compilation
#pragma once
#include "DiagnosticInterruptHandler.hpp"
DiagnosticInterruptHandler::DiagnosticInterruptHandler(uint8_t _pin) : _pin {_pin} {}
DiagnosticInterruptHandler::~DiagnosticInterruptHandler() { this->end(); }
// Im sure there's a better way to do this. I should probably have these in a separate header
// file (eg: FaultCodeLookup.hpp or something)
const char* DiagnosticInterruptHandler::faultExceptions[] = {
"OK",
"Battery protection cut-out",
"Fan over-current cut-out",
"Motor start error",
"Minimum motor speed error",
"Thermal cut-out of electronic unit",
"Controller hardware failure"
};
void DiagnosticInterruptHandler::init() {
pinMode( this->_pin, INPUT_PULLUP );
attachInterruptCtx(digitalPinToInterrupt( this->_pin ));
}
void DiagnosticInterruptHandler::end() {
if ( this->_attached == INTERRUPT_HANDLER_ATTACHED )
detachInterruptCtx(digitalPinToInterrupt( this->_pin ));
}
void DiagnosticInterruptHandler::_processInterrupt() {
unsigned long currentMs = millis();
int msSinceLastStateChange = (currentMs - this->_lastChangeMs);
int pinValue = digitalRead( this->_pin );
// Is the interrupt pin in a different value than it was in the last check?..
if ( pinValue != this->_lastPinValue ){
// .. If so, then this is likely triggered by the interrupt..
//
// Encountering state change - likely triggered by fault CHANGE interrupt
//
if ( pinValue == HIGH ){
this->faultCycleStatus = FAULT_CYCLE_STATE_MID_REPORT;
// Increment the counter since were just enterd a new HIGH signal
this->_counter++;
}
else {
// Todo
}
this->_lastChangeMs = millis();
}
//
// Conditions below here are likely not triggered by an interrupt, but rather a manual
// call to follow up on an active fault report cycle
//
else if ( pinValue == HIGH ) {
//
// Signal is HIGH and was HIGH on last _processInterrupt() execution. This
// was either executed manually too soon or there's an issue with the
// fault signal that's keeping it at HIGH
//
if ( msSinceLastStateChange > ( FAULT_SIG_DURATION_MS + FAULT_FUDGE_FACTOR_MS ) ) {
// I will add better error handling/reporting later.
Serial.print("ERROR: The interrupt pin has been in HIGH state for ");
Serial.print(msSinceLastStateChange);
Serial.print(" ms (");
Serial.print( FAULT_SIG_DURATION_MS - msSinceLastStateChange);
Serial.println(" ms longer than expected)");
this->faultCycleStatus = FAULT_CYCLE_STATE_ERROR;
}
}
else if ( msSinceLastStateChange > ( FAULT_REPORT_INTERIM_MS + FAULT_FUDGE_FACTOR_MS ) ) {
//
// Fault signal has been LOW for longer than it would take for a fault report
// cycle to be triggered by a CHANGE interrupt - thus, we can confidently
// infer that the fault has been cleared.
//
// Clear the fault cycle status
this->faultCycleStatus = FAULT_CYCLE_STATE_OK;
}
else if ( msSinceLastStateChange > ( FAULT_SIG_INTERIM_MS + FAULT_FUDGE_FACTOR_MS ) ){
//
// Fault signal has been LOW for longer than it would take for another signal
// to increment the report peak _counter - thus, we can confidently infer that
// the compressor has finished sending all peaks for this fault, and can now
// determine what the fault is.
//
// Update fault cycle status to reported
this->faultCycleStatus = FAULT_CYCLE_STATE_REPORTED;
// Set the last confirmed fault
this->confirmedFault = this->_counter;
// And reset the interrupt signal counter
this->_counter = 0;
}
else {
// Todo
}
// Store the current pin value so it can be compared against in the next cycle
this->_lastPinValue = pinValue;
}
void DiagnosticInterruptHandler::attachInterruptCtx(int interrupt) {
if ( this->_attached == INTERRUPT_HANDLER_ATTACHED ) {
//FATAL_ERROR(F("This instance was attached already"));
Serial.println("This instance was attached already");
return;
}
DiagnosticInterruptHandler::instance_handler = this;
this->_attached = INTERRUPT_HANDLER_ATTACHED;
attachInterrupt(interrupt, []{
DiagnosticInterruptHandler::instance_handler->_processInterrupt();
}, CHANGE);
}
void DiagnosticInterruptHandler::detachInterruptCtx(int interrupt) {
detachInterrupt(interrupt);
this->_attached = INTERRUPT_HANDLER_DETACHED;
}
DiagnosticInterruptHandler *DiagnosticInterruptHandler::instance_handler {};
File: Settings.hpp
// Tells compiler this file is to be included only once in a single compilation
#pragma once
#ifndef DIAGNOSTIC_INTERRUPT_HANDLER_SETTINGS_H
#define DIAGNOSTIC_INTERRUPT_HANDLER_SETTINGS_H
// Times used for determining what state the fault report from the compressor is in
constexpr unsigned int FAULT_SIG_INTERIM_MS = 250; // Milliseconds between flashes of the current sequence (1/4 of a second)
constexpr unsigned int FAULT_SIG_DURATION_MS = 250; // How many ms is the signal expected to stay in HIGH state for?
constexpr unsigned int FAULT_REPORT_INTERIM_MS = 4000; // Miliseconds between when the last blink of the error ends and the
// first blink of the next error.
constexpr unsigned int FAULT_FUDGE_FACTOR_MS = 100; // Just some extra MS to add to the checks to account for slight variations,
// (which there shouldn't actually be any)
// States of the fault analysis loop iterations
constexpr unsigned int FAULT_CYCLE_STATE_OK = 0; // No error found
constexpr unsigned int FAULT_CYCLE_STATE_MID_REPORT = 1; // In process of reporting (signals detected but not yet completed sending)
constexpr unsigned int FAULT_CYCLE_STATE_REPORTED = 2; // Fault report cycle completed (ie: the beep has stopped for ~4 seconds)
constexpr unsigned int FAULT_CYCLE_STATE_ERROR = 3; // When an unexpected state is encountered (eg: LED signal on for too long)
constexpr unsigned int INTERRUPT_HANDLER_DETACHED = false;
constexpr unsigned int INTERRUPT_HANDLER_ATTACHED = true;
#endif
File: FaultInterruptTester.ino
#include "DiagnosticInterruptHandler.hpp"
int initMs;
int currentFaultStatus;
DiagnosticInterruptHandler FaultHandler {2};
int updateCheckInterval = 1500;
int lastCheck = millis();
void setup() {
Serial.begin(9600);
Serial.println("IN SETUP!");
FaultHandler.init();
currentFaultStatus = FaultHandler.faultCycleStatus;
initMs = millis();
}
void loop() {
int nowMs = millis();
if ( currentFaultStatus != 0 && updateCheckInterval < ( nowMs - lastCheck ) ){
lastCheck = millis();
FaultHandler._processInterrupt();
}
if ( FaultHandler.faultCycleStatus != currentFaultStatus ){
Serial.print("Fault status changed from ");
Serial.print(currentFaultStatus);
Serial.print(" to ");
Serial.println(FaultHandler.faultCycleStatus);
currentFaultStatus = FaultHandler.faultCycleStatus;
if ( currentFaultStatus == 2 ){
Serial.print("FaultHandler.confirmedFault: ");
Serial.println(FaultHandler.confirmedFault);
if ( FaultHandler.confirmedFault != 0 ){
Serial.print("Description: ");
Serial.println(DiagnosticInterruptHandler::faultExceptions[FaultHandler.confirmedFault]);
}
}
}
}
Note: I'm not posting this expecting more input. If you want to provide some more constructive criticism then that's always welcome, but I just wanted to share the above so yall can see the end result of your input
Thanks again.
-J
P.S. I'll test this out on the actual compressor tomorrow, but for now I simulate the diagnostic signals using another Arduino (Uno). I can select what fault to signal (or stop signaling) and it'll send the pulses over at the correct duration and delays.
Right, you mentioned that before, but I'm curious why you think that? I get it can be done without interrupts, but since I don't know exactly when these HIGH signals will fire off I think using an interrupt is an ideal solution. It saves me from having to write code to check it on everyloop() iteration.
The only reason I may somewhat agree with you is that if you look in the main execution file (FaultInterruptTester.ino), you can see that I do have to execute some logic manually anyways. Since I can only determine if all of the HIGH pulses have been sent 1/4th of a second after the last pulse, and similarly can only determine if the fault has been cleared by seeing if there's been no pulse for 4+ seconds, those checks need to be executed some other way.
I'm trying to find a way around having to execute the "followup checks" in every single loop iteration for this. Like maybe executing the logic using an interrupt on a second pin, then use something like a 555 timer to trigger the interrupt >250ms and >4000ms after the HIGH pulse from the other interrupt.
Do you think that's a poor solution? If so, what would you recommend?
Im doing this with very limited experience in C++ and Arduino, so any alternative solutions you could recommend are always welcome.
P.S. Using some external circuitry to trigger the followup checks/method may seem excessive, but since the voltage being sent through the diagnostic output on the compressor actually can go up and down depending on the current in the compressor, I've decided to pass the fault signal through an optocoupler anyways to protect it against any spikes. So adding a 555 timer to trigger another interrupt seems ok to me.
Alternatively, instead of having to use two separate pins (1 for main fault signal interrupts, 1 for triggering the "followup checks"), I suppose I could use a single Analog pin for the interrupt and make a circuit that can send two different types of signals (one for a pulse when the compressor triggers a "blink" in the interrupt, and another for when the 555 timer is triggered), then only watch for that in the Arduino code - But my electrical ability is even more limited than my C++, so I think using 2 interrupts may be an acceptable solution, and in the future maybe I can change it once I get more proficient with electrical circuitry.
P.P.S. - Jeez... I need to stop spamming you guys with long replies. Sorry about that.
(As you said) handling faults gracefully usually is not done in the interrupt context and the ISR will basically just set a flag / some data and you end up picking up that flag in the loop so you depend on the loop's latency anyway to deal with whatever was caught by the interrupt. If your loop is slow, you are at risk of missing a flag so you can't afford a slow loop anyway
sometimes what triggers the action in the main loop is not directly triggered by the interrupt (cf your case where there needs to be no interrupt for some time to confirm the value of the error state) thus needing code in the loop.
interrupts are always creating complexity (protected area in the code, possible conflicts with other interrupt dependant elements (Serial, I2C, SPI, neopixels, PWM, ...) , portability, readability, ...). Not everyone is as good as @PieterP to manage interrupt in a class
your pulse signal (LOW HIGH LOW) lasts for 200ms to 250ms so even if your loop is slow, there is barely any chance you'd miss one pulse. I'd prefer to reserve interrupts for things changing fast that the loop could miss.
the idea of triggering another interrupt to let the main code know there is a flag set and ready to be handled is interesting but still means you would have to do something within an ISR context. By default on your Mega the interrupts are disabled within an interrupt, so depending on what you do and how long the process is, that can create contentions elsewhere or you need to reactivate the interrupts but then you need extra care for possible reentrant code etc..
long story short If I had to do this, I would write a "simple" state machine (possibly hidden in a class) that you call through a function which returns a bool and/or error code / state is possible
void loop() {
StatusType state; // some enum with the various issues
if (errorCodeDetected(state)) { // tick the state machine (polling)
// handle error code in state
...
}
// do everything else
}
side note, if you look at the Toggle library, you'll see it can return a presscode that can tell you how many long presses have been seen, so it actually might be all you need to get your code going
@Delta_G - Sorry I didn't reply to your previous post, somehow I missed it entirely.
Yeah, I thought you were saying "1/5th of a second is an eternity" as in complaining that the compressor was sending pulses too slowly, so 1/4th of a second would be even worse. But I see that's not what you meant now, my apologies.
You make some good points here. I may experiment with the same code, but just calling the processInterrupt method from the main loop() instead of an interrupt, and see which setup I like more or which one is quicker and uses less resources (and is easier to work with for future me).
I actually prefer to write libraries like this in OOP when possible, I just think having all of the functionality for it encapsulated in a class/object. It makes it easier to use in the future (though sometimes to the detriment of making future updates to the class a bit more difficult/tedious).
I see. So all of the extra logic I added (mostly in the _processInterrupt method) should actually be executed elsewhere.
Since I'm pretty certain that the interrupt should only get triggered at most about ~20 times a minute, and it shouldn't get any more complicated than what it is now (the code I shared last), I'm reasonably confident that the _processInterrupt shouldn't cause any latency issues in the loop. But since it's still a less than ideal solution, I'll give executing it in the loop a shot.
Yeah, as I mentioned above, I was planning on doing this using another interrupt triggered by some sort of timer circuit, but if I shouldn't have the _processInterrupt logic executed directly by an interrupt then obviously adding more code to be triggered by a separate interrupt should be avoided as well.
Indeed. I was definitely struggling with it, but PieterP made it seem very simple, lol.
Yes, I can see what you mean. I will definitely test it out and keep your suggestions in mind.
I'm not sure I follow what you mean, or maybe I didn't explain my idea sufficiently.
If you look at the last code snippets I pasted of the working example, in the .ino file the loop is as follows:
int updateCheckInterval = 1500;
void loop() {
int nowMs = millis();
int currentFaultStatus = FaultHandler.faultCycleStatus;
// If the current faultCycleStatus is not in OK status and its been more than 1.5 seconds
// since the last manual _processInterrupt, then execute it.
if ( currentFaultStatus != 0 && updateCheckInterval < ( nowMs - lastCheck ) ){
lastCheck = millis();
FaultHandler._processInterrupt();
}
if ( FaultHandler.faultCycleStatus != currentFaultStatus ){
// Fault state changed from currentFaultStatus to FaultHandler.faultCycleStatus
currentFaultStatus = FaultHandler.faultCycleStatus;
// If FaultCycleStatus is FAULT_CYCLE_STATE_MID_REPORT (meaning if it was last marked as in the middle of
// processing the interrupt signals for a fault)..
if ( currentFaultStatus == 1 ){
Serial.print("FaultHandler.confirmedFault: "); Serial.println(FaultHandler.confirmedFault);
if ( FaultHandler.confirmedFault != 0 ){
Serial.print("Description: "); Serial.println(DiagnosticInterruptHandler::faultExceptions[FaultHandler.confirmedFault]);
}
}
}
}
So what I was saying is that instead of executing this logic in the loop(), I could instead throw it in another method (probably as another method in the class), and that method would get executed when the interrupt is triggered.
Does that clear it up a bit?
I think that's similar to what I was saying I would do. Ill check out the Toggle library if I decide to do it like this (though you're half way there to talking me out of it, lol
I thought it was wasteful. Like having to execute it every loop when there will probably rarely be an active fault may be wasteful, but I see your point.
Lol, jeez. I'm definitely taking your input man, I just like to experiment with other ways so I get some actual experience as to why you suggested I do it differently.
Yes, I can see what you mean. I will definitely test it out and keep your suggestions in mind.