RobTillaart / MCP23S17

Arduino library for SPI based MCP23S17 16 channel port expander
MIT License
27 stars 11 forks source link

Using interrupts #40

Closed ryanaukes closed 5 months ago

ryanaukes commented 5 months ago

First of all, thank you for creating this library! It has been really useful to me.

I am wondering if you have any short term plans to implement the interrupt functions that the MCP23S17 supports? As far as I can tell the library currently does not support this.

Thanks in advance, Ryan

RobTillaart commented 5 months ago

Thanks for the question,

The library does not provide functions for interrupt handling yet as I did not need it and it is not requested until today :) Need to dive into the datasheet again to see what the device can do.

The MCP23S17 has 2 INT pins for the A and B register and I expect that the INT pins can be set to trigger. INT A triggers for changes in the A register and INT B for changes in the B register. Possibly it can do more interrupt related tricks, need to read the datasheet.

What kind of interrupt behavior do you need? Can you describe this in short?

RobTillaart commented 5 months ago

From datasheet page 12.

The 16-bit I/O port functionally consists of two 8-bit ports (PORTA and PORTB). The MCP23X17 can be configured to operate in the 8-bit or 16-bit modes via IOCON.BANK. There are two interrupt pins, INTA and INTB, that can be associated with their respective ports, or can be logically OR’ed together so that both pins will activate if either port causes an interrupt. The interrupt output can be configured to activate under two conditions (mutually exclusive):

  1. When any input state differs from its corresponding Input Port register state. This is used to indicate to the system master that an input state has changed.

  2. When an input state differs from a preconfigured register value (DEFVAL register). The Interrupt Capture register captures port values at the time of the interrupt, thereby saving the condition that caused the interrupt.


Datasheet Page 20:

3.5.5 INTERRUPT CONTROL REGISTER The INTCON register controls how the associated pin value is compared for the interrupt-on-change feature. If a bit is set, the corresponding I/O pin is compared against the associated bit in the DEFVAL register. If a bit value is clear, the corresponding I/O pin is compared against the previous value.


3.5.6 CONFIGURATION REGISTER The IOCON.MIRROR bit controls how the INTA and INTB pins function with respect to each other.


Datasheet Page 24 + 25 3.6 Interrupt Logic too long to repeat here

Summary The interrupt control module uses the following registers/bits:

So it looks not very complex, but I need to think how the API could be made

RobTillaart commented 5 months ago

Note to myself:

ryanaukes commented 5 months ago

Thanks for the question,

The library does not provide functions for interrupt handling yet as I did not need it and it is not requested until today :) Need to dive into the datasheet again to see what the device can do.

The MCP23S17 has 2 INT pins for the A and B register and I expect that the INT pins can be set to trigger. INT A triggers for changes in the A register and INT B for changes in the B register. Possibly it can do more interrupt related tricks, need to read the datasheet.

What kind of interrupt behavior do you need? Can you describe this in short?

Thanks for the quick reply! For a project I have to read about 64 analog inputs and about 80 digital inputs. For the digital inputs, I want to use 5 MCP23S17 I/O expanders. To make the program run as fast as possible, I prefer not to keep reading all the pins to check for button pressen. Using the interrupts I can only read the pins if there is a change. I only need to detect keypresses, I don’t necessarily have to detect key releases now, but maybe in the future I do. I think interrupt behaviour 1 (activate on pin change) best suits my (future) needs. I prefer not to use the mirror function, since that would mean that I have to read both port A and B if the interrupt triggers for that chip.

RobTillaart commented 5 months ago

...To make the program run as fast as possible, I prefer not to keep reading all the pins to check for button pressen. Using the interrupts I can only read the pins if there is a change. ...

OK, I understand the needs, need to think over the interface, find some time to work it out. Will take a few days. 5 MCP23S17 means you have 10 interrupt lines to handle, need to think that over too.

Just curious For the 64 analog inputs, what do you use? 10,12 bit?, multiplexer?

ryanaukes commented 5 months ago

My current plan is to use the PinChangeInterrupt library for the 10 interrupt pins. Another option would be to use a Teensy, but since I have a Arduino Mega lying around, I will try that out first.

For the analog inputs I am using 8 MCP3208 external 12 bit ADC's with SPI communication. With my current test sketch I am able to read all 64 inputs about 1000 times a second when running at the max clock speed the chip is rated for (2 MHz). In a real world scenario that number will probably decrease quite a bit though, since I am not doing anything with the values yet in my test sketch.

RobTillaart commented 5 months ago

Goodmorning After a night sleep I think the API could be in line with Arduino interrupts.

Per pin enableInterrupt(pin, mode)

CHANGE would compare with previous value The other two against DEFVAL.

Furthermore a enableInterruptMask(mask, mode) to set a group in one call.

Then disable functions to complete it.

Opinion?

ryanaukes commented 5 months ago

That is indeed a really need implementation, but the downside is that it limits the user to use the (limited) amount of pins that are usable for external interrupts.

For my use, it would be better to keep make the library only set the MCP's interrupt behaviour, so I can program the Arduino's reaction to the interrupt pins myself. This way, I could use the 'PinChange Interrupt' library and use any pin for interrupts.

Edit: Maybe an alternative is to make the mode parameter optional. When it is not used, the library only sets active the MCP23S17's interrupt pins. When the mode is entered, it could directly be coupled with the arduino AttachInterruot function.

I don't need (or want) to mirror the two interrupt pins, but maybe another optional parameter (mirror, default false) or separate function (setInterruptMode(separate / mirror)) is useful for someone else who does.

RobTillaart commented 5 months ago

That is indeed a really need implementation, but the downside is that it limits the user to use the (limited) amount of pins that are usable for external interrupts.

What I meant was that the MCP23S17 library gets these functions to set the interrupt behavior of the MCP23S17 itself.

It is not how to handle the interrupts in the Arduino/Teensy .

PinChange interrupt looks like a good solution. I worked on that library with GrayNomad (?) if I recall correctly long long ago. It is quite optimized for AVR but still relative slow compared to an hardware solution.


There might be even a faster way which involves some extra hardware like - https://www.ti.com/lit/ds/symlink/sn54hc148.pdf This would allow to put all 5 INT A lines in half a byte and all 5 INT B lines in the other half a byte. If these are combined in an IO register of your MEGA you could check the status in only a few clock cycles. (See direct port manipulation).

Because the 5 lines of INT A are converted to 3 lines the bit pattern e.g. 011 represents the device that has the interrupt. E.g. an interrupt of device 3 INT A would give 0011 0000, interrupt on device 3 on INT B would give 0000 0011.

Get the idea? Opinion?

As I am quite busy at the moment implementation / enhancement of the library will be later this week.

ryanaukes commented 5 months ago

Thanks for thinking along! I do get the concept. How would this handle multiple buttons connected to different chips being pressed at the same time?

I do prefer not to use extra hardware though, especially if there is a good way to solve it in software. According to this page the speed of the Pin Change Interrupt library is really good (see 'Arduino Pin Change Interrupt Speed').

RobTillaart commented 5 months ago

Created a develop branch + PR as placeholder for work to come

RobTillaart commented 5 months ago

Thanks for thinking along! I do get the concept. How would this handle multiple buttons connected to different chips being pressed at the same time?

When a MCP device generates an interrupt you must check all 8 lines as you do not know which lines triggered the INT. Furthermore I do not know what the device does if a second button is pressed and there is already an INT pending. Never tried but we might find out in a week or so.

RobTillaart commented 5 months ago

From datasheet, page 23

3.5.9 INTERRUPT CAPTURED REGISTER The INTCAP register captures the GPIO port value at the time the interrupt occurred. The register is read-only and is updated only when an interrupt occurs. The register remains unchanged until the interrupt is cleared via a read of INTCAP or GPIO.

So this register (1) helps to capture short pulses and (2) it helps to capture multiple changes. (answers the question above)

ryanaukes commented 5 months ago

Thanks for thinking along! I do get the concept. How would this handle multiple buttons connected to different chips being pressed at the same time?

Sorry, probably I wasn't clear with my question. I do know how the MCP23S17 and the interrupt pins respond to pin changes, but I meant to ask how the option you mentioned using the SN54HC148 would respond if different MCP23S17's send a interrupt signal.

E.g. an interrupt of device 3 INT A would give 0011 0000, interrupt on device 3 on INT B would give 0000 0011.

What would be the result when both device 2 INT A and device 3 INT were high at the same time? 0101 000? How would you be able to distinguish between 2A and 3A or 5A being active?

RobTillaart commented 5 months ago

Good thinking and good question, I did not consider such scenario (stupid me). It would cause problems, as these encoders assume only one line high/low at a time. So I dont know how such device would react, but it wont work for sure. Thank you for the learning moment :)

The PCINT library does handle it correctly as it can check all "pending pins".

RobTillaart commented 5 months ago

@ryanaukes

The development branch has now almost all interrupt support (except polarity) the chip has to offer, Can you please verify if it works for your needs? Thanks,

ryanaukes commented 5 months ago

@ryanaukes

The development branch has now almost all interrupt support (except polarity) the chip has to offer, Can you please verify if it works for your needs? Thanks,

Thanks for adding this feature so quick! Unfortunately, I do seem to have some problems. At first, everything seemed to work, but often it gets stuck and does not trigger the interrupt again.

This is the code that I am currently using to test the functionality:

// MISO / CIPO = 50 = PB3
// MOSI / COPI = 51 = PB2
// SCK = 52 = PB1
// MCP3208_CS = 53 = PB0
// MCP23S17_CS = 49 = PL0
// MCP23S17_INTA = 10
// MCP23S17_INTB = 11

#include "MCP23S17.h"
#include "PinChangeInterrupt.h"

MCP23S17 MCP(49);
int rv = 0;

void setup()
{
  Serial.begin(115200);

  pinMode(10, INPUT_PULLUP);
  pinMode(11, INPUT_PULLUP);
  attachPinChangeInterrupt(digitalPinToPinChangeInterrupt(10), checkBankA, FALLING);
  attachPinChangeInterrupt(digitalPinToPinChangeInterrupt(11), checkBankB, FALLING);

  SPI.begin();
  pinMode(49, OUTPUT);
  digitalWrite(49, HIGH);
  pinMode(53, OUTPUT);
  digitalWrite(53, HIGH);

  rv = MCP.begin();
  Serial.println(rv ? "true" : "false");
  MCP.setSPIspeed(8000000);

  rv = MCP.pinMode16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  rv = MCP.setPullup16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  rv = MCP.setPolarity16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  rv = MCP.enableInterrupt16(0B1111111111111111, CHANGE);
  Serial.println(rv ? "true" : "false");

  Serial.print("HWSPI: ");
  Serial.println(MCP.usesHWSPI() ? "true" : "false");
}

void loop()
{
}

void checkBankA() {
  Serial.println("Bank A triggered");
  uint8_t val = MCP.read8(0);
  for (int pin = 0; pin < 8; pin++)
  {
    Serial.print(bitRead(val, pin));
    Serial.print(' ');
  }
  Serial.println();
}

void checkBankB() {
  Serial.println("Bank B triggered");
  uint8_t val = MCP.read8(1);
  for (int pin = 0; pin < 8; pin++)
  {
    Serial.print(bitRead(val, pin));
    Serial.print(' ');
  }
  Serial.println();
}

This is the serial output

true
true
true
true
true
HWSPI: true
Bank A triggered
1 0 0 0 0 0 0 0 
Bank A triggered
0 0 0 0 0 0 0 0 
Bank A triggered
1 0 0 0 0 0 0 0 
Bank A triggered
0 0 0 0 0 0 0 0 
Bank A triggered
1 0 0 0 0 0 0 0 
Bank A triggered
0 0 0 0 0 0 0 0 
Bank A triggered
1 0 0 0 0 0 0 0 
Bank A triggered
0 0 0 0 0 0 0 0 
Bank A triggered
1 0 0 0 0 0 0 0 
Bank A triggered
0 0 0 0 0 0 0 0 
Bank A triggered
1 0 0 0 0 0 0 0 
Bank A triggered
1 0 0 0 0 0 0 0 

After this, the interrupt does not get triggered anymore. I completely have to unplug the arduino, resitting it does not work.

Honestly, I am not sure if this is caused by a problem in the PinChangeInterrupt library or the MCP23S17 library.

RobTillaart commented 5 months ago

Honestly, I am not sure if this is caused by a problem in the PinChangeInterrupt library or the MCP23S17 library.

Remove the PCINT library (temporary) and use one or two hardware IRQ pin. If that fails the problem is not in the PCINT lib. If that works the problem point towards the PCINT library.

Do you have pull up resistors on the IO lines of the MCP23S17? If not it could perhaps be caused by some static build up or so?

Another test is to read the MCP every X seconds to see if the MCP freezes

PS I updated your last post with cpp after the three backquotes to get syntax highlighting :)

RobTillaart commented 5 months ago

Another tip is to use my heartbeat library to pulse a LED as long as loop() runs. It could also be that the Arduino freezes e.g. memory/heap error? If so the heartbeat would stop.

ryanaukes commented 5 months ago

Remove the PCINT library (temporary) and use one or two hardware IRQ pin. If that fails the problem is not in the PCINT lib. If that works the problem point towards the PCINT library.

It seems to be the PCINT library that makes the interrupts freeze. Using harware interrupt pins works fine. Unfortunately those will be used for some rotary encoders in the future.

Do you have pull up resistors on the IO lines of the MCP23S17? If not it could perhaps be caused by some static build up or so?

I do have the internal pull up resistors of the MCP23S17 enabled using the setPullup16 function.

Another tip is to use my heartbeat library to pulse a LED as long as loop() runs. It could also be that the Arduino freezes e.g. memory/heap error? If so the heartbeat would stop.

The arduino seems to keep running, even when the interrupts stop working when using the PCINT library. It heart keeps beating :)

All in all, It seems like the PCINT library freezes for some reason.....

ryanaukes commented 5 months ago

I just found out that it not advised to use Serial.println functions in an Interrupt Service Routines.

Do not use loops, delay(), millis(), serial print and read commands, or micros() inside an ISR.

Source: DigiKey

I changed that by setting a boolean in the ISR's and reading and printing the pin values in the loop function, but this unfortunately did not solve the problem I was having with the PCINT library.

RobTillaart commented 5 months ago

I do have the internal pull up resistors of the MCP23S17 enabled using the setPullup16 function.

OK should be sufficient

All in all, It seems like the PCINT library freezes for some reason.....

Your interrupt routines are BIG in terms of time to handle. It is better to just set a flag and process the value in loop.

Give this a try

// MISO / CIPO = 50 = PB3
// MOSI / COPI = 51 = PB2
// SCK = 52 = PB1
// MCP3208_CS = 53 = PB0
// MCP23S17_CS = 49 = PL0
// MCP23S17_INTA = 10
// MCP23S17_INTB = 11

#include "MCP23S17.h"
#include "PinChangeInterrupt.h"

MCP23S17 MCP(49);
int rv = 0;

volatile bool flagA = false;
volatile bool flagB = false;
uint8_t valA = 0;
uint8_t valB = 0;

void setup()
{
  Serial.begin(115200);
  Serial.println(__FILE__);

  pinMode(10, INPUT_PULLUP);
  pinMode(11, INPUT_PULLUP);
  attachPinChangeInterrupt(digitalPinToPinChangeInterrupt(10), checkBankA, FALLING);
  attachPinChangeInterrupt(digitalPinToPinChangeInterrupt(11), checkBankB, FALLING);

  SPI.begin();
  pinMode(49, OUTPUT);
  digitalWrite(49, HIGH);
  pinMode(53, OUTPUT);
  digitalWrite(53, HIGH);

  rv = MCP.begin();
  Serial.println(rv ? "true" : "false");
  MCP.setSPIspeed(8000000);

  rv = MCP.pinMode16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  rv = MCP.setPullup16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  rv = MCP.setPolarity16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  rv = MCP.enableInterrupt16(0B1111111111111111, CHANGE);
  Serial.println(rv ? "true" : "false");

  Serial.print("HWSPI: ");
  Serial.println(MCP.usesHWSPI() ? "true" : "false");
}

void loop()
{
  if (flagA)
  {
    valA = MCP.read8(0);
    Serial.println("Bank A triggered");
    for (uint8_t mask = 0x80; mask; mask >>= 1)
    {
      Serial.print(valA & mask));
      Serial.print(' ');
    }
    Serial.println();
    flagA = false;
  }
  if (flagB)
  {
    valB = MCP.read8(0);
    Serial.println("Bank B triggered");
    for (uint8_t mask = 0x80; mask; mask >>= 1)
    {
      Serial.print(valB & mask));
      Serial.print(' ');
    }
    Serial.println();
    flagB = false;
  }
}

void checkBankA() 
{
  flagA = true;
}

void checkBankB() 
{
  flagB = true;
}
RobTillaart commented 5 months ago

OK, same conclusion overhere

RobTillaart commented 5 months ago

Note to myself:

ryanaukes commented 5 months ago

Give this a try

There were a few typo's in the code, which I had to change:

Unfortunately it still does not work. Do you mind checking if the code works for you?

RobTillaart commented 5 months ago

I do not have the hardware nearby as I am working on a few other libraries which fill my desk. Furthermore I have a project to prototype coming days so little time, sorry.

ryanaukes commented 5 months ago

No problem, I understand!

In the meantime, I also tried using Pin Change Interrupt without any libraries by using the registers. This works a lot better, but sometimes still makes one of the two interrupt pins (for example the interrupt pin for bank A) stuck. Most of the times when I pres a button connected to the other bank (for example a button connected to bank A), both interrupts start working again.

So it doesn't seem to be a problem with the PCINT library.

RobTillaart commented 5 months ago

just a last thought of the day,

maybe instead of the read() function, on should use

It would at least show which pin had the interrupt correctly in case same pin had (a multiple of) 2 changes. These are not seen with read16()

ryanaukes commented 5 months ago

Today I did some more testing. When measuring the output of the interrupt pins of the McP23S17, they stay stuck in the low state, even though I am reading the pin state, which should reset the interrupt pins. They stay stuck in this low state until I unplug the USB cable (disconnect power)

Can it be a timing issue (reading the pins too quickly for example), related to the times on page 9/10 on the datasheet?

What I've tried:

maybe instead of the read() function, on should use

  • uint16_t getInterruptCaptureRegister(); or

  • uint16_t getInterruptFlagRegister();

I tried that, unfortunately without any improvements.

RobTillaart commented 5 months ago

Can it be a timing issue (reading the pins too quickly for example), related to the times on page 9/10 on the datasheet?

Yes, but how I read that table it would be about 1-2 microsecond at most. Given the delays you use and low SPI speed that should be enough to have no conflicts with time.

mmm, have you tried a pull up resistor on the INT line? I assume you have, but ask to be sure.

Maybe I should add the setInterruptPolarity() so you can see if that helps.


setting the trigger for the interrupt to LOW instead of FALLING. This so far seems to fix it, but is not possible for PCINT. Besides, this is more of a workaround than a real fix of the underlying problem.

You are talking about the hardware interrupt of the MEGA I assume.

RobTillaart commented 5 months ago

@ryanaukes

Added the bool setInterruptPolarity() function to the develop branch MCP library. build is running

ryanaukes commented 5 months ago

mmm, have you tried a pull up resistor on the INT line? I assume you have, but ask to be sure.

I have set the pinMode to INPUT_PULLUP

You are talking about the hardware interrupt of the MEGA I assume.

Correct

While looking at the code I was wondering if the getInterruptFlagRegister and getInterruptCaptureRegister are correct. Shouldn't there be a port parameter? It looks like it is only reading the interrupt flag and interrupt capture of port A:

//  which pins caused the INT?
uint16_t MCP23S17::getInterruptFlagRegister()
{
  return readReg(MCP23S17_INTF_A);
}

uint16_t MCP23S17::getInterruptCaptureRegister()
{
  return readReg(MCP23S17_INTCAP_A);
}
RobTillaart commented 5 months ago

Good catch should be readReg16()

ryanaukes commented 5 months ago

Good catch should be readReg16()

In that case there is no advantage of not mirroring the interrupt pins, since you always have to read both ports. I think to fully take advantage of the two individual interrupt pins, a separate function that just reads one of the ports is needed. In my opinion thats not a priority though, but something for the future. Later this week I will see what happens when I change the polarity if the the interrupt pins!

Thanks again for all the great work and help!

RobTillaart commented 5 months ago

Fixed readReg16() in the develop branch.

RobTillaart commented 5 months ago

Found this one - https://arduino.stackexchange.com/questions/90940/why-doesnt-my-mcp23s17-interrupts-work-anymore


Datasheet P25, 3.6.5

Pins configured for interrupt-on-change from register value will cause an interrupt to occur if the corresponding input pin differs from the register bit. The interrupt condition will remain as long as the condition exists, regardless if the INTCAP or GPIO is read.

Could explain why the interrupt stays?

ryanaukes commented 5 months ago

Found this one - https://arduino.stackexchange.com/questions/90940/why-doesnt-my-mcp23s17-interrupts-work-anymore

Doesn't that post describe the opposite of my problem? In that post the OP was using the LOW trigger that made the interrupt keep triggering in a loop. If i am using the LOW trigger my code works, but when I just use a FALLING trigger it does not.

Datasheet P25, 3.6.5

Pins configured for interrupt-on-change from register value will cause an interrupt to occur if the corresponding input pin differs from the register bit. The interrupt condition will remain as long as the condition exists, regardless if the INTCAP or GPIO is read.

Could explain why the interrupt stays?

In my code I use the interrupt mode CHANGE. If I am correct the pins should be configured for interrupt-on-pin-change (INTCON bit not set). In this case, the interrupt pin should reset after reading the GPIO or INTCAP register.

Datasheet 3.6.5: 1. Pins configured for interrupt-on-pin change will cause an interrupt to occur if a pin changes to the opposite state. The default state is reset after an interrupt occurs and after clearing the interrupt condition (i.e., after reading GPIO or INTCAP). For example, an interrupt occurs by an input changing from ‘1’ to ‘0’. The new initial state for the pin is a logic ‘0’ after the interrupt is cleared.

If the pin is configured for interrupt-on-change from register value (INTCON bit set), it should indeed behave as you quoted.

Besides that, when I plug in the Arduino it works fine at first, untill it gets stuck.

This is what I expect and how I read the datasheet for my configuration:

Sometimes (it looks random to me), the interrupt pins does not get reset after reading the INTCAP register. The interrupt pin stays low. Because it stays low, the interrupt does not get triggered again. This makes the interrupt for that bank 'blocked'. The other bank keeps working, untill that gets stuck too.

RobTillaart commented 5 months ago

Sometimes (it looks random to me), the interrupt pins does not get reset after reading the INTCAP register.

I gave it some thoughts however no ideas / possible scenarios popped up.

If I understand well the MCP23S17 itself blocks and reading registers does not unblock the device.

Does reading the pin register read8() or read16() affect the blocked state?

ryanaukes commented 5 months ago

Sometimes (it looks random to me), the interrupt pins does not get reset after reading the INTCAP register.

I gave it some thoughts however no ideas / possible scenarios popped up.

If I understand well the MCP23S17 itself blocks and reading registers does not unblock the device.

Does reading the pin register read8() or read16() affect the blocked state?

Somtimes it does reset the interrupt pin when doing a read8/read16/getInterruptCaptureRegister. In that case, everything works fine.

Unfortunately sometimes the interrupt pin does not reset after a read8/read16/getInterruptCaptureRegister. Because I only do execute a read8/read16/getInterruptCaptureRegister once when the interrupt signal is falling, if it misses the reset the interrupt pin stays low and cannot be triggered again. It is not the whole chip that gets stuck though. The other bank keeps working. It is just the interrupt pin that stays low and thus can't trigger an interrupt function again.

I hope my explanation is clear.

When I set the interrupt trigger to LOW instead of FALLING, it keeps reading the port until it resets. Then it does work. Unfortunately, that is not possible with PCINT

RobTillaart commented 5 months ago

I hope my explanation is clear.

yes


...Because I only do execute a read8/read16/getInterruptCaptureRegister once when the interrupt signal is falling,

thinking out loud, would a "watchdog" help, a function that checks on a low pace if the interrupt lines are reset properly and if not "read until they do"? (not a nice workaround but if it works it might give a better understanding what happens)

ryanaukes commented 5 months ago

I made the code output the interrupt pin states every second. Below is the code and serial output.

Code:

#include "MCP23S17.h"
#include "PinChangeInterrupt.h"

MCP23S17 MCP(49);
int rv = 0;

bool checkA = false;
bool checkB = false;

unsigned long lastCheck = 0;

void setup()
{
  Serial.begin(115200);
  Serial.println();
  Serial.print("MCP23S17_LIB_VERSION: ");
  Serial.println(MCP23S17_LIB_VERSION);
  delay(100);

  SPI.begin();
  pinMode(49, OUTPUT);
  digitalWrite(49, HIGH);
  pinMode(53, OUTPUT);
  digitalWrite(53, HIGH);

  pinMode(10, INPUT_PULLUP);
  pinMode(11, INPUT_PULLUP);

  attachPCINT(digitalPinToPCINT(10), interruptA, FALLING);
  attachPCINT(digitalPinToPCINT(11), interruptB, FALLING);

  rv = MCP.begin();
  Serial.println(rv ? "true" : "false");
  MCP.setSPIspeed(8000000);

  // Set all pins to input
  rv = MCP.pinMode16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  // Enable internal pull-up resistor for all pins
  rv = MCP.setPullup16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  // Set polarity of all pins
  rv = MCP.setPolarity16(0B1111111111111111);
  Serial.println(rv ? "true" : "false");

  Serial.print("HWSPI: ");
  Serial.println(MCP.usesHWSPI() ? "true" : "false");

  // Set interrupt polarity
  rv = MCP.setInterruptPolarity(0);
  Serial.println(rv ? "true" : "false");

  // Enable interrupt for all pins
  rv = MCP.enableInterrupt16(0B1111111111111111, CHANGE);
  Serial.println(rv ? "true" : "false");
}

void interruptA() {
  checkA = true;
}

void interruptB() {
  checkB = true;
}

void loop()
{
  if(lastCheck + 1000 < millis()) {
    Serial.print("Interrupt A (pin 10): ");
    Serial.print(digitalRead(10));
    Serial.print("\tInterrupt B (pin 11): ");
    Serial.println(digitalRead(11));
    lastCheck = millis();
  }

  if(checkA) {
    Serial.println("PCINT A triggered");
    Serial.print("Interrupt A (pin 10): ");
    Serial.print(digitalRead(10));
    Serial.print("\tInterrupt B (pin 11): ");
    Serial.println(digitalRead(11));
    Serial.print("FlagRegister\t");

    // ToDo: only read bank A when input A is triggered
    uint16_t flag = MCP.getInterruptFlagRegister();
    for (int pin = 0; pin < 16; pin++)
    {
      uint8_t val = bitRead(flag, pin);
      Serial.print(val);
      Serial.print(" ");
    }
    Serial.println();
    Serial.print("CaptRegister\t");
    uint16_t capt = MCP.getInterruptCaptureRegister();
    for (int pin = 0; pin < 16; pin++)
    {
      uint8_t val = bitRead(capt, pin);
      Serial.print(val);
      Serial.print(" ");
    }
    Serial.println();

    Serial.print("Interrupt A (pin 10): ");
    Serial.print(digitalRead(10));
    Serial.print("\tInterrupt B (pin 11): ");
    Serial.println(digitalRead(11));
    checkA = false;
  }

  if(checkB) {
    Serial.println("PCINT B triggered");
    Serial.print("Interrupt A (pin 10): ");
    Serial.print(digitalRead(10));
    Serial.print("\tInterrupt B (pin 11): ");
    Serial.println(digitalRead(11));
    Serial.print("FlagRegister\t");

    // ToDo: only read bank B when input B is triggered
    uint16_t flag = MCP.getInterruptFlagRegister();
    for (int pin = 0; pin < 16; pin++)
    {
      uint8_t val = bitRead(flag, pin);
      Serial.print(val);
      Serial.print(" ");
    }
    Serial.println();
    Serial.print("CaptRegister\t");
    uint16_t capt = MCP.getInterruptCaptureRegister();
    for (int pin = 0; pin < 16; pin++)
    {
      uint8_t val = bitRead(capt, pin);
      Serial.print(val);
      Serial.print(" ");
    }
    Serial.println();

    Serial.print("Interrupt A (pin 10): ");
    Serial.print(digitalRead(10));
    Serial.print("\tInterrupt B (pin 11): ");
    Serial.println(digitalRead(11));
    checkB = false;
  }
}

Serial output

MCP23S17_LIB_VERSION: 0.5.2
true
true
true
true
HWSPI: true
true
true
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT B triggered
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
FlagRegister    1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
CaptRegister    1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT B triggered
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
FlagRegister    1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT B triggered
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
FlagRegister    0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
CaptRegister    0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT B triggered
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
FlagRegister    0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT A triggered
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
FlagRegister    0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1
Interrupt A (pin 10): 0 Interrupt B (pin 11): 1

As you can see the interrupt pin stays low even though the INTCAP register has been read. I tried changing the getInterruptCaptureRegister function to read8 or read16, but this doesn't change anything. I also tried changing the interrupt polarity and changing the interrupt trigger to RISING instead of FALLING.

ryanaukes commented 5 months ago

I changed the following part in the code to see what happens when I do a read16 when one of the pins stay low

if(lastCheck + 1000 < millis()) {
    Serial.print("Interrupt A (pin 10): ");
    Serial.print(digitalRead(10));
    Serial.print("\tInterrupt B (pin 11): ");
    Serial.println(digitalRead(11));

    if(digitalRead(10) == LOW || digitalRead(11) == LOW) {
      Serial.println("INTERRUPT STUCK!!!!!");
      MCP.read16();
    }

    lastCheck = millis();
  }

Here is a part of the serial output:

Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT B triggered
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
FlagRegister    1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
CaptRegister    1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
PCINT B triggered
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
FlagRegister    1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
CaptRegister    0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
Interrupt A (pin 10): 1 Interrupt B (pin 11): 0
INTERRUPT STUCK!!!!!
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1
Interrupt A (pin 10): 1 Interrupt B (pin 11): 1

So it really seems that sometimes reading the INTCAP register does not reset the interrupt pin, but when you read the pins again it does reset them.

The big question is why this happens. I am completely stuck (No pun intended...) Timing (reading too quick)? Bad hardware (even though all the other function work properly)?

ryanaukes commented 5 months ago

I might be on to something... I think it is caused by the interrupt triggering for a second time while the handling of the interrupt is not finished in the loop. Here is a really simple piece of code that shows that:

#include "MCP23S17.h"

MCP23S17 MCP(49);

bool checkA = false;

const int interruptAPin = 2;

void setup()
{
  Serial.begin(115200);
  SPI.begin();

  pinMode(49, OUTPUT);
  digitalWrite(49, HIGH);
  pinMode(53, OUTPUT);
  digitalWrite(53, HIGH); // disable other SPI device

  pinMode(interruptAPin, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(interruptAPin), interruptA, FALLING);

  MCP.begin();
  MCP.setSPIspeed(8000000);
  MCP.pinMode16(0B1111111111111111); // Set all pins to input
  MCP.setPullup16(0B1111111111111111); // Enable pull-up resistors for all pins
  MCP.setPolarity16(0B1111111111111111); // Set polarity of all pins  
  MCP.setInterruptPolarity(0); // Set interrupt polarity
  MCP.enableInterrupt16(0B1111111111111111, CHANGE); // Enable interrupt for all pins
  MCP.getInterruptCaptureRegister(); // read interrupt capture register to reset interrupt
}

void interruptA() {
  checkA = true;
  Serial.println("Interrupt A triggered!");
}

void loop()
{  
  if(checkA) {
    // ToDo: only read bank A when input A is triggered
    uint16_t flag = MCP.getInterruptFlagRegister();
    uint16_t capt = MCP.getInterruptCaptureRegister();

    while(digitalRead(interruptAPin) == LOW) {
      delay(10);
      Serial.println("Waiting for int to be reset...");
    }

    Serial.println("Done!");

    checkA = false;
  }
}

And the serial output:

15:09:32.576 -> Interrupt A triggered!
15:09:32.576 -> Done!
15:09:32.718 -> Interrupt A triggered!
15:09:32.718 -> Done!
15:09:33.520 -> Interrupt A triggered!
15:09:33.520 -> Done!
15:09:33.615 -> Interrupt A triggered!
15:09:33.615 -> Done!
15:09:34.135 -> Interrupt A triggered!
15:09:34.135 -> Done!
15:09:34.277 -> Interrupt A triggered!
15:09:34.277 -> Interrupt A triggered!
15:09:34.277 -> Waiting for int to be reset...
15:09:34.277 -> Waiting for int to be reset...
15:09:34.277 -> Waiting for int to be reset...
15:09:34.324 -> Waiting for int to be reset...
15:09:34.324 -> Waiting for int to be reset...

I suspect this is due to bouncing of the button

RobTillaart commented 5 months ago

Re-entrance of interrupts is a known problem.

You can tackle this by adding a few lines in the ISR.

void ISR()
{
  if (micros() - lastInterruptA < 20) return;
  lastInterruptA= micros();
  ...

For every ISR you need a last interrupt timestamp.

RobTillaart commented 5 months ago

Note, variables shared with interrupts should be declared volatile. Then the compiler will not optimize e.g. cache the value.

volatile bool checkA = false
ryanaukes commented 5 months ago

I fixed it by adding a simple RC circuit to prevent the bouncing. This, together with the Schmitt triggers that are already present in the MCP23S17, makes everything work fine!

RobTillaart commented 5 months ago

What values dit you use for R and C? (Future reference)

ryanaukes commented 5 months ago

For now I used a 10k resistor. Together with the internal pull-up resistor of 100K that makes the low voltage about 0.45V, well below the input low voltage of the MCP23S17 (0.2 VDD = 1V @ 5V VDD).

I used a 100nF capacitor. Unfortunatly I don't have an oscilloscope to check what the signal looks like and if I can reduce the size of the capacitor to make the changes as fast as possible.

RobTillaart commented 5 months ago

Thanks, Be careful with optimizing as tolerance in R and C can "disrupt" the timing just over the edge.

ryanaukes commented 5 months ago

Before you merge the dev branch into master: This part of the enableInterrupt16 does not look right, since they both do the same for RISING and FALLING mode Shouldn't the deval be set to mask when the mode is set to FALLING instead of the inverse of mask?

    if (mode == RISING)
    {
      intcon = mask;
      defval = ~mask;  //  RISING == compare to 0
    }
    else if (mode == FALLING)
    {
      intcon = mask;
      defval = ~mask;  //  FALLING == compare to 1
    }