Skip to content

Commit 6b1c234

Browse files
bors[bot]ia0
andauthored
Merge #374
374: Improve Nvmc implementation of embedded-storage traits r=jonas-schievink a=ia0 This PR opens a design discussion regarding the Nvmc API and implementation. It contains the following 3 commits: 1. Modify the example to surface some of the following issues: - Bug: [`new`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L28) doesn't check that the storage is well-aligned. - Bug: [`read`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L112) confuses offsets in the storage and offsets in the output buffer. - Bug: Many arithmetic overflows like [here](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L106) - Bug: [Off-by-one error](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L105) when the `read` output buffer is word-aligned. - Bug: [`read`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L112) reads big-endian words regardless of the architecture. - Bug: [`erase`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L155) doesn't check that the input is within bounds. - Bug: [`erase`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L158) divides by 4 twice (other time is in [`erase_page`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L71)). - Bug: [`write`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L169) doesn't check that the input is within bounds. - Bug: [`write`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L172) confuses offsets in the storage and offsets in the input buffer. - Bug: [`write`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L172) writes big-endian words regardless of the architecture. - Error-prone: `read` is very complicated and seems to have other bugs. - Not ideal: [`READ_SIZE`](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L100) is 4 instead of 1, which is unnecessarily constraining (at least for nRF52840). - Not ideal: The [storage slice](https://github.com/nrf-rs/nrf-hal/blob/bcd5c5da6e5087b7efd943806ec599d42b5e2c3f/nrf-hal-common/src/nvmc.rs#L19) has type `[u32]` instead of `[u8]`, which optimizes for writes instead of reads. 2. Fix most of the issues. This is a breaking change because it changes the `READ_SIZE` from 4 to 1 and adds `NvmcError::OutOfBounds`. But those changes seem needed. 3. Fix the remaining issues. This is a more controversial breaking change. It simplifies the implementation a bit more but is not needed (although it could improve the read performance significantly which is probably the most common operation). Co-authored-by: ia0 <[email protected]>
2 parents 1b504bf + b89cc3c commit 6b1c234

File tree

6 files changed

+186
-127
lines changed

6 files changed

+186
-127
lines changed

examples/nvmc-demo/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ cortex-m = "0.7.3"
1212
cortex-m-rt = "0.7.0"
1313
embedded-storage = "0.2.0"
1414
rtt-target = {version = "0.2.0", features = ["cortex-m"] }
15+
panic-probe = { version = "0.3.0", features = ["print-rtt"] }
1516

1617
[dependencies.embedded-hal]
1718
version = "0.2.3"

examples/nvmc-demo/memory.x

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
MEMORY
22
{
3-
/* NOTE 1 K = 1 KiBi = 1024 bytes */
4-
FLASH : ORIGIN = 0x00000000, LENGTH = 1020K
5-
CONFIG : ORIGIN = ORIGIN(FLASH) + LENGTH(FLASH), LENGTH = 4K /* 4K is the flash page size */
3+
/* NOTE 1 K = 1 KiB = 1024 bytes */
4+
FLASH : ORIGIN = 0x00000000, LENGTH = 1024K - NUM_PAGES * 4K
5+
CONFIG : ORIGIN = ORIGIN(FLASH) + LENGTH(FLASH), LENGTH = NUM_PAGES * 4K
66
RAM : ORIGIN = 0x20000000, LENGTH = 256K
77
}
88

9+
NUM_PAGES = 6;
910
_config = ORIGIN(CONFIG);
1011

1112
/* This is where the call stack will be allocated. */

examples/nvmc-demo/src/main.rs

Lines changed: 75 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,25 @@
66
#[cfg(feature = "52840")]
77
use nrf52840_hal as hal;
88

9+
use core::convert::TryInto;
910
use embedded_storage::nor_flash::NorFlash;
1011
use embedded_storage::nor_flash::ReadNorFlash;
1112
use hal::nvmc::Nvmc;
13+
use hal::pac::NVMC;
14+
use panic_probe as _;
1215
use rtt_target::{rprintln, rtt_init_print};
1316

14-
const CONFIG_SIZE: usize = 1024;
17+
const NUM_PAGES: u32 = 6;
18+
const PAGE_SIZE: u32 = 4 * 1024;
19+
const LAST_PAGE: u32 = (NUM_PAGES - 1) * PAGE_SIZE;
1520
extern "C" {
1621
#[link_name = "_config"]
17-
static mut CONFIG: [u32; CONFIG_SIZE];
22+
static mut CONFIG: [u8; (NUM_PAGES * PAGE_SIZE) as usize];
1823
}
1924

20-
// To run this: `cargo embed --features "52840" --target thumbv7em-none-eabihf`
25+
// To run this example:
26+
// cargo build --features=52840 --target=thumbv7em-none-eabi && \
27+
// probe-run --chip nRF52840_xxAA ../../target/thumbv7em-none-eabi/debug/nvmc-demo
2128

2229
#[cortex_m_rt::entry]
2330
fn main() -> ! {
@@ -28,23 +35,77 @@ fn main() -> ! {
2835
#[cfg(feature = "52840")]
2936
let mut nvmc = Nvmc::new(p.NVMC, unsafe { &mut CONFIG });
3037

31-
assert!(nvmc.erase(0, CONFIG_SIZE as u32 * 4).is_ok());
32-
let write_buf: [u8; 4] = [1, 2, 3, 4];
33-
assert!(nvmc.write(0, &write_buf).is_ok());
34-
let mut read_buf = [0u8; 2];
35-
assert!(nvmc.read(0, &mut read_buf).is_ok());
36-
assert_eq!(read_buf, write_buf[0..2]);
38+
erase_if_needed(&mut nvmc, LAST_PAGE);
3739

38-
rprintln!("What was written to flash was read!");
40+
let mut write_buf: [u8; 32] = [0; 32];
41+
for (i, x) in write_buf.iter_mut().enumerate() {
42+
*x = i as u8;
43+
}
44+
rprintln!("Writing at {:#x}: {:02x?}", LAST_PAGE, write_buf);
45+
nvmc.write(LAST_PAGE, &write_buf).unwrap();
46+
47+
for i in 0..4 {
48+
compare_read::<11>(&mut nvmc, LAST_PAGE + i);
49+
compare_read::<10>(&mut nvmc, LAST_PAGE + i);
50+
compare_read::<9>(&mut nvmc, LAST_PAGE + i + 4);
51+
compare_read::<8>(&mut nvmc, LAST_PAGE + i + 4);
52+
compare_read::<7>(&mut nvmc, LAST_PAGE + i + 8);
53+
compare_read::<6>(&mut nvmc, LAST_PAGE + i + 8);
54+
compare_read::<5>(&mut nvmc, LAST_PAGE + i + 16);
55+
compare_read::<4>(&mut nvmc, LAST_PAGE + i + 16);
56+
compare_read::<3>(&mut nvmc, LAST_PAGE + i + 20);
57+
compare_read::<2>(&mut nvmc, LAST_PAGE + i + 20);
58+
compare_read::<1>(&mut nvmc, LAST_PAGE + i + 24);
59+
}
60+
61+
erase_if_needed(&mut nvmc, LAST_PAGE);
3962

4063
loop {
4164
cortex_m::asm::wfe();
4265
}
4366
}
4467

45-
#[panic_handler] // panicking behavior
46-
fn panic(_: &core::panic::PanicInfo) -> ! {
47-
loop {
48-
cortex_m::asm::bkpt();
68+
fn compare_read<const LENGTH: usize>(nvmc: &mut Nvmc<NVMC>, offset: u32) {
69+
let actual = read::<LENGTH>(nvmc, offset);
70+
let expected = unsafe { direct_read::<LENGTH>(offset) };
71+
if actual == expected {
72+
rprintln!("Read at {:#x}: {:02x?} as expected", offset, actual);
73+
} else {
74+
rprintln!(
75+
"Error: Read at {:#x}: {:02x?} instead of {:02x?}",
76+
offset,
77+
actual,
78+
expected,
79+
);
4980
}
5081
}
82+
83+
fn read<const LENGTH: usize>(nvmc: &mut Nvmc<NVMC>, offset: u32) -> [u8; LENGTH] {
84+
let mut buf = [0; LENGTH];
85+
nvmc.read(offset, &mut buf).unwrap();
86+
buf
87+
}
88+
89+
unsafe fn direct_read<const LENGTH: usize>(offset: u32) -> [u8; LENGTH] {
90+
CONFIG[offset as usize..][..LENGTH].try_into().unwrap()
91+
}
92+
93+
fn erase_if_needed(nvmc: &mut Nvmc<NVMC>, offset: u32) {
94+
let mut page = [0; PAGE_SIZE as usize];
95+
nvmc.read(offset, &mut page).unwrap();
96+
if page_is_erased(&page) {
97+
return;
98+
}
99+
rprintln!("Erasing at {:#x}", offset);
100+
nvmc.erase(offset, offset + PAGE_SIZE).unwrap();
101+
nvmc.read(offset, &mut page).unwrap();
102+
if page_is_erased(&page) {
103+
rprintln!("The page was correctly erased.");
104+
} else {
105+
rprintln!("Error: The page was not correctly erased.");
106+
}
107+
}
108+
109+
fn page_is_erased(page: &[u8; PAGE_SIZE as usize]) -> bool {
110+
page.iter().all(|&x| x == 0xff)
111+
}

nrf-hal-common/src/nvmc.rs

Lines changed: 62 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -11,25 +11,34 @@ use crate::pac::NVMC;
1111
#[cfg(any(feature = "9160", feature = "5340-app"))]
1212
use crate::pac::NVMC_NS as NVMC;
1313

14+
use core::convert::TryInto;
1415
use embedded_storage::nor_flash::{NorFlash, ReadNorFlash};
1516

17+
type WORD = u32;
18+
const WORD_SIZE: usize = core::mem::size_of::<WORD>();
19+
const PAGE_SIZE: usize = 4 * 1024;
20+
1621
/// Interface to an NVMC instance.
1722
pub struct Nvmc<T: Instance> {
1823
nvmc: T,
19-
storage: &'static mut [u32],
24+
storage: &'static mut [u8],
2025
}
2126

2227
impl<T> Nvmc<T>
2328
where
2429
T: Instance,
2530
{
2631
/// Takes ownership of the peripheral and storage area.
27-
pub fn new(nvmc: T, storage: &'static mut [u32]) -> Nvmc<T> {
32+
///
33+
/// The storage area must be page-aligned.
34+
pub fn new(nvmc: T, storage: &'static mut [u8]) -> Nvmc<T> {
35+
assert!(storage.as_ptr() as usize % PAGE_SIZE == 0);
36+
assert!(storage.len() % PAGE_SIZE == 0);
2837
Self { nvmc, storage }
2938
}
3039

3140
/// Consumes `self` and returns back the raw peripheral and associated storage.
32-
pub fn free(self) -> (T, &'static mut [u32]) {
41+
pub fn free(self) -> (T, &'static mut [u8]) {
3342
(self.nvmc, self.storage)
3443
}
3544

@@ -67,28 +76,36 @@ where
6776

6877
#[cfg(not(any(feature = "9160", feature = "5340-app")))]
6978
#[inline]
70-
fn erase_page(&mut self, offset: usize) {
71-
let bits = &mut (self.storage[offset as usize >> 2]) as *mut _ as u32;
79+
fn erase_page(&mut self, page_offset: usize) {
80+
let bits = &mut (self.storage[page_offset * PAGE_SIZE]) as *mut _ as u32;
7281
self.nvmc.erasepage().write(|w| unsafe { w.bits(bits) });
7382
self.wait_ready();
7483
}
7584

7685
#[cfg(any(feature = "9160", feature = "5340-app"))]
7786
#[inline]
78-
fn erase_page(&mut self, offset: usize) {
79-
self.storage[offset as usize >> 2] = 0xffffffff;
87+
fn erase_page(&mut self, page_offset: usize) {
88+
self.direct_write_word(page_offset * PAGE_SIZE, 0xffffffff);
8089
self.wait_ready();
8190
}
8291

8392
#[inline]
84-
fn write_word(&mut self, offset: usize, word: u32) {
93+
fn write_word(&mut self, word_offset: usize, word: u32) {
8594
#[cfg(not(any(feature = "9160", feature = "5340-app")))]
8695
self.wait_ready();
8796
#[cfg(any(feature = "9160", feature = "5340-app"))]
8897
self.wait_write_ready();
89-
self.storage[offset] = word;
98+
self.direct_write_word(word_offset, word);
9099
cortex_m::asm::dmb();
91100
}
101+
102+
#[inline]
103+
fn direct_write_word(&mut self, word_offset: usize, word: u32) {
104+
let target: &mut [u8] = &mut self.storage[word_offset * WORD_SIZE..][..WORD_SIZE];
105+
let target: &mut [u8; WORD_SIZE] = target.try_into().unwrap();
106+
let target: &mut u32 = unsafe { core::mem::transmute(target) };
107+
*target = word;
108+
}
92109
}
93110

94111
impl<T> ReadNorFlash for Nvmc<T>
@@ -97,89 +114,63 @@ where
97114
{
98115
type Error = NvmcError;
99116

100-
const READ_SIZE: usize = 4;
117+
const READ_SIZE: usize = 1;
101118

102119
fn read(&mut self, offset: u32, bytes: &mut [u8]) -> Result<(), Self::Error> {
103120
let offset = offset as usize;
104-
let bytes_len = bytes.len();
105-
let read_len = bytes_len + (Self::READ_SIZE - (bytes_len % Self::READ_SIZE));
106-
let target_offset = offset + read_len;
107-
if offset % Self::READ_SIZE == 0 && target_offset <= self.capacity() {
108-
self.wait_ready();
109-
let last_offset = target_offset - Self::READ_SIZE;
110-
for offset in (offset..last_offset).step_by(Self::READ_SIZE) {
111-
let word = self.storage[offset >> 2];
112-
bytes[offset] = (word >> 24) as u8;
113-
bytes[offset + 1] = (word >> 16) as u8;
114-
bytes[offset + 2] = (word >> 8) as u8;
115-
bytes[offset + 3] = (word >> 0) as u8;
116-
}
117-
let offset = last_offset;
118-
let word = self.storage[offset >> 2];
119-
let mut bytes_offset = offset;
120-
if bytes_offset < bytes_len {
121-
bytes[bytes_offset] = (word >> 24) as u8;
122-
bytes_offset += 1;
123-
if bytes_offset < bytes_len {
124-
bytes[bytes_offset] = (word >> 16) as u8;
125-
bytes_offset += 1;
126-
if bytes_offset < bytes_len {
127-
bytes[bytes_offset] = (word >> 8) as u8;
128-
bytes_offset += 1;
129-
if bytes_offset < bytes_len {
130-
bytes[bytes_offset] = (word >> 0) as u8;
131-
}
132-
}
133-
}
134-
}
135-
Ok(())
136-
} else {
137-
Err(NvmcError::Unaligned)
121+
if bytes.len() > self.capacity() || offset > self.capacity() - bytes.len() {
122+
return Err(NvmcError::OutOfBounds);
138123
}
124+
self.wait_ready();
125+
bytes.copy_from_slice(&self.storage[offset..][..bytes.len()]);
126+
Ok(())
139127
}
140128

141129
fn capacity(&self) -> usize {
142-
self.storage.len() << 2
130+
self.storage.len()
143131
}
144132
}
145133

146134
impl<T> NorFlash for Nvmc<T>
147135
where
148136
T: Instance,
149137
{
150-
const WRITE_SIZE: usize = 4;
138+
const WRITE_SIZE: usize = WORD_SIZE;
151139

152-
const ERASE_SIZE: usize = 4 * 1024;
140+
const ERASE_SIZE: usize = PAGE_SIZE;
153141

154142
fn erase(&mut self, from: u32, to: u32) -> Result<(), Self::Error> {
155-
if from as usize % Self::ERASE_SIZE == 0 && to as usize % Self::ERASE_SIZE == 0 {
156-
self.enable_erase();
157-
for offset in (from..to).step_by(Self::ERASE_SIZE) {
158-
self.erase_page(offset as usize >> 2);
159-
}
160-
self.enable_read();
161-
Ok(())
162-
} else {
163-
Err(NvmcError::Unaligned)
143+
let (from, to) = (from as usize, to as usize);
144+
if from > to || to > self.capacity() {
145+
return Err(NvmcError::OutOfBounds);
164146
}
147+
if from % PAGE_SIZE != 0 || to % PAGE_SIZE != 0 {
148+
return Err(NvmcError::Unaligned);
149+
}
150+
let (page_from, page_to) = (from / PAGE_SIZE, to / PAGE_SIZE);
151+
self.enable_erase();
152+
for page_offset in page_from..page_to {
153+
self.erase_page(page_offset);
154+
}
155+
self.enable_read();
156+
Ok(())
165157
}
166158

167159
fn write(&mut self, offset: u32, bytes: &[u8]) -> Result<(), Self::Error> {
168160
let offset = offset as usize;
169-
if offset % Self::WRITE_SIZE == 0 && bytes.len() % Self::WRITE_SIZE == 0 {
170-
self.enable_write();
171-
for offset in (offset..(offset + bytes.len())).step_by(Self::WRITE_SIZE) {
172-
let word = ((bytes[offset] as u32) << 24)
173-
| ((bytes[offset + 1] as u32) << 16)
174-
| ((bytes[offset + 2] as u32) << 8)
175-
| ((bytes[offset + 3] as u32) << 0);
176-
self.write_word(offset >> 2, word);
177-
}
178-
self.enable_read();
179-
Ok(())
180-
} else {
181-
Err(NvmcError::Unaligned)
161+
if bytes.len() > self.capacity() || offset as usize > self.capacity() - bytes.len() {
162+
return Err(NvmcError::OutOfBounds);
163+
}
164+
if offset % WORD_SIZE != 0 || bytes.len() % WORD_SIZE != 0 {
165+
return Err(NvmcError::Unaligned);
166+
}
167+
let word_offset = offset / WORD_SIZE;
168+
self.enable_write();
169+
for (word_offset, bytes) in (word_offset..).zip(bytes.chunks_exact(WORD_SIZE)) {
170+
self.write_word(word_offset, u32::from_ne_bytes(bytes.try_into().unwrap()));
182171
}
172+
self.enable_read();
173+
Ok(())
183174
}
184175
}
185176

@@ -214,4 +205,6 @@ mod sealed {
214205
pub enum NvmcError {
215206
/// An operation was attempted on an unaligned boundary
216207
Unaligned,
208+
/// An operation was attempted outside the boundaries
209+
OutOfBounds,
217210
}

nrf52840-hal-tests/memory.x

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
MEMORY
22
{
3-
/* NOTE 1 K = 1 KiBi = 1024 bytes */
4-
FLASH : ORIGIN = 0x00000000, LENGTH = 1020K
5-
CONFIG : ORIGIN = ORIGIN(FLASH) + LENGTH(FLASH), LENGTH = 4K /* 4K is the flash page size */
3+
/* NOTE 1 K = 1 KiB = 1024 bytes */
4+
FLASH : ORIGIN = 0x00000000, LENGTH = 1024K - NUM_PAGES * 4K
5+
CONFIG : ORIGIN = ORIGIN(FLASH) + LENGTH(FLASH), LENGTH = NUM_PAGES * 4K
66
RAM : ORIGIN = 0x20000000, LENGTH = 256K
77
}
88

9+
NUM_PAGES = 6;
910
_config = ORIGIN(CONFIG);
1011

1112
/* This is where the call stack will be allocated. */

0 commit comments

Comments
 (0)