Preprocessor for ULP/RTC macros #43

wnienhaus · 2021-07-31T16:42:05Z

This PR implements issue #19 and in a way also issue #12, because now many examples from the net can be assembled unmodified.

This is a big PR. I therefore split it up into many small commits, each standing on its own and intended to be short and simple to review. It will be best to review this PR one commit at a time. Each commit describes in more detail what the change is.

Each commit has at least unit tests (that also pass) covering the change. All changes also have compat tests (even though they sometimes have their own commit), to ensure the resulting binary always matches what binutils-esp32ulp would have produced. There are also a new set of compat tests, that download sources from the net (including the examples from the tests of binutils-esp32ulp) and run them through both py-esp32-ulp and binutils-esp32ulp to ensure the output is the same).

In summary, the PR has 4 changes:

Fixes encountered during testing or to support the change.
Adding support for expression evaluation to the assembler (e.g. 1+1 or 1<<symbol)
Adding a preprocessor for preprocessing ESP-IDF macros/constants before the code goes to the assembler
Add a new suite of compat tests using example code from the net as input

I originally set myself the goal to be able to compile all examples from this repo (https://github.com/duff2013/ulptool/tree/master/src/ulp_examples) without modification to the sources. I will say I managed (including it working on the ESP32 itself), except for the cases that use assembler macros (see issue #40), which I intentionally did not implement. (I might try that as a future side project).

The back story to this all is that I used to do a lot of manual steps with my ULP assembly source code. I passed it through the gcc preprocessor to get the macros I used expanded, but those macros spit out long expressions with countless brackets. Manually evaluating those (especially when using my own constants in the expressions) was tedious. I also kept the original code (before preprocessing) in comments above the resulting statements, so I could more easily understand where the final reg_rd or reg_wr statements came from. While that helped, keeping the statements and comments in sync was also messy.

Ultimately, I wanted a better way. I never used binutils-esp32ulp to compile my ULP sources, because when I started with ULP coding, I didn't want to download and build this massive tool on my machine. I found py-esp32-ulp and it was just so simple and fast and did what I needed. So while using biunutils-esp32ulp is an alternative (especially, when creating a docker image of it), I wanted to continue with the lightweight and simple approach of py-esp32-ulp, directly in MicroPython. (Of course this also seemed like a fun side project, which is definitely was! In particular, getting it to work on the actual device with little memory!)

wnienhaus · 2021-07-31T16:48:40Z

Actually... it fixes issue #16 too (commit with subject: "add support for the .global directive. only symbols flagged as global will be exported")

ThomasWaldmann · 2021-07-31T19:52:49Z

Wow, this is big. :-)

Guess I'll need a bit for reviewing this. Could you please work on #44 while I do that?

Would be good to have master branch testing successfully "as is" and then rebase this PR onto new master.

wnienhaus · 2021-07-31T20:03:53Z

Will do. Started looking into making the Travis build pass - the failure is because of the old gcc on Ubuntu Trusty. But if you want to move to Github Actions, I will look into that!

.travis.yml

ThomasWaldmann · 2021-08-02T13:13:48Z

just merged another PR of yours into master, so refresh master and rebase again.

wnienhaus · 2021-08-02T15:22:27Z

So. It's rebased now. And all tests pass :) .

tests/assemble.py

ThomasWaldmann

started reviewing:

add units test for the .set directive 5a9b7e9
add support for left aligned assembler directives (e.g. .set) … 8d7981d
fix a crash bug where BSS size calculation was attemped on the value … … 45b3769
raise error when attempting to store values in .bss section … f65cbc2
fix reference to non-exiting variable f0d86db
fixed typo in comment of instruction definition 15633eb
add support for the .global directive. only symbols flagged as global… … 3a2d312
let SymbolTable.export() optionally export non-global symbols too … 2417778

i only found a few nitpicks. overall, great stuff so far. i like how you do clean commits!

tests/assemble.py

esp32_ulp/assemble.py

tests/assemble.py

wnienhaus · 2021-08-03T17:48:56Z

For now, I have pushed the fixed as additional commits (because I dont know what force pushing will do to this PR's history). But I want to rebase/squash them later, so they "original" commits are clean, without those commits.

Much open-source code out there has .global, .set, etc directives starting in the first column of a line. This change allows assembling such code. Incidentally this also fixes a bug, where directives without parameters, such as .text, .data, etc were silently accepted when left-aligned but in those cases treated as labels instead of section headers.

ThomasWaldmann

Reviewed:
support ULP opcodes in upper case … 33b4d3d
add a compatibilty test for the recent fixes and improvements … 9c94247
add support for evaluating expressions … 4962139
add a compatibilty test for evaluating expressions … be27cdc
docs: add that expressions are suppported 521e924

guess that eval() is dangerous.

tests/assemble.py

esp32_ulp/opcodes.py

ThomasWaldmann · 2021-08-05T21:42:24Z

esp32_ulp/opcodes.py

+    try:
+        entry = symbols.get_sym(arg)
+        return ARG(SYM, entry, arg)
+    except KeyError:
+        pass
+    return ARG(IMM, int(eval_arg(arg)), arg)


try: entry = symbols.get_sym(arg) except KeyError: return ARG(IMM, int(eval_arg(arg)), arg) else: return ARG(SYM, entry, arg)

seen my previous comment?

esp32_ulp/util.py

tests/util.py

ThomasWaldmann

reviewed:
add preprocessor that can replace simple #define values in code … d5c274f
fix evaluation of expressions during first assembler pass … 5480e1d
remove no-longer-needed pass dependent code from SymbolTable … 5811060

esp32_ulp/__main__.py

esp32_ulp/preprocess.py

esp32_ulp/assemble.py

wnienhaus · 2021-08-07T11:02:03Z

For now I am done fixing what I could. Happy to get more feedback.

esp32_ulp/preprocess.py

tests/02_compat_rtc_tests.sh

esp32_ulp/opcodes.py

esp32_ulp/preprocess.py

esp32_ulp/util.py

tests/preprocess.py

ThomasWaldmann · 2021-08-07T13:55:24Z

BTW, i first thought "OMG, this DefinesDB is a bit too much". But considering the memory issues lurking and that the db has to be built only once, guess this is the only non-cumbersome method we can use.

The only (cumbersome) alternative would be to extract "used symbols only" from the large header files.

This defines what the preprocessor aims to do, why and what its intentional limitations are. Examples on how to use it and how to use the "Defines DB" are also provided

…utput in all cases This fix makes compat tests pass for: https://github.com/duff2013/ulptool/blob/master/src/ulp_examples/ulp_rtc_gpio/rtcio.s

This change allows specifying the address in 32-bit words (i.e. the address as seen from the ULP), in addition to the existing mode of specifying a register's full address on the DPORT bus. If an address is between 0 and DR_REG_MAX_DIRECT (0x3ff), treat it as a word offset (ULP address), otherwise treat it as a full address on the DPORT bus as before.

.long and .int are the same as per GNU assembler manual: https://sourceware.org/binutils/docs/as/Long.html binutils-esp32ulp also treats them the same (compat test included to verify this)

wnienhaus · 2021-08-08T15:34:07Z

Ok, I have cleaned up everything, and rebased/squashed it all into as clean commits as I could.

I updated the preprocessor docs too, since there's now the convenience function for preprocessing which defaults to using the DefinesDB.

In one of the unit test files, I built an assert_raises method, which makes the test read nicer. I did not copy the approach to other unit test files - this could come in a refactoring commit at some later stage.

I also built a context manager for opening and closing the DefinesDB when preprocessing or parsing include files into the defines db. That definitely makes the code look cleaner. Thank you for that suggestion.

You can now take a 2nd pass over the commits - they should be pretty close to what they were like before, but with the improvements embedded into them.

ThomasWaldmann · 2021-08-08T18:37:59Z

BTW, please add yourself either to the LICENSE file or add a new file AUTHORS and reference that from LICENSE (see borgbackup/borg for an example).

ThomasWaldmann

ok, did a 2nd pass and found some minor stuff.

please only fix these without extending scope, do additional commits for now.

esp32_ulp/assemble.py

ThomasWaldmann · 2021-08-08T18:52:30Z

esp32_ulp/opcodes.py

+    try:
+        entry = symbols.get_sym(arg)
+        return ARG(SYM, entry, arg)
+    except KeyError:
+        pass
+    return ARG(IMM, int(eval_arg(arg)), arg)


seen my previous comment?

esp32_ulp/util.py

ThomasWaldmann · 2021-08-08T19:00:19Z

tests/compat/expr.S

+  #test that expressions evaluate correctly for all supported operators
+  move r3, 1+2
+  move r3, 3-5
+  move r3, -5
+  move r3, 2*3
+  move r3, 4/2
+  move r3, 4 % 3
+  move r3, 0xff << 2
+  move r3, 0xff >> 1
+  move r3, (0xabcdef | 0xff) & 0xff
+  move r3, 0x1234 & ~2
+  move r3, 42|4&0xf  # 46 (4&0xf is evaluated first)
+  move r3, (42|4)&0xf  # 14 (42|4 is evaluated first)


this is a bit strange mix of a real-world code example and some lines of assembler testing.

maybe separate these lines at least?

Did you mean into a separate file, or maybe I can move them below the exit: halt line and add a comment that they are all "artificial" examples?

whatever you like better.

esp32_ulp/preprocess.py

tests/fixtures/incl.h

tests/fixtures/incl2.h

ThomasWaldmann · 2021-08-08T19:28:10Z

esp32_ulp/preprocess.py

+        class ctx:
+            def __init__(self, db):
+                self._db = db
+
+            def __enter__(self):
+                # not opening DefinesDB - it opens itself when needed
+                return self._db
+
+            def __exit__(self, type, value, traceback):
+                if isinstance(self._db, DefinesDB):
+                    self._db.close()
+
+        if self._defines_db:
+            return ctx(self._defines_db)
+
+        return ctx(self._defines)


did you try having the contextmanager inside DefinesDB class?

if it works, also call the context manager from the tests.

I have tried to put the context manager into the DefinesDB class, but because it handles both the "with DefinesDB" and the "without a DefinesDB" case, this doesn't fit into the DefinesDB class.

I could find an elegant way to add a condition to the "with" statement - ie. only use the context manager from DefinesDB when we're actually using the DefinesDB, otherwise work with the class-internal dict.

I'll mull over this a bit more and see if I can find a way.

Regarding testing: in effect this was tested - because the existing tests exercise the code which uses the context manager, but I added another test now to check specifically that the db is actually closed too.

wnienhaus

Made all the fixes now (except the Context Manger move to the DefinesDB class, which I need to think over more). I'll push this as a commit on top now, and then check the Context Manager topic.

esp32_ulp/assemble.py

esp32_ulp/preprocess.py

tests/fixtures/incl.h

tests/fixtures/incl2.h

wnienhaus · 2021-08-09T16:41:21Z

esp32_ulp/preprocess.py

+        class ctx:
+            def __init__(self, db):
+                self._db = db
+
+            def __enter__(self):
+                # not opening DefinesDB - it opens itself when needed
+                return self._db
+
+            def __exit__(self, type, value, traceback):
+                if isinstance(self._db, DefinesDB):
+                    self._db.close()
+
+        if self._defines_db:
+            return ctx(self._defines_db)
+
+        return ctx(self._defines)


I have tried to put the context manager into the DefinesDB class, but because it handles both the "with DefinesDB" and the "without a DefinesDB" case, this doesn't fit into the DefinesDB class.

I could find an elegant way to add a condition to the "with" statement - ie. only use the context manager from DefinesDB when we're actually using the DefinesDB, otherwise work with the class-internal dict.

I'll mull over this a bit more and see if I can find a way.

Regarding testing: in effect this was tested - because the existing tests exercise the code which uses the context manager, but I added another test now to check specifically that the db is actually closed too.

wnienhaus · 2021-08-09T16:56:06Z

Pushed latest improvements as a commit on top: 5fd45a2

ThomasWaldmann

LGTM - merge and do the remaining stuff in later PRs?

wnienhaus · 2021-08-09T17:23:27Z

Ok. I force pushed one small thing - moving the "made up" statement in tests/compat/expr.S to the bottom of the file with an improved comment.

And I updated the LICENCE file and added an AUTHORS file. Perhaps check if you are happy with that, and then we're good for a merge.

(For now I don't manage to find a solution to the Context Manager challenge, but that can be improved later)

AUTHORS

ThomasWaldmann · 2021-08-09T17:29:27Z

LICENSE

-Copyright (c) 2018 Thomas Waldmann
+Copyright (c) 2018 Thomas Waldmann (see AUTHORS file)
+Copyright (c) 2021 Wilko Nienhaus (see AUTHORS file)


Maybe it is simpler to update authors only at one place, so I suggest this:

Copyright 2018-2021 by the py-esp32-ulp authors, see AUTHORS file.

Also, I would not go into details about who has written what, that's where git log is better anyway and a file describing this will likely be always outdated.

Done. 47d5e8a

ThomasWaldmann · 2021-08-09T20:07:07Z

Yay! \o/

wnienhaus · 2021-08-10T06:05:41Z

Yay! And thank you for all your effort with this big PR!

wnienhaus force-pushed the preprocessor branch from 4f891db to e2e2195 Compare August 1, 2021 12:58

ThomasWaldmann reviewed Aug 1, 2021

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

wnienhaus force-pushed the preprocessor branch from e2e2195 to afd1690 Compare August 2, 2021 15:17

micropython deleted a comment from wnienhaus Aug 3, 2021

ThomasWaldmann reviewed Aug 3, 2021

View reviewed changes

tests/assemble.py Outdated Show resolved Hide resolved

ThomasWaldmann reviewed Aug 3, 2021

View reviewed changes

tests/assemble.py Show resolved Hide resolved

esp32_ulp/assemble.py Show resolved Hide resolved

esp32_ulp/assemble.py Outdated Show resolved Hide resolved

tests/assemble.py Outdated Show resolved Hide resolved

tests/assemble.py Outdated Show resolved Hide resolved

wnienhaus added 2 commits August 5, 2021 07:49

add units test for the .set directive

79db90f

wnienhaus force-pushed the preprocessor branch 2 times, most recently from f78130e to 5f76237 Compare August 5, 2021 05:28

ThomasWaldmann reviewed Aug 5, 2021

View reviewed changes

wnienhaus force-pushed the preprocessor branch from dd3a04b to 50f9ed7 Compare August 6, 2021 14:58

ThomasWaldmann reviewed Aug 6, 2021

View reviewed changes

esp32_ulp/util.py Outdated Show resolved Hide resolved

ThomasWaldmann reviewed Aug 6, 2021

View reviewed changes

tests/util.py Show resolved Hide resolved

ThomasWaldmann reviewed Aug 6, 2021

View reviewed changes

wnienhaus force-pushed the preprocessor branch 3 times, most recently from 806bcf2 to 1606b5d Compare August 7, 2021 10:54

ThomasWaldmann reviewed Aug 7, 2021

View reviewed changes

esp32_ulp/preprocess.py Outdated Show resolved Hide resolved

esp32_ulp/preprocess.py Outdated Show resolved Hide resolved

esp32_ulp/preprocess.py Show resolved Hide resolved

esp32_ulp/preprocess.py Outdated Show resolved Hide resolved

esp32_ulp/preprocess.py Outdated Show resolved Hide resolved

ThomasWaldmann reviewed Aug 7, 2021

View reviewed changes

tests/02_compat_rtc_tests.sh Outdated Show resolved Hide resolved

ThomasWaldmann reviewed Aug 7, 2021

View reviewed changes

esp32_ulp/opcodes.py Outdated Show resolved Hide resolved

ThomasWaldmann reviewed Aug 7, 2021

View reviewed changes

esp32_ulp/preprocess.py Outdated Show resolved Hide resolved

esp32_ulp/util.py Outdated Show resolved Hide resolved

tests/preprocess.py Show resolved Hide resolved

wnienhaus force-pushed the preprocessor branch 2 times, most recently from 0e932f8 to 4f70602 Compare August 8, 2021 15:25

wnienhaus added 4 commits August 8, 2021 18:28

add documentation for the preprocessor

4f90f76

This defines what the preprocessor aims to do, why and what its intentional limitations are. Examples on how to use it and how to use the "Defines DB" are also provided

fix use of treg field in i_move instruction to match binutils-esp32 o…

d44384f

…utput in all cases This fix makes compat tests pass for: https://github.com/duff2013/ulptool/blob/master/src/ulp_examples/ulp_rtc_gpio/rtcio.s

support .int data type

c3bd101

.long and .int are the same as per GNU assembler manual: https://sourceware.org/binutils/docs/as/Long.html binutils-esp32ulp also treats them the same (compat test included to verify this)

wnienhaus force-pushed the preprocessor branch from 4f70602 to c3bd101 Compare August 8, 2021 15:28

ThomasWaldmann reviewed Aug 8, 2021

View reviewed changes

wnienhaus commented Aug 9, 2021

View reviewed changes

wnienhaus mentioned this pull request Aug 9, 2021

1.0.0 milestone #49

Closed

ThomasWaldmann approved these changes Aug 9, 2021

View reviewed changes

ThomasWaldmann added this to the 1.0 milestone Aug 9, 2021

refactor: small improvements based on PR comments.

2a0a39a

wnienhaus force-pushed the preprocessor branch from 5fd45a2 to 2a0a39a Compare August 9, 2021 17:20

wnienhaus force-pushed the preprocessor branch from d16cb43 to bd22de6 Compare August 9, 2021 17:24

ThomasWaldmann reviewed Aug 9, 2021

View reviewed changes

AUTHORS Outdated Show resolved Hide resolved

wnienhaus force-pushed the preprocessor branch from bd22de6 to 95f8ad9 Compare August 9, 2021 17:27

ThomasWaldmann reviewed Aug 9, 2021

View reviewed changes

Updated LICENSE file and added AUTHORS file

47d5e8a

wnienhaus force-pushed the preprocessor branch from 95f8ad9 to 47d5e8a Compare August 9, 2021 17:32

ThomasWaldmann merged commit 2844227 into micropython:master Aug 9, 2021

ThomasWaldmann mentioned this pull request Aug 9, 2021

C-preprocessor-like macros? #19

Closed

wnienhaus deleted the preprocessor branch August 14, 2021 06:38

wnienhaus mentioned this pull request Sep 26, 2021

assembler directive: .global name #16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preprocessor for ULP/RTC macros #43

Preprocessor for ULP/RTC macros #43

wnienhaus commented Jul 31, 2021

wnienhaus commented Jul 31, 2021

ThomasWaldmann commented Jul 31, 2021

wnienhaus commented Jul 31, 2021

ThomasWaldmann commented Aug 2, 2021

wnienhaus commented Aug 2, 2021

ThomasWaldmann left a comment

wnienhaus commented Aug 3, 2021

ThomasWaldmann left a comment

ThomasWaldmann Aug 5, 2021

ThomasWaldmann Aug 8, 2021

wnienhaus Aug 9, 2021

ThomasWaldmann left a comment

wnienhaus commented Aug 7, 2021

ThomasWaldmann commented Aug 7, 2021

wnienhaus commented Aug 8, 2021

ThomasWaldmann commented Aug 8, 2021

ThomasWaldmann left a comment

ThomasWaldmann Aug 8, 2021

ThomasWaldmann Aug 8, 2021

wnienhaus Aug 9, 2021

ThomasWaldmann Aug 9, 2021

ThomasWaldmann Aug 8, 2021

ThomasWaldmann Aug 8, 2021

wnienhaus Aug 9, 2021

wnienhaus left a comment

wnienhaus Aug 9, 2021

wnienhaus commented Aug 9, 2021

ThomasWaldmann left a comment

wnienhaus commented Aug 9, 2021

ThomasWaldmann Aug 9, 2021

wnienhaus Aug 9, 2021

ThomasWaldmann commented Aug 9, 2021

wnienhaus commented Aug 10, 2021

Preprocessor for ULP/RTC macros #43

Preprocessor for ULP/RTC macros #43

Conversation

wnienhaus commented Jul 31, 2021

wnienhaus commented Jul 31, 2021

ThomasWaldmann commented Jul 31, 2021

wnienhaus commented Jul 31, 2021

ThomasWaldmann commented Aug 2, 2021

wnienhaus commented Aug 2, 2021

ThomasWaldmann left a comment

Choose a reason for hiding this comment

wnienhaus commented Aug 3, 2021

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasWaldmann left a comment

Choose a reason for hiding this comment

wnienhaus commented Aug 7, 2021

ThomasWaldmann commented Aug 7, 2021

wnienhaus commented Aug 8, 2021

ThomasWaldmann commented Aug 8, 2021

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnienhaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnienhaus commented Aug 9, 2021

ThomasWaldmann left a comment

Choose a reason for hiding this comment

wnienhaus commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasWaldmann commented Aug 9, 2021

wnienhaus commented Aug 10, 2021