micropython / micropython-esp32-ulp

ESP32 ULP Co-Processor toolchain implemented in MicroPython
MIT License
116 stars 24 forks source link

1.0.0 milestone #49

Closed ThomasWaldmann closed 2 years ago

ThomasWaldmann commented 3 years ago

considering the great work of @wnienhaus, this is much closer to be practically useful than previously.

so i create a milestone for first release - maybe only add very important features and bug fixes into there, so we can do a first release soon.

wnienhaus commented 3 years ago

Thank you. Thanks for the great review!

As a next step (after #43 is merged) I wanted to solve #41 - because that is a bad bug. The ULP only implements some conditions for the JUMPS instructions and the other "supported" conditions are implemented by the binutils-esp32ulp assembler, by emitting 2 instructions that emulate the desired condition, using those conditions actually supported by the ULP. I would suggest this is fixed before 1.0.

Other than that I had intended to go through the currently skipped tests in the 02_compat_rtc_tests.sh script to see if I can get them all to pass.

ThomasWaldmann commented 3 years ago

OK, currently all tickets in 1.0 milestone are closed, but I'll add this one for the skipped tests mentioned above.

wnienhaus commented 3 years ago

Great. I will take on the skipped tests. Let's see how deep the rabbit hole will be.

wnienhaus commented 3 years ago

oi ... did it close due to the merge? I did not intend to have this closed yet. There are still a number of things to fix. 3 more issues fixed already - will send PR tomorrow likely. And potentially still a few more things. 2 test scripts left to get to pass (out of the 5 we skipped so far)

wnienhaus commented 3 years ago

Actually looking over the open issue list, I was wondering if a few small things could still be nice before version 1

Maybe these 2?

  1. 48 (I tested on 1.11 and 1.16 so I think the min version is 1.11 and versions thereafter will work)

  2. The README says "There might be some stuff missing, some bugs and other symptoms of alpha software". With v1.0, maybe we can drop the reference to "alpha" and maybe we can clarify what is missing? (I only know of: assembler macro support, #including other .S files)

I was also wondering about:

  1. 39

  2. 12

For the first - any idea what the module could be called that takes over what esp32_ulp.__main__ does, and which esp32_ulp.__main__ then should call? It's not doing "assembly" and not "linking", but something that wraps those operations into a working application. esp32_ulp.app? esp32_ulp.build? Or maybe the functions should move into esp32_ulp.__init__, so that it can be imported as just esp32_ulp and other code (and also esp32_ulp.__main__) could then call a method like do_assemble or build or run with a filename?

For the second, technically some of the ulp code we're assembling with the compat test scripts are actually working examples. Should we simply document this and call that enough, or maybe just create our own simple blink example using the RTC_* methods now supported (I think I could appropriate some code I have actually running somewhere)?

But maybe these 2 last ones are just about "perfection" and they're not really that important for the v1.0 milestone. Happy to hear thoughts.

ThomasWaldmann commented 3 years ago
  1. I didn't run the code on an esp32 since quite a while. But I know that doing this often has revealed additional todo, because some problems only showed on the esp32. Did you run the latest code from master on esp32?

Yeah, #48 should be done before 1.0.

  1. Sure. Due to your recent work, guess we also can declare beta now.

  2. See #39.

  3. Yeah, maybe move your suggestion to #12, sounds good.

wnienhaus commented 3 years ago

Thanks for that. I have now:

  1. Re-tested everything (unit tests and compat tests) on the ESP32 directly, with MicroPython versions v1.10, v1.11, v1.12 and v1.17
  2. Updated the README regarding beta quality level
  3. Made imports less ugly as per your suggestion in #39
  4. Added a blink example to blink an LED with the ULP (to cover #12)

Should I create 1 PR per improvement above (in that case, could I ask you to merge the currently open PR #53 , so I can create PRs against that latest code), or should I simply push those "last minute changes" to the same open PR?

ThomasWaldmann commented 3 years ago

I've merged it. You can put stuff into a single PR, but try to keep the commits clean (as you usually do).

ThomasWaldmann commented 3 years ago

OK, looks like we reached 1.0! :) (reopening for remainder of todo)

What's left to do?

wnienhaus commented 3 years ago

Cool!

Tagging:

pip-installable:

Advertising:

ThomasWaldmann commented 3 years ago

https://github.com/fizista/micropython-umqtt.simple2 this is a recent project on pypi that supports upip. I am familiar with normal pypi packages, but not with micropython pypi packages.

I can add the tag as soon as everything is ready.

mastodon: similar to twitter, but non-commercial, federated, FOSS. less traffic than twitter though.

HN: good idea.

wnienhaus commented 3 years ago

So. Publishing to pypi was rather easy. I needed to put a special "sdist_upip.py" into the project and use that instead of normal sdist, which takes care of gzipping with a smaller dictionary size (that upip needs). The rest was just a normal setup.py and running python3 setup.py sdist.

I then uploaded the resulting package to test.pypi.org using twine and now there's a package: https://test.pypi.org/project/micropython-py-esp32-ulp/

Testing on the ESP32 it worked and I got this:

>>> # connect wifi
>>> import network
>>> w=network.WLAN(network.STA_IF)
>>> w.active(True)
True
>>> w.connect('XXXX', 'YYYY')
>>> 
>>> import upip
>>> upip.index_urls
['https://micropython.org/pi', 'https://pypi.org/pypi']
>>> 
>>> # point upip at test pypi
>>> upip.index_urls=['https://test.pypi.org/pypi']
>>> upip.install('micropython-py-esp32-ulp')
Installing to: /lib/
Warning: test.pypi.org SSL certificate is not validated
Installing micropython-py-esp32-ulp 1.0.0 from https://test-files.pythonhosted.org/packages/6b/bd/03de3dfdc57ca5d93ad0818be422e844a413ebb0609931d2d9b236e04f4e/micropython-py-esp32-ulp-1.0.0.tar.gz
>>> 
>>> # show that files were downloaded
>>> import os
>>> os.listdir("lib/esp32_ulp")
['__init__.py', '__main__.py', 'assemble.py', 'definesdb.py', 'link.py', 'nocomment.py', 'opcodes.py', 'parse_to_db.py', 'preprocess.py', 'soc.py', 'util.py']
>>> 
>>> # show that esp32_ulp loads and from the right place
>>> import esp32_ulp
>>> esp32_ulp.__file__
'/lib/esp32_ulp/__init__.py'

The only thing I did (learned) was that I had some test .py files accidentally included in the distribution package, which I then deleted from pypi to re-upload a "cleaned" distribution package. But I then learned that filenames can never be reused on pypi. So version 1.0.0 is now no longer usable as a filename (at least on test pypi). We will need some mechanism to allow republishing a version if ever needed. For now I simply appended another number to the version, so now there's 1.0.0.1 in the index. I tried the Debian way of using 1.0.0-1 but that didnt work (or at least setuptools changed that into 1.0.0-post1, which could work I guess). Perhaps you have some best-practice suggestion how best to version distribution packages?

About the code (setup.py, et al), I will clean up and make a PR tomorrow.

PS: about advertising it: I guess https://forum.micropython.org could be another place?

mattytrentini commented 3 years ago

I've been watching the two of you progress py-esp32-ulp for a little while now...thank you! These contributions are great.

I've not been in a position to help but I can at least help advertise a 1.0.0 when it's ready; I run the Melbourne MicroPython Meetup, and the MicroPython Slack Channel so can make sure it's announced at both of those. There's also the Adafruit Python on Hardware newsletter which has a wide distribution and the MicroPython forum and MicroPython Discord. It might even be possible to announce it at some of the Espressif sources (email newsletter and forums) but I have less contact with those.

The only suggestion I could make before releasing 1.0.0 would perhaps be to tidy up some of the documentation - the README is good but could potentially be friendlier to newcomers.

ThomasWaldmann commented 3 years ago

@wnienhaus Guess we should stay with x.y.z, that is the usual thing for python packages.

As you tested on the testing pypi, 1.0.0 would still work on the production pypi. If anything goes wrong, we can still go to 1.0.1 there.

@mattytrentini sounds good, if you can announce it there also after we published an official release, that would be great!

ThomasWaldmann commented 3 years ago

About newcomer friendliness: guess we could point to the counter.py example (or to the examples folder) from the README.

I guess it can be assumed that people who dive in as deep as using the ULP know how to use an esp32 (install python code on it, do python esp32 development), but do not know yet how to use our code.

counter.py is good as it is basically the simplest example code that actually does something visible.

wnienhaus commented 3 years ago

@mattytrentini Thank you for the great feedback and offer to help advertise this!

When you say "could potentially be friendlier", do you refer to the tone of the README, or the detail of it, or mostly that it lacks a clear "how to get started" section? The last part I will propose an improvement to, in line with what @ThomasWaldmann suggested above. But perhaps in your eyes there is more that we can do to make it friendlier?

wnienhaus commented 3 years ago

@ThomasWaldmann I'm fairly sure 1.0.0 would work on the production PyPi. So lets stick with the 3 part version number. Incrementing the last part of the version number, whenever we need to republish for some reason sounds "good enough for our purposes" (a phrase a friend of mine loved to use) 😄

Regarding a pointer to the examples from README, I will make a proposal via a PR.

I have also made a PR #56 for the setup.py part - making it upip installable. That is what I tested with test.pypi.org yesterday and subsequently on my ESP32. There is a question about including a file from another project, but I have more detail in the PR.

mattytrentini commented 3 years ago

When you say "could potentially be friendlier", do you refer to the tone of the README, or the detail of it, or mostly that it lacks a clear "how to get started" section?

Yes, mostly the latter part; how to get started. But also an overview. But now that #57 is opened I'll add more detail there. Thanks for trying to improve it!

ThomasWaldmann commented 2 years ago

OK, docs improvements have been merged, guess we can do a release soon.

wnienhaus commented 2 years ago

Yes, it looks like we can release soon. From what I see, the following are the next steps:

  1. Tag the repo with 1.0.0 (likely using the Github release functionality)
  2. Build and upload the py-esp32-ulp package to PyPi
  3. Announce

I wanted to document how to do step 2, so that you (@ThomasWaldmann) could do this with your credentials, as you are the owner of this repo/project.

However, while thinking about how to cut the process to as few steps as possible, I started considering to automate this process. This would then require only configuring the PyPI auth token as a repo secret once and thereafter simply doing Github Releases. I have already tested this in a test repo (https://github.com/wnienhaus/ujqtvms) and it works really well.

So I created an issue (#58) to automate the process and will spend a bit of time adapting the working solution from my test repo and make one more (hopefully last) PR for this.

ThomasWaldmann commented 2 years ago

I had a look at the automatically built/released package file and added a MANIFEST.in so we are able to exclude some files which are committed (so setuptools_scm adds them to the auto-generated manifest), but which we do not really want to add to the pypi pkg:

Currently also packaged (do we want to exclude that stuff from the pkg?):

On a PC/Mac dev machine guess it doesn't matter much, but how about when using upip directly on the esp32? Guess we only want to have the bare minimum in the package for that scenario?

wnienhaus commented 2 years ago

Hmm.. That was definitely not my intention. I believe I tested that when I first added setup.py and it only packaged files under esp32_ulp, which I understood came from the packages=["esp32_ulp"], line in setup.py. Because of that I decided to skip adding a MANIFEST.in because I got what I wanted from the automatic behaviour. So something changed and I wonder if switching to setuptools_scm is the difference (of course I should have checked what I got, but thank you for noticing).

I will add a MANIFEST.in and I think that we should only package the files under esp32_ulp, especially for the esp32. The rest, if needed can be gotten on a PC by cloning the repo.

ThomasWaldmann commented 2 years ago

setuptools_scm is auto-generating the manifest ("in memory", not in the file), git-committed files are "in", everything else is "out".

see the comment i added in MANIFEST.in.

wnienhaus commented 2 years ago

Thanks. That makes a lot of sense. I have now also seen this quite clearly defined in setuptools-scm's documentation :)

I just sent a PR #62 for an updated MANIFEST.in which results in only the main source code being put in the package. I think we don't need (or want) other files on an esp32. (Not sure about examples. They might be nice to test that it works, but after that users would likely upload custom code anyway).

Let me know what you think.

wnienhaus commented 2 years ago

Thanks for merging #62. Looks like all is ready for release now.

Probably a release of a pre 1.0.0 (e.g. 0.1.2) would still be good, for ensuring that publishing to real PyPI works correctly. When I did this with a test project, I needed to first create an API token in PyPI, which had scope set to "Entire account", because for the first publish, the project didn't exist in PyPI yet. Once I had published the first release for that project, I then deleted the API token and created a new one, limited to the scope of only the specific project.

Do you want to do this first release to real PyPI (with a pre-release version) to "create the project" or should I do that, by manually publishing something under the "micropython-py-esp32-ulp" name, and add you as a collaborator (as I did for the project on TestPyPI)?

Once the whole flow works for the pre-release version, there should be nothing standing in the way anymore to release 1.0.0 (I can't think of anything).

ThomasWaldmann commented 2 years ago

@wnienhaus can you give that pypi.org release a final manual test / review?

https://pypi.org/project/micropython-py-esp32-ulp/#files

(this was created automatically by the publish action after doing a github release of 0.1.5)

ThomasWaldmann commented 2 years ago

Next steps (if pkg is ok):

Do another dummy commit, tag 1.0.0, git push --tags, wait for CI to finish, github-release it.

The dummy commit is needed because having 2 tags on same commit confuses setuptools_scm - it might generate the wrong version number in such a case.

wnienhaus commented 2 years ago

Very nice! I just tested the package directly on a clean ESP32 (installed via upip). Works as advertised! I also took a manual look over the files in the package and it all looks good to me.

About next steps, the dummy commit makes sense, but likely tagging should not be done with git, but rather let the Github release create the tag as part of the release?

ThomasWaldmann commented 2 years ago

Thanks for testing, so I'll create the release now.

AFAIK, it does not make a difference whether one tags with git (and then used that tag to create a release) or creates the release tag with github. Do you know more?

ThomasWaldmann commented 2 years ago

OK, so 1.0.0 publishing failed, because there already were some 1.0.x releases on test.pypi.org (and as that cancels the workflow, it didn't publish to pypi.org either).

To avoid these conflicts, we now have a 1.1.0 as first real release.

ThomasWaldmann commented 2 years ago

done! o/

wnienhaus commented 2 years ago

Yay! We made it! This needs some champagne :champagne: ! Thank you for all your effort reviewing and giving feedback! The result is better for it!