Updated per @hathach requested changes

* Changed to use compiler intrinsics
* Significantly reduced comments near memory barrier usage, as per request
* Updating `attachInterrupt()` loops to more closely match requested style

Author: henrygab

cores/nRF5/WInterrupts.c

@@ -107,57 +107,52 @@ int attachInterrupt(uint32_t pin, voidFuncPtr callback, uint32_t mode)
     ((polarity                 << GPIOTE_CONFIG_POLARITY_Pos) & GPIOTE_CONFIG_POLARITY_Msk) |
     ((GPIOTE_CONFIG_MODE_Event << GPIOTE_CONFIG_MODE_Pos    ) & GPIOTE_CONFIG_MODE_Msk    ) ;
 
-  // To avoid unexpected consequences, first loop through the channelMap
-  // to preferably re-use any channel that was already in use (even if
-  // earlier channel is no longer in use).
-  for (int ch = 0; ch < NUMBER_OF_GPIO_TE; ch++) {
-    // skip if the channel was not already assigned to this pin
-    if ((uint32_t)channelMap[ch] != pin) continue;
-
-    // The pin is already allocated in the channelMap
-    // update the polarity (when events fire) and callbacks
-    // However, do NOT clear any GPIOTE events
-    uint32_t tmp = NRF_GPIOTE->CONFIG[ch];
-    channelMap[ch] = pin;
-    callbacksInt[ch] = callback;
-    callbackDeferred[ch] = deferred;
-    tmp &= oldRegMask;
-    tmp |= newRegBits;
-    NRF_GPIOTE->CONFIG[ch] = tmp;
-    asm volatile ("" : : : "memory");
-    __asm__ __volatile__ ("nop\n\tnop\n\tnop\n\tnop\n");
-    NRF_GPIOTE->INTENSET = (1 << ch); // old code did this ... no harm in ensuring this is set
-    return (1 << ch);
+  int ch = -1;
+  int newChannel = 0;
+
+  // Find channel where pin is already assigned, if any
+  for (int i = 0; i < NUMBER_OF_GPIO_TE; i++) {
+    if ((uint32_t)channelMap[i] != pin) continue;
+    ch = i;
+    break;
+  }
+  // else, find one not already mapped and also not in use by others
+  if (ch == -1) {
+    for (int i = 0; i < NUMBER_OF_GPIO_TE; i++) {
+      if (channelMap[i] != -1) continue;
+      if (nrf_gpiote_te_is_enabled(NRF_GPIOTE, i)) continue;
+      
+      ch = i;
+      newChannel = 1;
+      break;
+    }
+  }
+  // if no channel found, exit
+  if (ch == -1) {
+    return 0; // no channel available
   }
 
-  // When the pin isn't already configured for interrupts, then attempt to
-  // find an unused GPIOTE channel.  This code is identical to the above,
-  // except for also validating that the MODE bits show the channel is disabled.
-  for (int ch=0; ch < NUMBER_OF_GPIO_TE; ch++) {
-    // skip if already used in AttachInterrupt for another pin
-    if (channelMap[ch] != -1) continue;
-    // skip if channel is not disabled (e.g., in use by some other component or library)
-    if (nrf_gpiote_te_is_enabled(NRF_GPIOTE, ch)) continue;
-
-    // clear any old events on this GPIOTE channel
-    uint32_t tmp = NRF_GPIOTE->CONFIG[ch];
-    channelMap[ch] = pin;
-    callbacksInt[ch] = callback;
-    callbackDeferred[ch] = deferred;
-    tmp &= oldRegMask;
-    tmp |= newRegBits;
-    // TODO: make check/set for new channel an atomic operation
-    NRF_GPIOTE->CONFIG[ch] = tmp;
-    asm volatile ("" : : : "memory");
-    __asm__ __volatile__ ("nop\n\tnop\n\tnop\n\tnop\n");
+  uint32_t tmp = NRF_GPIOTE->CONFIG[ch];
+  tmp &= oldRegMask;
+  tmp |= newRegBits;                 // for existing channel, effectively updates only the polarity
+  channelMap[ch]         = pin;      // harmless for existing channel
+  callbacksInt[ch]       = callback; // caller might be updating this for existing channel
+  callbackDeferred[ch]   = deferred; // caller might be updating this for existing channel
+  NRF_GPIOTE->CONFIG[ch] = tmp;
+
+  // For a new channel, additionally ensure no old events existed, and enable the interrupt
+  if (newChannel) {
+    // Why the memory barrier and NOPs? 
+    // See page 4 at https://web.archive.org/web/20190823222657/http://infocenter.arm.com/help/topic/com.arm.doc.dai0321a/DAI0321A_programming_guide_memory_barriers_for_m_profile.pdf
+    // and recall the AHB runs at 16MHz, so may take 4 cycles before peripheral register is actually updated.
+    __DSB(); __NOP();__NOP();__NOP();__NOP();
     NRF_GPIOTE->EVENTS_IN[ch] = 0;
-    asm volatile ("" : : : "memory");
-    __asm__ __volatile__ ("nop\n\tnop\n\tnop\n\tnop\n");
+    __DSB(); __NOP();__NOP();__NOP();__NOP();
     NRF_GPIOTE->INTENSET = (1 << ch);
-    return (1 << ch);
   }
-  // Else, pin was neither already setup, nor could a GPIOTE be allocated
-  return 0;
+
+  // Finally, indicate to caller the allocated / updated channel
+  return (1 << ch);
 }
 
 /*
@@ -173,28 +168,19 @@ void detachInterrupt(uint32_t pin)
 
   for (int ch = 0; ch < NUMBER_OF_GPIO_TE; ch++) {
     if ((uint32_t)channelMap[ch] == pin) {
-      // Unfortunately, simply marking a variable as volatile is not enough to
-      // prevent GCC from reordering or combining memory accesses, without also
-      // having an explicit sequence point.
-      //   See https://gcc.gnu.org/onlinedocs/gcc/Volatiles.html
-      //
-      // Here, ensure a sequence point by adding a memory barrier
-      // followed with four nops after having disabled the interrupt,
-      // to ensure the peripheral registers have been updated.
+      // Why the memory barrier and NOPs? 
+      // See page 4 at https://web.archive.org/web/20190823222657/http://infocenter.arm.com/help/topic/com.arm.doc.dai0321a/DAI0321A_programming_guide_memory_barriers_for_m_profile.pdf
+      // and recall the AHB runs at 16MHz, so may take 4 cycles before peripheral register is actually updated.
       NRF_GPIOTE->INTENCLR = (1 << ch);
-      asm volatile ("" : : : "memory");
-      __asm__ __volatile__ ("nop\n\tnop\n\tnop\n\tnop\n");
+      __DSB(); __NOP();__NOP();__NOP();__NOP();
+      NRF_GPIOTE->CONFIG[ch] = 0;
+      __DSB(); __NOP();__NOP();__NOP();__NOP();
+      NRF_GPIOTE->EVENTS_IN[ch] = 0; // clear any final events
 
       // now cleanup the rest of the use of the channel
       channelMap[ch] = -1;
       callbacksInt[ch] = NULL;
       callbackDeferred[ch] = false;
-      NRF_GPIOTE->EVENTS_IN[ch] = 0; // clear the event
-      // Finally, clear the CONFIG register only after ensure
-      // all the other state has been written to the peripheral registers
-      asm volatile ("" : : : "memory");
-      __asm__ __volatile__ ("nop\n\tnop\n\tnop\n\tnop\n");
-      NRF_GPIOTE->CONFIG[ch] = 0;
       break;
     }
   }
@@ -232,29 +218,10 @@ void GPIOTE_IRQHandler()
     NRF_GPIOTE->EVENTS_IN[ch] = 0;
   }
 #if __CORTEX_M == 0x04
-  // To quote from nRF52840 product specification:
-  // --------------------------------------------------------------------------
-  //   Note: To avoid an interrupt reoccurring before a new event has come,
-  //         the program should perform a read from one of the peripheral
-  //         registers. For example, the event register that has been cleared,
-  //         or the INTENCLR register that has been used to disable the
-  //         interrupt. This will cause a one to three cycle delay and ensure
-  //         the interrupt is cleared before exiting the interrupt handler.
-  //   Care should be taken to ensure the compiler does not remove the read
-  //   operation as an optimization. If the program can guarantee a four-cycle
-  //   delay after event being cleared or interrupt disabled in any other way,
-  //   then a read of a register is not required.
-  // --------------------------------------------------------------------------
-  //
-  // Unfortunately, simply marking a variable as volatile is not enough to
-  // prevent GCC from reordering or combining memory accesses, without also
-  // having an explicit sequence point.
-  //   See https://gcc.gnu.org/onlinedocs/gcc/Volatiles.html
-  //
-  // Here, ensure a sequence point by adding a memory barrier
-  // followed with four nops before exiting the ISR
-  asm volatile ("" : : : "memory");
-  __asm__ __volatile__ ("nop\n\tnop\n\tnop\n\tnop\n");
+  // See note at nRF52840_PS_v1.1.pdf section 6.1.8 ("interrupt clearing")
+  // See also https://gcc.gnu.org/onlinedocs/gcc/Volatiles.html for why
+  // using memory barrier instead of read of an unrelated volatile
+  __DSB(); __NOP();__NOP();__NOP();__NOP();
 #endif
 
 #if CFG_SYSVIEW