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

Twim: Implicitly copy buffer into RAM if needed when using embedded hal traits #165

Merged
merged 7 commits into from
Jul 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added nrf-hal-common/,
Empty file.
4 changes: 4 additions & 0 deletions nrf-hal-common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ pub mod target_constants {
pub const SRAM_LOWER: usize = 0x2000_0000;
pub const SRAM_UPPER: usize = 0x3000_0000;
pub const FORCE_COPY_BUFFER_SIZE: usize = 255;
const _CHECK_FORCE_COPY_BUFFER_SIZE: usize = EASY_DMA_SIZE - FORCE_COPY_BUFFER_SIZE;
// ERROR: FORCE_COPY_BUFFER_SIZE must be <= EASY_DMA_SIZE
}
#[cfg(any(feature = "52840", feature = "52833", feature = "9160"))]
pub mod target_constants {
Expand All @@ -79,6 +81,8 @@ pub mod target_constants {
pub const SRAM_LOWER: usize = 0x2000_0000;
pub const SRAM_UPPER: usize = 0x3000_0000;
pub const FORCE_COPY_BUFFER_SIZE: usize = 1024;
const _CHECK_FORCE_COPY_BUFFER_SIZE: usize = EASY_DMA_SIZE - FORCE_COPY_BUFFER_SIZE;
// ERROR: FORCE_COPY_BUFFER_SIZE must be <= EASY_DMA_SIZE
}

/// Does this slice reside entirely within RAM?
Expand Down
144 changes: 137 additions & 7 deletions nrf-hal-common/src/twim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) });
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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][..];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let wr_buffer = &mut [0; FORCE_COPY_BUFFER_SIZE][..];
let wr_buffer = &mut [0; FORCE_COPY_BUFFER_SIZE.min(EASY_DMA_SIZE)][..];

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 define FORCE_COPY_BUFFER_SIZE even?

Copy link
Contributor Author

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 define FORCE_COPY_BUFFER_SIZE is the best solution, then we can just use FORCE_COPY_BUFFER_SIZE for our buffer and chunk sizes?

Copy link
Contributor

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.

Copy link
Contributor Author

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"?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yes true!

for chunk in tx_buffer.chunks(FORCE_COPY_BUFFER_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|
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it stop automatically when the requested data size was received?

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, according to the manual:

"Note that the TWI master does not stop by itself when the RAM buffer is full, or when an error occurs. The STOP task must be issued, through the use of a local or PPI shortcut, or in software as part of the error handler."

So i guess an alternative could be to add a PPI shortcut instead, like:
self.0.shorts.modify(|_r, w| w.lastrx_stop().enabled());

What do you prefer?
Thanks for your comments :)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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(())
}
}
}

Expand All @@ -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 {
Expand Down