[riot-notifications] [RIOT-OS/RIOT] cpu/atmega_common: pseudomodule-based pin change interrupt implementation (#11122)

Marian Buschsieweke notifications at github.com
Wed Jul 24 10:37:35 CEST 2019


maribu requested changes on this pull request.

I found a bug and added some suggestions for improvement. See inline comments.

Also no spaces around the braces in array accesses are not used in other code of RIOT. E.g. `array[ idx ]` should be `array[idx]`.

> +#ifndef ATMEGA_PCINT_H
+#define ATMEGA_PCINT_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define ATMEGA_PCINT_MAP_PCINT0 GPIO_PIN(PORT_B, 0), GPIO_PIN(PORT_B, 1), GPIO_PIN(PORT_B, 2), GPIO_PIN(PORT_B, 3), GPIO_PIN(PORT_B, 4), GPIO_PIN(PORT_B, 5), GPIO_PIN(PORT_B, 6), GPIO_PIN(PORT_B, 7)
+#define ATMEGA_PCINT_MAP_PCINT1 GPIO_PIN(PORT_E, 0), GPIO_PIN(PORT_J, 0), GPIO_PIN(PORT_J, 1), GPIO_UNDEF, GPIO_UNDEF, GPIO_UNDEF, GPIO_UNDEF, GPIO_UNDEF
+#define ATMEGA_PCINT_MAP_PCINT2 GPIO_PIN(PORT_K, 0), GPIO_PIN(PORT_K, 1), GPIO_PIN(PORT_K, 2), GPIO_PIN(PORT_K, 3), GPIO_PIN(PORT_K, 4), GPIO_PIN(PORT_K, 5), GPIO_PIN(PORT_K, 6), GPIO_PIN(PORT_K, 7)
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* ATMEGA_PCINT_H */

This file now conflicts with the test: The test sets up also PCINT interrupts for pins not connected to the Arduino Mega2560 header. (If I recall correctly it was my suggestion to override the pcint mapping for this board. But now I think this was a bad idea. E.g. other RIOT code does not prevent the user from using pins available at the MCU but not connected to pin header. Maybe it would be better to remove this file and stick with the default mapping for the ATmega2560 MCU?)

> +        if (enabled_pcints & 0x1) {
+            /* re-construct mask from pin number*/
+            uint8_t pin_mask = (1 << pin_num);
+            /* get pin from mapping (assumes 8 entries per bank!) */
+            gpio_t pin = pcint_mapping[ bank * 8 + pin_num ];
+            uint8_t port_value = (_SFR_MEM8(_pin_addr( pin )));
+            uint8_t pin_value = ((port_value & pin_mask) != 0);
+            uint8_t old_state = ((pcint_state[ bank ] & pin_mask) != 0);
+            gpio_isr_ctx_pcint_t *conf = &pcint_config[ bank * 8 + pin_num ];
+            if (old_state != pin_value) {
+                if (pin_value) {
+                    pcint_state[ bank ] |= (pin_mask);
+                }
+                else {
+                    pcint_state[ bank ] &= ~(pin_mask);
+                }

```C
                if (pin_value) {
                    pcint_state[ bank ] |= (pin_mask);
                }
                else {
                    pcint_state[ bank ] &= ~(pin_mask);
                }
```
This could be simplified to `pcint_state[bank] ^= pin_mask;`, which will just toggle the bit. That would be correct for both cases

> @@ -254,6 +428,77 @@ static inline void irq_handler(uint8_t int_num)
     __exit_isr();
 }
 
+#ifdef PCINT_NUM_BANKS
+/* inline function that is used by the PCINT ISR */
+static inline void pcint_handler(uint8_t bank, volatile uint8_t *mask_reg)
+{
+    __enter_isr();
+    /* Find right item */
+    uint8_t pin_num = 0;
+    uint8_t enabled_pcints = *mask_reg;
+
+    while (enabled_pcints > 0) {
+        /* check if this pin is enabled & has changed */
+        if (enabled_pcints & 0x1) {
+            /* re-construct mask from pin number*/
+            uint8_t pin_mask = (1 << pin_num);

This does not work, as `pin_num` is the idx in the PCINT mapping, not the pin number in the GPIO port. (Those happen to be the same for all but the ATmega2560.)

Here is my suggestion to fix this: https://github.com/maribu/RIOT/commit/8f28ebd73272a4dac4b112b6fa2a3ad8c348ddd3

> @@ -254,6 +428,77 @@ static inline void irq_handler(uint8_t int_num)
     __exit_isr();
 }
 
+#ifdef PCINT_NUM_BANKS
+/* inline function that is used by the PCINT ISR */
+static inline void pcint_handler(uint8_t bank, volatile uint8_t *mask_reg)

instead of `volatile uint8_t *mask_reg`, why not directly pass the value as `uint8_t enabled_pcints` in this function?

> +                     (!pin_value && conf->flank == GPIO_FALLING))) {
+                    /* execute callback routine */
+                    conf->cb(conf->arg);
+                }
+            }
+        }
+        enabled_pcints = enabled_pcints >> 1;
+        pin_num++;
+    }
+
+    __exit_isr();
+}
+#if defined(PCINT0_IDX)
+ISR(PCINT0_vect, ISR_BLOCK)
+{
+    pcint_handler(PCINT0_IDX, &PCMSK0);

With my suggestion above, this would become

``` C
cint_handler(PCINT0_IDX, PCMSK0);
```

instead of

``` C
cint_handler(PCINT0_IDX, &PCMSK0);
```

> +        pin_num++;
+    }
+
+    __exit_isr();
+}
+#if defined(PCINT0_IDX)
+ISR(PCINT0_vect, ISR_BLOCK)
+{
+    pcint_handler(PCINT0_IDX, &PCMSK0);
+}
+#endif /* PCINT0_IDX */
+
+#if defined(PCINT1_IDX)
+ISR(PCINT1_vect, ISR_BLOCK)
+{
+    pcint_handler(PCINT1_IDX, &PCMSK1);

same as above

> +{
+    pcint_handler(PCINT0_IDX, &PCMSK0);
+}
+#endif /* PCINT0_IDX */
+
+#if defined(PCINT1_IDX)
+ISR(PCINT1_vect, ISR_BLOCK)
+{
+    pcint_handler(PCINT1_IDX, &PCMSK1);
+}
+#endif  /* PCINT1_IDX */
+
+#if defined(PCINT2_IDX)
+ISR(PCINT2_vect, ISR_BLOCK)
+{
+    pcint_handler(PCINT2_IDX, &PCMSK2);

same as above

> +{
+    pcint_handler(PCINT1_IDX, &PCMSK1);
+}
+#endif  /* PCINT1_IDX */
+
+#if defined(PCINT2_IDX)
+ISR(PCINT2_vect, ISR_BLOCK)
+{
+    pcint_handler(PCINT2_IDX, &PCMSK2);
+}
+#endif  /* PCINT2_IDX */
+
+#if defined(PCINT3_IDX)
+ISR(PCINT3_vect, ISR_BLOCK)
+{
+    pcint_handler(PCINT3_IDX, &PCMSK3);

same as above

-- 
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/11122#pullrequestreview-265865627
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190724/cd08991a/attachment.htm>


More information about the notifications mailing list