search

rust-embedded-community / embedded-storage

An Embedded Storage Abstraction Layer
Apache License 2.0
68 stars 23 forks source link

Enhancement: RmwNorFlash #14

Closed MathiasKoch closed 3 years ago

MathiasKoch commented 3 years ago

This PR implements RMW helpers for Nor flashes.

It was extracted from the previous PR #12, in order to land the traits and release them faster.

Sympatron commented 3 years ago

I like the approach. Especially that the merge_buffer does allow you to offload the merge process off the stack.

I am not a big fan of the overlaps() function, because it is not very readable / unclear what's going on at first. On the other hand it makes the implementation quite concise. So I have no strong opinion either way.

Currently every write out of bounds is just ignored. Returning an error might be better, but I think you would have to wrap the Storage's Error type to do that.

MathiasKoch commented 3 years ago

I am not a big fan of the overlaps() function, because it is not very readable / unclear what's going on at first. On the other hand it makes the implementation quite concise. So I have no strong opinion either way.

I have the exact same reservation, thus the remark here: https://github.com/rust-embedded-community/embedded-storage/blob/abcbd3df5bbfa32540eb0f0922c8994d688f32aa/src/iter.rs#L34

So anything cleaner or easier to read is more than welcomed!

MathiasKoch commented 3 years ago

Could we merge and release this PR as is? Or do any of you have any objections, nit-picks or similar? @Dirbaio @eldruin @Sympatron

Sympatron commented 3 years ago

I'd say we can. My main concern was that writes out of bounds are just silently ignored, but this could probably be fixed in a separate PR.