Added blocking I²C support #50

ilya-epifanov · 2018-02-14T00:06:53Z

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

lnicola

I didn't test the PR, but I tried to implement this before (and got stuck at the clock setup part) and I noticed some discrepancies.

lnicola · 2018-02-14T06:21:20Z

src/i2c.rs

+                    self.i2c.cr1.modify(|_, w| w.start().set_bit());
+                    busy_wait!(self.i2c, sb);
+
+                    self.i2c.dr.write(|w| unsafe { w.dr().bits(addr & 0b1111_1110) });


Should addr be shifted by 1, or is that already by convention?

I've seen implementations doing it both ways, but Arduino and RIOT OS do shift it themselves. I think we also need to

lnicola · 2018-02-14T06:21:51Z

src/i2c.rs

+                type Error = Error;
+
+                fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error> {
+                    // TODO support transfers of more than 255 bytes


I think this TODO was left from the stm32f30x-hal implementation?

Yep, but it's still valid

Why do you say that? I didn't see anything suggesting this wouldn't work for larger transfers.

lnicola · 2018-02-14T06:26:04Z

src/i2c.rs

+                    bytes: &[u8],
+                    buffer: &mut [u8],
+                ) -> Result<(), Error> {
+                    // TODO support transfers of more than 255 bytes


lnicola · 2018-02-14T06:36:05Z

src/i2c.rs

+                    busy_wait!(self.i2c, addr);
+                    let _ = self.i2c.sr2.read();
+
+                    for byte in buffer {


This looks like "method 1" in the manual, but "method 2" is supposed to work better for polling.

It also looks like it might be missing setting the ACK flag (it should be set before checking rx_ne for all the received bytes except the last).

Thanks for the hint, gonna fix it.
I haven't yet tested read part at all, so it probably doesn't work.

ilya-epifanov · 2018-02-15T22:56:47Z

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 · 2018-02-16T00:47:33Z

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

ilya-epifanov · 2018-02-16T00:48:39Z

src/i2c.rs

+
+                    self.send_start_and_wait()?;
+
+                    match buffer.len() {


such a horror show

ilya-epifanov · 2018-02-16T00:54:08Z

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 · 2018-02-17T10:42:48Z

@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 · 2018-02-17T10:55:24Z

@lnicola Not at all a stupid question and topic of this issue rust-embedded/embedded-hal#35

lnicola · 2018-02-17T13:59:13Z

@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 · 2018-02-17T15:32:40Z

@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 · 2018-02-17T16:26:22Z

@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 · 2018-02-17T21:38:42Z

@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 · 2018-02-17T21:42:46Z

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

lnicola · 2018-02-17T21:59:45Z

@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 · 2018-02-17T22:11:37Z

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 · 2018-02-17T22:13:00Z

@lnicola That won't work.

wose · 2018-02-17T22:14:24Z

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

therealprof · 2018-02-17T22:31:09Z

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 · 2018-02-17T22:43:06Z

@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 · 2018-02-17T22:53:30Z

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 · 2018-02-18T00:04:19Z

@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:

Don't forget proper external pull-ups, the signal looks really shitty without
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 · 2018-02-20T01:10:15Z

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 · 2018-02-20T22:37:02Z

@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 · 2018-02-21T00:15:45Z

src/gpio.rs

@@ -314,6 +335,27 @@ macro_rules! gpio {
                        unsafe { (*$GPIOX::ptr()).bsrr.write(|w| w.bits(1 << (16 + $i))) }
                    }
                }
+
+                impl<MODE> OutputPin for $PXi<Alternate<MODE>> {


Should we add UnsafeOutputPin with all methods marked unsafe?
Impls like this are necessary to implement workarounds like 2.14.1 in this errata sheet: http://www.st.com/content/ccc/resource/technical/document/errata_sheet/f5/50/c9/46/56/db/4a/f6/CD00197763.pdf/files/CD00197763.pdf/jcr:content/translations/en.CD00197763.pdf.
Also it makes it much more convenient to initlialize pins to proper value on startup.

Could you elaborate on how marking the method as unsafe helps in working around the errata?

I don't think that Alternate-mode pins should allow regular I/O in safe code, but it's needed for various workarounds. Hence I propose to make such impls unsafe.

Oh, I hadn't notice this were the Alternate pins. I agree that this should not be public and safe. But, since this is working around a errata, would be possible to keep this private? I'm not 100% sure it's a good idea to expose this to users, regardless of safety.

ilya-epifanov · 2018-02-21T00:18:25Z

src/i2c.rs

+                    apb.rstr().modify(|_, w| w.$i2cXrst().clear_bit());
+
+                    // 25 ms bus timeout @ ~10 clks per busy_wait cycle
+                    let bus_timeout = (clocks.sysclk().0 / 400).max(10);


I don't assume DWT to be enabled here

ilya-epifanov · 2018-02-21T00:19:50Z

src/i2c.rs

+                    let bus_timeout = (clocks.sysclk().0 / 400).max(10);
+                    let pclk1 = clocks.pclk1().0;
+
+                    assert!(mode.get_frequency() <= 400_000);


I'm not sure it's a good idea, since I've unintentionally overclocked the bus to 600kHz in standard mode once and it worked fine.

FWIW, the only reason I have something similar in the f30x-hal crate is because I don't know how to compute the different parameters for frequencies higher than 1 MHz -- the reference manual only covered frequencies up to that number.

ilya-epifanov · 2018-02-21T00:22:02Z

src/i2c.rs

+                    assert!(mode.get_frequency() <= 400_000);
+
+                    let mut i2c = I2c { i2c, pins, bus_timeout, mode, pclk1 };
+                    i2c.init();


I'd rather compute proper values for the registers and save them here instead of doing every time we init() after communication errors, but I'll lose the typesafe register API then.

ilya-epifanov · 2018-02-21T21:18:21Z

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

therealprof · 2018-02-21T22:08:53Z

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 · 2018-02-23T09:31:17Z

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 · 2018-02-23T16:43:53Z

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

japaric · 2018-03-26T15:30:33Z

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 · 2018-03-31T11:38:18Z

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

lnicola · 2018-04-14T22:00:47Z

Is the timeout issue the only remaining thing here?

jamwaffles · 2018-05-01T12:31:02Z

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 · 2018-05-01T13:08:45Z

@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 · 2018-05-09T14:51:23Z

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 · 2018-07-10T12:58:02Z

@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 · 2018-07-22T23:00:40Z

@kellerkindt Here you go!

implemented method 2 from the reference manual for reading, fixed wrong frequency in Fm with 1:1 duty cycle

resetting the bus after errors

…ous timeouts

ilya-epifanov · 2018-07-22T23:08:13Z

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

therealprof · 2018-07-23T07:34:57Z

@ilya-epifanov Don't have write permissions.

lnicola · 2018-07-23T07:39:55Z

@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.

TeXitoi · 2018-08-06T23:10:22Z

src/i2c.rs

+
+                fn send_addr_and_wait(&self, addr: u8, read: bool) -> NbResult<(), Error> {
+                    self.nb.send_addr(addr, read);
+                    busy_wait_cycles!(self.nb.wait_after_sent_addr(), self.addr_timeout)


I have an infinite loop here in release mode. It's working in debug mode. Strange, seems that DWT::get_cycle_count() always returns 0. I don't know where to search mode.

That's hanging here: https://github.com/TeXitoi/bmp380-test/blob/master/src/bmp280.rs#L365

(gdb) bt #0 _$LT$stm32f103xx_hal..i2c..BlockingI2c$LT$stm32f103xx..I2C1$C$$u20$PINS$GT$$GT$::send_addr_and_wait::h8e287e53a5adee35 (read=false, self=<optimized out>, addr=<optimized out>) at /home/texitoi/.cargo/git/checkouts/stm32f103xx-hal-7d0db38b85df9102/3a8d64d/src/i2c.rs:213 #1 _$LT$stm32f103xx_hal..i2c..BlockingI2c$LT$stm32f103xx..I2C1$C$$u20$PINS$GT$$GT$::write_without_stop::h99cb76b55b93dcc7 (self=0x20004f34, addr=<optimized out>, bytes=...) at /home/texitoi/.cargo/git/checkouts/stm32f103xx-hal-7d0db38b85df9102/3a8d64d/src/i2c.rs:374 #2 0x080012fa in _$LT$stm32f103xx_hal..i2c..BlockingI2c$LT$stm32f103xx..I2C1$C$$u20$PINS$GT$$u20$as$u20$embedded_hal..blocking..i2c..WriteRead$GT$::write_read::h949b7ab3b3bfbb31 ( self=<optimized out>, addr=118, bytes=..., buffer=...) at /home/texitoi/.cargo/git/checkouts/stm32f103xx-hal-7d0db38b85df9102/3a8d64d/src/i2c.rs:461 #3 0x08000bb6 in _$LT$bmp380_test..bmp280..BME280$LT$I2C$C$$u20$D$GT$$GT$::read_register::hd6e0c68d133d7431 (self=<optimized out>, register=244) at src/bmp280.rs:436 #4 _$LT$bmp380_test..bmp280..BME280$LT$I2C$C$$u20$D$GT$$GT$::configure::h0afac1b198eb57dc ( self=<optimized out>) at src/bmp280.rs:365 #5 _$LT$bmp380_test..bmp280..BME280$LT$I2C$C$$u20$D$GT$$GT$::init::h4bc76ebdfc030868 ( self=0x20004f34) at src/bmp280.rs:330 #6 0x08000576 in bmp380_test::main::h02b25e9e644215b2 () at src/main.rs:47 #7 0x08000790 in main () at <entry macros>:3

That's working using Ratio16to9.

The BlockingI2c trait actually assumes you have DWT enabled, I'll add a comment

Could be great to add an example.

Even better: asking for &mut std32f103xx::DWT at the BlockingI2c creation to enable DWT if needed.

MajorBreakfast · 2018-08-19T15:19:50Z

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

TeXitoi · 2018-10-11T15:50:29Z

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?

therealprof

LGTM, let's get this moving finally.

eldruin · 2018-11-30T13:29:05Z

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

lnicola reviewed Feb 14, 2018

View reviewed changes

ilya-epifanov changed the title ~~Added blocking I²C support~~ #49 Added blocking I²C support Feb 14, 2018

ilya-epifanov changed the title ~~#49 Added blocking I²C support~~ Added blocking I²C support Feb 14, 2018

ilya-epifanov mentioned this pull request Feb 14, 2018

Implement I2C #49

Closed

ilya-epifanov commented Feb 16, 2018

View reviewed changes

src/i2c.rs Outdated

self.send_start_and_wait()?;

match buffer.len() {

Copy link

Contributor Author

ilya-epifanov Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

such a horror show

ilya-epifanov commented Feb 21, 2018

View reviewed changes

This was referenced May 15, 2018

the trait bound ... is not satisfied rust-embedded-community/ssd1306#66

Closed

Major faux I2C interface incompatibility between 0.1.2 and 0.2.0 rust-embedded/embedded-hal#81

Closed

ilya-epifanov added 9 commits July 23, 2018 01:02

Added blocking I²C support

43ef809

removed transfer size checks,

4bc65b6

implemented method 2 from the reference manual for reading, fixed wrong frequency in Fm with 1:1 duty cycle

fixed fast mode. both 1:1 and 16:9 duty cycle modes work

c7f50f6

added docs and Debug traits to enums

6741f55

added Read impl

83178cd

added timeout to busy waits,

41acb73

resetting the bus after errors

forgot to move comment about bus timeout calculation

0d9916e

Made I2c non-blocking, added BlockingI2c which allows to specify vari…

3aafaa0

…ous timeouts

Renamed Ratio1to1 into Ratio2to1

3a8d64d

TeXitoi mentioned this pull request Aug 6, 2018

Embedded WG meeting, August 6 rust-embedded/wg#149

Closed

TeXitoi reviewed Aug 6, 2018

View reviewed changes

therealprof approved these changes Oct 11, 2018

View reviewed changes

therealprof merged commit 4a0abfd into japaric:master Oct 11, 2018

ilya-epifanov deleted the i2c-support branch January 2, 2019 00:51

Added blocking I²C support #50

Added blocking I²C support #50

Conversation

ilya-epifanov commented Feb 14, 2018

lnicola left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-epifanov commented Feb 15, 2018

ilya-epifanov commented Feb 16, 2018

Choose a reason for hiding this comment

ilya-epifanov commented Feb 16, 2018

lnicola commented Feb 17, 2018

therealprof commented Feb 17, 2018

lnicola commented Feb 17, 2018 • edited Loading

therealprof commented Feb 17, 2018

therealprof commented Feb 17, 2018

ilya-epifanov commented Feb 17, 2018

ilya-epifanov commented Feb 17, 2018

lnicola commented Feb 17, 2018

therealprof commented Feb 17, 2018

therealprof commented Feb 17, 2018

wose commented Feb 17, 2018

therealprof commented Feb 17, 2018

ilya-epifanov commented Feb 17, 2018

ilya-epifanov commented Feb 17, 2018

therealprof commented Feb 18, 2018

ilya-epifanov commented Feb 20, 2018 • edited Loading

therealprof commented Feb 20, 2018

ilya-epifanov Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-epifanov commented Feb 21, 2018

therealprof commented Feb 21, 2018

therealprof commented Feb 23, 2018

therealprof commented Feb 23, 2018

japaric commented Mar 26, 2018

fmckeogh commented Mar 31, 2018 • edited Loading

lnicola commented Apr 14, 2018

jamwaffles commented May 1, 2018

therealprof commented May 1, 2018

ilya-epifanov commented May 9, 2018

kellerkindt commented Jul 10, 2018 • edited Loading

ilya-epifanov commented Jul 22, 2018

ilya-epifanov commented Jul 22, 2018

therealprof commented Jul 23, 2018

lnicola commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast commented Aug 19, 2018

TeXitoi commented Oct 11, 2018

therealprof left a comment

Choose a reason for hiding this comment

eldruin commented Nov 30, 2018

lnicola left a comment •

edited

Loading

lnicola commented Feb 17, 2018 •

edited

Loading

ilya-epifanov commented Feb 20, 2018 •

edited

Loading

ilya-epifanov Feb 21, 2018 •

edited

Loading

fmckeogh commented Mar 31, 2018 •

edited

Loading

kellerkindt commented Jul 10, 2018 •

edited

Loading