-
Notifications
You must be signed in to change notification settings - Fork 44
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 embedded-hal #94
Conversation
I tested this with hardware today on a Raspberry Pi 4 and a ADXL355 accelerometer connected over SPI with this code: let mut spi_dev = Spidev::open(SPI_PATH)?;
let options = SpidevOptions::new()
.bits_per_word(8)
.max_speed_hz(10_000_000)
.mode(SpiModeFlags::SPI_MODE_0)
.build();
spi_dev.configure(&options)?;
let mut buf_1 = [0u8; 1];
let mut buf_2 = [0u8; 1];
let mut operations = [
SpiOperation::DelayUs(1_000_000),
SpiOperation::Write(&[(0x2D << 1), 0x01]),
SpiOperation::DelayUs(500),
SpiOperation::Write(&[(0x2D << 1) | 0x01]),
SpiOperation::Read(&mut buf_1),
SpiOperation::DelayUs(500),
SpiOperation::Write(&[(0x2D << 1), 0x00]),
SpiOperation::DelayUs(500),
SpiOperation::Write(&[(0x2D << 1) | 0x01]),
SpiOperation::Read(&mut buf_2),
SpiOperation::DelayUs(5_000_000),
];
spi_dev.transaction(&mut operations)?;
println!("buf_1: {:X}", buf_1[0]);
println!("buf_2: {:X}", buf_2[0]); It worked as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work here.
Unfortunately, this approach is suboptimal in that it requires multiple transfers and flushes on the bus every time there is a delay, even though the underlying linux device supports delay transfers.
The optimal approach here would be to first extend spidev: rust-embedded/rust-spidev#40 to support delay operations and then updating this crate in line with what was outlined in: #87 (comment)
Sorry for the confusion and lack of more explicit guidance.
Thanks for the feedback! I agree with you. |
@@ -132,6 +96,9 @@ mod embedded_hal_impl { | |||
}; | |||
SpidevTransfer::read_write(tx, buf) | |||
} | |||
SpiOperation::DelayUs(us) => { | |||
SpidevTransfer::delay((*us).try_into().unwrap_or(u16::MAX)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to max out the delay to u16::MAX
but that may not be what we want here.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave at that here but please document in this method and in the Spidev
struct that delays are capped to 65535 microseconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@@ -132,6 +96,9 @@ mod embedded_hal_impl { | |||
}; | |||
SpidevTransfer::read_write(tx, buf) | |||
} | |||
SpiOperation::DelayUs(us) => { | |||
SpidevTransfer::delay((*us).try_into().unwrap_or(u16::MAX)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave at that here but please document in this method and in the Spidev
struct that delays are capped to 65535 microseconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
Looks good to me, thank you!
Ach, CI has to be migrated to GitHub merge queue for this to be merged. |
@LehMaxence could you rebase this to master now that CI is fixed? |
This allows using the delay transfer support.
03f2b54
to
847e93e
Compare
Rebased :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you!
Removed write-only and read-only spi implementations
Handled Delay operation between transfers