Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update to embedded-hal=1.0.0-alpha.11 #76

Closed

Conversation

rursprung
Copy link
Contributor

@rursprung rursprung commented Jul 5, 2023

see their changelog for further details.

src/spi.rs Outdated Show resolved Hide resolved
@rursprung rursprung force-pushed the embedded-hal-1.0.0-alpha.11 branch from 2457d3a to da84971 Compare July 5, 2023 14:24
@rursprung rursprung force-pushed the embedded-hal-1.0.0-alpha.11 branch 2 times, most recently from 8c42c04 to f6748d0 Compare July 6, 2023 09:01
@rursprung rursprung force-pushed the embedded-hal-1.0.0-alpha.11 branch from f6748d0 to de956ee Compare July 6, 2023 09:04
@rursprung rursprung marked this pull request as ready for review July 6, 2023 09:09
@rursprung
Copy link
Contributor Author

thanks all for your feedback! i've now added a new transaction type Delay for the mock and also added the test coverage for this. this is now ready for review.

Copy link
Collaborator

@newAM newAM left a comment

Choose a reason for hiding this comment

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

Works for me with my W5500 crate.

Copy link
Owner

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I'm not too familiar with SPI, so this is only a superficial review. However, I noticed a few issues.

Reviews by other users or contributors are welcome! Otherwise I'd merge this PR once the open issues are resolved.

//! assert_eq!(spi.read().unwrap(), 0xFF);
//! FullDuplex::write(&mut spi, 0x09).expect("TODO: panic message");
//! assert_eq!(FullDuplex::read(&mut spi).unwrap(), 0x0A);
//! FullDuplex::write(&mut spi, 0xFE).expect("TODO: panic message");
Copy link
Owner

Choose a reason for hiding this comment

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

Should these TODO messages be in here (i.e. are they TODOs for the reader)? Generally I'd just .unwrap() in example code.


/// spi::TransferInplace implementation for Mock
///
/// This writes the provided response to the buffer and will cause an assertion if the written data does not match the next expectation
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: The documentation is not correct anymore (the impl is for SpiBus, not TransferInplace). Also, it's a bit weird that there are docs only for one of the functions, not for the others.

If the documentation does not add much value, I'd remove it in this case for consistency reasons.

Operation::DelayUs(delay) => {
let w = self
.next()
.expect("no expectation for spi::transaction call");
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be spi::delay?

Operation::DelayUs(delay) => {
let w = self
.next()
.expect("no expectation for spi::transaction call");
Copy link
Owner

Choose a reason for hiding this comment

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

Again: This should probably be spi::delay?

@dbrgn
Copy link
Owner

dbrgn commented Jul 30, 2023

Additionally, the changes from #75 were merged into main, so: Do you have time to update your branch?

@newAM
Copy link
Collaborator

newAM commented Aug 1, 2023

@rursprung let me know if you want help with this :)

@newAM
Copy link
Collaborator

newAM commented Aug 1, 2023

I opened #85 with the feedback applied & rebased onto main. Feel free to use any part of that.

@rursprung
Copy link
Contributor Author

fully covered in #86 - thanks @newAM for picking this up while i was away!

@rursprung rursprung closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants