Clear should be called first in atm128rfa1 pinchange interrupt enable

[GoogleCodeExporter]

There is at least reference that suggests to clear pinchange interrupt register 
before it is enabled: 
http://www.avrfreaks.net/index.php?name=PNphpBB2&file=viewtopic&t=71109

I am currently moving from our custom pinchange interrupt implementation for 
Iris to one that is in chips/atm128rfa1/pins directory.

Maybe there is no issues with it, however to be sure it should be reconsidered 
to clear interrupt before enabling it.

I propose chips/atm128rfa1/pins/AtmegaPinChangeP.nc enable functions should 
start with reset.

  async command error_t GpioInterrupt.enableRisingEdge[uint8_t pin](){
    atomic{
      isFalling &= ~(1<<pin);
      call HplAtmegaPinChange.reset();
      call HplAtmegaPinChange.setMask(call HplAtmegaPinChange.getMask() | (1<<pin));
      call HplAtmegaPinChange.enable();
    }
    return SUCCESS;
  }

Original issue reported on code.google.com by andres.v...@smartdustsolutions.com on 13 Dec 2011 at 12:46

[GoogleCodeExporter]

What would be helpful is if you test your proposed fix; I'm wary of checking in 
a bug fix to a potential problem if it breaks existing code.

Original comment by philip.l...@gmail.com on 8 Feb 2012 at 5:49

[GoogleCodeExporter]

Actually I have not seen an issue with the current implementation. I just 
spotted this avrfreaks topic and wondered whether is it the right way.

It worked with both implementations. I guess it should be closed, probably no 
one knows which is actually the correct way.

Original comment by andres.v...@smartdustsolutions.com on 10 Feb 2012 at 12:01

[GoogleCodeExporter]

I wrote this code, and you're right, the interrupt should be cleared, but only 
if it was disabled earlier:

  async command error_t GpioInterrupt.enableRisingEdge[uint8_t pin](){
    atomic{
      isFalling &= ~(1<<pin);
      call HplAtmegaPinChange.setMask(call HplAtmegaPinChange.getMask() | (1<<pin));
      if ( !call HplAtmegaPinChange.isEnabled() ){
        call HplAtmegaPinChange.reset();
        call HplAtmegaPinChange.enable();
      }
    }
    return SUCCESS;
  }

I will forward this to Miklos Maroti, who commits my codes.

Original comment by andras.b...@unicomp.hu on 15 Feb 2012 at 11:03

[GoogleCodeExporter]

Oh, and the original code works fine, only the interrupt handler will be 
executed without reason, but it won't signal anything to the upper layer.

Original comment by andras.b...@unicomp.hu on 15 Feb 2012 at 11:06

[GoogleCodeExporter]

Original comment by philip.l...@gmail.com on 12 Mar 2012 at 11:01

[GoogleCodeExporter]

I am not sure that there is a bug in the code. You can clear the interrupt 
flag, but still a pin change can slip by before you call enable. This will not 
eliminate interrupts, nor should it do. 

It is impossible to accurately model an edge sensitive GpioInterrupt with a 
shared pin change interrupt line, since there could be a sudden set/clear and 
by the time the interrupt routine is called, the pin is back to its original 
state. Therefore, only such devices should be connected to a PCINT that hold 
their interrupt in UP or DOWN for long (until it is cleared via e.g. SPI or 
I2C). 

For such devices, the code does exactly what it should. The actual filtering is 
done anyways in the HplAtmegaPinChange.fired method, so the real enable stuff 
happens when you set the mask. I think we leave the code but I add a comment. 
What do you think?

Original comment by mmar...@gmail.com on 13 Mar 2012 at 8:36

• Changed state: Started
• Added labels: Type-Task
• Removed labels: Type-Defect

[GoogleCodeExporter]

I agree, it works.

And a comment that clarifies this implementation detail should do the trick.

Original comment by andres.v...@smartdustsolutions.com on 13 Mar 2012 at 8:51

[GoogleCodeExporter]

Original comment by mmar...@gmail.com on 13 Mar 2012 at 9:16

• Changed state: Done