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

pbdrv/block_device: Implement async program load and save. #110

Merged
merged 4 commits into from
Aug 26, 2022

Conversation

laurensvalk
Copy link
Member

@laurensvalk laurensvalk commented Aug 18, 2022

Note: this is to be merged into master instead of the branch listed above.


This replaces the polling SPI implementation (deactivated in the
previous commit) with asynchronous DMA driven SPI transfers.
This fixes motors going out of sync when starting programs.

Fixes pybricks/support#599
Fixes pybricks/support#679

This also avoids touching file systems used by other known firmwares,
so that Pybricks can operate entirely stand alone. This reduces
the complexity of firmware installation and troubleshooting, because
there are no dependencies on prior firmwares or hub state.

It also lays the groundwork for saving and storing program data on
Powered Up hubs, but this isn't fully implemented in order to preserve
compatibility with Pybricks Code and the existing tutorial videos.
This restores the ability to run the program appended to the firmware,
which was temporarily removed in the previous commit.

This also frees up about 7K of build size on Prime Hub.

To keep the diff easier to go through, I have not yet done the following, but this should be done before merging:

  • Remove littlefs submodule
  • Remove pb_flash.{c,h}
  • Remove flash related calls from pybricks.experimental.

Other things to consider and test:

Copy link
Member

@dlech dlech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we lose power during a write operation? Do we want to have some sort of hash to validate the data when we read it back to detect this? (this hash could also be used to avoid redownload of the same program later). Another option could be to write the "file" data first and the metadata last (at the end of the block?) so that the metadata will only be written if everything else was successfully written.

If pbio_task was in better shape, it would be a good fit for the block read/write functions, so maybe add a TODO/REVIST comment to that effect?

@dlech
Copy link
Member

dlech commented Aug 24, 2022

The flash chip supports reading the whole flash, but we may need to break up reads in smaller steps if the DMA has upper limits.

The DMA NDT registers are 16-bit so 65535 bytes is probably the limit.

@laurensvalk
Copy link
Member Author

laurensvalk commented Aug 24, 2022

What happens if we lose power during a write operation? Do we want to have some sort of hash to validate the data when we read it back to detect this? (this hash could also be used to avoid redownload of the same program later). Another option could be to write the "file" data first and the metadata last (at the end of the block?) so that the metadata will only be written if everything else was successfully written.

Yes - there is an existing TODO for this in program_load. There was even an entry to prepare for it in the data map in the original PR, but it was removed to keep the changes smaller.

By doing it in program_load, we can keep the block drivers generic while using the same checks on the powered up hubs too. The hash to avoid redownloading fits right in there as well, good idea.

If pbio_task was in better shape, it would be a good fit for the block read/write functions, so maybe add a TODO/REVIST comment to that effect?

Several PRs back it started out that way, but the implementation with protothreads turned out to be simpler and more effective for now.

@dlech
Copy link
Member

dlech commented Aug 24, 2022

See if we can generalize the w25qxx driver beyond just two devices.

I wouldn't spend time doing this since we don't actually have a use for it.

@laurensvalk
Copy link
Member Author

Thanks for the review! I’ll try getting this fixed and merged tomorrow.

dlech and others added 2 commits August 24, 2022 17:18
This makes each upload have a unique name so you don't end up with
xyzhub-firmware(1).zip, xyzhub-firmware(2).zip in your downloads folder.
This replaces the polling SPI implementation (deactivated in the
previous commit) with asynchronous DMA driven SPI transfers.
This fixes motors going out of sync when starting programs.

Fixes pybricks/support#679

This also avoids touching file systems used by other known firmwares,
so that Pybricks can operate entirely stand alone. This reduces
the complexity of firmware installation and troubleshooting, because
there are no dependencies on prior firmwares or hub state.

It also lays the groundwork for saving and storing program data on
Powered Up hubs, which can be implemented in a future commit.
This implementation is no longer used.
This has been replaced by a generic raw block device that can be read asynchronously on all hub types.

We may still use littlefs in the future for user file access, but this should use the implementation and littlefs library provided by MicroPython.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants