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

Support hot-plugging microSD cards for better UX #157

Closed
ghost opened this issue Aug 10, 2022 · 10 comments
Closed

Support hot-plugging microSD cards for better UX #157

ghost opened this issue Aug 10, 2022 · 10 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@ghost
Copy link

ghost commented Aug 10, 2022

Krux can only detect a microSD card at boot which makes the PSBT signing flow awkward and cumbersome if you're reading and writing the files to the card. It would be better if you could remove and insert a card at any point during runtime.

To solve this, I think we should update our MaixPy fork to no longer mount an SD card on startup in maixpy_main.c, but rather add support for the Micropython machine.SDCard module which should, in theory, let us mount (and re-mount) an SD card from our "userspace" Krux python code.

See here for how we could do it:
https://github.com/loboris/MicroPython_K210_LoBo/blob/master/k210-freertos/mpy_support/standard_lib/uos/vfs_sdcard.c

@ghost ghost added bug Something isn't working help wanted Extra attention is needed labels Aug 10, 2022
@ghost ghost added this to the Next Major Release milestone Sep 17, 2022
@tadeubas
Copy link
Contributor

tadeubas commented Feb 7, 2023

I've made a simple test to see if it was possible, and it is!

See the gist for a simple loop implementation:
https://gist.github.com/tadeubas/96443b817af2be4a30a949f537ca76c4

The idea came from the MaixPy wiki docs here: https://wiki.sipeed.com/soft/maixpy/en/others/maixpy_faq.html#TF-card-format-is-right%2C-but-can%27t-read-tf-card-anf-failed-mount

@ghost
Copy link
Author

ghost commented Feb 7, 2023

Awesome, thanks for finding and testing this! Did you happen to check if this worked with the selfcustody fork of MaixPy that Krux uses? I ask because we're about a year or more out of date with the MaixPy upstream repo right now, so I'm not sure if this will work or not without updating it. But we should probably try to update our fork regardless.

@tadeubas
Copy link
Contributor

tadeubas commented Feb 7, 2023

Yes! I've tested and it is working on the current MaixPy from selfcustody, the one Krux uses. It freezes a little when trying to mount the SD (~1 sec), so I don't think it is good to put on the While loops, but instead I think we should check every time right before the usage (when click on PSBT or message for example)

@ghost
Copy link
Author

ghost commented Feb 7, 2023

@tadeubas Sounds good to me. Are you working on a PR for this?

@tadeubas
Copy link
Contributor

tadeubas commented Feb 7, 2023

Not yet... I would need to know more about all the functionalities that Krux enables the use of a SD card

@ghost
Copy link
Author

ghost commented Feb 8, 2023

The easiest way to find all uses of the SD card would be to look for the string /sd in the src folder. Currently, it's used for:

  • Writing to a log file when the log level is set to something other than NONE (default)
  • Writing to a settings file to persist settings
  • Reading a firmware update + signature file when performing airgapped firmware updates
  • Reading and writing PSBT files
  • Writing the GCode instructions file for a CNC to use to mill a QR card out of wood or metal

In terms of implementing this, maybe we could streamline this mounting process by adding a mount_sd contextmanager function that returns a new SDCard class for reading and writing to an SD card, where the class assumes:

  1. It's mounted
  2. Its root path is always /sd/

So, for example, you could convert this:

        if self.ctx.sd_card is not None:
            self.ctx.display.clear()
            if self.prompt(
                t("Save signature to SD card?"), self.ctx.display.height() // 2
            ):
                sig_filename = "signed-message.sig"
                with open("/sd/%s" % sig_filename, "wb") as sig_file:
                    sig_file.write(sig)
                self.ctx.display.flash_text(
                    t("Saved signature to SD card:\n%s") % sig_filename
                )

to this:

        with mount_sdcard() as sd:
            self.ctx.display.clear()
            if self.prompt(
                t("Save signature to SD card?"), self.ctx.display.height() // 2
            ):
                sig_filename = "signed-message.sig"
                sd.write_binary(sig_filename, sig)
                self.ctx.display.flash_text(
                    t("Saved signature to SD card:\n%s") % sig_filename
                )

The idea being that this new mount_sdcard context manager function would mount the SD card, then yield an SDCard object so you know any operation on it inside the with should be able to read or write to the card. And if the SD card cannot be mounted, the with block would just not execute.

Alternatively, we could add a contextmanager function similar to open that opens a file for either reading or writing (depending on the flags passed in, just like a normal open), taking care to mount first. Something like open_sd perhaps.

I think either would suffice, but the latter might be more standard/pythonic and simpler, since you wouldn't need to add an SDCard class and could instead just return a file handle from open. I'm open to either approach (or another alternative). Curious to hear your thoughts!

@tadeubas
Copy link
Contributor

Awesome idea! I can't think of a better alternative. Will do this

@tadeubas
Copy link
Contributor

tadeubas commented Feb 11, 2023

The only problem is that micropython doesn't have the contextlib to create with-statement contexts - https://stackoverflow.com/a/5520844/8806187

Should we add this lib in the vendor folder? For Python 3.11 version? https://github.com/python/cpython/blob/3.11/Lib/contextlib.py

EDIT:
Unfortunately Contextlib has other dependencies like abc that doesn't exist in micropython... I think we will have to abandon the idea to use the with-statement

@tadeubas
Copy link
Contributor

Forget what I've said, it is possible to customize a class def __enter__(self) and def __exit__(self, exc_type, exc_val, exc_tb) to make use of the with-statement, we don't need to use contextlib

@tadeubas
Copy link
Contributor

Hi @jreesun this issue is finished in the PR. Only 2 tests are broken, but the feature is working fine on the device

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant