I don’t have a github account so can’t contribute to the discussion in the issue itself, so will post here instead. Reviewing the MCP23017 data sheet, it appears that all I2C reads must be preceded by a SR (restart). Therefore I think the adafruit library port is in error and should be:
I believe the MCP23017 is expecting either another data byte (value to write into address) or a restart. The premature and unexpected stop/start sequence causes it to hang.
Can you try this and see whether there is any change in behavior. If this solves the problem then all the read methods in the library should be changed accordingly.
@pra and @BDub, thanks so much for looking into this. Keen observation @pra! Interesting that this wasn’t part of the Adafruit driver.
Just deployed code with this change on a few select motherboards running our application, with serial loggers hooked up to watch for i2c errors. Let’s see what happens over the next few days. Thanks again!
changes alone did not change our error rate in a statistically significant manner.
However, when coupled with an additional change of delayMicroseconds(1) inserted after each endTranmission and requestFrom, i’m seeing the error rate drop significantly.
So perhaps both changes are necessary to allow fewer glitches with this troublesome IC.
I’ve used this chip extensively with my TI CC3200 project and never had any problem with it, although I do use my own I2C interface code.
The data sheet states that if it receives a premature stop during a write transfer transaction, the write actions will be undone. This obviously would take some time and a small delay would be useful. However, with endTransmiission(false) we should no longer be performing a stop between the write address/read register sequence and a delay should have no effect.
couple of questions:
There are a number of these write/read sequences in the Adafruit MCP23017 library not just read register(). Especially those related to accessing the GPIO registers may be an issue. Did you change all of them so the register address setting’s end transmission is false?
Any chance you can get a Logic Analyzer trace with and without the delayMicroseconds() for the readRegister() example?
Hello @pra, we have some new info to share with you. It appears we were chasing a red herring regarding the delays, so I apologize for that misinformation. This is much more solid, as we have spent time isolating the issue.
Our QA team noticed that our binary with this updated code
and more importantly, starting with Photon firmware 0.4.6,
SYSTEM_THREAD(ENABLED);
we began to see corruption of our front panel LED state (driven by the MCP23017). This never before occurred in our program before 0.4.6.
We’ve spent the entire day slowly eliminating variables, until we finally found the root cause: Multi-threading.
With the exact same code except the SYSTEM_THREAD(ENABLED) line commented out, our program is running just as it should. When this line is un-commented, we’re seeing inconsistent forms of corruption (random LEDs on this port expander turning on and off) , likely due to a corrupted i2c message emanating from the Photon. We have never seen this issue until 0.4.6 (with Multi-threading enabled).
Has anyone else yet reported i2c issues using SYSTEM_THREAD(ENABLED)?
This seems to be quite repeatable, so i’ll try to capture this on a logic analyzer ASAP.
The i2c_hal functions may need to be encapsulated between taskEnterCritical() and taskExitCritical() functions to block any task switching while within the function. Same principle may apply to others such as SPI, Serial and USB.
@pra, here’s the issue I opened related to what we’ve seen. When blocking task switching, we’re seeing unprecedentedly low error rates using your change of Wire.endTransmission(false);
Not sure if this is the best way to re-open this one.
I am using trying out the MCP23017 with I2C comms to a Photon before building it into a product. I thought I would write the driver library myself and saw there were a few examples around.
The basic stuff - mcp.digitalWrite() and mcp.digitalRead() and map.pinMode() all works fine and reliably. I thought I would explore the interrupt features on the MCP23017.
I have a test rig with a button on one pin of each port and use an led output to signal which port pin has been taken HIGH.
I don’t have any capacitor on these switches to debounce.
What I have noticed is the following:
The first time in after a power down and power on it works nicely for a while then freezes - does not respond to button presses, during this time I can receive errors from the handler (when it read the interrupt pin the value was not HIGH).
A reset of the Photon will not clear the problem- it will go through the setup and then does not respond to button presses
I am on 0.6.0 RC2 and I have inserted and removed SYSTEM_THREAD(ENABLED) this doesn’t seem to make a difference.
I have tried to ensure that there is very little in the interrupt handler function - this just sets a volatile boolean with the main checks being done outside of the interrupt handler. I am wondering whether the issue is interrupt related should I put the I2C back into the interrupt handler. I wasn’t clear from the above what the advice was and whether this is now different with 0.6.0? Any Ideas?
#include "MCP23017.h"
SYSTEM_THREAD(ENABLED);
MCP23017 mcp;
//using the interrupt functionality
volatile boolean isAwokenByInterrupt = false;
void intHandler(void);
// Two pins on the MCP (Ports A/B where some buttons have been setup
// Buttons connect the pin to ground, and pins are pulled up to 5V when button pressed
byte mcpPinA = 7;
byte mcpPinB = 8;
int flashes = 0;
void setup()
{
Serial.begin(9600);
while (!Serial.available()) Particle.process();
Serial.println("MCP23017 Interrupt Test V6");
pinMode(D2,INPUT);
mcp.begin();
// Mirror INTA and INTB, so that only one line is required between MCP and Photon for interrupt
// The INTA/B will not be Floating, INTs will be signaled with a LOW (active LOW)
mcp.setupInterrupts(true,false,LOW);
mcp.pinMode(mcpPinA, INPUT); // interrupt will trigger when the pin is taken to 5V by a pushbutton
//mcp.pullUp(7, HIGH); // turn on a 100K pullup internally if button pulls pin to GND
mcp.setupInterruptPin(mcpPinA,RISING); //FALLING if pulled to GND or RISING if pulled to 5V
mcp.pinMode(mcpPinB, INPUT); // interrupt will trigger when the pin is taken to 5V by a pushbutton
//mcp.pullUp(8, HIGH); // turn on a 100K pullup internally if button pulls pin to GND
mcp.setupInterruptPin(mcpPinB,RISING); //FALLING if pulled to GND or RISING if pulled to 5V
mcp.pinMode(0, OUTPUT); // use the 0 LED as debugging
attachInterrupt(D2,intHandler,FALLING);
Serial.println("MCP23017 Interrupt Test - Setup completed");
}
void loop()
{
flashes = 0;
Serial.println("Waiting in Loop for interrupt");
while(!isAwokenByInterrupt) delay(200);
Serial.printlnf("Awoken by Interrupt A: %i B: %i", mcp.digitalRead(mcpPinA), mcp.digitalRead(mcpPinB));
if(isAwokenByInterrupt) handleInterrupt();
if (flashes > 0) //flash LED pin 0
{
for(int i=0;i<flashes;i++)
{
mcp.digitalWrite(0,HIGH);
delay(200);
mcp.digitalWrite(0,LOW);
delay(200);
}
}
}
// The int handler will just signal that the int has happened
// This will avoid resource clash issues on separate thread
void intHandler()
{
isAwokenByInterrupt = true;
}
//
void handleInterrupt()
{
// Get more information from the MCP from the INT
uint8_t pin=mcp.getLastInterruptPin();
uint8_t val=mcp.getLastInterruptPinValue();
// We will flash the led 1 (A) or 2 (B) times depending on the PIN that triggered the Interrupt
// 3 and 4 flashes are supposed to be impossible conditions... just for debugging.
flashes = 4;
if(pin==mcpPinA) flashes=1;
if(pin==mcpPinB) flashes=2;
if(val!=HIGH) flashes=3;
// we have to wait for the interrupt condition to finish - i.e. until all inputs are LOW
// an action is required to clear the INT flag, and allow it to trigger again.
while(!(mcp.digitalRead(mcpPinB) == LOW && mcp.digitalRead(mcpPinA) == LOW));
// and clean INT signal
isAwokenByInterrupt=false;
}
Can I ask why you are using the interrupt pin on the MCP23017 at all? I have used this chip as an input multiplexer on multiple projects and have never messed with the interrupt line at all. I just constantly poll the state of the inputs over I2C. It’s extremely fast.
The only time I have run into an application where the interrupt made sense was low power applications where I need to wake the Photon from deep sleep on interrupt from the MCP23017 but honestly that does not work all that well as by the time the module wakes up the pin is open again.
This is the class I used in my last project:
I have not used the MCP23017 library you are refering to but if you are just using the multiplexer as all inputs I can tell you this code is rock solid and has been running an irrigation system 24/7 for 8 months now without a single glitch.
To use the Library you first need to call setAddress on it. If you are not using the address lines of the MCP23017 then pass 0 for all setAddress function arguments.
To read the inputs simply call the readAllInputs function passing it a 2 position int array. The status for the first bank of inputs will fill the first int position in the array and the second bank of 8 inputs will fill the second int position in the array. You can then check the bits in the int to see the status of the inputs.
@IOTrav
I am using the Interrupt because it is there and interrupts are generally a better way to handle incoming signal changes than polling. I get your approach and if it has worked fine for you that’s great.
I wrote the library from scratch myself because I don’t want to use public libraries if I can avoid.
I did a bit more searching (on google) and found a thread and post on the Particle Community from December 14 where @Trekky had solved the issue. Essentially, the solution has not been included back in the community library example.
If you are interested the fix is:
The MCP Interrupt Register must be read to recognize interrupts correct. That means after the initialisation of the MCP (in setup() and after the interrupt has occurred it must be cleared with mcp.readGPIOAB().
The corresponding description is in the datasheet:
The interrupt will remain active until the INTCAP or GPIO register is read. Writing to these registers will not affect the interrupt. The interrupt condition will be cleared after the LSb of the data is clocked out during a read command of GPIO or INTCAP.
The first interrupt event will cause the port contents to be copied into the INTCAP register. Subsequent interrupt conditions on the port will not cause an interrupt to occur as long as the interrupt is not cleared by a read of INTCAP or GPIO.
Quick question - I want to read inputs that are active high and to avoid floating input pins thought I would use the MCP23017 to pullDown to ground the input pins - similar to pinMode(pin, INPUT_PULLDOWN); However, on close inspection I see that the GPPUP register on the MCP23017 only allows for PULLUP via a 100K ohms resistor. Other than wiring a 10K ohm resistor on each pin to GND - is there any other way to avoid false signals from drifting pins?
To my knowledge the MCP23017 and the MCP23008 only have internal pull up registers so you can pull inputs up but not down.
This is a Digital Multiplexer so it is only used for reading digital on/off signals when the IOs are set to inputs. It is preferred practice to pull digital inputs high rather than low. If you wanted a pulled down input this would be more applicable to Analog to Digital inputs rather than Digital inputs. Can I ask why you need the signal to be pulled high rather than low? If you are simply reading TTL lines(0-5V signals) those lines generally go high to 5VDC or low to ground so they would still work even if the digital lines are pulled high.
Thanks for replying quickly - simple answer is that the sensors I am using (PIR) are active HIGH, if I am looking for an input HIGH and the pin is not pulled down to GND I get stray signals where there is no sensor on the pin. I think what you are saying which is pull it up HIGH in all cases and then when a sensor is actually attached it will force it LOW? I don’t see how this works. I was thinking you might say invert the polarity of the signal and use pullup in that case.
@armor, do you know ahead of time which sensors will be connected or not? Or can you configure that via a Particle.function(). if so, you could just ignore the “not connected” pins.
Yes it is quite common for security devices such as motion detectors, window/door sensors, etc to be active low where they produce a voltage when not triggered and no voltage triggered(motion, open, etc). This is so you cannot simply cut the lines to them.
This however does pose an issue with using the MCP23017 as the input for monitoring these devices. If the device is triggered it will pull the input down to gnd and you can detect it on the input. However if someone were to cut the lines to the sensor you would never know it.
For this to work in a security application you really do need a pulled low input. This could be accomplished as you described earlier by simply pulling the input low to ground with a 100K resistor.
