RobTillaart / MCP23S17

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

enableInterrupt Pin Parameter Different from other Single Pin Interface Functions #47

Closed GlibSkunk closed 1 week ago

GlibSkunk commented 3 months ago

I presume this is not intended behavior, so I thought I'd bring this to your attention. I ran into an issue today trying to set up interrupts on specific pins using the single pin interface (rather than all the 16 pin functions). The final distinction I noticed is that this section of code (or a similar construction) appears in all single pin interface functions except for enableInterrupt().

uint8_t dataDirectionRegister = MCP23x17_DDR_A;
  if (pin > 7)
  {
    dataDirectionRegister = MCP23x17_DDR_B;
    pin -= 8;
  }

The upshot is that to set up an interrupt on GPA0, I have to use:

MUX.pinMode1(0,INPUT_PULLUP);
MUX.enableInterrupt(8,CHANGE);

This is not very intuitive and I presume all that would need to be modified within enableInterrupt() is adding the section that translates 0..15 into pins assuming 0..7 are within Port A and 8..15 are within Port B. If you agree that this should be done, I can make a pull request (my second ever, so excuse me if it takes a couple tries :) )

RobTillaart commented 3 months ago

Thanks for the issue, As I am rather busy I will look into this later this month (or when time permits)

Think your observation makes sense, have to study the datasheet again on this topic.

I can make a pull request (my second ever, so excuse me if it takes a couple tries :) )

Please create a PR, Please not that you have to handle configure 2 registers (INTCON + DEFVAL)


Note to myself: check MCP23017/008/S08 repo's too (look alikes)

RobTillaart commented 1 week ago

@GlibSkunk Finally had some time to have a first look, apologies for the too long delay.

(enable interrupt context) Your code works with 8 bit registers where mine works on16 bit register. And that is a different pattern than for the other single bit functions.

Side effect should be that the indices do not match.

No time to fix it completely (including testing) yet but I will try understand the proposed code today.


Noticed there is a bug in the existing code. defval |= ~(1 << pin); // FALLING == compare to 1

RobTillaart commented 1 week ago

Yes, read and understood your code proposal and I will merge it asap.

I need to bump the version info etc, thereafter.

Thanks again for spotting and reporting and fixing the issue, appreciated.

RobTillaart commented 1 week ago

@GlibSkunk

Created a develop branch for version 0.6.0 which includes your fix, updated readme, updated version. Please verify if this develop branch works if you have time. Thanks