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

(embassy-rp): Implementation of generic flash mutation access #951

Merged
merged 28 commits into from
Oct 28, 2022

Conversation

MathiasKoch
Copy link
Contributor

@MathiasKoch MathiasKoch commented Sep 16, 2022

I have attempted to utilize the work done in rp2040-flash by implementing embedded-storage traits on top, for RP2040.

Concerns:

  1. Should the DMA be paused where I have put a FIXME note? DMA_CHx.ctrl_trig().write(|w| { w.set_en(false) })? If so, how to properly do that without have control over the peripheral for the DMA channels? And if so, I assume we should only re-enable/unpause the ones that were enabled before?
  2. Should I make sure core2 is halted as part of this code? I am not sure if https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L103-L109 is heavy/slow code to run?
  3. Any good way of making this configurable over FLASH_SIZE, WRITE_SIZE and ERASE_SIZE without doing it as generics or parameters, as those make it possible to do differing configs throughout the same program, which feels wrong? Preferably, a compile-time option?

EDIT:
I have implemented the flash API here under the assumption that all external QSPI nor flashes are infact Multiwrite capable, as this makes it possible to use the ROM function for writes of 1 bytes at a time.

I have also added a HIL test for this, but because HIL tests are running 100% from RAM and I wanted to make sure it still works when running from flash, I have also added an example testing erase/write cycles of entire sectors, as well as single bytes in multi-write style.

Ping @Dirbaio

@Dirbaio
Copy link
Member

Dirbaio commented Sep 16, 2022

Should the DMA be paused

Oof this is cursed. I guess yes, or this will be a problem when DMAing from flash.

If so, how to properly do that without have control over the peripheral for the DMA channels?

If the flash code globally disables irqs and stops the other core you have the guarantee that no other code will race you, so you can go ahead and do it unsafely I guess...

And if so, I assume we should only re-enable/unpause the ones that were enabled before?

Yes...

Also, are we sure EN=0/EN=1 pauses/resumes the channel fully seamlessly?

Alternatively we can disallow DMAing from flash... nRF has this limitation in hardware and it's not that bad, it means you have to copy through a RAM buffer though. Perhaps it could be feature-gated, so you can either "DMA from flash" OR "write to flash", but not both in the same binary...

Should I make sure core2 is halted as part of this code? I

In the final version yes, but I'd say it's OK to not do it for now since we don't support muticore yet.

Note that code resets core2, we want to pause it instead. It'd have to send an irq to the other core (using the FIFO maybe) so that the other core "parks" itself in a RAM irq handler. Also note it's possible to do flash ops from core2, in this case it should park core1 😓 .

Any good way of making this configurable over FLASH_SIZE, WRITE_SIZE and ERASE_SIZE without doing it as generics or parameters

Cargo features with a range of possible options? :P They're always powers of 2 so there shouldn't be too many "reasonable" options...?

@MathiasKoch
Copy link
Contributor Author

Also, are we sure EN=0/EN=1 pauses/resumes the channel fully seamlessly?

From the RP2040 datasheet:

DMA Channel Enable.
When 1, the channel will respond to triggering events,
which will cause it to become BUSY and start transferring
data. When 0, the channel will ignore triggers, stop issuing
transfers, and pause the current transfer sequence (i.e.
BUSY will remain high if already high)

As I read that, it actually fully pauses any ongoing transfer, so I don't think this is too bad.

In the final version yes, but I'd say it's OK to not do it for now since we don't support muticore yet.

Note that code resets core2, we want to pause it instead. It'd have to send an irq to the other core (using the FIFO maybe) so that the other core "parks" itself in a RAM irq handler. Also note it's possible to do flash ops from core2, in this case it should park core1.

Noted.

Cargo features with a range of possible options? :P They're always powers of 2 so there shouldn't be too many "reasonable" options...?

Hmm, yeah. I'm not super fond of that way of doing it. It just doesn't seem to scale very well :(

@lulf
Copy link
Member

lulf commented Sep 19, 2022

I have attempted to utilize the work done in rp2040-flash by implementing embedded-storage traits on top, for RP2040.

Concerns:

1. Should the DMA be paused where I have put a FIXME note? `DMA_CHx.ctrl_trig().write(|w| { w.set_en(false) })`? If so, how to properly do that without have control over the peripheral for the DMA channels? And if so, I assume we should only re-enable/unpause the ones that were enabled before?

2. Should I make sure core2 is halted as part of this code? I am not sure if https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L103-L109 is heavy/slow code to run?

3. Any good way of making this configurable over `FLASH_SIZE`, `WRITE_SIZE` and `ERASE_SIZE` without doing it as generics or parameters, as those make it possible to do differing configs throughout the same program, which feels wrong? Preferably, a compile-time option?

Although a slightly different situation, I've had a similar dilemma in embassy-boot generalizing these, and ended up using const generics for them. What about a combination that allows both uses: a GenericFlash using const generics, and a type alias Flash based on cargo features? Or would that be too confusing?

@MathiasKoch
Copy link
Contributor Author

I have attempted to utilize the work done in rp2040-flash by implementing embedded-storage traits on top, for RP2040.

Concerns:

1. Should the DMA be paused where I have put a FIXME note? `DMA_CHx.ctrl_trig().write(|w| { w.set_en(false) })`? If so, how to properly do that without have control over the peripheral for the DMA channels? And if so, I assume we should only re-enable/unpause the ones that were enabled before?

2. Should I make sure core2 is halted as part of this code? I am not sure if https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L103-L109 is heavy/slow code to run?

3. Any good way of making this configurable over `FLASH_SIZE`, `WRITE_SIZE` and `ERASE_SIZE` without doing it as generics or parameters, as those make it possible to do differing configs throughout the same program, which feels wrong? Preferably, a compile-time option?

Although a slightly different situation, I've had a similar dilemma in embassy-boot generalizing these, and ended up using const generics for them. What about a combination that allows both uses: a GenericFlash using const generics, and a type alias Flash based on cargo features? Or would that be too confusing?

That was actually exactly what I was thinking about.
Either chip specific exports like boot2 does for their bootloaders, or '1M_4_1' kind of definitions, where 'FLASH-SIZE_ERASE-SIZE_READ-SIZE'

FrozenDroid and others added 17 commits September 23, 2022 07:58
Compiler will infer a different lifetime for BootFlash than for the
borrowed flash, which makes it require more type annotations than if it
was just owning the type. Since it doesn't really matter if it owns or
borrows in practical use, change it to own so that it simplifies usage.
Fixes a bug in the partition assertions that ensures that the state
page(s) have enough space for 2x active partition range.

Add unit test to verify that panic is observed.
Removes feature(generic_associated_types)
…ata functions, which are now part of this HAL as well
@Dirbaio
Copy link
Member

Dirbaio commented Sep 26, 2022

What about a combination that allows both uses: a GenericFlash using const generics, and a type alias Flash based on cargo features? Or would that be too confusing?

It should be one or the other. Having 2 ways of doing the same thing is very confusing.

@MathiasKoch
Copy link
Contributor Author

MathiasKoch commented Sep 26, 2022

Given the kind of extreme limitations of the rom-data flash API, that aims at wide flash support, my suggestion would be to make a repo similar to https://github.com/rp-rs/flash-algo where we can implement a few different implementations (QSPI, Dual-SPI, SPI) in various configurations, compile and link them fully for RAM, and then include_bytes() one of them similarly to https://github.com/rp-rs/rp2040-boot2.

Then we can do fallback to the flash API in rom-data, if users do not wish the added flash size of an optimized flash API.

EDIT: Limitations here being the WRITE_SIZE = 256

Parameters
flash_offs Flash address of the first byte to be programmed. Must be aligned to a 256-byte flash page.
data Pointer to the data to program into flash
count Number of bytes to program. Must be a multiple of 256 bytes (one page).

W.R.T flash configurations & cargo features vs const generics, how about adding a "custom" peripheral for flash, such that you can only instantiate it once (panic on re-entrance), where you can pass flash-size, erase-size etc., and then you would have to pass around the flash struct like all other microcontrollers?

Thoughts @Dirbaio @lulf ?

@Dirbaio
Copy link
Member

Dirbaio commented Sep 26, 2022

Sounds good to me! 👍 I think having to write RAM-linked "flash driver" blobs is inevitable given the ROM limitations...

adding a "custom" peripheral for flash, such that you can only instantiate it once (panic on re-entrance), where you can pass flash-size, erase-size etc., and then you would have to pass around the flash struct like all other microcontrollers?

Not sure what you mean by "custom"? I'd make a normal FLASH peripheral (just the singleton struct, no functionality) and then a Flash driver that you pass all that config when creating it. If we do this, the API would have the same structure as any other peripheral.

@MathiasKoch
Copy link
Contributor Author

Not sure what you mean by "custom"? I'd make a normal FLASH peripheral (just the singleton struct, no functionality) and then a Flash driver that you pass all that config when creating it. If we do this, the API would have the same structure as any other peripheral.

By custom i just meant that it does not come from a pac peripheral. Not sure what the requirements are for the peripheral! macro in embassy?

@Dirbaio
Copy link
Member

Dirbaio commented Sep 26, 2022

Ah! then all our peripherals are already "custom". They're all invented by the HAL, we don't use the PAC ownership model. (rp and stm32 PACs don't have owned singletons at all because they're generated by chiptool. nrf PACs do because we're using the nrf-rs ones, but we just ignore them)

@MathiasKoch
Copy link
Contributor Author

Perhaps it would make sense to just have the RAM-blobs implement the CMSIS-Pack ABI? That way it would be re-usable by probe-rs once they figure out how they want to tackle probe-rs/probe-rs#1212

@MathiasKoch
Copy link
Contributor Author

@Dirbaio Ready for merge!

In case you didn't see it on matrix, I figured out the issue was due to the RTT header residing in flash:

debugger looking for the RTT header which caused the flash access which in turn caused a fault?
I had the same issue with rp2040-flash, with the following workaround: https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L88-L92

For defmt-rtt, it should be fixed with version 0.4.0, which includes knurling-rs/defmt#683

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

bors d+

@bors
Copy link
Contributor

bors bot commented Oct 27, 2022

✌️ MathiasKoch can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@MathiasKoch
Copy link
Contributor Author

bors r+

@MathiasKoch
Copy link
Contributor Author

bors r-

@bors
Copy link
Contributor

bors bot commented Oct 27, 2022

Canceled.

@MathiasKoch
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2022
951: (embassy-rp): Implementation of generic flash mutation access r=MathiasKoch a=MathiasKoch

I have attempted to utilize the work done in `rp2040-flash` by implementing `embedded-storage` traits on top, for RP2040.

Concerns:
1. ~~Should the DMA be paused where I have put a FIXME note? `DMA_CHx.ctrl_trig().write(|w| { w.set_en(false) })`? If so, how to properly do that without have control over the peripheral for the DMA channels? And if so, I assume we should only re-enable/unpause the ones that were enabled before?~~
2. ~~Should I make sure core2 is halted as part of this code? I am not sure if https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L103-L109 is heavy/slow code to run?~~
3. ~~Any good way of making this configurable over `FLASH_SIZE`, `WRITE_SIZE` and `ERASE_SIZE` without doing it as generics or parameters, as those make it possible to do differing configs throughout the same program, which feels wrong? Preferably, a compile-time option?~~


**EDIT:**
I have implemented the flash API here under the assumption that all external QSPI nor flashes are infact `Multiwrite` capable, as this makes it possible to use the ROM function for writes of 1 bytes at a time.

I have also added a HIL test for this, but because HIL tests are running 100% from RAM and I wanted to make sure it still works when running from flash, I have also added an example testing erase/write cycles of entire sectors, as well as single bytes in multi-write style.

Ping `@Dirbaio` 

Co-authored-by: Mathias <mk@blackbird.online>
Co-authored-by: Vincent Stakenburg <v.stakenburg@sinewave.nl>
Co-authored-by: Joakim Hulthe <joakim@hulthe.net>
Co-authored-by: Alex Martens <alex@thinglab.org>
Co-authored-by: Ulf Lilleengen <ulf.lilleengen@gmail.com>
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
@bors
Copy link
Contributor

bors bot commented Oct 27, 2022

Build failed:

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

Awesome work 🚀

@MathiasKoch
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2022
951: (embassy-rp): Implementation of generic flash mutation access r=MathiasKoch a=MathiasKoch

I have attempted to utilize the work done in `rp2040-flash` by implementing `embedded-storage` traits on top, for RP2040.

Concerns:
1. ~~Should the DMA be paused where I have put a FIXME note? `DMA_CHx.ctrl_trig().write(|w| { w.set_en(false) })`? If so, how to properly do that without have control over the peripheral for the DMA channels? And if so, I assume we should only re-enable/unpause the ones that were enabled before?~~
2. ~~Should I make sure core2 is halted as part of this code? I am not sure if https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L103-L109 is heavy/slow code to run?~~
3. ~~Any good way of making this configurable over `FLASH_SIZE`, `WRITE_SIZE` and `ERASE_SIZE` without doing it as generics or parameters, as those make it possible to do differing configs throughout the same program, which feels wrong? Preferably, a compile-time option?~~


**EDIT:**
I have implemented the flash API here under the assumption that all external QSPI nor flashes are infact `Multiwrite` capable, as this makes it possible to use the ROM function for writes of 1 bytes at a time.

I have also added a HIL test for this, but because HIL tests are running 100% from RAM and I wanted to make sure it still works when running from flash, I have also added an example testing erase/write cycles of entire sectors, as well as single bytes in multi-write style.

Ping `@Dirbaio` 

Co-authored-by: Mathias <mk@blackbird.online>
Co-authored-by: Vincent Stakenburg <v.stakenburg@sinewave.nl>
Co-authored-by: Joakim Hulthe <joakim@hulthe.net>
Co-authored-by: Alex Martens <alex@thinglab.org>
Co-authored-by: Ulf Lilleengen <ulf.lilleengen@gmail.com>
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
@bors
Copy link
Contributor

bors bot commented Oct 27, 2022

Build failed:

@MathiasKoch
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2022
951: (embassy-rp): Implementation of generic flash mutation access r=MathiasKoch a=MathiasKoch

I have attempted to utilize the work done in `rp2040-flash` by implementing `embedded-storage` traits on top, for RP2040.

Concerns:
1. ~~Should the DMA be paused where I have put a FIXME note? `DMA_CHx.ctrl_trig().write(|w| { w.set_en(false) })`? If so, how to properly do that without have control over the peripheral for the DMA channels? And if so, I assume we should only re-enable/unpause the ones that were enabled before?~~
2. ~~Should I make sure core2 is halted as part of this code? I am not sure if https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L103-L109 is heavy/slow code to run?~~
3. ~~Any good way of making this configurable over `FLASH_SIZE`, `WRITE_SIZE` and `ERASE_SIZE` without doing it as generics or parameters, as those make it possible to do differing configs throughout the same program, which feels wrong? Preferably, a compile-time option?~~


**EDIT:**
I have implemented the flash API here under the assumption that all external QSPI nor flashes are infact `Multiwrite` capable, as this makes it possible to use the ROM function for writes of 1 bytes at a time.

I have also added a HIL test for this, but because HIL tests are running 100% from RAM and I wanted to make sure it still works when running from flash, I have also added an example testing erase/write cycles of entire sectors, as well as single bytes in multi-write style.

Ping `@Dirbaio` 

Co-authored-by: Mathias <mk@blackbird.online>
Co-authored-by: Vincent Stakenburg <v.stakenburg@sinewave.nl>
Co-authored-by: Joakim Hulthe <joakim@hulthe.net>
Co-authored-by: Alex Martens <alex@thinglab.org>
Co-authored-by: Ulf Lilleengen <ulf.lilleengen@gmail.com>
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
@bors
Copy link
Contributor

bors bot commented Oct 27, 2022

Build failed:

@MathiasKoch
Copy link
Contributor Author

I don't understand why the bors test is failing?
It works 100% of the times with teleprobe locally?
@Dirbaio

@Dirbaio
Copy link
Member

Dirbaio commented Oct 27, 2022

the CI has a Pico W, not sure if it matters..?

@MathiasKoch
Copy link
Contributor Author

Hmm, can't see how that should matter :/

@MathiasKoch
Copy link
Contributor Author

Hmm. I just tested it on a Pico I had lying around here (non W version), and it works great there as well. Teleprobe tests pass 100% of times.

Not sure what is different with the CI, but I don't have a Pico W to test it on :(

@Dirbaio
Copy link
Member

Dirbaio commented Oct 28, 2022

bors r+

bors bot added a commit that referenced this pull request Oct 28, 2022
951: (embassy-rp): Implementation of generic flash mutation access r=Dirbaio a=MathiasKoch

I have attempted to utilize the work done in `rp2040-flash` by implementing `embedded-storage` traits on top, for RP2040.

Concerns:
1. ~~Should the DMA be paused where I have put a FIXME note? `DMA_CHx.ctrl_trig().write(|w| { w.set_en(false) })`? If so, how to properly do that without have control over the peripheral for the DMA channels? And if so, I assume we should only re-enable/unpause the ones that were enabled before?~~
2. ~~Should I make sure core2 is halted as part of this code? I am not sure if https://github.com/jannic/rp2040-flash/blob/ea8ab1ac807a7ab2b28a18bb5ca2e42495bb744d/examples/flash_example.rs#L103-L109 is heavy/slow code to run?~~
3. ~~Any good way of making this configurable over `FLASH_SIZE`, `WRITE_SIZE` and `ERASE_SIZE` without doing it as generics or parameters, as those make it possible to do differing configs throughout the same program, which feels wrong? Preferably, a compile-time option?~~


**EDIT:**
I have implemented the flash API here under the assumption that all external QSPI nor flashes are infact `Multiwrite` capable, as this makes it possible to use the ROM function for writes of 1 bytes at a time.

I have also added a HIL test for this, but because HIL tests are running 100% from RAM and I wanted to make sure it still works when running from flash, I have also added an example testing erase/write cycles of entire sectors, as well as single bytes in multi-write style.

Ping `@Dirbaio` 

Co-authored-by: Mathias <mk@blackbird.online>
Co-authored-by: Vincent Stakenburg <v.stakenburg@sinewave.nl>
Co-authored-by: Joakim Hulthe <joakim@hulthe.net>
Co-authored-by: Alex Martens <alex@thinglab.org>
Co-authored-by: Ulf Lilleengen <ulf.lilleengen@gmail.com>
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
@bors
Copy link
Contributor

bors bot commented Oct 28, 2022

Build failed:

@Dirbaio
Copy link
Member

Dirbaio commented Oct 28, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 28, 2022

Build succeeded:

@bors bors bot merged commit e7fdd50 into embassy-rs:master Oct 28, 2022
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.

6 participants