-
Notifications
You must be signed in to change notification settings - Fork 139
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
Twim: Implicitly copy buffer into RAM if needed when using embedded hal traits #165
Changes from 6 commits
1afbb8e
aad5fb9
4c935d0
287ea22
4f64fc2
45d9201
edc316d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,8 +18,8 @@ use crate::target::TWIM1; | |
|
||
use crate::{ | ||
gpio::{Floating, Input, Pin}, | ||
slice_in_ram_or, | ||
target_constants::EASY_DMA_SIZE, | ||
slice_in_ram, slice_in_ram_or, | ||
target_constants::{EASY_DMA_SIZE, FORCE_COPY_BUFFER_SIZE}, | ||
}; | ||
|
||
pub use twim0::frequency::FREQUENCY_A as Frequency; | ||
|
@@ -133,7 +133,7 @@ where | |
while self.0.events_lasttx.read().bits() == 0 {} | ||
self.0.events_lasttx.write(|w| w); // reset event | ||
|
||
// Stop read operation | ||
// Stop write operation | ||
self.0.tasks_stop.write(|w| | ||
// `1` is a valid value to write to task registers. | ||
unsafe { w.bits(1) }); | ||
|
@@ -229,7 +229,8 @@ where | |
/// Write data to an I2C slave, then read data from the slave without | ||
/// triggering a stop condition between the two | ||
/// | ||
/// The buffer must have a length of at most 255 bytes. | ||
/// The buffers must have a length of at most 255 bytes on the nRF52832 | ||
/// and at most 65535 bytes on the nRF52840. | ||
pub fn write_then_read( | ||
&mut self, | ||
address: u8, | ||
|
@@ -333,6 +334,122 @@ where | |
Ok(()) | ||
} | ||
|
||
/// Copy data into RAM and write to an I2C slave, then read data from the slave without | ||
/// triggering a stop condition between the two | ||
/// | ||
/// The read buffer must have a length of at most 255 bytes on the nRF52832 | ||
/// and at most 65535 bytes on the nRF52840. | ||
pub fn copy_write_then_read( | ||
&mut self, | ||
address: u8, | ||
tx_buffer: &[u8], | ||
rx_buffer: &mut [u8], | ||
) -> Result<(), Error> { | ||
if rx_buffer.len() > EASY_DMA_SIZE { | ||
return Err(Error::RxBufferTooLong); | ||
} | ||
|
||
// Conservative compiler fence to prevent optimizations that do not | ||
// take in to account actions by DMA. The fence has been placed here, | ||
// before any DMA action has started | ||
compiler_fence(SeqCst); | ||
|
||
self.0 | ||
.address | ||
.write(|w| unsafe { w.address().bits(address) }); | ||
|
||
// Set up the DMA read | ||
self.0.rxd.ptr.write(|w| | ||
// We're giving the register a pointer to the stack. Since we're | ||
// waiting for the I2C transaction to end before this stack pointer | ||
// becomes invalid, there's nothing wrong here. | ||
// | ||
// The PTR field is a full 32 bits wide and accepts the full range | ||
// of values. | ||
unsafe { w.ptr().bits(rx_buffer.as_mut_ptr() as u32) }); | ||
self.0.rxd.maxcnt.write(|w| | ||
// We're giving it the length of the buffer, so no danger of | ||
// accessing invalid memory. We have verified that the length of the | ||
// buffer fits in an `u8`, so the cast to the type of maxcnt | ||
// is also fine. | ||
// | ||
// Note that that nrf52840 maxcnt is a wider | ||
// type than a u8, so we use a `_` cast rather than a `u8` cast. | ||
// The MAXCNT field is thus at least 8 bits wide and accepts the | ||
// full range of values that fit in a `u8`. | ||
unsafe { w.maxcnt().bits(rx_buffer.len() as _) }); | ||
|
||
// Chunk write data | ||
let wr_buffer = &mut [0; FORCE_COPY_BUFFER_SIZE][..]; | ||
for chunk in tx_buffer.chunks(FORCE_COPY_BUFFER_SIZE.min(EASY_DMA_SIZE)) { | ||
// Copy chunk into RAM | ||
wr_buffer[..chunk.len()].copy_from_slice(chunk); | ||
|
||
// Set up the DMA write | ||
self.0.txd.ptr.write(|w| | ||
// We're giving the register a pointer to the stack. Since we're | ||
// waiting for the I2C transaction to end before this stack pointer | ||
// becomes invalid, there's nothing wrong here. | ||
// | ||
// The PTR field is a full 32 bits wide and accepts the full range | ||
// of values. | ||
unsafe { w.ptr().bits(wr_buffer.as_ptr() as u32) }); | ||
|
||
self.0.txd.maxcnt.write(|w| | ||
// We're giving it the length of the buffer, so no danger of | ||
// accessing invalid memory. We have verified that the length of the | ||
// buffer fits in an `u8`, so the cast to `u8` is also fine. | ||
// | ||
// The MAXCNT field is 8 bits wide and accepts the full range of | ||
// values. | ||
unsafe { w.maxcnt().bits(wr_buffer.len() as _) }); | ||
|
||
// Start write operation | ||
self.0.tasks_starttx.write(|w| | ||
// `1` is a valid value to write to task registers. | ||
unsafe { w.bits(1) }); | ||
|
||
// Wait until write operation is about to end | ||
while self.0.events_lasttx.read().bits() == 0 {} | ||
self.0.events_lasttx.write(|w| w); // reset event | ||
|
||
// Check for bad writes | ||
if self.0.txd.amount.read().bits() != wr_buffer.len() as u32 { | ||
return Err(Error::Transmit); | ||
} | ||
} | ||
|
||
// Start read operation | ||
self.0.tasks_startrx.write(|w| | ||
// `1` is a valid value to write to task registers. | ||
unsafe { w.bits(1) }); | ||
|
||
// Wait until read operation is about to end | ||
while self.0.events_lastrx.read().bits() == 0 {} | ||
self.0.events_lastrx.write(|w| w); // reset event | ||
|
||
// Stop read operation | ||
self.0.tasks_stop.write(|w| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't it stop automatically when the requested data size was received? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, according to the manual:
So i guess an alternative could be to add a PPI shortcut instead, like: What do you prefer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, this seems odd to me tbh, but I guess if the manual states this, it must be true :) I think the short is cleaner but it also occupies a short slot which could be used by the user for another job. So I think we should leave it as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool! :) |
||
// `1` is a valid value to write to task registers. | ||
unsafe { w.bits(1) }); | ||
|
||
// Wait until total operation has ended | ||
while self.0.events_stopped.read().bits() == 0 {} | ||
self.0.events_stopped.write(|w| w); // reset event | ||
|
||
// Conservative compiler fence to prevent optimizations that do not | ||
// take in to account actions by DMA. The fence has been placed here, | ||
// after all possible DMA actions have completed | ||
compiler_fence(SeqCst); | ||
|
||
// Check for bad reads | ||
if self.0.rxd.amount.read().bits() != rx_buffer.len() as u32 { | ||
return Err(Error::Receive); | ||
} | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Return the raw interface to the underlying TWIM peripheral | ||
pub fn free(self) -> T { | ||
self.0 | ||
|
@@ -348,7 +465,16 @@ where | |
type Error = Error; | ||
|
||
fn write<'w>(&mut self, addr: u8, bytes: &'w [u8]) -> Result<(), Error> { | ||
self.write(addr, bytes) | ||
if slice_in_ram(bytes) { | ||
self.write(addr, bytes) | ||
} else { | ||
let buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..]; | ||
for chunk in bytes.chunks(FORCE_COPY_BUFFER_SIZE) { | ||
buf[..chunk.len()].copy_from_slice(chunk); | ||
self.write(addr, &buf[..chunk.len()])?; | ||
} | ||
Ok(()) | ||
} | ||
} | ||
} | ||
|
||
|
@@ -375,11 +501,15 @@ where | |
bytes: &'w [u8], | ||
buffer: &'w mut [u8], | ||
) -> Result<(), Error> { | ||
self.write_then_read(addr, bytes, buffer) | ||
if slice_in_ram(bytes) { | ||
self.write_then_read(addr, bytes, buffer) | ||
} else { | ||
self.copy_write_then_read(addr, bytes, buffer) | ||
} | ||
} | ||
} | ||
|
||
/// The pins used by the TWIN peripheral | ||
/// The pins used by the TWIM peripheral | ||
/// | ||
/// Currently, only P0 pins are supported. | ||
pub struct Pins { | ||
|
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 should take the minimum here too? Or ensure elseplace that
FORCE_COPY_BUFFER_SIZE
is always <=EASY_DMA_SIZE
. Maybe we should do that where we defineFORCE_COPY_BUFFER_SIZE
even?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.
Yes, i don't know how to do that min check in const though?
I guess ensuring
FORCE_COPY_BUFFER_SIZE <= EASY_DMA_SIZE
where we defineFORCE_COPY_BUFFER_SIZE
is the best solution, then we can just useFORCE_COPY_BUFFER_SIZE
for our buffer and chunk sizes?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.
Ok, I think we should use .min() at both places or at no place in this code then :) Otherwise it's just confusing I 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.
Totally agree, so i will use
FORCE_COPY_BUFFER_SIZE
in both places then :)How can i assert
FORCE_COPY_BUFFER_SIZE <= EASY_DMA_SIZE
when they are defined in "top-level"?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 am not sure we can ensure this at compile time other than manually making sure of this and maybe add a comment.
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.
The only hacky way i can think of right now is not pretty and not zero cost, something like:
const _CHECK_FORCE_COPY_BUFFER_SIZE: usize = EASY_DMA_SIZE - FORCE_COPY_BUFFER_SIZE;
// ERROR: FORCE_COPY_BUFFER_SIZE must be <= EASY_DMA_SIZE
..that would fail with "attempt to subtract with overflow" :)
But i guess just a comment should be enough right?
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.
Wont it just optimize that out and thus be zero cost?
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.
Ahh yes true!