tools: added tool for auto-generating ISR vectors by haukepetersen · Pull Request #7553 · RIOT-OS/RIOT

haukepetersen · 2017-09-01T09:54:05Z

Creating the ISR vector tables is error prone and cumbersome, so why don't we just do it automatically?!

This PR proposes a tool that parses CMSIS-style header files and outputs a compact version of vector table(s), for a single CPU or a group of CPU models. I parses the list of CPU(s) from the cpu_conf.h files, using the `#include' statements in those files to find the header files.

The output table style depends on #7535.

I tested the tools for all the STM and the Kinetis CPUs, and the results are valid as far as I can see and test it. But as I am a poor python noob, the code of the tool might be far from perfect... But instead of having a thousand change requests, I would prefer to merge it in the current state and improve it with separate PRs: this way we can already use the tool for cleaning up existing vector definitions while tuning the tool itself...

aabadie

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

aabadie · 2017-09-01T12:50:02Z

dist/tools/isrvecgen/isrvecgen.py

+    "isr_exti[_\d+]+": "isr_exti"
+}
+
+def parse_cpuconf( cpuconf ):


PEP8, no space around parameter, here and below

good to know, will fix.

aabadie · 2017-09-01T12:50:52Z

dist/tools/isrvecgen/isrvecgen.py

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


why making this only python 3 ?

no idea, thought I'll try to be 'modern' :-)

Are there still major distros or OSs that do not supply python 3 (it has not to be the default)? Even the grand old ladies Debian and CentOS have python 3 now.

I don't think mac OS users have python3 installed by default

As this is not needed for building, having python3 as dependency should be fine.

aabadie · 2017-09-01T12:51:27Z

dist/tools/isrvecgen/isrvecgen.py

+    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

aabadie · 2017-09-01T12:51:44Z

dist/tools/isrvecgen/isrvecgen.py

+            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

aabadie · 2017-09-01T12:53:23Z

dist/tools/isrvecgen/isrvecgen.py

+headermap = dict()
+
+alias = {
+    "isr_exti[_\d+]+": "isr_exti"


This can be on one line

could, but as this list is meant to be extended, I figured to start with a good style right away. I am thinking about even putting the filters in some external config file in the future...

aabadie · 2017-09-01T12:53:43Z

dist/tools/isrvecgen/isrvecgen.py

+
+def parse_header( file, cpus ):
+    filename = os.path.basename(file)
+    res = { "cpus": cpus, "vec": {} };


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

aabadie · 2017-09-01T12:54:56Z

dist/tools/isrvecgen/isrvecgen.py

+    for num in common:
+        for a in alias:
+            m = re.match(a, common[num]["name"])
+            if m:


are you sure m is mandatory ?

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

nope, will fix

aabadie · 2017-09-01T12:55:05Z

dist/tools/isrvecgen/isrvecgen.py

+        for num in cl["vec"]:
+            for a in alias:
+                m = re.match(a, cl["vec"][num]["name"])
+                if m:


aabadie · 2017-09-01T12:55:35Z

dist/tools/isrvecgen/isrvecgen.py

+            res += " || \\\n   "
+        elif i & 0x1:
+            res += " ||"
+        res += (" defined(%s)" % (cpu))


parenthesis

aabadie · 2017-09-01T13:02:21Z

dist/tools/isrvecgen/isrvecgen.py

+    # 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.

jnohlgard · 2017-09-01T13:11:43Z

I started toying with the idea of unifying the Kinetis K CPUs (k60, k64f, K22f) as a single CPU directory kinetis_k. I have some wip files which I haven't published anywhere yet, and I don't have time to work on it right now, but I can push it later today in case you find a use for it in this PR. The plan for kinetis_k includes generating the isr vectors from the header, and parsing the model name to find the correct ldscript and some other info.

haukepetersen · 2017-09-01T13:55:06Z

nice, sharing your starters won't hurt. Would be interesting to see what else we could generate to make porting even more simple

jnohlgard · 2017-09-01T14:00:33Z

Some interesting work in the same area: https://github.com/posborne/cmsis-svd

jnohlgard · 2017-09-06T11:52:40Z

@haukepetersen this is my WIP/experiment branch: https://github.com/gebart/RIOT/commits/wip/kinetis-k not much there so far, other than a sed version of the ISR vector generator and some makefile magic for splitting the model number into separate properties for family, subfamily, memory size, etc.

haukepetersen · 2017-10-06T10:09:41Z

fixed python style issues.

aabadie

Maybe adding a README ?

haukepetersen · 2017-10-06T11:47:35Z

Sure. Also found some other minor things, will push later today

aabadie

Re-read the script and found other things:

one typo
missing docstring
some refactoring is needed

A general comment: try to use variables with explicit names. It helps understand the code by improving readability.

aabadie · 2017-10-09T07:25:50Z

dist/tools/isrvecgen/README.md

@@ -0,0 +1,18 @@
+Overivew


s/Overivew/Overview/

aabadie · 2017-10-09T07:27:07Z

dist/tools/isrvecgen/isrvecgen.py

+from operator import itemgetter
+
+cpulist = set()
+headermap = dict()


The scope of variable can be reduced: the function parse_cpuconf can return this dict.

aabadie · 2017-10-09T07:29:55Z

dist/tools/isrvecgen/isrvecgen.py

+import copy
+from operator import itemgetter
+
+cpulist = set()


Same comment for this global variable. By the way, the same variable name is used internally in other functions, this looks weird:

for cpulist in specific: ...

for example

aabadie · 2017-10-09T07:30:59Z

dist/tools/isrvecgen/isrvecgen.py

+
+alias = {"isr_exti[_\d+]+": "isr_exti"}
+
+def parse_cpuconf(cpuconf):


Adding some docstrings won't hurt

aabadie · 2017-10-09T07:31:12Z

dist/tools/isrvecgen/isrvecgen.py

+
+
+def parse_header(file, cpus):
+    filename = os.path.basename(file)


docstring missing

aabadie · 2017-10-09T07:32:31Z

dist/tools/isrvecgen/isrvecgen.py

+                    cl["vec"][num]["name"] = alias[a]
+
+
+def depstr(cpus, prefix):


docstring missing + try to be explicit with function names. We are not writing C code

aabadie · 2017-10-09T07:32:42Z

dist/tools/isrvecgen/isrvecgen.py

+    return res + "\n"
+
+
+def pull_common_vectors(specific):


aabadie · 2017-10-09T07:34:19Z

dist/tools/isrvecgen/isrvecgen.py

+        specific.append(parse_header(h, headermap[h]))
+
+    # list items by CPU name
+    specific = sorted(specific, key=lambda k: sorted(list(k["cpus"]))[0])


This is personal opinion but in general I tend to avoid this kind of oneliner, for code readability reasons.

I think this is perfectly readable and one very good example why lamdas are so useful :-)

aabadie · 2017-10-09T07:34:47Z

dist/tools/isrvecgen/isrvecgen.py

+
+if __name__ == "__main__":
+    # Define some command line args
+    p = argparse.ArgumentParser()


s/p/parser/

aabadie · 2017-10-09T07:37:22Z

dist/tools/isrvecgen/isrvecgen.py

+    return table
+
+
+if __name__ == "__main__":


There is a lot of code in this block, adding a main function is required:

def main(args): ... # call it with: if __name__ == "__main__": parser = argparse.ArgumentParser() parser.add_argument("cpu_conf", help="path to a cpu_conf.h file") main(parser.parse_args())

haukepetersen · 2017-10-09T08:34:06Z

Hm, this is getting too cumbersome for me, don't see myself putting so much effort into a small helper tool... Will put this tool in its own repo. I welcome anyone to beautify it and move it to the RIOT repo, but I don't see myself spending so much time on this...

haukepetersen · 2017-10-09T08:34:36Z

The current version of the tool resides now here:
https://github.com/haukepetersen/riotsandbox/tree/master/tools/isrvecgen

So as said, feel free to copy/adapt/move to RIOT...

haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools labels Sep 1, 2017

haukepetersen added this to the Release 2017.10 milestone Sep 1, 2017

haukepetersen assigned jnohlgard, aabadie and kaspar030 Sep 1, 2017

haukepetersen mentioned this pull request Sep 1, 2017

cpu: use shared Cortex-M base vector table #7535

Merged

aabadie requested changes Sep 1, 2017

View reviewed changes

This was referenced Sep 6, 2017

cpu/stm32f7: optimized CPU specific vector table haukepetersen/RIOT#43

Closed

cpu/stm32f3: optimized CPU specific vector table haukepetersen/RIOT#42

Closed

cpu/stm32f2: optimized CPU specific vector table haukepetersen/RIOT#41

Closed

haukepetersen added 2 commits October 6, 2017 11:30

tools: added tool for auto-generating ISR vectors

f0c61f2

fixup! using format() and fixed some style issues

87dd773

haukepetersen force-pushed the add_tool_isrvecgen branch from 9fbcf01 to 87dd773 Compare October 6, 2017 10:09

aabadie requested changes Oct 6, 2017

View reviewed changes

haukepetersen added 2 commits October 6, 2017 16:42

fixup! fixed some issues and added output of weak defines

634d63d

fixup! added README.md

2b7d3cc

aabadie requested changes Oct 9, 2017

View reviewed changes

haukepetersen closed this Oct 9, 2017


		alias = {"isr_exti[_\d+]+": "isr_exti"}

		def parse_cpuconf(cpuconf):



		def parse_header(file, cpus):
		filename = os.path.basename(file)

Conversation

haukepetersen commented Sep 1, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Sep 1, 2017

Uh oh!

haukepetersen commented Sep 1, 2017

Uh oh!

jnohlgard commented Sep 1, 2017

Uh oh!

jnohlgard commented Sep 6, 2017

Uh oh!

haukepetersen commented Oct 6, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Oct 6, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!