Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
GlibSkunk opened this issue Aug 2, 2024 · 4 comments · Fixed by #48
Closed
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@GlibSkunk
Copy link
Contributor

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 RobTillaart self-assigned this Aug 2, 2024
@RobTillaart
Copy link
Owner

RobTillaart commented Aug 2, 2024

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
Copy link
Owner

RobTillaart commented Nov 19, 2024

@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 RobTillaart added the bug Something isn't working label Nov 19, 2024
@RobTillaart
Copy link
Owner

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
Copy link
Owner

@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

RobTillaart added a commit that referenced this issue Nov 22, 2024
- fix #47, interrupt handling. Kudos to GlibSkunk!
- update readme.md
- minor edits.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
2 participants