renet
/
ENC424J600
10
1
Fork 1
Code Issues 3 Pull Requests 1 Projects Releases Wiki Activity

Remove cortex-m dependencies for delay #2

Merged
sb10q merged 8 commits from occheung/ENC424J600:generalize_delay into master 2021-01-25 12:35:23 +08:00
Conversation 11 Commits 8 Files Changed 4
occheung commented 2021-01-22 13:03:12 +08:00

Replaced cortex-m specific delay with DelayUs from embedded-hal.

Replaced cortex-m specific delay with `DelayUs` from `embedded-hal`.
occheung added 4 commits 2021-01-22 13:03:13 +08:00
harry commented 2021-01-22 13:56:41 +08:00

What is the significance of b8094f84f3 ?

What is the significance of b8094f84f37d01454ec3c92f5ae4a2329c65d375 ?
occheung commented 2021-01-22 14:00:30 +08:00
Poster

What is the significance of b8094f84f3 ?

revert the accidental change of example in eeeb162cc5

> What is the significance of b8094f84f37d01454ec3c92f5ae4a2329c65d375 ? revert the accidental change of example in eeeb162cc5
harry commented 2021-01-22 14:08:36 +08:00

😄 OK. What about eeeb162cc5 that uses asm_delay and hal_delay at different places? Is there an issue with using the same kind of timer for delaying for different purposes, e.g. setting SPI high/low, and delay 25us after writing ETHRST?

😄 OK. What about eeeb162cc544670d4a339575f7f08999e56ae844 that uses `asm_delay` and `hal_delay` at different places? Is there an issue with using the same kind of timer for delaying for different purposes, e.g. setting SPI high/low, and delay 25us after writing ETHRST?
occheung commented 2021-01-22 14:26:44 +08:00
Poster

SpiPort needs to own a delay.

The issue of using the delay from hal is that one of its component (SYST) is pretty much a singleton. If SpiPort takes away the delay, then delay in the main program cannot be implemented using Delay in hal.
The delay() in cortex-m is totally reusable, as long as we know the sysclk frequency.

We should probably just not use the Delay from hal at all.

`SpiPort` needs to own a delay. The issue of using the delay from hal is that one of its component (`SYST`) is pretty much a singleton. If `SpiPort` takes away the delay, then delay in the main program cannot be implemented using `Delay` in hal. The `delay()` in `cortex-m` is totally reusable, as long as we know the `sysclk` frequency. We should probably just not use the `Delay` from hal at all.
occheung added 1 commit 2021-01-22 14:36:11 +08:00
sb10q reviewed 2021-01-22 15:02:53 +08:00
src/spi.rs Outdated
@ -132,3 +140,3 @@
Ok(_) => {
// Disable chip select
cortex_m::asm::delay(10_u32);
self.delay_us(1);
sb10q commented 2021-01-22 15:02:53 +08:00
Owner

Wouldn't that significantly increase the delay, and noticeably slow things down?

Wouldn't that significantly increase the delay, and noticeably slow things down?
occheung commented 2021-01-22 15:12:05 +08:00
Poster

This delay gets called when modifying register.

Sending/Receiving packets do require read/write to register. But most of the SPI transcation should be to the SRAM buffer, given a large enough packet is sent.

This delay gets called when modifying register. Sending/Receiving packets do require read/write to register. But most of the SPI transcation should be to the SRAM buffer, given a large enough packet is sent.
harry commented 2021-04-21 11:35:55 +08:00

When Dip comes back, I'd like to know the actual reason behind this delay between CS assertion/dessertion and the actual SPI transfer. Must the delay be in nanoseconds?

When Dip comes back, I'd like to know the actual reason behind this delay between CS assertion/dessertion and the actual SPI transfer. Must the delay be in nanoseconds?
occheung commented 2021-04-21 12:56:05 +08:00
Poster

The set of delay is to comply with the SPI specification of ENC424J600 mentioned in the datasheet (p.148). However, other functions that involve SPI transaction does not have such delay. The delay could be unnecessary.

(Note: However, if the delay is removed and the code is optimized, the CS could possibly be too short for 16-bits read/write functions, as the 8-bits R/W is called back-to-back. I can only faintly recall this potential issue, please verify if that is an issue at all.)

Anyway, these delay is only called after a complete SPI transaction. It can be extended to 1 microsecond. This delay is only called a fixed number of times per packet sent.

The set of delay is to comply with the SPI specification of ENC424J600 mentioned in the [datasheet](http://ww1.microchip.com/downloads/en/devicedoc/39935b.pdf) (p.148). However, other functions that involve SPI transaction does not have such delay. The delay could be unnecessary. (Note: However, if the delay is removed and the code is optimized, the CS *could possibly* be too short for 16-bits read/write functions, as the 8-bits R/W is called back-to-back. I can only faintly recall this potential issue, please verify if that is an issue at all.) Anyway, these delay is only called after a complete SPI transaction. It can be extended to 1 microsecond. This delay is only called a fixed number of times per packet sent.
harry commented 2021-04-21 17:16:51 +08:00

Agh! Finally found someone else complaining about a similar situation as we do: https://github.com/rust-embedded/embedded-hal/issues/264

I'm going to read through it and see what steps we can take.

@occheung Thanks for typing the feedback 😃

Agh! Finally found someone else complaining about a similar situation as we do: https://github.com/rust-embedded/embedded-hal/issues/264 I'm going to read through it and see what steps we can take. @occheung Thanks for typing the feedback 😃
sb10q reviewed 2021-01-22 15:06:13 +08:00
examples/delay.rs Outdated
@ -0,0 +6,4 @@
frequency_ms: u32,
}
impl AsmDelay {
sb10q commented 2021-01-22 15:06:13 +08:00
Owner

Do we really need to reinvent this? I would have thought there's already a stm32 crate that implements these embedded-hal delays.

Do we really need to reinvent this? I would have thought there's already a stm32 crate that implements these embedded-hal delays.
harry commented 2021-01-22 15:48:39 +08:00

Perhaps we can try this? https://crates.io/crates/asm-delay

Perhaps we can try this? https://crates.io/crates/asm-delay
sb10q reviewed 2021-01-22 15:09:14 +08:00
src/lib.rs Outdated
@ -81,14 +84,14 @@ impl <SPI: Transfer<u8>,
let econ2 = self.spi_port.read_reg_8b(spi::addrs::ECON2)?;
self.spi_port.write_reg_8b(spi::addrs::ECON2, 0x10 | (econ2 & 0b11101111))?;
// Wait for 25us
sb10q commented 2021-01-22 15:09:14 +08:00
Owner

This comment is just an obvious English translation of the code. Remove.

This comment is just an obvious English translation of the code. Remove.
👍 1
sb10q commented 2021-01-22 15:13:00 +08:00
Owner

It seems the embedded-hal delays have a resolution (µs) that is too coarse for this anyway.
What about simply putting a delay_ns callback in the SpiPort object, and then users of this library can set it to their own function based on cortex_m::asm::delay?

It seems the embedded-hal delays have a resolution (µs) that is too coarse for this anyway. What about simply putting a ``delay_ns`` callback in the ``SpiPort`` object, and then users of this library can set it to their own function based on ``cortex_m::asm::delay``?
harry commented 2021-01-22 15:45:59 +08:00

So, the reason I started using HAL's SysTick timer in 50c3003210 is that using CYCCNT isn't too accurate e.g. for socket timeout. I think it would be nice to keep that in our example since I don't think polling the sockets require nanosecond precision.

However, I do see that embedded_hal's delay might not be suitable for interacting with the Ethernet controller. It looks plausible to me that SpiEth and spi::SpiPort absorbs a "delay in nanoseconds" function in some way. For example, we can make spi::SpiPort take a function pointer when being init'ed. By doing so we could get rid of choosing between asm's and hal's delays completely. But one of the STM32-specific restriction that still stands is that there's only one SYST to serve one purpose at a time.

So, the reason I started using HAL's SysTick timer in 50c3003210971dfaf9565fa50921b1227306325f is that using CYCCNT isn't too accurate e.g. for socket timeout. I think it would be nice to keep that in our example since I don't think polling the sockets require nanosecond precision. However, I do see that embedded_hal's delay might not be suitable for interacting with the Ethernet controller. It looks plausible to me that `SpiEth` and `spi::SpiPort` absorbs a "delay in nanoseconds" function in some way. For example, we can make `spi::SpiPort` take a function pointer when being init'ed. By doing so we could get rid of choosing between asm's and hal's delays completely. But one of the STM32-specific restriction that still stands is that there's only one SYST to serve one purpose at a time.
sb10q commented 2021-01-22 15:52:13 +08:00
Owner

But one of the STM32-specific restriction that still stands is that there's only one SYST to serve one purpose at a time.

asm::delay doesn't use SYST, does it?

> But one of the STM32-specific restriction that still stands is that there's only one SYST to serve one purpose at a time. ``asm::delay`` doesn't use ``SYST``, does it?
occheung commented 2021-01-22 15:53:30 +08:00
Poster

Ok. With a delay_ns closure, it should eliminate the need to own a delay structure inside the Ethernet driver as well.

Ok. With a `delay_ns` closure, it should eliminate the need to own a delay structure inside the Ethernet driver as well.
harry commented 2021-01-22 15:57:44 +08:00

asm::delay doesn't use SYST, does it?

No asm::delay doesn't need to borrow the timer hardware, but the user of this crate needs to know what kind of function / closure they're going to pass to SpiEth/spi::SpiPort, like whether it is making use of the timers or the Cortex-M instruction cycles. Either the user is responsible for that, or we restrict the type of the "delay" closure - which I'm not too sure how to do yet.

> ``asm::delay`` doesn't use ``SYST``, does it? No ``asm::delay`` doesn't need to borrow the timer hardware, but the user of this crate needs to know what kind of function / closure they're going to pass to `SpiEth`/`spi::SpiPort`, like whether it is making use of the timers or the Cortex-M instruction cycles. Either the user is responsible for that, or we restrict the type of the "delay" closure - which I'm not too sure how to do yet.
sb10q commented 2021-01-22 16:06:12 +08:00
Owner

Is there a benefit to being able to specify a timer, or is it always going to be short delays done by asm::delay or a similar function?

Is there a benefit to being able to specify a timer, or is it always going to be short delays done by ``asm::delay`` or a similar function?
harry commented 2021-01-22 16:26:11 +08:00

To the best of my knowledge, using hardware timers must produce more accurate timing than using assembly because one can make use of STM32 timers to enable interrupts to update a counter (e.g. wall clock) accurately, while asm delays could be prolonged when interrupts are being serviced. That's why I think the driver crate itself doesn't need to use hardware timers as we're just giving enough time for the controller hardware to process data or switch between states, but our TCP example can keep using them.

To the best of my knowledge, using hardware timers must produce more accurate timing than using assembly because one can make use of STM32 timers to enable interrupts to update a counter (e.g. wall clock) accurately, while asm delays could be prolonged when interrupts are being serviced. That's why I think the driver crate itself doesn't need to use hardware timers as we're just giving enough time for the controller hardware to process data or switch between states, but our TCP example can keep using them.
👍 2
occheung added 1 commit 2021-01-22 17:47:20 +08:00
366ff1c80e * Changed delay source from DelayUs from embedded-hal to user-defined closure 
* Updated examples
- Removed delay.rs
- Removed over-obvious comments
sb10q reviewed 2021-01-22 18:01:48 +08:00
examples/tcp_stm32f407.rs Outdated
@ -169,3 +181,3 @@
// Read MAC
let mut eth_mac_addr: [u8; 6] = [0; 6];
spi_eth.read_from_mac(&mut eth_mac_addr);
spi_eth.read_from_mac(&mut eth_mac_addr).unwrap();
sb10q commented 2021-01-22 18:01:48 +08:00
Owner

unrelated change

unrelated change
sb10q reviewed 2021-01-22 18:02:29 +08:00
src/spi.rs Outdated
@ -1,5 +1,5 @@
use embedded_hal::{
blocking::spi::Transfer,
blocking::{spi::Transfer},
sb10q commented 2021-01-22 18:02:29 +08:00
Owner

not needed

not needed
sb10q reviewed 2021-01-22 18:03:04 +08:00
src/spi.rs Outdated
@ -67,2 +69,3 @@
F: FnMut(u32) -> ()> SpiPort<SPI, NSS, F> {
// TODO: return as Result()
pub fn new(spi: SPI, mut nss: NSS) -> Self {
pub fn new(spi: SPI, mut nss: NSS, f: F) -> Self {
sb10q commented 2021-01-22 18:03:04 +08:00
Owner

why f and not delay_ns?

why ``f`` and not ``delay_ns``?
occheung added 1 commit 2021-01-25 10:48:55 +08:00
d557e2542d Modify changes: 
- Revert unwrap() addition to examples
- Remove redundant bracket
* Changed closure variable to delay_ns for clarity
sb10q reviewed 2021-01-25 10:59:07 +08:00
sb10q reviewed 2021-01-25 11:00:35 +08:00
examples/tcp_stm32f407.rs Outdated
@ -97,1 +101,4 @@
pub fn delay_ns(time_ns: u32) {
cortex_m::asm::delay((time_ns*168_000_000)/1_000_000_000 + 1)
}
sb10q commented 2021-01-25 11:00:35 +08:00
Owner

Where is this used?

Where is this used?
occheung commented 2021-01-25 11:51:27 +08:00
Poster

Removed in 5bdfd21e93.

Removed in 5bdfd21e93.
sb10q reviewed 2021-01-25 11:01:23 +08:00
examples/tcp_stm32f407.rs Outdated
@ -155,2 +163,3 @@
clocks);
enc424j600::SpiEth::new(spi_eth_port, spi1_nss)
let delay_ns_fp: fn(u32) -> () = |time_ns| {
sb10q commented 2021-01-25 11:01:23 +08:00
Owner

fp?

fp?
occheung commented 2021-01-25 11:50:38 +08:00
Poster

I meant function pointer.
Changed to delay_ns.

I meant function pointer. Changed to `delay_ns`.
occheung added 1 commit 2021-01-25 11:47:37 +08:00
sb10q merged commit 1add94c12e into master 2021-01-25 12:35:23 +08:00
occheung deleted branch generalize_delay 2021-01-26 15:47:22 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: renet/ENC424J600#2
There is no content yet.
Delete Branch "occheung/ENC424J600:generalize_delay"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?