[riot-notifications] [RIOT-OS/RIOT] cpu/atmega: WIP implementation of pin change interrupts (#7610)

lebrush notifications at github.com
Mon Sep 18 15:18:26 CEST 2017


lebrush requested changes on this pull request.

This is not trivial to implement.

On one hand implementing it in gpio_init_int makes it transparent for the user, but on the other hand (with the current implementation) requires `32 * (sizeof(gpio_isr_ctx_t) + sizeof(gpio_flank_t)) ~= 32 * (4 + 1) = 160 bytes` which are not going to be used if PCINT is not used. It's actually more than that, since you have to keep track of the previous state of the port to detect which pin triggered the interrupt, and consider if you want to act or not.

One option is to implement it as a separate function (some CPUs have speciall stuff in gpio (i.e. STM32)). And if the function is not used then the memory will be stripped out. But if it's used only once, all the memory requirements are pulled in.

Another option (which is what is done in the STM32 IRQ lines) is tho allow only 1/vector. This means 4 (not anymore 32, in the worst case). Actually 3, considering that one vector is used for the context switch.

I would prefer the laster (I've it working locally but never got the time to cleanup and publish it). What do you think?

> +static inline uint8_t pcint_port_pin(volatile uint8_t *reg)
+{
+    uint8_t pin = 0;
+    uint8_t val = *reg;
+
+    while (val > 1) {
+        val = val >> 1;
+        pin++;
+    }
+    return pin;
+}
+
+#ifdef GPIO_PC_INT_NUMOF
+
+#ifndef AVR_CONTEXT_SWAP_INTERRUPT_VECT_NUM
+#error gpio.c requires the definition of AVR_CONTEXT_SWAP_INTERRUPT_VECT_NUM

This should not go here. It wil be catched already by thread_arch.c

> @@ -237,61 +293,114 @@ void gpio_write(gpio_t pin, int value)
     }
 }
 
+static inline void pcint_handler(uint8_t port_num, uint8_t pin_num)
+{
+    int gpio_state = gpio_read( GPIO_PIN(port_num, pin_num) );
+    gpio_flank_t flank = pcint_flank[ port_num * 8 + pin_num ];
+
+    if (flank == GPIO_BOTH || (gpio_state && flank == GPIO_RISING) || (!gpio_state && flank == GPIO_FALLING)) {
+        __enter_isr();
+        pcint[port_num * 8 + pin_num].cb( pcint[port_num * 8 + pin_num].arg );
+        __exit_isr();
+    }
+}
+
+static inline uint8_t pcint_port_pin(volatile uint8_t *reg)

This doesn't work, imagine PC3 and PC0 are 1, there's an pcint enabled for PC1. You'll call `pcint_handler` with C and 2.

> @@ -237,61 +293,114 @@ void gpio_write(gpio_t pin, int value)
     }
 }
 
+static inline void pcint_handler(uint8_t port_num, uint8_t pin_num)
+{
+    int gpio_state = gpio_read( GPIO_PIN(port_num, pin_num) );
+    gpio_flank_t flank = pcint_flank[ port_num * 8 + pin_num ];
+
+    if (flank == GPIO_BOTH || (gpio_state && flank == GPIO_RISING) || (!gpio_state && flank == GPIO_FALLING)) {

You can't assume which the pin triggered the interrupt. You have to keep track of the previous state of the port state.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/7610#pullrequestreview-63341062
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20170918/5ac3e933/attachment.html>


More information about the notifications mailing list