Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPI no longer works after update from v0.30 -> v0.37 #79

Closed
akselbor opened this issue May 24, 2022 · 11 comments · Fixed by #86
Closed

SPI no longer works after update from v0.30 -> v0.37 #79

akselbor opened this issue May 24, 2022 · 11 comments · Fixed by #86

Comments

@akselbor
Copy link

I've been using an ESP32-S3 to control a couple of TMC5160s over SPI using esp-idf-hal v0.30. Yesterday, I updated the project to esp-idf-hal v0.37 and found that the SPI communication no longer works. The code is unchanged between v0.30 and v0.37, except for the necessary changes to spi::config::Config during initialization.

Any idea what might be causing this? I would guess that it is related to 2d0554d? I'll try to debug the issue by myself in the meantime.

The relevant section of code looks roughly like this, after the abstractions have been removed:

// Note: spi_pins.cs == None
let spi_pins = ...
let mut cs = ...

let spi = Master::<SPI2, _, _, _, _>::new(
        peripherals.spi2,
        spi_pins,
        esp_idf_hal::spi::config::Config {
            baudrate: Hertz(4_000_000),
            data_mode: embedded_hal::spi::MODE_3,                    // v0.37: V02Type(MODE_3).into()
            bit_order: esp_idf_hal::spi::config::BitOrder::MSBFirst, // v0.37: Removed
        },
    )?;
    
// Note: config::BitOrder should (?) be irrelevant, since I construct the bytes myself. As far as I could see from spi.rs
// the BitOrder only affects the ordering of the bytes (weirdly enough) when using Word::load/store for u16, u32.
let data: &mut [[u8; 5]] = ...
for datagram in data {
    cs.set_low()?;
    spi.transfer(&mut datagram)?;
    cs.set_high()?;
}

Versions used:

# Old
esp-idf-sys = { version = "0.28.3", features = ["binstart"] }
embedded-svc = "0.15.4"
esp-idf-svc = "0.34.3"
esp-idf-hal = "0.30.0"
embedded-hal = "0.2"

# New
esp-idf-sys = { version = "0.31", features = ["binstart"] }
embedded-svc = "0.21"
esp-idf-svc = "0.41"
esp-idf-hal = "0.37"
embedded-hal = "0.2"

# Common
ESP_IDF_VERSION = { value = "release/v4.4" } 
@akselbor
Copy link
Author

akselbor commented May 26, 2022

I put the SPI under an oscilloscope, and found that the two versions produce very different signals. Each transaction with the TMC5160 should consist of 8 + 32 bits. Where the first 8 corresponds to a register at the slave device and a R/W bit, and the 32 remaining bits are data. For each transaction, the slave's behaviour is to send back 8 + 32 bits, where the first 8 is a status byte, while the remaining 32 is an echo of the data it received in the previous transaction.

In all of the pictures (sorry for the picture quality) below, we have:

  • Yellow is MOSI
  • Orange is MISO
  • Green is SLCK

Under v0.30, I get this signal:
image

While v0.37 seems to transfer two distinct words for each transfer(...):
image

Zoomed in view of the first and second exchange, respectively:
image
image

A minimal sample to reproduce for esp32-s3, sans the response from the slave:

fn main() -> Result<()> {
    esp_idf_sys::link_patches();
    // Bind the log crate to the ESP Logging facilities
    esp_idf_svc::log::EspLogger::initialize_default();

    let peripherals = Peripherals::take().unwrap();
    let pins = peripherals.pins;

    // SPI pins
    let sclk = pins.gpio1.into_output()?;
    let sdo = pins.gpio2.into_input()?;
    let sdi = Some(pins.gpio42.into_output()?);
    let cs = Option::<gpio::Gpio2<gpio::Output>>::None;
    // Note: CS pins have been toggled manually, since I needed > 1 (which esp-idf-hal had (has?) no support for)
    let mut cs1 = pins.gpio48.into_output()?.degrade();

    let spi_pins = spi::Pins { sclk, sdo, sdi, cs };
    let mut spi = Master::<SPI2, _, _, _, _>::new(
        peripherals.spi2,
        spi_pins,
        esp_idf_hal::spi::config::Config {
            baudrate: Hertz(4_000_000),
            data_mode: V02Type(embedded_hal::spi::MODE_3).into(),
            // <- add BitOrder here if v0.30
        },
    )?;

    loop {
        cs1.set_low()?;
       // This just happens to be the message that is sent in the pictures above
        spi.transfer(&mut [
            33 | 0b1000_0000u8,
            0b1011_0011,
            0b1000_1111,
            0b0000_1110,
            0b0011_0111,
        ]);
        cs1.set_high()?;
    }
}

@akselbor akselbor reopened this May 26, 2022
@daagaak
Copy link

daagaak commented May 28, 2022

As a data point, I had to roll back to here to get the display on my Adafruit Magtag to work again

esp-idf-hal = "0.36.0"

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jun 1, 2022

@Dominaezzz Any ideas?

@Dominaezzz
Copy link
Contributor

From staring at the code I can't tell what's wrong.

@daagaak Does esp-idf-hal = "=0.37.0" work?

I didn't touch any of the e-hal 0.2 code paths yet at this point so I'm puzzled as to how it suddenly stopped working then.

@akselbor Any chance you could do a binary search to see which exact version introduces the behaviour?

@daagaak
Copy link

daagaak commented Jun 1, 2022

@Dominaezzz
so I can't select =0.37.0

error: failed to select a version for the requirement `esp-idf-hal = "=0.37.0"`
candidate versions found which didn't match: 0.37.4, 0.36.0, 0.35.2, ...

However, if I allow it to select 0.37.4 then it does not work. My binary gets stuck during the SPI transfer to the display and the idle task WDT timer starts firing:

E (29362) task_wdt: Task watchdog got triggered. The following tasks did not reset the watchdog in time:
E (29362) task_wdt:  - IDLE (CPU 0)
E (29362) task_wdt: Tasks currently running:
E (29362) task_wdt: CPU 0: main
E (29362) task_wdt: Print CPU 0 (current core) backtrace
Backtrace:0x400D7476
[task_wdt_isr:/Users/matt/Code/esp32-eink-display-rs/.embuild/espressif/esp-idf/v4.3.2/components/esp_common/src/task_wdt.c:189]:0x3FFC7E20 0x400259EE 
[_xt_lowint1:/Users/matt/Code/esp32-eink-display-rs/.embuild/espressif/esp-idf/v4.3.2/components/freertos/port/xtensa/xtensa_vectors.S:1105]:0x3FFC7E40 0x40024C6F 
[spi_device_polling_end:/Users/matt/Code/esp32-eink-display-rs/.embuild/espressif/esp-idf/v4.3.2/components/driver/spi_master.c:960]:0x3FFD4040 0x40024CD5 
[spi_device_polling_transmit:/Users/matt/Code/esp32-eink-display-rs/.embuild/espressif/esp-idf/v4.3.2/components/driver/spi_master.c:988]:0x3FFD4070 0x40083961 
[esp_idf_hal::spi::MasterBus::finish:/Users/matt/.cargo/registry/src/github.com-1ecc6299db9ec823/esp-idf-hal-0.37.4/src/spi.rs:276]:0x3FFD4090 0x4009D090
[<il0373::interface::Interface<SPI,CS,DC,BUSY,RESET> as device_driver::ll::register::RegisterInterface>::write_register:/Users/matt/.cargo/git/checkouts/il0373-e725e4e4a696def5/8b898ef/src/interface.rs:138]:0x3FFD40F0 0x40097820 
[il0373::register::registers::vdcs::<impl il0373::register::registers::RegAccessor<I,(),il0373::register::registers::vdcs::W>>::write_direct:/Users/matt/.cargo/registry/src/github.com-1ecc6299db9ec823/device-driver-0.3.1/src/ll/register.rs:446]:0x3FFD4120 0x4009639C 
[firmware::display::il0373::Graphics<T>::full_update:/Users/matt/Code/esp32-eink-display-rs/firmware/src/display.rs:32]:0x3FFD4180 0x400936FF
[main:??:??]:0x3FFD4470 0x4009099D 
[app_main:/Users/matt/.cargo/registry/src/github.com-1ecc6299db9ec823/esp-idf-sys-0.31.5/src/start.rs:46]:0x3FFD4500 0x40125147
[main_task:/Users/matt/Code/esp32-eink-display-rs/.embuild/espressif/esp-idf/v4.3.2/components/freertos/port/port_common.c:133]:0x3FFD4520

@Dominaezzz
Copy link
Contributor

esp_idf_hal::spi::MasterBus::finish is the most suspicious thing.

@daagaak Which device are you running that on? ESP32, esp32-s3, etc.

@daagaak
Copy link

daagaak commented Jun 1, 2022

@Dominaezzz That's an on ESP32-S2. I just tried the same source on ESP32 with 0.37.4 and it seems to run fine (though its a slightly different eink display/driver but I don't think that matters here).

@Dominaezzz
Copy link
Contributor

Yeah I figured that might be the case.

This code snippet to de-assert CS doesn't work on the ESP32-S2.

esp-idf-hal/src/spi.rs

Lines 314 to 333 in 2c3741a

/// Empty transaction to de-assert CS.
fn finish(&mut self) -> Result<(), SpiError> {
let flags = 0;
let mut transaction = spi_transaction_t {
flags,
__bindgen_anon_1: spi_transaction_t__bindgen_ty_1 {
tx_buffer: ptr::null(),
},
__bindgen_anon_2: spi_transaction_t__bindgen_ty_2 {
rx_buffer: ptr::null_mut(),
},
length: 0,
rxlength: 0,
..Default::default()
};
esp!(unsafe { spi_device_polling_transmit(self.handle, &mut transaction as *mut _) })
.map_err(SpiError::other)
}

Not sure how to work around it besides just using software CS.

@georgik
Copy link

georgik commented Jun 10, 2022

I discovered the similar issue. Let me add few notes. The following configuration does not work on ESP32-S3, the display shows just random noise:
20220610_075832

esp-idf-svc = "0.41.3"
esp-idf-hal = "0.37.4"
embedded-svc = "0.21.2"

Working configuration for ESP32-S3:

esp-idf-svc = "0.39.1"
esp-idf-hal = "0.35.1"
embedded-svc = "0.19"

How to simulate with ESP32-S3-USB-OTG:

git clone git@github.com:georgik/rustzx-esp32.git -b bug/esp32s3-broken-spi
cd rustzx-esp32
cargo +esp espflash --target xtensa-esp32s3-espidf --release --features "esp32s3_usb_otg" --monitor

@Dominaezzz
Copy link
Contributor

Does #86 fix the issue for you?

esp-idf-hal = { git = "https://github.com/Dominaezzz/esp-idf-hal/", branch = "fix_spi" }

@daagaak
Copy link

daagaak commented Jun 11, 2022

Yep, that branch is working on my ESP32-S2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants