Add enumeration with pin values for digitalWrite etc

[dok-net]

For better code comprehension, it's useful to work with identifiers instead of integer literals.

[blemasle]

@dok-net Do you have more PR to come, or should I release a new version with these improvements right now ? 🙂

[dok-net]

@dok-net Do you have more PR to come, or should I release a new version with these improvements right now ? 🙂

There is something the library may or may not be able to help with: digitalRead corrupts the interrupt states for any of the other pins of the same port. At the very least, it should probably be mentioned in the library, otherwise this can only be found out about by deeply understanding the datasheet. That said, releases are cheap, plus, release early, release often, applies anyway :-)

[blemasle]

otherwise this can only be found out about by deeply understanding the datasheet

Given that the library is a very low level (relatively speaking of course given that it is intended for arduino) support of the mcp23017, having a good understanding of the datasheet is required to use most of it. Indeed, there is nothing we could do here I think apart from mentioning it in the comments.

That being said, you shouldn't use digitalRead after an interrupt before clearing them. You should instead capture and clear the interrupts as soon as possible before doing any call to digitalRead (see the PortCopyOnInterrupt example).

[dok-net]

That being said, you shouldn't use digitalRead after an interrupt before clearing them.

I think it's a permitted use calling only digitalRead in the context of using interrupts, as long as you know what you are doing (sic). I certain have implemented my application that way. Doesn't the datasheet document that digitalRead clears the interrupt states?

The aforementioned example can be a little confusing in it's own right, too few comments, and the discarding of the captured interrupt states doesn't help - in the end, it's even redundant given that digitalRead does the same thing. There should really be some more explanation, who's going to write that :-)

[blemasle]

I think it's a permitted use calling only digitalRead in the context of using interrupts, as long as you know what you are doing (sic).

I think you not fully aware of what you're doing 🙂 You should never do time sensitive / long running operations in interrupts. Reading through i2c sure is a long running operation with communication back and forth with two ics..

Doesn't the datasheet document that digitalRead clears the interrupt states?

Yes, it does indeed.

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.

it's even redundant given that digitalRead does the same thing.

It does not do the same thing. digitalRead read the current pins states, while clearInterrupts clear the interrupt and read the pin states the moment the interrupt was triggered. Those are two very different things that, depending on your wiring might gives very different results 🙂

and the discarding of the captured interrupt states doesn't help

Not sure to grasp what's bothering you here. clearInterrupts seems a good name for something that does just that. Plus, a look in the .h clears any remaining doubt regarding interrupts captures.

[dok-net]

I think you not fully aware of what you're doing 🙂 You should never do time sensitive / long running operations in interrupts. Reading through i2c sure is a long running operation with communication back and forth with two ics..

That's kind of offensive. I should ask you to consider the context and stay away from making assumptions. I was referring to the context of using interrupts on the GPIO expander, not the MCU.

[dok-net]

it's even redundant given that digitalRead does the same thing.

It does not do the same thing.

Given the context of my explanations, what "the same thing" meant was exactly and only "clears the interrupt states". You can trust me that I have read the datasheet before talking about it, and I know about the differences, too :-)

[blemasle]

That's kind of offensive.

Apologies, it was not meant that way but re-reading that sentence I do feel the issue 🙂

[dok-net]

and the discarding of the captured interrupt states doesn't help

Not sure to grasp what's bothering you here

I think I was talking about the example, not the library. The example makes no further use of the captured states after getting them for the lib. So it's only the example that I found a bit confusing for beginners, not the lib. Which is quite good. The MCP23017 is certainly one of the more complex I2C devices.

[blemasle]

in the end, it's even redundant given that digitalRead does the same thing

meant was exactly and only "clears the interrupt states"

Well, it does the same thing (clearing the interrupts) and reads the capture registers. So it is not that redundant in the sense that it does not read the current status of the MCP but does read the capture registers. It does however have the same side-effects of clearing the interrupts.

But using digitalRead will not only clear the interrupts but the capture registers as well. So the clearing part is redundant, but everything else is different.

[blemasle]

The example makes no further use of the captured states after getting them for the lib.

I remember the pain of getting some examples back when I was writing the lib.

The issue is that there so much you can do with these expanders, and so much way to hook them up that I found it difficult to convey any meaning in the examples without laying down some wirings. Doing so would not really made the usage of the lib clearer as it would kind of require you to grasp the example circuitry which would probably have no functional purpose to start understanding the example.

[dok-net]

I do have to say that I must have been misreading the identifiers in the example, so some of my statements about the example code are incorrect. I'm sorry.

[blemasle]

No worries. If you do see a way to make this clearer, I'm all in for another PR of yours 😉

[dok-net]

@blemasle One specific expression I wonder about in the PortCopyOnInterrupt.ino example:

    if((b & ~currentB) == (b & ~captureB)) {

Why use bitwise complement on both instead of comparing bit b verbatim?

Also, XOR-toggling may not be digestible to beginning users, and I am not sure this can't get out of sync with the actual state of B:

        mcp.writeRegister(MCP23017Register::GPIO_A, currentA ^ b);