Twim: Implicitly copy buffer into RAM if needed when using embedded hal traits#165
Twim: Implicitly copy buffer into RAM if needed when using embedded hal traits#165jonas-schievink merged 7 commits intonrf-rs:masterfrom
Conversation
…mbedded hal trait
… using embedded hal traits
…already be in RAM, so no check neccessary
kalkyl
changed the title
|
Could you be so kind and run |
Yatekii
left a comment
Yatekii
left a comment
There was a problem hiding this comment.
Looks very good to me :) I added two questions about minor things.
| unsafe { w.maxcnt().bits(rx_buffer.len() as _) }); | ||
|
|
||
| // Chunk write data | ||
| let wr_buffer = &mut [0; FORCE_COPY_BUFFER_SIZE][..]; |
There was a problem hiding this comment.
| 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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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.
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.
Wont it just optimize that out and thus be zero cost?
| 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.
Doesn't it stop automatically when the requested data size was received?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
3 participants
Issue #150