stm32 i2c slave #2909

jrmoulton · 2024-05-05T07:56:13Z

This isn't quite finished. It only has the async slave implementation and the code needs to be cleaned up.

I could use some help debugging also.

In the listen method I enable the address match interrupt and register the waker in the poll closure.

The issue is that the runtime never polls the function again even when an address is matched.

If I write code that blocks instead of yielding and send and I2C communication the peripheral will still be enabled and will match the address.

I've tried also adding the wupen bit but that didn't make a difference

WUPEN:Wakeup from Stop mode enable 
0: Wakeup from Stop mode disable.
1: Wakeup from Stop mode enable.

Any idea why the runtime wouldn't poll the function again?

bsodmike · 2024-05-07T02:34:35Z

Hi Jared, interesting. Will respond soon if I figure anything out.

jrmoulton · 2024-05-07T19:54:49Z

I've figured out why the function wasn't being polled. Now just working on finishing up the implementation

jrmoulton · 2024-05-15T23:52:57Z

@Dirbaio the stm32 i2c supports multimaster for allowing both a master and a slave functionality. The slave mode is the default and the master mode is enabled by a start condition and ends with a stop condition.

Currently in this implementation I have split the slave from the master. This more closely matches the API in the embassy-rp crate. Would it be better to join them and merge the slave functionality into the regular i2c driver (in order to let you switch from slave to master without reconfiguring) or is it better to leave them split?

(edited)

bsodmike · 2024-05-16T02:47:18Z

I think being part of the regular driver works, if say you introduce a Option<Mode> type config option that is very obvious.

Thoughts?

bsodmike · 2024-06-01T06:31:35Z

@jrmoulton see CI issues please, just a heads up. keen to test this soon - thanks so much!!

edit: did some tracing to help you with picking this up mate - these seem CI related only, as your build works

bsodmike · 2024-06-01T06:34:53Z

@Dirbaio I like your CI page title "lol job"?? https://ci.embassy.dev/jobs/586c0e7a5b13

bsodmike · 2024-06-01T06:37:53Z

@jrmoulton see CI issues please, just a heads up. keen to test this soon - thanks so much!!

stm32_metapac::i2c::vals::Oamsk is there in the docs (v15.0.0)

test result: ok. 3 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.06s

    Updating crates.io index
    Updating git repository `https://github.com/embassy-rs/stm32-data-generated`
   Compiling embassy-hal-internal v0.1.0 (/ci/code/embassy-hal-internal)
   Compiling embassy-time v0.3.0 (/ci/code/embassy-time)
   Compiling embassy-usb-synopsys-otg v0.1.0 (/ci/code/embassy-usb-synopsys-otg)
   Compiling embassy-stm32 v0.1.0 (/ci/code/embassy-stm32)
   Compiling embassy-net-driver v0.2.0 (/ci/code/embassy-net-driver)
   Compiling embassy-embedded-hal v0.1.0 (/ci/code/embassy-embedded-hal)
error[E0432]: unresolved import `stm32_metapac::i2c::vals::Oamsk`
  --> src/i2c/mod.rs:19:5
   |
19 | use stm32_metapac::i2c::vals::Oamsk;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `Oamsk` in `i2c::vals`

warning: unused import: `interrupt`
  --> src/time_driver.rs:19:13
   |
19 | use crate::{interrupt, peripherals};
   |             ^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `interrupt`
  --> src/exti.rs:14:13
   |
14 | use crate::{interrupt, pac, peripherals, Peripheral};
   |             ^^^^^^^^^

error[E0599]: no method named `init` found for struct `I2cSlave` in the current scope
   --> src/i2c/mod.rs:367:14
    |
299 | pub struct I2cSlave<'d, T: Instance, M: Mode> {
    | --------------------------------------------- method `init` not found for this struct
...
367 |         this.init(freq, config);
    |              ^^^^ method not found in `I2cSlave<'_, T, M>`

warning: unused variable: `waker`
   --> src/dma/ringbuffer.rs:429:33
    |
429 |         fn set_waker(&mut self, waker: &Waker) {}
    |                                 ^^^^^ help: if this is intentional, prefix it with an underscore: `_waker`
    |
    = note: `#[warn(unused_variables)]` on by default

bsodmike · 2024-06-01T06:39:23Z

embassy-stm32/src/i2c/mod.rs

+            _phantom: PhantomData,
+        };
+
+        this.init(freq, config);


This is missing.

bsodmike · 2024-06-01T06:40:15Z

embassy-stm32/src/i2c/mod.rs

@@ -13,6 +16,7 @@ use embassy_hal_internal::{into_ref, Peripheral, PeripheralRef};
 use embassy_sync::waitqueue::AtomicWaker;
 #[cfg(feature = "time")]
 use embassy_time::{Duration, Instant};
+use stm32_metapac::i2c::vals::Oamsk;


Odd error in CI, needs a deeper look.

I haven't been able to figure out why I am getting this error in CI.

May be @Dirbaio can help, he's very familiar with his CI setup. I'm curious too.

jrmoulton · 2024-06-07T04:04:07Z

I'm in the process of reworking the I2C driver to be generic over an I2C mode which can either be Master, Slave, or MultiMaster.

jkorinth · 2024-06-11T20:55:06Z

Just wanted to let you know that I'm also eagerly awaiting slave mode, thanks a lot @jrmoulton!

jrmoulton · 2024-06-19T01:06:54Z

The last remaining ci issues are unrelated to this PR. Finally actually ready for review

jrmoulton · 2024-06-25T18:28:09Z

@Dirbaio is there anything else I need to do in order for this PR to be reviewed?

Dirbaio · 2024-06-25T19:08:41Z

yes, make CI pass. Once CI is green it'll be reviewed

okhsunrog · 2024-08-06T17:19:45Z

any updates on this?

jrmoulton · 2024-08-07T00:59:13Z

I need to undo a few changes that I made and then the CI should pass. It's close. I just haven't spent the last hour or two to get it finished.

okhsunrog · 2024-08-07T13:16:56Z

seems like now there are 2 pull requests for I2C slave on stm32. yesterday I tried to use I2C on stm32 and was confused which one to use.
#3196

okhsunrog · 2024-08-13T17:19:36Z

Can you please provide an example of using i2c slave for any stm32 mcu? @jrmoulton

jrmoulton · 2024-08-13T18:02:11Z

I'll add one

jrmoulton · 2024-08-13T19:05:33Z

I haven't actually tested the example that I've added. I've tested the slave code but not in this particular example. I've made this example match as closely as I could to the rp2040 i2c slave example.

okhsunrog · 2024-08-13T19:47:49Z

Thanks! I'll test the example with STM32L071CB tomorrow

okhsunrog · 2024-08-13T19:48:54Z

@Dirbaio can this be reviewed? It seem to look ready

okhsunrog · 2024-08-14T10:45:50Z

I tested the PR today, using STM32L071CB as slave and ESP32-C3 as master. I adapted your example, deleting master code, so only slave code it left. I'm looking at the transmission I see that time between read/write bit and ack from slave is about 280 us, so the clock gets stretched for 280 us. for some reason esp32-c3 doesn't like it. I see errors like:

E (44590) i2c.master: I2C transaction timeout detected
E (44590) i2c.master: s_i2c_synchronous_transaction(870): I2C transaction failed

(I set timeout to -1, I tried making it really big too, no result).
is there a way to decrease the gap? For now, when the slave sends ACK seems like the master is no longer listening, so the SDA stays pulled down and esp32-c3 just freezes. I guess, I should use different hardware as master or decrease the delay before ACK somehow?

jrmoulton · 2024-08-14T15:56:09Z

@okhsunrog it looks like the esp32c3 doesn't support clock stretching. When you say you are increasing the timeout, do you mean on the esp32c3 master or on the stm32 slave?

https://docs.esp-rs.org/esp-hal/esp-hal/0.19.0/esp32c3/esp_hal/i2c/struct.I2C.html#method.new_with_timeout_async

okhsunrog · 2024-08-14T16:20:32Z

@jrmoulton it turned out that even the maximum timeout for SCL pin stretching wasn't enough, so I just patched esp-idf code to set I2C_TIME_OUT_EN bit to 0 manually, and now it doesn't time out. Otherwise I2C_TIME_OUT_INT interrupt is triggered. Now the code doesn't exit with timeout, esp32-c3 waits patiently for stm32 to answer. I'm just using i2c-tools from esp-idf examples for testing, with timeouts disabled by patching one line in esp-idf (they hardcoded setting this bit to 1).
Now, about slave code. Here's my code for STM32L071CBT6, it's as simple as possible: https://gist.github.com/okhsunrog/74ab86798a9bd5a91cf4328c5e56f67f
I'm inspecting the signal with Hantek + Pulseview software. Here's what I have:
on esp32c3 side sending works as extected:

i2c-tools> i2cset -c 0x22 -r 0xC0
I (649060) cmd_i2ctools: Write OK
i2c-tools> i2cget -c 0x22 -r 0xC0 -l 1
0x05 
i2c-tools> i2cset -c 0x22 -r 0xC0
I (659080) cmd_i2ctools: Write OK
i2c-tools> i2cset -c 0x22 -r 0xC0
I (663830) cmd_i2ctools: Write OK
i2c-tools> i2cget -c 0x22 -r 0xC0 -l 1
0x05

on stm32 side I see strange timeouts:

0.000000 TRACE BDCR ok: 1c060003
└─ embassy_stm32::rcc::bd::{impl#3}::init @ /home/okhsunrog/.cargo/git/checkouts/embassy-cf3401b39709dec8/fc34291/embassy-stm32/src/rcc/bd.rs:196 
0.000000 DEBUG rcc: Clocks { hclk1: MaybeHertz(16000000), hsi: MaybeHertz(0), lse: MaybeHertz(0), lsi: MaybeHertz(0), pclk1: MaybeHertz(8000000), pclk1_tim: MaybeHertz(16000000), pclk2: MaybeHertz(8000000), pclk2_tim: MaybeHertz(16000000), rtc: MaybeHertz(32000), sys: MaybeHertz(32000000) }
└─ embassy_stm32::rcc::set_freqs @ /home/okhsunrog/.cargo/git/checkouts/embassy-cf3401b39709dec8/fc34291/embassy-stm32/src/rcc/mod.rs:71  
0.000152 INFO  Blinking LED, SDA, and SCL pins
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:77  
0.000274 INFO  Device start
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:80  
18.897460 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
19.897644 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
23.429931 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
24.430114 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
24.430358 INFO  Read command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:90  
24.430725 INFO  Successfully responded to read
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:96  
28.922821 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
29.923004 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
33.675384 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
34.675598 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
38.598083 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
39.598266 ERROR error while responding Timeout
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:110 
39.598510 INFO  Read command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:90  
39.598876 INFO  Successfully responded to read
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:96  
160.263458 INFO  Write command
└─ stm32l071_i2c::____embassy_main_task::{async_fn#0} @ src/main.rs:105 
161.263671 ERROR error while responding Timeout

And the I2C line stretching is really long for i2cget. For writing data, the SCL line is pulled only for about 270-280 us. When I do i2cget, the stretching for writing byte with "register address" is the same, about 280us, but for reading a byte of data, the stretching is really long! It's almost a second, 946ms. You can see on the screenshots below.

I found this in your code:

impl Default for Config {
    fn default() -> Self {
        Self {
            #[cfg(gpio_v2)]
            sda_pullup: false,
            #[cfg(gpio_v2)]
            scl_pullup: false,
            #[cfg(feature = "time")]
            timeout: embassy_time::Duration::from_millis(1000),
        }
    }
}

Maybe it's somehow related for the SCL line getting stretched for a whole second? And what exactly that timeout it about?

jrmoulton · 2024-08-14T16:43:22Z

That timeout is used for the slave as a maximum time to wait for the master when doing a read or write. It isn't directly related.

The reason that the clock is being stretched for so long is that, the way it is written, the executor has to schedule the task that will respond to the request from the master. This code does not immediately start responding.

There are reasons that I wrote it this way. The biggest is that in order to respond immediately, the slave needs to have a buffer ready which doesn't really work if the slave can match multiple addresses. Some logic based on the matched address is necessary. I tried to find a way to store a closure to handle this logic but without an allocator I couldn't find a way to do it. So instead the clock is stretched until the task is scheduled and the user decides how to respond once they have seen the matched address.

It would probably be useful to have functions that work without clock stretching but I haven't written those

(Edit)
1 full second is extremely long though. I'm not sure what is going on there

okhsunrog · 2024-08-14T16:47:40Z

Yes, one second is very long. And what about ERROR error while responding Timeout? Master says that everything is okay, the signal looks fine, why the timeout then?
@jrmoulton

jrmoulton · 2024-08-14T16:54:07Z

Not sure. There could be several reasons. Hard to say without seeing all of the code that you are using

okhsunrog · 2024-08-14T17:36:02Z

I'll send the full code tomorrow. I just got Discovery F4 board, I'll test it both as slave and as master and post the results here too

okhsunrog · 2024-08-16T14:57:14Z

That's the code I used for slave: https://github.com/okhsunrog/stm32l071_i2c_slave_test
That's the code I used for master: https://github.com/espressif/esp-idf/tree/release/v5.3/examples/peripherals/i2c/i2c_tools
The only difference is that in this line: https://github.com/espressif/esp-idf/blob/6568f8c553f89c01c101da4d6c735379b8221858/components/hal/esp32c3/include/hal/i2c_ll.h#L191 I replaced 1 with 0

okhsunrog · 2024-09-03T14:50:09Z

@jrmoulton yesterday I had some free time and I investigated into this issue some more time. The stretching of SCL for 1 second seems to be because of this:

impl Default for Config {
    fn default() -> Self {
        Self {
            #[cfg(gpio_v2)]
            sda_pullup: false,
            #[cfg(gpio_v2)]
            scl_pullup: false,
            #[cfg(feature = "time")]
            timeout: embassy_time::Duration::from_millis(1000),
        }
    }
}

I tried different timeouts manually and the scl is indeed stretched for the duration I set. When I set it to Duration::from_micros(100) the stretching is quite low, around 200-300 us, which is okay. The second issue is that your pub async fn respond_to_write function always return with timeout error, no matter which timeout I set, it never returns Ok. and the SCL line stretching time depends on the timeout value in config. could you look into this once again?

okhsunrog · 2024-09-03T15:00:54Z

@jrmoulton you're doing dma_transfer.await;, how does it work? when does this feature gets polled?
When I replace the code timeout.with(self.read_dma_internal_slave(buffer, timeout)).await with self.read_dma_internal_slave(buffer, timeout).await the code gets stuck forever. What could be the issue?

okhsunrog · 2024-09-03T15:17:55Z

Did you test both blocking and async?

bsodmike · 2024-09-03T16:24:22Z

That's the code I used for slave: https://github.com/okhsunrog/stm32l071_i2c_slave_test

That's the code I used for master: https://github.com/espressif/esp-idf/tree/release/v5.3/examples/peripherals/i2c/i2c_tools

The only difference is that in this line: https://github.com/espressif/esp-idf/blob/6568f8c553f89c01c101da4d6c735379b8221858/components/hal/esp32c3/include/hal/i2c_ll.h#L191 I replaced 1 with 0

Can you share your memory.x as well please?

jrmoulton mentioned this pull request May 7, 2024

stm32/i2c: slave mode support #622

Open

jrmoulton force-pushed the i2c-slave-new branch from d1e6455 to 3981083 Compare May 15, 2024 23:51

bsodmike reviewed Jun 1, 2024

View reviewed changes

jrmoulton force-pushed the i2c-slave-new branch 3 times, most recently from 796b5c0 to b3dcfd4 Compare June 18, 2024 03:12

jrmoulton marked this pull request as ready for review June 18, 2024 03:12

jrmoulton force-pushed the i2c-slave-new branch from 8df4692 to a4f6590 Compare June 18, 2024 17:02

jrmoulton marked this pull request as draft June 18, 2024 17:23

jrmoulton marked this pull request as ready for review June 19, 2024 01:06

jrmoulton force-pushed the i2c-slave-new branch 4 times, most recently from 4da9bd1 to daeefdd Compare June 25, 2024 21:19

jrmoulton added 8 commits August 13, 2024 09:46

Add more docs

70dfbc0

move embedded hal impl to version modules to allow for 10bit addr on v2

31f224f

move addr mask impl to be v2 only

98437e3

Fix v1 mod

bec0eac

remove dead code when v1

8b50f2a

add drop guard, clear interrupts, fix len return

c7b4a07

fix ci

d1f5a4c

remove 10 bit support

b4eb4a3

jrmoulton force-pushed the i2c-slave-new branch from daeefdd to b4eb4a3 Compare August 13, 2024 15:47

fix examples

5061963

jrmoulton force-pushed the i2c-slave-new branch from 27d70a0 to 5061963 Compare August 13, 2024 16:42

fix rustfmt

bfc162d

add stm32 i2c slave example

fc34291

stm32 i2c slave #2909

Are you sure you want to change the base?

stm32 i2c slave #2909

Conversation

jrmoulton commented May 5, 2024

bsodmike commented May 7, 2024

jrmoulton commented May 7, 2024

jrmoulton commented May 15, 2024 • edited Loading

bsodmike commented May 16, 2024 • edited Loading

bsodmike commented Jun 1, 2024 • edited Loading

bsodmike commented Jun 1, 2024

bsodmike commented Jun 1, 2024

bsodmike Jun 1, 2024

Choose a reason for hiding this comment

bsodmike Jun 1, 2024

Choose a reason for hiding this comment

jrmoulton Jun 18, 2024

Choose a reason for hiding this comment

bsodmike Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

jrmoulton commented Jun 7, 2024

jkorinth commented Jun 11, 2024

jrmoulton commented Jun 19, 2024

jrmoulton commented Jun 25, 2024

Dirbaio commented Jun 25, 2024

okhsunrog commented Aug 6, 2024

jrmoulton commented Aug 7, 2024

okhsunrog commented Aug 7, 2024

okhsunrog commented Aug 13, 2024

jrmoulton commented Aug 13, 2024

jrmoulton commented Aug 13, 2024

okhsunrog commented Aug 13, 2024

okhsunrog commented Aug 13, 2024

okhsunrog commented Aug 14, 2024 • edited Loading

jrmoulton commented Aug 14, 2024

okhsunrog commented Aug 14, 2024

jrmoulton commented Aug 14, 2024 • edited Loading

okhsunrog commented Aug 14, 2024

jrmoulton commented Aug 14, 2024

okhsunrog commented Aug 14, 2024

okhsunrog commented Aug 16, 2024

okhsunrog commented Sep 3, 2024

okhsunrog commented Sep 3, 2024

okhsunrog commented Sep 3, 2024

bsodmike commented Sep 3, 2024

jrmoulton commented May 15, 2024 •

edited

Loading

bsodmike commented May 16, 2024 •

edited

Loading

bsodmike commented Jun 1, 2024 •

edited

Loading

bsodmike Jun 18, 2024 •

edited

Loading

okhsunrog commented Aug 14, 2024 •

edited

Loading

jrmoulton commented Aug 14, 2024 •

edited

Loading