Fix SPI: shorten timing & simplify API #11
Loading…
Reference in New Issue
There is no content yet.
Delete Branch "fix-spi"
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?
This PR will improve timing on the SPI bus by achieving the following goals:
Goal 1: Shorten the SPI timing
The following changes are relevant to reducing time required to perform any SPI transaction:
Goal 2: Simplify the API
This goal is mainly a direct response to my questions raised in #9. The following changes are relevant to simplifying the driver API:
cortex_m::asm::delay()for ns-delays: it is obvious that the embedded_hal SPI feature simply does not check that the SCK clock has stopped before the transfer function returns (c.f. issue #264 of embedded-hal repo), so we must impose delay in-between SPI transfer calls and CS_n assertions/deassertions to observe the SPI spec and the electrical timing behaviour. To precisely control the timing, this PR adds a crate featurecortex-m-cputo enable "instruction cycle"-based delays for Cortex-M platforms. Further discussion in the next section.delay_nsfunction pointer: no matter the platform supports cortex-m or not, one of the main caveats of having the user to supplydelay_nsis that we cannot validate the function, and, more importantly, it complicates the API.DelayUsfor device reset: this is specific to ENC424J600, and all the user needs to do is to pass a mutable reference of aDelayUsimplementation to the reset function call, often just once per appplication. The timescale is μs and not ns.Discussion
Point 1: ns-delays are unlikely to appear on external embedded_hal-based libraries
As long as we stick with the nanosecond timescale for CS_n assertion/deassertion, I believe we do not need to rely on external definitions or implementations of accurate delays in this timescale. In my reasoning, every application is free to choose a system clock at lower megahertzes while adopting our driver, giving little motivation for the maintainers of embedded_hal libaries to include an additional
DelayNstrait for portability.By reading some of their discussions (e.g. issue #63 of embedded-hal repo), I have also gathered that most use cases of nanosecond delays could be worked around with interrupts or tick timers, which are hardware-specific. So, instead of borrowing an external portable feature from the HAL or elsewhere, we should simply define our own nanosecond delay feature here, which is based on instruction cycles on Cortex-M.
There is also a specific case of the
AsmDelaycrate for Cortex-M. It is capable of implementing a theoreticalDelayNs, but I have my scepticism because it currently takes into account the fact thatcortex_m::asm::delay()could take 2 CPU cycles on M7 rather than 1, but that does not seem to describe the behaviour of all Cortex-M cores. On M4 platforms such as Booster,delay()would take 3-4 cycles so the current implementation ofAsmDelaywould produce even longer and less accurate delays, which violates my goal 1. I can go and discuss this with the maintainers.Point 2: CS_n delays are only implemented for Cortex-M targets and optionally
In this PR, we only care about producing ns-delays on Cortex-M platforms. In reality, embedded-hal is used on many other CPUs as well, so my PR has some degree of extensibility for other platforms in the future, and they are likely to be based on consumption of CPU cycles on their ISA -- if we assume that the CPU clock is at hundreds of MHz. No delay pointers should be required to extend ns-delays on other CPUs.
Besides, in my opinion, by letting the user choose to enable the
cortex-m-cpucrate feature, if the CPU clock is too low to produce ns-delays any way, they can omit the ns-delays since the time between SPItransfer()calls and CS_n assertions/deassertions would be in the order of microseconds.