Bring enableInterrupt()/disableInterrupt() in line with other single pin interfaces

[GlibSkunk]

Resolves #47

I tried to keep the spirit of the code as seen in the pinMode1 function. Also happened to hit some of the enhancements noted in the comments before the enableInterrupt function.

I don't have a very comprehensive testing setup but I did utilize the changes on the own board to set up interrupts and it behaved correctly.

[RobTillaart]

Thx for the PR?

A thought

Is the interrupt16() mask compatible with the interrupt()?

Assuming mask = [15..0] If you set pin 0 what mask is returned by getInterrupt16() (This kind of consistency)

[GlibSkunk]

You're welcome. Thanks for the Library!

So in general I believe the the 16 functions are a bit conceptually out of whack with the single pin versions. However I see that this has to do with what arises naturally out of the MCP23S17's register map rather than any Library issue.

The natural 0..15 (used in single pin functions) is: GPA0, GPA1 ... GPA7, GPB0, GPB1, ... GPB7

The 0..15 from the datasheet (used in 16 pin functions) is: GPB0, GPB1 ... GPB7, GPA0, GPA1, ... GPA7

I think the only way to get consistency would be to swap the first and second byte so that you could use reg[3] to access GPA4, matching the "natural" way (for me at least). I don't think using [15..0] would have the desired effect.

However, I'm honestly not sure that's worth doing regardless. The 16 mode as implemented matches the datasheet so I'd say it's best to leave it alone and let people choose to either use the 16 bit "full" interface or the single bit at a time interface. The only reason for my difficulty was because the enableInterrupt function appeared to match the pinMode1 function but didn't.

[GlibSkunk]

Alternatively, I had another thought of how to achieve consistency (again I don't know that it's worth upending the library).

This section of code in begin() exists to ensure the sequential mode is active:

//  disable address increment - datasheet P20
//  note that address increment must be enabled for readReg16() and writeReg16() to work.
//    SEQOP: Sequential Operation mode bit
//    1 = Sequential operation disabled, address pointer does not increment.
//    0 = Sequential operation enabled, address pointer increments.
uint8_t reg = readReg(MCP23x17_IOCR);
if (reg & MCP23x17_IOCR_SEQOP)  //  check if already zero
{
  reg &= ~MCP23x17_IOCR_SEQOP;  //  clear SEQOP bit for sequential read/write
  if (! writeReg(MCP23x17_IOCR, reg)) return false;
}

The phrase "note that address increment must be enabled for readReg16() and writeReg16() to work" is actually not quite accurate. On page 13 of the datasheet -

A special mode (Byte mode with IOCON.BANK = 0) causes the address pointer to toggle between associated A/B register pairs. For example, if the BANK bit is cleared and the Address Pointer is initially set to address 12h (GPIOA) or 13h (GPIOB), the pointer will toggle between GPIOA and GPIOB. Note that the Address Pointer can initially point to either address in the register pair.

Because all read16 and write16 work on A/B pairs (that I've seen at least), even with sequential mode off, they would work and bounce between the registers. So my proposal is: turn off sequential mode (IOCON.SEQOP=1) and target the B port for all read16/write16. That way the upper byte is associated with the B port and the following lower byte is associated with the A port.

Obviously this would be a breaking change and warrants more consideration, but I think it would have the desired outcome.

[RobTillaart]

@GlibSkunk

I extracted your other thoughts into a separate issue - https://github.com/RobTillaart/MCP23S17/issues/50 Don't know when I will dive into them but those thoughts are worth more investigation / time from my side. Thanks,

[RobTillaart]

@GlibSkunk

Fixed the same bug in the I2C version - MCP23017_RT repo