Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Release and publish uf2 asset upon version tag creation. #198

Merged
merged 27 commits into from
Dec 4, 2022

Conversation

awonak
Copy link
Collaborator

@awonak awonak commented Nov 27, 2022

Here is a successful run of the workflow:
https://github.com/awonak/EuroPi/actions/runs/3560476183/jobs/5980521188

The result of this workflow will create a new release with uploaded uf2 file:
https://github.com/awonak/EuroPi/releases/tag/v0.1.3

Copy link
Collaborator

@mjaskula mjaskula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me. I just have a few questions/discussions to be resolved before approval, but overall really nicely done.


jobs:
compile-uf2:
runs-on: ubuntu-latest
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[discussion] My preference in build scripts is to avoid unversioned and 'latest' dependencies. It is possible that a new version of a dependency will break our release. In that case, we'd rather deal with it when we are upgrading dependencies than when we are trying to create a release.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also apply this to:

  • apt-get packages
  • ssd1306
  • marvinpinto/action-automatic-releases

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoken like a true SRE. I've locked action-automatic-releases and left the other two as-is. The ssd1306 repo doesn't have any tags and hasn't been updated in 4 years, and I don't have any concerns about apt-get build tools throwing surprises.

.github/workflows/compile_to_uf2.yml Show resolved Hide resolved
BootloaderMenu(EUROPI_SCRIPT_CLASSES).main()
EOF

- run: tree micropython/ports/rp2/modules
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] Is this line some debug output?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, verify what was written.

cp -r europi/software/contrib/*.py micropython/ports/rp2/modules/contrib

- run: wget https://raw.githubusercontent.com/stlehmann/micropython-ssd1306/master/ssd1306.py -O micropython/ports/rp2/modules/ssd1306.py
- run: sed -i 's/progsize=256)/progsize=1024)/g' micropython/ports/rp2/modules/_boot.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] What's this line about? It should probably include a comment or clarifying name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redoxcode can you help shed more light on why we needed to increase the progsize?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I am not 100% sure how the different memory types of the pico work. However without this and without calling the garbage collector before the menu.py the software would freeze somewhere between the lines 15 and 34 of main.py. That's where all the other contib scripts get imported. After trying different import orders without luck I concluded, that it's not a singe module that causes the freeze, but some type of memory reaching it's limit. So yeah, there is a little try and error involved. Maybe we should test again, if changing the progsize is required or if just the garbage collection alone fixed it...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worries me. I've done some digging and can find any explanation as to why a frozen script would behave differently than an unfrozen one. That is, why this main.py needs a call to gc and the change in progsize, but we don't needed it in the 'normal' situation. It doesn't make sense to me. I'll keep digging.

.github/workflows/compile_to_uf2.yml Show resolved Hide resolved
.github/workflows/compile_to_uf2.yml Show resolved Hide resolved
@mjaskula
Copy link
Collaborator

I missed the documentation PR (#199) that went through just before this, I just caught up on that. I noticed two things:


[required] I think that, for consistency, that documentation and this workflow should agree, and that should be in the scope of this PR. Particularly if our script diverges from our own documentation we should say why.

To be fair, I haven't looked through closely to find any differences, but I suspect that you know if there are any. So this ask may be a noop.


[optional] I think that we should be freezing the included modules. The previous PR included a discussion about obfuscating code being the main benefit, but the documentation says this (emphasis mine):

The benefit of using a manifest is that frozen modules are more efficient: they are faster to import and take up less RAM once imported.

Since we've already taken steps to address the menu startup time and we know that it is caused by the many imports, this looks like it could be an easy win.

@redoxcode
Copy link
Contributor

[required] I think that, for consistency, that documentation and this workflow should agree, and that should be in the scope of this PR. Particularly if our script diverges from our own documentation we should say why.

To be fair, I haven't looked through closely to find any differences, but I suspect that you know if there are any. So this ask may be a noop.

I think currently there are no differences, but I agree, if we do something different here than in #199 that should be documented as well. But again, no differences yet.

[optional] I think that we should be freezing the included modules. The previous PR included a discussion about obfuscating code being the main benefit, but the documentation says this (emphasis mine):

So I think you are confusing freezing with bytecode here. Freezing is the process of baking any code into the firmware. The question is if you want to freeze python source code or pre-compiled bytecode. Bytecode is used for performance benefits. The obfuscation is just a side effect (we don't much care about). But as already discussed in #199 we'd have to see if the impact on performance is noticeable at all in our application, since I didn't do any speed comparison (I got modules in my case that take longer to boot up, so I don't care that much about boot time).

Hope this has answered some of your concerns. Keep in mind that there is always the option to improve and optimize on this with future PRs once we get things running.

@mjaskula
Copy link
Collaborator

So I think you are confusing freezing with bytecode here.

Sorry, yeah, I'm probably getting the terms mixed up here, but my point is that there's a nice potential to improve the wait time when starting up and switching between scripts, which could be huge for non-dev users.

Keep in mind that there is always the option to improve and optimize on this with future PRs once we get things running.

Yess, good point, we can improve import perf in future iterations.

@mjaskula
Copy link
Collaborator

You two have convinced me that the 'weirdness' in the main.py isn't a good reason to hold this PR, we can improve that in the future as we learn more. Great work.

@awonak awonak requested a review from mjaskula December 1, 2022 04:54
@awonak
Copy link
Collaborator Author

awonak commented Dec 1, 2022

I ended up adding the documentation updated to this PR. Please take a look and confirm the new documentation is clear. I also think that merging this PR should not happen until we are ready to cut a release to ensure the updated documentation matches the latest release.


The quickest way to get your EuroPi flashed with the latest firmware is to head over to the [releases](https://github.com/Allen-Synthesis/EuroPi/releases) page and download the latest `europi-vX.Y.Z.uf2` file. Then follow the 'BOOTSEL' instructions above to flash the EuroPi firmware to your pico. You can now skip the **Setting Up** section and proceed to the [Next Steps](#next-steps) section.

> **_NOTE:_** The EuroPi firmware and contrib scripts will be installed and accessible via Thonny for writing custom scripts on the pico, however Thonny will not show those firmware and contrib files in the pico filesystem. If you want to have access to the EuroPi firmware source code on your pico, follow the **Setting Up** instructions in the next section.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] Have we tested that this is correct? Does a main.py located in the standard location override the one delivered with the firmware? It makes sense that it does, but I'd like to be sure.

This opens up a bunch of other questions for me about how things will work, but I guess we'll need to document them as we find out exactly how people want to use this new distribution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can double check, but this follows the import priority documentation @redoxcode provided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing I did was done using REPL, I didn't test adding a main.py and rebooting, so I'll test that to be sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjaskula Good catch! My assumption was incorrect. I have updated the docs to reflect that main.py will not override the entry point.

We essentially now have 3 firmware options. 1) uf2 quick start drag and drop, 2) pypi distributable source, and 3) download source from GitHub.

I think the updates I've included here are good for now, but we should probably revisit the programming instructions at some point.

Copy link
Collaborator

@mjaskula mjaskula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new doc looks good to me. Just one question on what was tested. Thanks.

…mware.

Also we should default to generating a prerelase that we can edit.

Also updated Release Process instructions.
@@ -34,14 +34,14 @@ Pushes to the main [pypi.org](https://pypi.org/project/micropython-europi/#histo
### Release process

1. Decide which version number to release.
2. Update [`software/firmware/version.py`](/software/firmware/version.py) with the new version number.
3. Update [`changelog.md`](CHANGELOG.md) with a description of the release.
Copy link
Collaborator

@mjaskula mjaskula Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] Is there a reason we're removing this step form the release process? Seems unrelated to the new uf2 file

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see the "Update the generated Github Release" as the second to last step, I guess this replaces the release notes?. I don't know what this is. Should there be a link to it? or a description on how to publish?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I've added a link to the releases page. The rest is self-explanatory. Edit the release notes as described in the docs and the select either Publish or Save Draft to continue editing.

@awonak awonak merged commit d40f44b into Allen-Synthesis:main Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants