Remove cortex-m dependencies for delay #2

😄 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

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

examples: remove duplicate delay c146eb155d

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

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

occheung commented

2021-01-22 15:12:05 +08:00

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?

occheung commented

2021-04-21 12:56:05 +08:00

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

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

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

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

👍 1

sb10q commented

2021-01-22 15:13:00 +08:00

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

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

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

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.

👍 2

occheung added 1 commit 2021-01-22 17:47:20 +08:00

* Changed delay source from DelayUs from embedded-hal to user-defined closure 366ff1c80e

* 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

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

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

why f and not delay_ns?

why ``f`` and not ``delay_ns``?

occheung added 1 commit 2021-01-25 10:48:55 +08:00

Modify changes: d557e2542d

- 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

Where is this used?

occheung commented

2021-01-25 11:51:27 +08:00

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| {