tweedegolf / sequential-storage

A crate for storing data in flash memory with minimal need for erasing pages
Apache License 2.0
87 stars 8 forks source link

Async support #17

Closed lulf closed 6 months ago

lulf commented 6 months ago

@diondokter Just a headsup that I started looking at this... I've gotten the queue implementation to work nicely with async, and was able to use it with the Flash on nrf-softdevice. This revealed an issue which might be only specific to the softdevice, where the buffer to write into flash must be word-aligned, so the AlignedBuf is introduced for that. I set the alignment to 32 bytes, but I'm not sure if there are anyone besides the nRF flash which has this requirement (could be set to 4 bytes in that case).

I need to finish the map support, but just thought to give an early notice!

diondokter commented 6 months ago

Nice! I have just merged something big, so you're now getting conflicts :P But I don't have anything in the pipeline right now, so after resolving this you should be clear.

diondokter commented 6 months ago

Map is going to be annoying though because it uses a bit of recursion for its store operation. This can't be done with futures without alloc. So that has to be changed to a loop...

lulf commented 6 months ago

Map support is done now. Tested queue on nRF with nrf-softdevice and qspi flash.

diondokter commented 6 months ago

@lulf Thanks!

Took a quick look and looks good at first glance. Need to spend more time to do a thorough review.

I'm not really feeling the required input buffer alignment for the mock flash, though. The embedded-storage traits don't require it and if the nordic implementation does, then that's more strict than is allowed from the traits. I'm willing to make it work for it, for example by using the aligned buffer you made. But I don't want it to become annoying and having to use a new write_aligned function makes it more annoying.

Or are there more flash implementations that require the write buffer to be aligned?

lulf commented 6 months ago

@lulf Thanks!

Took a quick look and looks good at first glance. Need to spend more time to do a thorough review.

No rush on my part, thanks for taking the time.

I'm not really feeling the required input buffer alignment for the mock flash, though. The embedded-storage traits don't require it and if the nordic implementation does, then that's more strict than is allowed from the traits. I'm willing to make it work for it, for example by using the aligned buffer you made. But I don't want it to become annoying and having to use a new write_aligned function makes it more annoying.

We can remove the alignment requirement from the mock flash, it was merely added to trigger this condition in tests and in the fuzzer, otherwise it might sneak in a bug in the future and break nrf flash in the process, so I figured it would be useful for preventing a regression in the future. The write_aligned was added so that the unit test in lib.rs didn't have to do the alignment dance, I'm happy to move that into the test instead.

The important part is that the internal buffers in sequential-storage used to write things like item headers are aligned, otherwise it simply won't work with that nrf flash alignment.

Or are there more flash implementations that require the write buffer to be aligned?

No that I'm aware of, it's an nRF quirk, might even be specific to softdevice. It's not a lot to go by, but the closest source of information on this that I found is the description of the flash write error values: "NRF_ERROR_INVALID_ADDR Tried to write to a non existing flash address, or p_dst or p_src was unaligned." (From https://infocenter.nordicsemi.com/topic/com.nordic.infocenter.s140.api.v7.3.0/group___n_r_f___s_o_c___f_u_n_c_t_i_o_n_s.html#gae5361e65cbb5e7f6e258947a394c9b55)

lulf commented 6 months ago

@diondokter Thanks for the review! All comments should be addressed, except the bool one... I can change it to bool if you insist, but I really do think it hurts readability.

diondokter commented 6 months ago

Awesome, thanks for the work!