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

Marian Buschsieweke notifications at github.com
Thu Mar 7 11:18:14 CET 2019


maribu commented on this pull request.

Some preliminary comments, I now need to attend some other stuff and will resume the review later

> +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 gpio_t pcint_mapping[][8] = {

This can be `const`, can't it?

>      if (int_num < 0) {
+	    /* If pin change interrupts are enabled, enable mask and interrupt */
+        #ifdef PCINT_NUM_BANKS

We do not have an official RIOT coding style on indent of preprocessor, so treat this as a comment and feel free to just ignore it:

I personally do not favor to keep C language indent for CPP, as those are two different language and cannot reasonably be mixed. E.g. 

```C
switch(var) {
    case FOO:
        /* some code */
        break;
     #idef HAS_BAR
     case BAR:
         /* some code */
         break;
         #endif /* <-- two or one level of indent?
}
```

Therefore I personally favor to have no indent before the preprocessor macros. This also makes them more visible, as those look a bit out of place. This additional "spotlight" on c preprocessor code make overlooking them harder and - in my personal opinion - reading the code easier.

(But I again: Feel free to ignore this, there is no policy on that a.f.a.i.k.)

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

Forgot to remove comment?

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

Missing space between `for` and `(`. Same in multiple other lines. (I'd suggest to run `uncrustify`)

>      if (int_num < 0) {
+	    /* If pin change interrupts are enabled, enable mask and interrupt */
+        #ifdef PCINT_NUM_BANKS
+        int8_t offset = -1;
+        //uint8_t port_num = _port_num(pin);
+        uint8_t pin_num = _pin_num(pin);
+        uint8_t b;
+        for(b=0; b<PCINT_NUM_BANKS; b++) {
+            for(uint8_t p=0; p<8; p++) {
+                if(pin == pcint_mapping[b][p]) {
+                    offset = b*8+p;

spaces missing

>      if (int_num < 0) {
+	    /* If pin change interrupts are enabled, enable mask and interrupt */
+        #ifdef PCINT_NUM_BANKS
+        int8_t offset = -1;
+        //uint8_t port_num = _port_num(pin);
+        uint8_t pin_num = _pin_num(pin);
+        uint8_t b;
+        for(b=0; b<PCINT_NUM_BANKS; b++) {
+            for(uint8_t p=0; p<8; p++) {
+                if(pin == pcint_mapping[b][p]) {
+                    offset = b*8+p;
+                    //pcint_lookup[ _port_num(pin) * 8 + pin_num ] = i;

Forgot to remove comment?

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

I think a longer name than `b` could increase readability of the code

> @@ -59,6 +63,88 @@
 #endif
 
 static gpio_isr_ctx_t config[GPIO_EXT_INT_NUMOF];
+
+/**
+ * @brief detects ammount of possible PCINTs
+ */
+#if defined(MODULE_ATMEGA_PCINT) || defined(MODULE_ATMEGA_PCINT0) || defined(MODULE_ATMEGA_PCINT1) || defined(MODULE_ATMEGA_PCINT2) || defined(MODULE_ATMEGA_PCINT3)
+#include "atmega_pcint.h"
+
+#ifndef ATMEGA_PCINT_MAP_PCINT0
+#error Please define pin change interrupts in atmega_pcint.h
+#endif /* ATMEGA_PCINT_MAP_PCINT0 */
+
+/**
+ * @brief check which pcints should be enabled!
+ */
+#if defined(MODULE_ATMEGA_PCINT) || defined(MODULE_ATMEGA_PCINT0)

You can have module `atmega_pcint` depend on `atmega_pcint0` to `atmega_pcint3` in order to simplify your life here

> +        /* if pcint was not found: return -1  */
+        if(offset < 0) {
+            return offset;
+        }
+
+        /* save configuration for pin change interrupt */
+        pcint_config[offset].pin     = pin;
+        pcint_config[offset].flank   = flank;
+        pcint_config[offset].arg     = arg;
+        pcint_config[offset].cb      = cb;
+
+        /* init gpio */        
+        gpio_init(pin, mode);
+        /* configure pcint */
+        cli();
+        switch (b) {

Shouldn't the cases below be placed in `#ifdef MODULE_PCINT<NUM>` ... `#endif`?

-- 
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-211692354
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190307/e0aeeb3c/attachment-0001.html>


More information about the notifications mailing list