Storage Traits for on and off board storage #241

tl8roy · 2020-08-01T01:37:54Z

Closes #28
These are a set of traits that can be used for on and off board storage.

Each function is split into its own trait to deal with the different types of memory (Particularly Read Only vs Read/Write)

MCU Flash impl:
https://github.com/tl8roy/alt-stm32f30x-hal/blob/master/src/storage.rs

I2C EEPROM impl:
https://github.com/tl8roy/eeprom24x-rs/blob/master/src/eeprom24x.rs

SPI PSRAM implementation:
https://github.com/tl8roy/esp_psram
This is a completely new implementation using SPI.

Feedback has come from #28, diondokter/device-driver#1 and twitter https://twitter.com/tl8roy/status/1287254634512310272

rust-highfive · 2020-08-01T01:37:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eldruin (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ryankurte · 2020-08-01T06:25:21Z

hey thanks for the PR, this looks like a super useful abstraction! (and i needed it this week 😂)

i am not completely sure whether this is the right level to include in the embedded-hal or not, though it would be great to have a standard flash API for HAL implementations which is a serious point in favour. as it is a reasonably complex abstraction i suspect that there remains some room for experimentation (and some changes may occur during this), so i think my preference at this time would be to create a flash-hal crate to build out and evaluate this api prior to either adopting it into the WG or merging it into embedded-hal.

@rust-embedded/hal how do y'all feel like this, and is it worth a discussion at the next meeting about formalising the experimental-crate approach a bit more?

a few miscellaneous thoughts:

is there an advantage to an API for page / flash sizes rather than associated constants? i guess flash size might be runtime detected but page size is probably not?
how would you implement multiple erase methods (often devices support chip, page, half-page, and sector erase)?
is it important to communicate the erase state? in the comments you mention setting the memory to 0, however, this depends on the underlying technology and may impact the api consumer
is it expected that the flash-hal will fail when writing across some boundaries and thus this must be handled by the application, or that the driver will manage this? it is worth noting that this may not be a page boundary (many chips i have used can only write 255 bytes at a time as well as the standard page boundary limit). it seems to me the latter would be a neater abstraction for consumers?
is it worth having a mechanism for locking/unlocking memory as is required for writing to most internal flash or is that outside the scope of this abstraction? (and is it important to signal that writing to internal flash is going to stop the processor doing anything or is that a hal implementation concern).

tl8roy · 2020-08-01T06:30:31Z

hey thanks for the PR, this looks like a super useful abstraction! (and i needed it this week 😂)

i am not completely sure whether this is the right level to include in the embedded-hal or not, though it would be great to have a standard flash API for HAL implementations which is a serious point in favour. as it is a reasonably complex abstraction i suspect that there remains some room for experimentation (and some changes may occur during this), so i think my preference at this time would be to create a flash-hal crate to build out and evaluate this api prior to either adopting it into the WG or merging it into embedded-hal.

While the inspiration is that issue, the goal is absolutely a generic storage traits that aren't flash specific.

eldruin

Thank you for this PR.
I agree with @ryankurte. I would really like this merged but I also think it still needs a couple iterations. Whether that needs to be done in a separate crate I do not really care.
More thoughts:

Non-blocking
All methods offer a non-blocking interface. While that makes sense since storage devices take a while to complete, both implementations presented above are actually blocking.
It would be interesting to see how real non-blocking implementations look like.
Paging
The eeprom24x implementation just writes one byte at a time. It would be interesting to see how a more advanced implementation would look like, where actual arrays are written and how to deal with page boundaries, specially in the face of a non-blocking operation.
Storage size
I am also wondering if stateful fallible methods are necessary here or if this can be determined at compile time and associated constants would do just as well.
Anyway example implementations are necessary here.
Example implementation for eeprom24x
This is simple: I think it would be better to implement the storage traits in a separate file. I think this would be easier for other people to understand what the addition is without having to look at the diff.

eldruin · 2020-08-05T06:56:37Z

src/storage.rs

+/// Allows for Read Only Memory as well as Read and Write Memory
+use nb;
+
+/// Address represents an unsigned int. This allows for devices that have bigger or smaller address spaces than the host.


nitpick: I would rather refer to an "unsigned integer" (also below)

eldruin · 2020-08-05T07:05:02Z

src/storage.rs

+    type Error;
+
+    /// Erase the page of memory
+    /// For flash devices, this sets the whole page to 0xFF


I also think the reset value is probably better left to implementations. Anything written in the documentation of the trait would be part of a contract. (also below)

eldruin · 2020-08-05T07:09:03Z

src/storage.rs

+    type Error;
+
+    /// Returns the start address and the maximum size that can be stored by the device
+    fn try_total_size(&mut self) -> nb::Result<(Address<U>, AddressOffset<U>), Self::Error>;


I think two different methods may be better, since it would be fairly easy to make a mistake.

eldruin · 2020-08-05T07:13:40Z

src/storage.rs

+use core::ops::Add;
+
+/// Implement add for the Address and AddressOffset Types.
+impl<'a, 'b, U> Add<&'b AddressOffset<U>> for &'a Address<U>


Subtraction of an offset from an address is probably also interesting. And if we already at it, maybe addition and subtraction of address offsets. However I start wondering where to stop :) and then if it makes sense to include these implementations at all since the user already needs to deal with the inner values.

eldruin · 2020-08-05T07:23:29Z

src/storage.rs

+}
+
+/// Page represents an unsigned int that is a Page ID in the device memory space.
+pub struct Page<U>(U);


I am wondering about a conversion from Page to Address but I guess the best way would be adding a From implementation and that is something that needs to be done in the implementation and not nicely enforcible here.

tl8roy · 2020-08-05T09:33:05Z

I think the SPI flash chip library is non blocking. I'll have a crack at that.

tl8roy · 2020-08-22T00:25:39Z

What I have done so far:

Moved the EEPROM into its own file
Implemented a SPI SRAM IC driver as an example

What I am planning to do next:

Improve the quality of the STM32 flash example
Attempt to make it async as there appear to be no async SPI or I2C drivers
Failing that, revert the traits back to blocking only
Create an example of using all 3 crates, probably as a mini OTA example

MathiasKoch · 2020-09-08T12:18:17Z

I think it would make sense to make Address, AddressOffset and Page implement/derive a bunch of basic traits.
Of the top of my head it would make sense with something like Clone, Copy, Debug, PartialEq, PartialOrd?

Also i think it makes sense to be able to sub two addresses? very common way of getting a length?
and an Address % AddressOffset might also give value? I know the last couple here are easy to do myself, as the inner is public, but still applicable i think.

Sympatron · 2020-11-12T21:42:40Z

What do you think about starting with a new crate to get this going? I think something like embedded-storage would be fitting. That way people could start using it immediately.

After some time it could still be merging into embedded-hal.

MathiasKoch · 2020-11-13T07:36:57Z

@Sympatron I think that is the right approach! Currently i am forcing all of my crates into separate branches just to use a storage branch here, and it doesn't quite make sense.

Maybe it would make sense to get @eldruin to create it in embedded-rust-community? I think that would be a great fit!

tl8roy · 2020-11-13T07:45:47Z

I'm happy to support which ever solution works best for the community. I do have a bit of time this weekend.

eldruin · 2020-11-13T08:30:32Z

embedded-storage sounds good to me. As previously discussed, it is probable that this takes a while to get ready for embedded-hal. It should be noted as well that there are two proposals for storage traits: this one and also #248.
Maybe both can live within embedded-storage until it is settled which one is best or so.
Adding it to the rust-embedded-community might be possible. I am preparing an overhaul of the org's mission.

MathiasKoch · 2020-12-10T12:44:08Z

@tl8roy @eldruin

I have created this: https://github.com/BlackbirdHQ/embedded-storage in an attempt to bring this forward, knowingly that it does not address all our issues, but at least it is a starting point for discussions.. It is currently a 1:1 copy of #248

I would love to see it become part of the embedded-rust-community, but had to start it somewhere else..? @eldruin If you would be okay with it, i can transfer it?

eldruin · 2020-12-10T12:59:55Z

@MathiasKoch sure thing. It would be great if this triggers a development impulse like with embedded-nal. If you run into problems transferring it, let me know.

MathiasKoch · 2020-12-10T13:08:49Z

Awesome!

Let me introduce: https://github.com/rust-embedded-community/embedded-storage

@eldruin can you close #28 with a reference to the new repo?
Then i will close #248, leaving only this PR that @tl8roy could maybe move to embedded-storage for further discussion?

tl8roy · 2020-12-11T09:54:38Z

Now moved to rust-embedded-community/embedded-storage#8 so closing off for now.

tl8roy added 9 commits July 26, 2020 13:30

Added the first pass of the Storage Trait

8034bb6

Adds the Address Struct and Renames Clear

99ffba9

Removes an extra Word type

7d0f2f2

Adds a Page Struct and change erase to use page rather than address

f557817

Adds another function for erasing via an address

5eba046

Formatted the dicumentation to read better

04b5eef

Getting ready for a pull request

cbc5e6e

Makes the fields public

5d2a59b

Fixes a small typo in an example

ec1757b

tl8roy requested review from ilya-epifanov and a team as code owners August 1, 2020 01:37

rust-highfive assigned eldruin Aug 1, 2020

rust-highfive added S-waiting-on-review Review is incomplete T-hal labels Aug 1, 2020

Fixes the formatting

6da102b

eldruin reviewed Aug 5, 2020

View reviewed changes

Fixes a couple of issues raised in the first past

e115ed3

diondokter mentioned this pull request Aug 12, 2020

How should this crate work? diondokter/device-driver#1

Closed

MathiasKoch mentioned this pull request Aug 20, 2020

Persistence of outgoing packets in case of network loss (buffering) FactbirdHQ/mqttrust#9

Open

Fixes some formating issues and a requirement for some drivers

3c6b3ce

tl8roy added 2 commits August 23, 2020 11:20

Cargo Fmt fixes

181e795

Allow for non uniform page sizes

d6018bb

MathiasKoch mentioned this pull request Sep 18, 2020

Add storage ReadWrite trait, and helper structs, traits and iterators #248

Closed

MathiasKoch mentioned this pull request Nov 15, 2020

global_logger + printer for non-realtime logging? knurling-rs/defmt#251

Closed

MathiasKoch mentioned this pull request Dec 10, 2020

Use embedded-storage one available FactbirdHQ/rustot#10

Closed

tl8roy closed this Dec 11, 2020

eldruin mentioned this pull request Dec 11, 2020

Adds in my alternate storage traits rust-embedded-community/embedded-storage#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage Traits for on and off board storage #241

Storage Traits for on and off board storage #241

tl8roy commented Aug 1, 2020 •

edited

Loading

rust-highfive commented Aug 1, 2020

ryankurte commented Aug 1, 2020

tl8roy commented Aug 1, 2020

eldruin left a comment •

edited

Loading

eldruin Aug 5, 2020

eldruin Aug 5, 2020

eldruin Aug 5, 2020

eldruin Aug 5, 2020

eldruin Aug 5, 2020

tl8roy commented Aug 5, 2020

tl8roy commented Aug 22, 2020 •

edited

Loading

MathiasKoch commented Sep 8, 2020

Sympatron commented Nov 12, 2020

MathiasKoch commented Nov 13, 2020

tl8roy commented Nov 13, 2020

eldruin commented Nov 13, 2020

MathiasKoch commented Dec 10, 2020

eldruin commented Dec 10, 2020

MathiasKoch commented Dec 10, 2020

tl8roy commented Dec 11, 2020

Storage Traits for on and off board storage #241

Storage Traits for on and off board storage #241

Conversation

tl8roy commented Aug 1, 2020 • edited Loading

rust-highfive commented Aug 1, 2020

ryankurte commented Aug 1, 2020

tl8roy commented Aug 1, 2020

eldruin left a comment • edited Loading

Choose a reason for hiding this comment

eldruin Aug 5, 2020

Choose a reason for hiding this comment

eldruin Aug 5, 2020

Choose a reason for hiding this comment

eldruin Aug 5, 2020

Choose a reason for hiding this comment

eldruin Aug 5, 2020

Choose a reason for hiding this comment

eldruin Aug 5, 2020

Choose a reason for hiding this comment

tl8roy commented Aug 5, 2020

tl8roy commented Aug 22, 2020 • edited Loading

MathiasKoch commented Sep 8, 2020

Sympatron commented Nov 12, 2020

MathiasKoch commented Nov 13, 2020

tl8roy commented Nov 13, 2020

eldruin commented Nov 13, 2020

MathiasKoch commented Dec 10, 2020

eldruin commented Dec 10, 2020

MathiasKoch commented Dec 10, 2020

tl8roy commented Dec 11, 2020

tl8roy commented Aug 1, 2020 •

edited

Loading

eldruin left a comment •

edited

Loading

tl8roy commented Aug 22, 2020 •

edited

Loading