code review

[stefanbachert]

I detected a defect from reading your source code

In the function pifacedigital_wait_for_input the register 0x11 will be cleared. This will throwing away any interrupts which happened between last call of pifacedigital_wait_for_input. This make application blind for this time frame.

The better approach is to separate it into two calls. One for clearing and one for waiting. So applications which depend on current behavior (are there any) could call both function, while application which don't want to get blind uses the new version.

void pifacedigital_clear_interrupt(uint8_t hw_addr)
{
    // Flush any pending interrupts
    pifacedigital_read_reg(0x11, hw_addr);
}

int pifacedigital_wait_for_input(uint8_t *data,
                             int timeout,
                             uint8_t hw_addr)
{
   // Wait for input state change
   int ret = mcp23s17_wait_for_interrupt(timeout);

   if (ret<= 0) {
        return ret;
   }
   // Read & return input register, thus clearing interrupt
   // but only when there is an interrupt!!
   *data = pifacedigital_read_reg(0x11, hw_addr);
   return ret;
}

A typical appication will be structured like

pifacedigital_enable_interrupts();
pifacedigital_clear_interrupt (HW_ADDR);

while (1) {  // non blind version
     int val = 0;
     int cause = pifacedigital_wait_for_input (&val, timeout, HW_ADDR);
     if (cause > 0) {
         // handle data
     } else if (cause == 0) {
         // handle timeout
     } else {
         // handle error, may be break
     }
}

[tompreston]

Hm, interesting point. However, can't this already be achieved by using the mcp23s17 functions?

So, in your typical application:

pifacedigital_enable_interrupts();
// clear interrupt 
// pifacedigital_clear_interrupt (HW_ADDR);
pifacedigital_read_reg(0x11, HW_ADDR);

while (1) {  // non blind version
    int val = 0;
    // int cause = pifacedigital_wait_for_input (&val, timeout, HW_ADDR);
    int cause = mcp23s17_wait_for_interrupt(timeout);
    if (cause > 0) {
        // handle data
        val = pifacedigital_read_reg(0x11, HW_ADDR);
    } else if (cause == 0) {
        // handle timeout
    } else {
        // handle error, may be break
    }
}

I do agree it would be nicer/clearer to have this in the libpifacedigital library however fixing it introduces an backwards incompatible change and I'm not sure this cosmetic difference is worth it. What do you think?