rust-iot / rust-radio-sx127x

Rust driver for the Semtech SX127x series of Sub-GHz LoRa/ISM radio transceivers
Mozilla Public License 2.0
32 stars 16 forks source link

Question: Integration testing example #10

Open henrikssn opened 3 years ago

henrikssn commented 3 years ago

I have trouble getting the integration test to work. I have two radios hooked up to the SPI0 bus on a Raspberry Pi and I can use both of them (but not at the same time) with the sx127x-util tool without problems.

If I try the integration test, the first radio setup succeeds but the second fails due to zero ("0") version (just assuming this means nothing is going on at the SPI bus).

If I swap the radios in the config, then same thing happens ("second" radio succeeds and "first" one fails). It seems to me like the SPI bus doesn't like me using two radios at once, yet that seems to be the case in the checked in version.

I also tried running two sx127x-util commands (one tx, one rx) but then "funny" things happen, probably due to no bus synchronization.

The reason I care is that I want to reuse this great idea of having an integration test for a new sx1231 driver, which is written on top of rust-radio-hal :)

Any ideas? Someone ran into issues before with SPI on RPi?

henrikssn commented 3 years ago

I am able to reproduce the issue with this Python script:

import time
import spidev

def init_spi(device):
  spi = spidev.SpiDev()
  spi.open(0, device)
  spi.max_speed_hz = 500000
  spi.mode = 0
  return spi

spi0 = init_spi(0)
spi1 = init_spi(1)

def read_reg(spi, reg):
  return spi.xfer([reg, 0x00])[1]

print("/dev/spidev0.0: 0x%02x" % read_reg(spi0, 0x10)) // 0x10 is the version register on sx1231
print("/dev/spidev0.1: 0x%02x" % read_reg(spi1, 0x10))

spi0.close()
spi1.close()

Prints:

/dev/spidev0.0: 0x24
/dev/spidev0.1: 0x00
ryankurte commented 3 years ago

so you're right on all counts, you can't drive the radios from different places (on different SPI bus) because of the lack of bus synchronisation, and you can't run them on the same bus because we don't (currently) have a mechanism to synchronise SPI over transactions (this was dropped from shared-bus due to unsoundness pending the PRs mentioned below).

the reason the integration test kindof worked is that if you're running both radios from one thread, on one SPI bus, you know there's no pre-emption so the SPI transactions should be both exclusive and in order. i think it should be possible to get this to work, usually 0x00 versions mean there's something wrong with the CS/RST signals (and a logic analyser is a huuge help here). i haven't dug into the integration tests in some time but, have applications that share the bus in this way reliably enough.

to properly solve this we need to a) be able to express whether a CS pin is managed by the SPI driver (or manually) https://github.com/rust-embedded/embedded-hal/pull/245, and b) to be able to group operations into a single bus operation to ensure consistent ordering https://github.com/rust-embedded/embedded-hal/pull/191, but we haven't quite got to landing these.

wrt. the python script, do bare in mind that you need to explicitly drive CS (or, to configure CS to be driven correctly in the OS / SPI driver), i tend towards the latter as this is more realisable on most embedded devices / works with the above approach more smoothly.

an interim solution would be to implement embedded_spi::Transactional and embedded_spi::ManagedCS from here over a newtype containing an Arc<Mutex<(linux_embedded_hal::Spidev, linux_embedded_hal::Pin)>>, then to use those bounds in the driver (as i have with the sx127/8x ones). i apologise that embedded-spi is undergoing a bit of flux at the moment as i rework it to be a more generic basis for rust drivers / utilities.

The reason I care is that I want to reuse this great idea of having an integration test for a new sx1231 driver

oh that's exciting~! i've been hoping to need one of these in the near future, and to try and standardise on the configuration of the utilities / tests / etc. for the radio drivers. feel free to ping me with PRs/MRs/anything if it's useful to do so (or you would like a collaborator with a somewhat unfortunate breadth of rf experience) ^_^

henrikssn commented 3 years ago

I decided to try something different, and replaced spidev with bitbang_hal::spi. Surprisingly, it works like a charm. Maybe something is off with my spidev kernel driver?

henrikssn commented 3 years ago

so you're right on all counts, you can't drive the radios from different places (on different SPI bus) because of the lack of bus synchronisation, and you can't run them on the same bus because we don't (currently) have a mechanism to synchronise SPI over transactions (this was dropped from shared-bus due to unsoundness pending the PRs mentioned below).

the reason the integration test kindof worked is that if you're running both radios from one thread, on one SPI bus, you know there's no pre-emption so the SPI transactions should be both exclusive and in order. i think it should be possible to get this to work, usually 0x00 versions mean there's something wrong with the CS/RST signals (and a logic analyser is a huuge help here). i haven't dug into the integration tests in some time but, have applications that share the bus in this way reliably enough.

to properly solve this we need to a) be able to express whether a CS pin is managed by the SPI driver (or manually) rust-embedded/embedded-hal#245, and b) to be able to group operations into a single bus operation to ensure consistent ordering rust-embedded/embedded-hal#191, but we haven't quite got to landing these.

wrt. the python script, do bare in mind that you need to explicitly drive CS (or, to configure CS to be driven correctly in the OS / SPI driver), i tend towards the latter as this is more realisable on most embedded devices / works with the above approach more smoothly.

The problem I had is that the CS pins are owned by the spidev driver in python so I had to use the hardware support for it.

an interim solution would be to implement embedded_spi::Transactional and embedded_spi::ManagedCS from here over a newtype containing an Arc<Mutex<(linux_embedded_hal::Spidev, linux_embedded_hal::Pin)>>, then to use those bounds in the driver (as i have with the sx127/8x ones). i apologise that embedded-spi is undergoing a bit of flux at the moment as i rework it to be a more generic basis for rust drivers / utilities.

This is very cool, definitely will make these things less of an issue in the future. Is the plan to eventually fold in embedded-spi into embedded-hal or are there parts that do not fit?

The reason I care is that I want to reuse this great idea of having an integration test for a new sx1231 driver

oh that's exciting~! i've been hoping to need one of these in the near future, and to try and standardise on the configuration of the utilities / tests / etc. for the radio drivers. feel free to ping me with PRs/MRs/anything if it's useful to do so (or you would like a collaborator with a somewhat unfortunate breadth of rf experience) ^_^

I would be happy to include you there, I have previous experience with embedded c++ but is new to both radios and Rust.

Some things can definitely be extracted from the current library, for example it would be nice to have a single util tool for all the rust-radio-hal drivers to limit the amount of code duplication.

Are there already any MAC/APP layers implemented on top of rust-radio-hal? I have to build something for my sx1231 radios and it would be nice to have something to reuse/start from.

ryankurte commented 3 years ago

This is very cool, definitely will make these things less of an issue in the future. Is the plan to eventually fold in embedded-spi into embedded-hal or are there parts that do not fit?

the intent is that Transactional and ManagedCS bits will be replaced by those in embedded-hal, which means a bunch of the other internal traits there could go away, then embedded-spi will probably become driver-pal or something and keep / expand on the common stuff for writing drivers and driver utilities w/ support for UART and I2C as well.

Some things can definitely be extracted from the current library, for example it would be nice to have a single util tool for all the rust-radio-hal drivers to limit the amount of code duplication.

hmm, i have been working to extract commonality between the helpers here. it's a wee bit tricky to combine it all because the operation of radios can be pretty different, and it means you now have to update the util every time you ship a new driver, but, it'd also be pretty neat to have.

Are there already any MAC/APP layers implemented on top of rust-radio-hal? I have to build something for my sx1231 radios and it would be nice to have something to reuse/start from.

it is extremely rough and at the moment there's only one rather non-compliant 802.15.4 MAC, but, i've been playing with rust-lpwan with the goal of one day having a standard 802.15.4/6LoWPAN and LoRAWAN stack there.

ryankurte commented 3 years ago

heads up i've moved embedded-spi over to driver-pal, though we're still stacking too many patches to publish just yet, and the transactional SPI PR has landed to hopefully be released in embedded-hal@v1.0.0-alpha.3 which should resolve p much all of these.