selfcustody / krux

Open-source signing device firmware for Bitcoin
https://selfcustody.github.io/krux/
Other
179 stars 36 forks source link

Support hot-plugging microSD cards for better UX #157

Closed ghost closed 1 year ago

ghost commented 2 years ago

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

tadeubas commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

tadeubas commented 1 year ago

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

ghost commented 1 year ago

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:

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 commented 1 year ago

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

tadeubas commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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