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

Marian Buschsieweke notifications at github.com
Fri Mar 8 16:00:36 CET 2019


maribu commented on this pull request.

OK, this PR adds 147 B RAM with 3 pcint banks enabled. The minimum required RAM seems to be:

- Per bank: 1 byte to store the banks previous state
- Per pin:
    - 2 byte to store the callback function
    - 2 byte to store the user argument (`void *arg`)
    - 1 byte for the flank

In total this is `3 + 3 * 8 * 5 = 123` bytes. With the changes suggest above this PR reaches this requirements.

Also currently it adds 908B  to the ROM size. I think by using a 1d array for `pcint_mapping` this will become a bit less, but I didn't try.

> +static uint8_t pcint_state[ PCINT_NUM_BANKS ];
+
+/**
+ * @brief stores all cb and args for all defined pcint.
+ */
+typedef struct {
+    gpio_cb_t cb;           /**< interrupt callback */
+    gpio_t pin;             /**< pin which the interrupt is for */
+    gpio_flank_t flank;     /**< type of interrupt the flank should be triggered on */
+    void *arg;              /**< optional argument */
+} gpio_isr_ctx_pcint_t;
+
+/**
+ * @brief
+ */
+static const gpio_t pcint_mapping[][8] = {

How about making this a 1d array and ... (see below)

>      if (int_num < 0) {
+ /* If pin change interrupts are enabled, enable mask and interrupt */
+ #ifdef PCINT_NUM_BANKS
+        int8_t offset = -1;
+        uint8_t pin_num = _pin_num(pin);
+        uint8_t bank;
+        for (bank = 0; bank < PCINT_NUM_BANKS; bank++) {

... (see above) make this a single loop running, e.g.

```C
for (unsigned i = 0; i < sizeof(pcint_mapping)/sizeof(pcint_mapping[0]); i++) {
    if (pin == pcint_mapping[i]) {
        offset = i;
        break;
    }
}
```

> @@ -115,6 +213,7 @@ static inline uint16_t _pin_addr(gpio_t pin)
 int gpio_init(gpio_t pin, gpio_mode_t mode)
 {
     uint8_t pin_mask = (1 << _pin_num(pin));
+

unrelated change

> @@ -254,6 +422,66 @@ 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) {
+            uint8_t pin_mask = (1 << pin_num);
+            gpio_t pin = pcint_mapping[ bank ][ pin_num ];

This would need to become `pcint_mapping[(bank << 3) + pin_num]` for the 1d array

> +#define _COUNTER3 _COUNTER2
+#endif /* MODULE_ATMEGA_PCINT3 */ 
+
+#define PCINT_NUM_BANKS (_COUNTER3)
+
+/**
+ * @brief stores the last pcint state of each port
+ */
+static uint8_t pcint_state[ PCINT_NUM_BANKS ];
+
+/**
+ * @brief stores all cb and args for all defined pcint.
+ */
+typedef struct {
+    gpio_cb_t cb;           /**< interrupt callback */
+    gpio_t pin;             /**< pin which the interrupt is for */

unused member, please delete

> +                    offset = bank * 8 + p;
+                    break;
+                }
+            }
+            /* break outer loop if pin was found */
+            if(offset != -1) {
+                break;
+            }
+        }
+        /* if pcint was not found: return -1  */
+        if (offset < 0) {
+            return offset;
+        }
+
+        /* save configuration for pin change interrupt */
+        pcint_config[offset].pin = pin;

unused, please delete

> +
+#define PCINT_NUM_BANKS (_COUNTER3)
+
+/**
+ * @brief stores the last pcint state of each port
+ */
+static uint8_t pcint_state[ PCINT_NUM_BANKS ];
+
+/**
+ * @brief stores all cb and args for all defined pcint.
+ */
+typedef struct {
+    gpio_cb_t cb;           /**< interrupt callback */
+    gpio_t pin;             /**< pin which the interrupt is for */
+    gpio_flank_t flank;     /**< type of interrupt the flank should be triggered on */
+    void *arg;              /**< optional argument */

I would prefer to `arg` directly below `cb`

-- 
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-212309057
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190308/3482bb00/attachment.html>


More information about the notifications mailing list