Update examples with umount #28
Conversation
Help avoid data corruption by unmounting SD card after use
ran black on directory
|
This looks good to me, did you test these as well? |
|
@tekktrik Only tested the TFT featherwing example and can confirm it works well. The other 2 I did just because I was in there. Should I not submit things I cannot explicitly test? It seemed like a shame to leave the other 2 examples alone without umount. I probably should have specified that in the original commit sorry. |
|
No, totally fine to PR things you haven't tested! Just wanted to check, definitely always makes me more confident when it is tested is all :) |
|
I'm out today but will look tomorrow! |
There was a problem hiding this comment.
Does it make sense to just take out TAKE_SCREENSHOT (assuming it's value would always be true) and always begin the block of taking the screenshot and then unmounting the SD card? Or is there a specific reason it would be helpful to toggle it, and why that default value is False?
|
Take screenshot is False by default. Set to true to take a screenshot. A good real world example is at the very end of my feather weather project. That's my old "YOLO version" as Anecdata puts it where I race a 120 second timer to remove the SD card, upload it to the PC, and put it back. Still have to update my own scripts. I included the if statement because I believe its in everyone's best interest to become accustomed to using it in that way, especially beginners. Technically it doesn't need to be there... but then neither does umount. It adds safety measures so you don't
Stopping an SD write mid-stream won't just corrupt the file you're trying to write, it will create gibberish files that cannot be deleted and/or corrupt your entire SD card. Between the if statement and umount the risk of data corruption and infinite screenshot taking loop is virtually eliminated. For advanced users with never ending scripts at some point you'll likely want to remount but that is unnecessary in the simpletest examples. umount however is definitely necessary. They're features I wish would have been included in the examples from the beginning. I've used this library more than anyone else I know. I've used it on the NRF52840, M0's, M4's, and ESP32-S2 & S3. Beginners in particular will run into data corruption issues if those 2 features are not included. I learned the hard way, these features are necessary. |
There was a problem hiding this comment.
Thank you @DJDevon3.
I tested all 3 examples, two on a PyPortal Titano, and the other with Feather RP2040 + TFT Featherwing.
I think it's okay to have the TAKE_SCREENSHOT variable even though it's not strictly necessary. It's convenient to a high level switch like this so that you can keep the code on your device but only enable it when specifically interested in writing the screenshots.
However I do think we should make one of two possible further changes:
Move the initialization of the SDCard to inside of the if TAKE_SCREENSHOT: block so that if the variable is false the SDCard won't get initialized or mounted.
Or
Move storage.umount(vfs) to outside / below the if statement.
Because as it is now, when TAKE_SCREENSHOT is False it means the SDCard will get initialized and mounted, but then not unmounted since that is currently inside the if block. Which is one of the things this change is intending to help guard against.
|
@FoamyGuy that's a great point and points out a flaw in the way I've been using it. Will take another shot at improving it. Thank you for the excellent guidance. |
As requested by review. Unable to test the "bitmapsaver_simpletest" bitmap palette example on my featherwing since SCK is in use by other things.
|
Moved SD Card initialization into take screenshot if statement. Works well.
was unable to successfully test the bitmapsaver_simpletest.py example on my tft featherwing because sck in use. if a reviewer could test/confirm that works then push it. it just won't work by default on a 3.5" tft featherwing which is why we have a separate example for the tft featherwing. :) I'm not sure what hardware it's designed for so I don't want to mess with that one too much since I can't successfully test for it. |
Updating https://github.com/adafruit/Adafruit_CircuitPython_BitmapSaver to 1.2.10 from 1.2.9: > Merge pull request adafruit/Adafruit_CircuitPython_BitmapSaver#28 from DJDevon3/DJDevon3_WorkingBranch Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 1.14.1 from 1.14.0: > Merge pull request adafruit/Adafruit_CircuitPython_Requests#133 from DJDevon3/WorkingBranch Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
Help avoid data corruption by unmounting SD card after use