[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 13:01:44 CET 2019


maribu commented on this pull request.

Some more comments. (Sorry for spotting that not the first time.)

> +#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 ];
+            uint8_t port_value = (_SFR_MEM8(_pin_addr( GPIO_PIN( _port_num(pin), pin_num ))));
+            uint8_t pin_value = (port_value & pin_mask) == 0 ? 0 : 1;

maybe `(port_value & pin_mask) != 0` is more readable? (Next line the same)

> @@ -0,0 +1,7 @@
+#ifndef ATMEGA_PCINT_H
+#define ATMEGA_PCINT_H
+#define ATMEGA_PCINT_MAP_PCINT0 { GPIO_PIN(0, 0), GPIO_PIN(0, 1), GPIO_PIN(0, 2), GPIO_PIN(0, 3), GPIO_PIN(0, 4), GPIO_PIN(0, 5), GPIO_PIN(0, 6), GPIO_PIN(0, 7) }
+#define ATMEGA_PCINT_MAP_PCINT1 { GPIO_PIN(1, 0), GPIO_PIN(1, 1), GPIO_PIN(1, 2), GPIO_PIN(1, 3), GPIO_PIN(1, 4), GPIO_PIN(1, 5), GPIO_PIN(1, 6), GPIO_PIN(1, 7) }
+#define ATMEGA_PCINT_MAP_PCINT2 { GPIO_PIN(2, 0), GPIO_PIN(2, 1), GPIO_PIN(2, 2), GPIO_PIN(2, 3), GPIO_PIN(2, 4), GPIO_PIN(2, 5), GPIO_PIN(2, 6), GPIO_PIN(2, 7) }
+#define ATMEGA_PCINT_MAP_PCINT3 { GPIO_PIN(3, 0), GPIO_PIN(3, 1), GPIO_PIN(3, 2), GPIO_PIN(3, 3), GPIO_PIN(3, 4), GPIO_PIN(3, 5), GPIO_PIN(3, 6), GPIO_PIN(3, 7) }
+#endif

OK, for both ATmega1284p and ATmega328p (I just checked the data sheet) the pin change interrupt banks match perfectly with an GPIO port. (Note: PCINT0 on ATmega328p matches `GPIO_PIN(PORT_B, 0)` to `GPIO_PIN(PORT_B, 7)`, not `GPIO_PIN(PORT_A, 0)` to `GPIO_PIN(PORT_A, 7)`.) I'm not sure if it is worth the effort, but if you want to only use the pin map for the ATmega2560 and have the other AVRs handled more efficiently, feel free to do so.

> @@ -0,0 +1,7 @@
+#ifndef ATMEGA_PCINT_H
+#define ATMEGA_PCINT_H
+#define ATMEGA_PCINT_MAP_PCINT0 { GPIO_PIN(0, 0), GPIO_PIN(0, 1), GPIO_PIN(0, 2), GPIO_PIN(0, 3), GPIO_PIN(0, 4), GPIO_PIN(0, 5), GPIO_PIN(0, 6), GPIO_PIN(0, 7) }

Please use `GPIO_PIN(PORT_A, 0)` instead of `GPIO_PIN(0, 0)`. This will make reading it easier, as the code better matches the names used in the datasheet. (E.g. the pins are name PB0, PA7, ...)

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


More information about the notifications mailing list