[riot-notifications] [RIOT-OS/RIOT] tools: added tool for auto-generating ISR vectors (#7553)

Alexandre Abadie notifications at github.com
Fri Sep 1 15:03:59 CEST 2017


aabadie requested changes on this pull request.

Looks quite good. This tool is definitely useful.
I pointed to mainly small python style issues.
I'll test it when time permits.

> +import argparse
+import sys
+import os
+import glob
+import re
+import copy
+from operator import itemgetter
+
+cpulist = set()
+headermap = dict()
+
+alias = {
+    "isr_exti[_\d+]+": "isr_exti"
+}
+
+def parse_cpuconf( cpuconf ):

PEP8, no space around parameter, here and below

> @@ -0,0 +1,200 @@
+#!/usr/bin/env python3

why making this only python 3 ? 

> +
+alias = {
+    "isr_exti[_\d+]+": "isr_exti"
+}
+
+def parse_cpuconf( cpuconf ):
+    buf = open(cpuconf, 'r').read()
+    path = os.path.dirname(os.path.abspath(cpuconf))
+
+    pat_line = re.compile("(#if|#elif|#ifdef)(( \|\|| \|\| \\\\\n| \\\\\n +\|\|)?"
+                          " +(defined\()?CPU_MODEL_[0-9A-Z]+\)?)*"
+                          "\n#include \"(.+\.h)\"", re.MULTILINE)
+    pat_cpu = re.compile("CPU_MODEL_([0-9A-Z]+)")
+
+    for m in pat_line.finditer(buf):
+        header = ("%s/%s" % (path, m.group(5)))

parenthesis are useless. Prefer using `format`

> +            cpulist.add(cpu)
+            headermap[header].add(cpu)
+
+
+def parse_header( file, cpus ):
+    filename = os.path.basename(file)
+    res = { "cpus": cpus, "vec": {} };
+    vec = res["vec"]
+
+    # read through file and find all lines
+    with open(file, 'r', encoding = "ISO-8859-1") as f:
+         for line in f:
+            m = re.match(" +([_0-9A-Za-z]+)_IRQn += (-?\d+),? +(/\*!<|/\*\*<) (.+[a-zA-Z0-9]) +\*/", line)
+            if m:
+                num = int(m.group(2))
+                name = ("isr_%s" % (m.group(1).lower()))

parenthesis, here and the line below

> +#
+# Author:   Hauke Petersen <hauke.petersen at fu-berlin.de>
+
+import argparse
+import sys
+import os
+import glob
+import re
+import copy
+from operator import itemgetter
+
+cpulist = set()
+headermap = dict()
+
+alias = {
+    "isr_exti[_\d+]+": "isr_exti"

This can be on one line

> +
+    for m in pat_line.finditer(buf):
+        header = ("%s/%s" % (path, m.group(5)))
+
+        if header not in headermap:
+            headermap[header] = set()
+
+        for c in pat_cpu.finditer(m.group(0)):
+            cpu = c.group(1).lower()
+            cpulist.add(cpu)
+            headermap[header].add(cpu)
+
+
+def parse_header( file, cpus ):
+    filename = os.path.basename(file)
+    res = { "cpus": cpus, "vec": {} };

no space after `{` and before `}`, here and in other places

> +                nm = re.match(".+reserved.+", name, re.IGNORECASE)
+                if num < 0 or nm:
+                    continue
+
+                if num in vec:
+                    sys.exit("Error: vector defined twice")
+                vec[num] = { "name": name, "comment": comment }
+
+    return res;
+
+
+def apply_alias( common, specific ):
+    for num in common:
+        for a in alias:
+            m = re.match(a, common[num]["name"])
+            if m:

are you sure `m` is mandatory ?

```python
if re.match(a, common[num]["name"]):
    common[num]["name"] = alias[a]
```

> +
+    return res;
+
+
+def apply_alias( common, specific ):
+    for num in common:
+        for a in alias:
+            m = re.match(a, common[num]["name"])
+            if m:
+                common[num]["name"] = alias[a]
+
+    for cl in specific:
+        for num in cl["vec"]:
+            for a in alias:
+                m = re.match(a, cl["vec"][num]["name"])
+                if m:

same here

> +
+
+def depstr( cpus, prefix ):
+    res = prefix
+    i = 0
+
+    # if the list of CPUs contains all the CPUs known, no if-def'ing needed
+    if cpulist == cpus:
+        return ""
+
+    for cpu in sorted(list(cpus)):
+        if (i % 0x2) == 0 and i != 0:
+            res += " || \\\n   "
+        elif i & 0x1:
+            res += " ||"
+        res += (" defined(%s)" % (cpu))

parenthesis

> +
+        for c in pat_cpu.finditer(m.group(0)):
+            cpu = c.group(1).lower()
+            cpulist.add(cpu)
+            headermap[header].add(cpu)
+
+
+def parse_header( file, cpus ):
+    filename = os.path.basename(file)
+    res = { "cpus": cpus, "vec": {} };
+    vec = res["vec"]
+
+    # read through file and find all lines
+    with open(file, 'r', encoding = "ISO-8859-1") as f:
+         for line in f:
+            m = re.match(" +([_0-9A-Za-z]+)_IRQn += (-?\d+),? +(/\*!<|/\*\*<) (.+[a-zA-Z0-9]) +\*/", line)

This line is too long (PEP8), it should be 80 characters long max.

-- 
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/7553#pullrequestreview-60125646
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20170901/1fe9d8d5/attachment-0001.html>


More information about the notifications mailing list