Added blocking I²C support

[ilya-epifanov]

WIP, only 7-bit address standard mode writes have been tested

[ilya-epifanov]

ok, I've tested writing and reading 1, 2, 3 and 8 bytes in standard (50kHz, 100kHz) and fast modes (50kHz, 100kHz, 200kHz, 300kHz, 400kHz), except Fm with 16:9 duty cycle.

[ilya-epifanov]

All modes (Sm, Fm 1:1, Fm 16:9) work.

[ilya-epifanov]

I consider it to be almost finished. At least the I²C itself works properly. @japaric @lnicola @wose @therealprof could you please give it a try with the peripherals you have at hand?

[lnicola]

@ilya-epifanov I tested write, but not write_read yet.

This might be a stupid question, but how can I share an I2C peripheral with multiple drivers?

[therealprof]

@lnicola Not at all a stupid question and topic of this issue https://github.com/japaric/embedded-hal/issues/35

[lnicola]

@ilya-epifanov Can you add a Read impl?

I'd send a PR for that and to avoid the duplication between write_read and the other two, but I'm not sure whether not ending the transaction after sending the data is important or not.

[therealprof]

@ilya-epifanov Since you beat me to it, I of course had to create a new BSP crate for the Nucleo-F103RB in order to being able to test it... I've now done a crude porting of my SSD1302 code over to your HAL driver. It more or less works but I'm easily able to freeze it; will have to check what exactly is foul here after cleaning it up.

[therealprof]

@ilya-epifanov The freeze happens somewhere in I2C land, either due to the driver not properly handling a condition or the device keeping the bus hostage; maybe I'll find time to hook it up to the scope later. I've pushed my example code to a new feature branch for the time being: https://github.com/therealprof/nucleo-f103rb/tree/features/blocking-i2c-hal-test

[ilya-epifanov]

@lnicola I've added Read impl but frankly I don't remember if I've ever seen an I2C device which allowed to read without writing something first.

[ilya-epifanov]

@therealprof thank you! I'll try to hook up an SSD1306 and give it a spin

[lnicola]

@ilya-epifanov I know. I was trying to use a driver that (currently, at least) is sending a command, closing the transaction, then starting a new one to read back the response.

[therealprof]

I've just done a few more tests and added another example not relying on serial input to output stuff on the SSD1306. Tests have shown that any speed below around 370kHz is somewhat uncritical but at 400kHz (in 1:1 ratio mode) it'll hang reliably. Also it is not very robust against I2C problems, if I wiggle or disconnect SDA/SCL it'll also hang.

[therealprof]

@lnicola That won't work.

[wose]

@ilya-epifanov I just tested your implementation with the BH1750 sensor. The driver uses the Write and Read Traits. Worked without issues.

[therealprof]

I believe there're a number of sensors which will spill their data just by issuing a read without prior write (which is typically used to specify a "register" from which should be read), like the HIH6130.

[ilya-epifanov]

@therealprof Could you please take a look at SR1 and SR2 registers when the thing hangs? Those should have addresses 0x40005400 and 0x40005404 for I2C1

[ilya-epifanov]

As a general protection mechanism we need timeouts for busy_waits. But as different operations on different peripherals might require completely different timeouts, those should be stored somewhere.

I propose the following:

    let mut i2c = I2c::i2c1(
        p.device.I2C1,
        (scl, sda),
        &mut afio.mapr,
        i2c::Mode::Fast { frequency: 400000, duty_cycle: i2c::DutyCycle::Ratio16to9 },
        clocks,
        &mut rcc.apb1,
    );

    let mut i2c_1 = i2c.channel("__ timeouts, additional settings, address might go here as well? __");
    let si5351 = SI5351::new(&mut i2c_1, false, 25_000_000).unwrap();

    let mut i2c_2 = i2c.channel("__ timeouts, additional settings, address might go here as well? __");
    let _ = ssd1306::init(&mut i2c_2);
    let _ = ssd1306::pos(&mut i2c_2, 0, 0);
    let _ = ssd1306::print_bytes(&mut i2c_2, b"Send ASCII over serial for some action");

[therealprof]

@ilya-epifanov I believe you mean 0x14 and 0x18 if you're interested in the SRs, offsets 0x00 and 0x04 are the CRs. However I'm happy to provide both after putting so much work in it to hook it up. ;)

(gdb) x 0x40005400
0x40005400: 0x00000101
(gdb) x 0x40005404
0x40005404: 0x00000008

(gdb) x 0x40005414
0x40005414: 0x00000201
(gdb) x 0x40005418
0x40005418: 0x00000003

Two more observations by hooking up the scope:

1. Don't forget proper external pull-ups, the signal looks really shitty without
2. It seems that when running at a nomimal 400kHz, the MCU might clock out slightly over 400kHz and strangely enough when I caught it doing that, it would not work

[ilya-epifanov]

I've added timeouts to the busy waits which at least don't lock up the whole MCU in case of errors. Also, I'm resetting the peripheral after any errors (which might be a little bit brute force approach). @therealprof could you please check with your setup?

[therealprof]

@ilya-epifanov I checked and it works totally fine now. Not sure the timeout was actually needed but it really is very robust now, I can even disconnect SDA or SCL or induce errors and it will recover just fine.

@japaric Please consider merging this.

[ilya-epifanov]

@therealprof is the binary any different? Can you reproduce it reliably?

[therealprof]

Yes, the binary is quite different. ARMv7-M obviously allows to embed load/store addresses directly in the code whereas with ARMv6-M they're all at the end of the function. I've put it here: https://gist.github.com/therealprof/b43b0d6ca39a2a1eb04bb3abbfc431a3

[therealprof]

Something isn't playing ball with the timeout implementation it seems. It actually works fine with ARMv7 if I lower the speed (e.g. to 380kHz) or use 16:9 ratio at 400kHz.

I don't quite understand this assumption:

                    // 25 ms bus timeout @ ~10 clks per busy_wait cycle
                    let bus_timeout = (clocks.sysclk().0 / 400).max(10);

Not only can the sysclk be pretty much be any value from ridiculously slow to all the way up at 72MHz and the busy_wait!() will also depend on quite a lot of factors like whether it's inlined or not, subject to flash wait states and the amount of CPU instructions it compiles into...

[therealprof]

NB: The issue in ARMv7-M mode also vanishes with a faster clock setup.

[ilya-epifanov]

@therealprof regarding bus_timeout calculation, it's measured in cycles and anything that would give between 1ms and 100ms should be ok. I took 25ms as a baseline only because it's an SMBus timeout, so I assumed it makes sense for a raw I²C bus as well. The reason I don't use DWT is that there's no guarantee it's enabled.

[ilya-epifanov]

@therealprof BTW >= 400kHz is only intended to be used with 16:9 duty cycle, but I still don't understand what ARMv7-M and ARMv6-M have to do with this.

[ilya-epifanov]

The only thing that comes to mind is some weird timing issue with some flag not being set quickly enough, resulting in some bus fault during START condition (the only place where I try recover from errors since no transaction take place at that time)

[ilya-epifanov]

Can you put a breakpoint somewhere in fn reset(&mut self) just to see if it's being called or not? It shouldn't be called at all on a clean bus.

[therealprof]

@therealprof regarding bus_timeout calculation, it's measured in cycles and anything that would give between 1ms and 100ms should be ok. I took 25ms as a baseline only because it's an SMBus timeout, so I assumed it makes sense for a raw I²C bus as well.

25ms sounds great. I just have no idea where the 400 is coming from and why you're clamping at 10.

let bus_timeout = (clocks.sysclk().0 / 400).max(10);

[therealprof]

BTW >= 400kHz is only intended to be used with 16:9 duty cycle

Fair enough. Didn't even know about this before I checked the interwebs just now.

but I still don't understand what ARMv7-M and ARMv6-M have to do with this.

My suspicion is that the different instruction sequence generated for ARMv7-M and ARMv6-M completely throws off your busy_wait!() macro at the default clock frequencies. If I remove all error conditions except for the timeout it also works, same for higher non-default frequencies.

I'm really not a big fan of the timeout handling, this should probably be rather handled by a user of the async version and the sync version should really just busy wait for any Error or Ok condition. I'd probably also change the Error handling to check whether any (relevant) bit in I2C_SR1 is set before checking which one exactly is set. And I'd probably also refactor the Error to include all bits of the register, so if someone is interested in what exactly the Error was they can figure it out later without generating huge (and costly) jump carpets throughout all of the code.

[ilya-epifanov]

@japaric This timeout thing is the only ugly part of this PR and it's unavoidable in such a blocking implementation. The upside is that it's very stable. It survived every electrical mayhem I have thrown at it, like shorting to vcc, gnd and disconnecting.

[japaric]

it's unavoidable in such a blocking implementation.

Is it though? For instance, does i2cdev (the Linux subsystem, not the crate) have a timeout by default? I'm concerned that (a) there's no way to opt out of the timeout and (b) it's not configurable. I'm not sure that the I2C spec says that the I2C slaves has to answer in X us so I don't think a hardcoded value is the proper choice here; some applications might want to allow the slave to hold the bus for a long period, maybe?

[fmckeogh]

@japaric @ilya-epifanov There is explicitly no maximum hold, low, high, or set up time in the I2C specification.

[lnicola]

Is the timeout issue the only remaining thing here?

[jamwaffles]

It would be great to get this PR moving again. I've been using @ilya-epifanov's fork for a while now which works great, but it feels bad using a Git dependency in Cargo.toml. I understand the need to get this HAL right, but it seems to be in a pretty good place to me. Why not merge now and fix forward if anything comes up?

[therealprof]

@jamwaffles The interim conclusion seems to be that the timeout behaviour is not acceptable by @japaric and the only way to get this implemented with timeout would be to do the non-blocking implementation and offer different blocking implementations (with and without timeout) based on the non-blocking one which @ilya-epifanov wanted to implement but seemingly didn't get around to.

Maybe it would be acceptable to put in the blocking implementation without the timeout and then switch implementations once the non-blocking is available?

[ilya-epifanov]

Guys, sorry for the delay. I've extracted the blocking part into BlockingI2c, which now needs to be created like this:

let mut i2c = BlockingI2c::i2c1(
    device.I2C1,
    (scl, sda),
    &mut afio.mapr,
    i2c::Mode::Fast { frequency: 400000, duty_cycle: i2c::DutyCycle::Ratio16to9 },
    clocks,
    &mut rcc.apb1,
    10000,
    3,
    10000,
    20000
);

[kellerkindt]

@ilya-epifanov would be neat if you could update this, so it has no conflicts. Then I could use it at least locally :)

[ilya-epifanov]

@kellerkindt Here you go!

[ilya-epifanov]

@japaric @therealprof @lnicola Is it possible to merge this PR? It's non-blocking now but only exposes the blocking API.

[therealprof]

@ilya-epifanov Don't have write permissions.

[lnicola]

@ilya-epifanov Me neither. I'd like to see this merged, even though I hope a future version will offer a real non-blocking (that is, supporting await!) interrupt-based API.

[MajorBreakfast]

It'd be nice if this was merged. I currently have to use the fork because my project needs i2c.

[TeXitoi]

This PR is the most wanted feature. Even if it is not perfect right now, I think we need to merge it as is because it will be better that the current state of the crate.

@therealprof what do you think of merging it, and follow up PRs can then improve it?

[eldruin]

Could somebody with knowledge of this discussion create a tracking issue listing the items to be improved?