Make spi-flash no-std compatible (still requires alloc)

[Sympatron]

Fixes #2

This PR adds no-std support.

PS:

• I tried to minimize reformatting, but I could not find rustfmt settings which resulted in the same formatting.
• To remove the dependency on alloc a major redesign would be necessary.
  If you are interested I would try to do it.

[adamgreig]

Thanks for this PR!

Is it possible for you to disable rustfmt instead? It seems like many of the changes are just formatting, but it's harder to review what's going on exactly and mixes up two things in one commit. Similarly with adding the dead_code lints; I would rather not include them in this PR.

Otherwise the changes overall seem OK, though it's a shame to have to redefine the errors twice for std/no-std. Hopefully soon the Error trait will be in core! I think it would be most useful if the crate could work without alloc at all, but I haven't given any thought to how it might look. I wanted to get this version working on std (since that's where I use it) so it was convenient to use Vec etc. Perhaps it could be a no_std core that provides the base functionality, and a std-using top-layer that provides more convenient methods when std is available? I'm not sure.

Possibly the FlashAccess trait should not return an anyhow Result at all, so it could be the same between std/no-std? It's only used for this trait (and so that our overall Error can return an Access error too); perhaps we could drop it as a dependency entirely. The new sleep method will definitely need documentation; perhaps it should be called "delay" or "wait" instead, or directly use milliseconds or microseconds instead of a Duration.

[Sympatron]

Is it possible for you to disable rustfmt instead? It seems like many of the changes are just formatting, but it's harder to review what's going on exactly and mixes up two things in one commit.

I will do that.

Similarly with adding the dead_code lints; I would rather not include them in this PR.

I added them because some things are not used anymore with no-std and Rust always emits a warning otherwise. Alternatively I could also disable them entirely for no-std.

it's a shame to have to redefine the errors twice for std/no-std.

This is unfortunate indeed, but I don't think it's too bad. It was the easiest solution I could come up with.

Perhaps it could be a no_std core that provides the base functionality, and a std-using top-layer that provides more convenient methods when std is available?

The FlashAccess trait would have to be changed. exchange would have to be changed to fn exchange(&mut self, data: &mut [u8]) -> AnyhowResult<()>, because embedded-hal's spi::Transfer trait only works that way.

Possibly the FlashAccess trait should not return an anyhow Result at all, so it could be the same between std/no-std?

I have not done any non-embedded stuff with Rust, so I have no idea how anyhow, thiserror, etc. work.

The new sleep method will definitely need documentation

I will add that.

perhaps it should be called "delay" or "wait" instead

You are right. I would go with delay

or directly use milliseconds or microseconds instead of a Duration

I left Duration there, because it is in core, but to be more compatible with the embedded ecosystem it would probably be better to use embedded-time for no-std instead.

[adamgreig]

Similarly with adding the dead_code lints; I would rather not include them in this PR.

I added them because some things are not used anymore with no-std and Rust always emits a warning otherwise. Alternatively I could also disable them entirely for no-std.

Oh, I missed that this was due to the no-std change. Disabling them for no-std seems clearer, then.

it's a shame to have to redefine the errors twice for std/no-std.

This is unfortunate indeed, but I don't think it's too bad. It was the easiest solution I could come up with.

I haven't tried it, but maybe we could use cfg_attr to only derive thiserror::Error and add the error attributes when building for std?

Perhaps it could be a no_std core that provides the base functionality, and a std-using top-layer that provides more convenient methods when std is available?

The FlashAccess trait would have to be changed. exchange would have to be changed to fn exchange(&mut self, data: &mut [u8]) -> AnyhowResult<()>, because embedded-hal's spi::Transfer trait only works that way.

Ah, that's true. That might also be something that can change with embedded-hal v1.0 (rust-embedded/embedded-hal#287) though.

Possibly the FlashAccess trait should not return an anyhow Result at all, so it could be the same between std/no-std?

I have not done any non-embedded stuff with Rust, so I have no idea how anyhow, thiserror, etc. work.

I'll think about this then, don't worry about it for now.

or directly use milliseconds or microseconds instead of a Duration

I left Duration there, because it is in core, but to be more compatible with the embedded ecosystem it would probably be better to use embedded-time for no-std instead.

I was thinking of embedded-hal's delay_us and delay_ms methods; perhaps we could just use one or both of those. I think all our delays would be >=1ms so just delay_ms is probably OK. We could still use Duration internally or swap to storing all the times in milliseconds.

All the other points sound good, thanks.

[Sympatron]

I tried to change it to delay_ms, but that was actually more involved than I thought. And it became clear that core::time::Duration is not what should be used in embedded anyway. So I would like to work on that and replace it with embedded-time in a separate PR.

[adamgreig]

What do you think of just using an integer milliseconds everywhere for times, instead of Duration or embedded-time?

[adamgreig]

If you're happy with this PR as-is I'm happy to merge it now, and make any other changes in later PRs.

[Sympatron]

What do you think of just using an integer milliseconds everywhere for times, instead of Duration or embedded-time?

I've looked into it and think it should be doable. What type should be used instead? u32?

[adamgreig]

u32 sounds fine.

[Sympatron]

Ok, but I still think I'll do it in a separate PR. So this one is good to go.