-
-
Notifications
You must be signed in to change notification settings - Fork 80
Save programs on Prime Hub as they get downloaded #75
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
Conversation
dlech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Any reason we couldn't make this non-blocking and put it in pbdrv right away instead of having this temporary implementation?
| if (size > MPY_MAX_BYTES) { | ||
| return 0; | ||
| } | ||
| *buf = m_malloc(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of copying the file to RAM, it would be nice to just pass the file reader to compiler.
| static void stm32_main(void) { | ||
|
|
||
| #if PYBRICKS_HUB_PRIMEHUB | ||
| mp_hal_delay_ms(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this delay for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason Bluetooth wouldn't come up without it.
This initial read/write operation takes fairly long, so maybe there's still a bit too much blocking going on while everything else is initializing.
| static bool flash_initialized = false; | ||
|
|
||
| // True sets the notCS pin, False resets it | ||
| static void flash_enable(bool enable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It there a reason we do this manually instead of setting the pin to AF5 and letting the SPI hardware take care of it automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason against this. I've been following various tutorials that did it this way, but it would certainly be nicer if the appropriate drivers can take care of this.
| .erase = block_device_erase, | ||
| .sync = block_device_sync, | ||
|
|
||
| .read_size = 256, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this info come from the data sheet? If so, we should have a comment explaining which chip we have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I think we can read an arbitrary lengths from the device. Since reads are blocking for now, this provides a reasonable balance between read time and giving the run loop enough cycles.
|
|
||
| // Close the file | ||
| if (lfs_file_close(&lfs, &file) != LFS_ERR_OK) { | ||
| return PBIO_ERROR_FAILED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we map LFS errors to PBIO errors to get a more specific error?
No particular reason against it, just lack of skill at this point. Getting this to work seemed like a good learning opportunity and the possibility to try out the resulting user experience (which is positive.) I made sure to keep everything out of But now that we've figured out the essentials, maybe doing the right way isn't too bad in terms of time. |
I didn't get hardware control for NSS to work yet. I tried this with PB12 in Then it reads all zeros after the first read. Some resources [1, 2]. I haven't confirmed with a scope, but if it really does only control the pin during init/deinit, this isn't what we need. We need to drive this pin to start and stop data transfers. |
|
I've updated the PR with additional functionality to restore backed up firmware. First, make sure you are running the official firmware. If you aren't doing so already, do this, for example: Then check out this PR and follow these steps If you don't want to build from source, just get the build for this PR from the CI and run with the latest This will upload the firmware to the hub and install it, all without DFU. Then to restore it, connect to the hub using Pybricks Code, and run this: from pybricks.experimental import restore_firmware
restore_firmware() |
9c65f5b to
acef7fd
Compare
0385408 to
15b37eb
Compare
This provides some test code to experiment with the file system.
This combines the following commits: pybricks.experimental: Use 4 bytes for flash addresses. The upstream MicroPython board exits 4 byte mode, but 4 bytes are needed beyond 16Mb. pybricks.experimental: Toggle flash to activate write state. pybricks.experimental: Only write enable. Disable happens automatically on each erase and/or write. pybricks.experimental: Drop flash debug print. pybricks.experimental: Wait for write complete.
Make it somewhat more generic so we can experiment with reading/saving a main.py user script from main.c Also enlarge buffers. Also add mkdir command to create /_pybricks if it doesn't exist.
This hub has dedicated storage space for user programs. Now the downloaded program is stored so it can be started with the button later. It is saved to /_pybricks/main.mpy. This needs to be reworked extensively to make file access clean and non-blocking, but it makes the hub much more useable for now.
This makes reading and writing much faster. Also fix lookahead buffer size.
Init and scan file system only once.
This refactors some common code for writing SPI commands and removes some hardcoded parameters.
This adds an experimental function that can restore a previously backed up firmware file.
This is quite useful for hubs that can read and write from external storage. It is currently enabled on all hubs with the PYBRICKS_STM32_OPT_EXTRA_MOD flag. It is also used for the firmware restore process, so we can parse JSON files to check meta data without reinventing the wheel.
Verify the firmware meta data before installing it. Also reset the hub when we are done in order to complete the installation.
Since flash needs to be rewritten anyway, just use #if for hub-specific code.
Since flash read/write is blocking, we shouldn't allow this for user programs. These are useful for debugging the firmare installation process, but we can drop them once we have decent file i/o capability.
This ensures the installer does not give unexpected result when it is used with newer firmware.
|
We've decided to merge this so we can get an early beta version for Prime Hub and Inventor Hub out. 🚀 We'll need to make this non-blocking later. |
The Prime Hub, Inventor Hub, and Essential Hub have a separate chip to store user programs and other files. This is sometimes referred to as "external" storage, as opposed to the "internal" storage on the STM32 device, on which the firmware is stored.
This PR is a first attempt at:
littlefsas the file system layer. This uses (the somewhat outdated) V1.7 in order to be compatible with the file system used by the stock firmware of the Prime Hub and Inventor Hub. If we can manage to keep using this, we'd be able to avoid formatting the external storage. This would be nice even if we'd discontinue dual boot support.main.mpyon external storage when it is downloaded to the hub via Pybricks Code or Pybricksdev. The program can be restarted by pressing the button. This makes the user experience a whole lot nicer, and it is a key feature to making Pybricks useable in First LEGO League. It's part of the #V3.2 roadmap.There are currently also a few experimental MicroPython functions to read and write files. I've used them for debugging, but I'd like to remove them before merging this. Since file access is blocking, this isn't something we should support in user programs. As long as Bluetooth keeps working well enough, we can probably get away with this for download-and-run since no other critical services like motors are running at that time.
The build size does increase, particularly due to
littlefs, getting us quite close to our self-imposed dual boot size limit. So we might want to rethink the dual boot approach at some point. But since we have access to external flash now, there are a few other creative ways we can back up and restore the original firmware, all without DFU. We could also have a closer look at where the boundary really should be; perhaps we're making it harder than we need to.It is currently not activated on the Essential Hub. It should work with very little modifications, but I just didn't get around to working on this yet. We'll want to check that it uses the same
littlefsversion and/or file system properties.Before we can even think about merging this, we'll need to carefully review that we aren't doing any unwanted writes, in order to preserve the integrity of user files and the bootloader partition.