Skip to content
This repository has been archived by the owner on Oct 18, 2022. It is now read-only.

Adding support for the SPI stack chips #10

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

hargoniX
Copy link
Contributor

@hargoniX hargoniX commented Oct 3, 2019

Sadly I was only able to get the API for the user to this form

    let mut flash: Flash<W25N<_, _>, W25N<_, _>, _> = Flash::init(spi, cs).unwrap();

If rust would not explicitly require me to place the two type parameters for the W25N it obviously knows already there it would look a lot more beautiful but for some reason it does that

Copy link
Owner

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Nice work, this looks pretty good!

It would be nice to address the code duplication though. Also, please run cargo fmt on the code.

src/lib.rs Outdated Show resolved Hide resolved
#[allow(missing_debug_implementations)]
pub struct Die0;
#[allow(missing_debug_implementations)]
pub struct Die1;
Copy link
Owner

Choose a reason for hiding this comment

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

These can just #[derive(Debug)], there shouldn't be any harm in that

Copy link
Owner

Choose a reason for hiding this comment

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

You can also make them enums instead of structs since they're just used as type markers

src/w25m.rs Outdated
}

#[derive(Debug)]
pub struct Flash<DIE0, DIE1, DIE>
Copy link
Owner

Choose a reason for hiding this comment

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

Please add documentation to this type that explains what the type parameters are for

Copy link
Owner

Choose a reason for hiding this comment

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

DIE could also be bounded by an ActiveDie trait, which would be implemented by Die0 and Die1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, DIE is actually supposed to be a type state, it doesn`t have to fulfill any trait bounds it is just there so rust knows which switch_die implementation it is supposed to use.

Copy link
Owner

Choose a reason for hiding this comment

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

Sure. If you bound it by a trait that is only implemented by the 2 types that are supposed to be used though, users wouldn't be able to name invalid Flash types like Flash<Bla, Bla, ()>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean technically the user is just supposed to _ the type state as the constructor only produces Die0 types, so if a user starts to mess around with the state parameter he is most likely doing something wrong isn't he?

src/w25m.rs Show resolved Hide resolved
src/w25n.rs Outdated

// write to config register 2, set BUF=0 (continious mode) and everything else on reset
this.command(&mut [Opcode::WriteStatus as u8, 0xA0, 0b00000010])?;
this.command(&mut [Opcode::WriteStatus as u8, 0xB0, 0b00010000])?;
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, could this perhaps be integrated into the w25m Flash? Then we wouldn't have to duplicate the series25 module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we still have to duplicate it because of the method the w25n chips use to read and write based on their internal buffer which is a thing the series25 chips are not doing at the moment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants