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

List index out of range fix, remove default QR message if text files are available and file housekeeping #658

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

SimonShirley
Copy link
Contributor

Hello!

I'm planning on taking my Badger2040 to an event soon and in doing so, needed to update the QR codes that were available for display in qrgen.py. In setting up the new files, I ended up with one fewer file than I had previously uploaded and so when trying to display the QR codes from the launcher, instead of seeing the first QR code, I received a warning saying "List index out of range".

After some debugging, I discovered that this was because the state/qrcodes.json file had remembered the last QR code viewed and that the index of this was now higher than the count of text files in the qrcodes folder.

Commit eb7eaae loads the state, checks to see if the last viewed index is in range and if the index isn't, set it in range. Finally, saving the state back.

It is a little annoying for the demo QR code to show, regardless of whether QR code files have been uploaded, so commit 77ee234 checks to see if there are any QR code text files in the qrcodes directory and only create the demo screen file if there aren't any.

I also noticed that files are being opened on screen refresh but not closed. I don't know if micropython automatically closes these file handles after a set period (like garbage collection) but commit 9aedf16 closes them explicitly, in case these aren't tidied up automatically.

@SimonShirley
Copy link
Contributor Author

I probably should have created this in a separate branch (or multiple branches), so will wait for review.

@ZodiusInfuser ZodiusInfuser added enhancement New feature or request [- badger2040 -] https://shop.pimoroni.com/products/badger-2040 micropython This issue or request relates to micropython (either code or bindings) labels Jan 31, 2023
@Gadgetoid Gadgetoid self-requested a review January 31, 2023 12:20
@Gadgetoid Gadgetoid self-assigned this Jan 31, 2023
@Gadgetoid
Copy link
Member

This looks like a great set of changes, thank you! A set of clean commits in a single branch is perfect. (Though, yes, for your own purposes you might want to work on a feature branch, but it doesn't make a lick of difference to me.)

Few little things-

  • First check the results of the Python linting, Thonny has a habit of spamming whitespace everywhere and that needs to be removed to pass
  • newQRcodeFilename should be new_qr_code_filename since snake_case is the convention for variable names in Python
  • File paths should be prefixed with / - I know we don't do this everywhere, but this tripped me up while working on Badger 2040W since Thonny will do something like os.chdir when you browse the filesystem and really trip up examples with relative paths. Eg open("qrcodes/{}"... should be open("/qrcodes/{}"...
  • You can use f-strings in lieu of format, eg: text = open("qrcodes/{}".format(newQRcodeFilename), "w") can be text = open(f"qrcodes/{newQRcodeFilename}", "w")

Thank you!

@SimonShirley
Copy link
Contributor Author

Thank you @Gadgetoid for your feedback!

I've installed flake8 on my dev machine and have fixed up (or attempted to!) the warnings that it presented. I've also made the changes in your feedback. :)

@Gadgetoid Gadgetoid merged commit f17c04b into pimoroni:main Feb 22, 2023
@Gadgetoid
Copy link
Member

Great stuff, thank you! Just got a chance to test and it works nicely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[- badger2040 -] https://shop.pimoroni.com/products/badger-2040 enhancement New feature or request micropython This issue or request relates to micropython (either code or bindings)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants