stm32/i2c: Use timeout passed through from machine.I2C() if set, else existing default of 50ms. #4711

andrewleech · 2019-04-23T01:21:26Z

This PR ensure stm32 hardware i2c implementation honors the existing timeout argument on machine.I2C init commands.

Previously the stm32 i2c timeout is hard coded to 50ms which isn't guaranteed to be enough depending on the clock stretching specs of the I2C device(s) in use.

Ah, I now see timeout is an undocumented arg, not described on: https://docs.micropython.org/en/latest/library/machine.I2C.html
I'm not sure if it's implemented for any other ports at this stage.

dpgeorge · 2019-04-25T05:24:48Z

ports/stm32/i2c.c

@@ -30,11 +30,11 @@

 #if MICROPY_HW_ENABLE_HW_I2C

-#define I2C_POLL_TIMEOUT_MS (50)
+#define I2C_POLL_TIMEOUT_MS(t) ((t)?t:50)  // Use provided timeout if set, or 50ms by default


It'd be more efficient to do this "default check" when timeout is provided, rather than each time it's used. So I'd suggest to remove this macro completely and just use the timeout variable as-is in the functions below.

dpgeorge · 2019-04-25T05:24:57Z

ports/stm32/i2c.c


 #if defined(STM32F4)

-int i2c_init(i2c_t *i2c, mp_hal_pin_obj_t scl, mp_hal_pin_obj_t sda, uint32_t freq) {
+int i2c_init(i2c_t *i2c, mp_hal_pin_obj_t scl, mp_hal_pin_obj_t sda, uint32_t freq, uint32_t timeout) {


I think this was added in error.

dpgeorge · 2019-04-25T05:25:21Z

ports/stm32/i2c.c

    uint32_t t0 = HAL_GetTick();
    while (!(i2c->SR1 & mask)) {
-        if (HAL_GetTick() - t0 >= I2C_POLL_TIMEOUT_MS) {
+        if (HAL_GetTick() - t0 >= I2C_POLL_TIMEOUT_MS(timeout)) {


As mentioned above, this can just be HAL_GetTick() - t0 >= timeout

dpgeorge · 2019-04-25T05:27:51Z

ports/stm32/machine_i2c.c

@@ -44,28 +44,29 @@ typedef struct _machine_hard_i2c_obj_t {
    i2c_t *i2c;
    mp_hal_pin_obj_t scl;
    mp_hal_pin_obj_t sda;
+    uint32_t timeout;


This won't work because the objects below are defined "const" (to not use RAM, they live in ROM). Best way to deal with it is to have a separate non-const array to hold just the timeouts (the mutable data).

And then have this as a pointer to an entry in that array: uint32_t *timeout.

dpgeorge · 2019-04-25T05:31:29Z

ports/stm32/machine_i2c.c

@@ -44,28 +44,29 @@ typedef struct _machine_hard_i2c_obj_t {
    i2c_t *i2c;
    mp_hal_pin_obj_t scl;
    mp_hal_pin_obj_t sda;
+    uint32_t timeout;
 } machine_hard_i2c_obj_t;

 STATIC const machine_hard_i2c_obj_t machine_hard_i2c_obj[] = {
    #if defined(MICROPY_HW_I2C1_SCL)
    {{&machine_hard_i2c_type}, I2C1, MICROPY_HW_I2C1_SCL, MICROPY_HW_I2C1_SDA},


This would become: {{&machine_hard_i2c_type}, I2C1, MICROPY_HW_I2C1_SCL, MICROPY_HW_I2C1_SDA, &timeout_array[0]}

dpgeorge · 2019-04-25T05:32:16Z

ports/stm32/machine_i2c.c

@@ -105,18 +106,18 @@ STATIC void machine_hard_i2c_print(const mp_print_t *print, mp_obj_t self_in, mp
 }

 void machine_hard_i2c_init(machine_hard_i2c_obj_t *self, uint32_t freq, uint32_t timeout) {
-    (void)timeout;
+    self->timeout = timeout;


This would become: *self->timeout = timeout

dpgeorge · 2019-04-25T05:42:26Z

Ah, I now see timeout is an undocumented arg, not described on: https://docs.micropython.org/en/latest/library/machine.I2C.html
I'm not sure if it's implemented for any other ports at this stage.

Ports that use software I2C will have this timeout feature implemented, so I think it's a good idea to add it here (for HW I2C) in the same way.

But note that the units of the timeout are microseconds for the software I2C, so they should also be microseconds here for consistency. Probably simplest way to do this is just divide the input timeout by 1000 to get milliseconds.

(Note: it'd be a separate discussion if the timeout arg should be changed to milliseconds for the software I2C, or should be renamed timeout_us to make explicit the units.)

andrewleech · 2019-04-26T06:03:00Z

Thanks, I really don't know what I was thinking sprinkling that macro through everything, really messy and inefficient.

The default timeout for the arg in machine_hard_i2c_make_new is 1000us, whereas the existing default in stm32/i2c.c is 50ms. Would you prefer to keep the 50ms as default for the hard interface to not break existing implementations, or let it be simplified to the the common 1 ms?

dpgeorge · 2019-04-26T06:14:30Z

Would you prefer to keep the 50ms as default for the hard interface to not break existing implementations, or let it be simplified to the the common 1 ms?

Good question... the timeout is really only there to prevent lock up in the case something goes wrong on the bus (a slave holds it low forever). If the bus is functioning properly (which is most of the time) then the timeout can be arbitrarily long. So maybe it's best to keep it 50ms to support slaves which stretch it for a while. And so just make it 50,000us in machine_hard_i2c_make_new for consistency.

andrewleech · 2019-04-29T00:37:06Z

I think everything should be resolved and good to go now.

dpgeorge

Thanks. Can you please also fix accel.c (eg build for PYBV10). Just add a macro like at the top of accel.c:

#define I2C_TIMEOUT_MS (50)

then use it.

dpgeorge · 2019-04-29T07:06:05Z

ports/stm32/machine_i2c.c

    #endif
 };

+STATIC uint32_t machine_hard_i2c_timeout[sizeof(machine_hard_i2c_obj)/sizeof(machine_hard_i2c_obj_t)] = {I2C_POLL_TIMEOUT_US_DEFAULT};


I'm not sure all compilers will like this use of sizeof() here... is it legal? Note there's the macro MP_ARRAY_SIZE() for this.

Also, I don't think it's necessary to initialise the value, it can just stay "blank", ie in the BSS. The timeout should always be populated upon creation of an I2C object. (And being in the BSS is slightly more optimal, no code space taken, initing it at boot is faster.)

dpgeorge · 2019-04-29T07:06:41Z

ports/stm32/machine_i2c.c

    i2c_init(self->i2c, self->scl, self->sda, freq);
+    if (self->us_timeout != NULL) {


I don't think it's necessary to test for NULL, if the I2C is created it should exist and have a valid pointer (and there's no corresponding test in the read from this value below, and it's not needed there).

dpgeorge · 2019-05-20T05:29:25Z

@andrewleech I'm happy to make the 3 fixes/changes mentioned above, just let me know either way.

andrewleech · 2019-05-20T10:44:16Z

Oh sorry I missed your last set of reviews. That would be great if you wouldn't mind cleaning them up, I'll keep trying to get confirmation on the sd card issue tomorrow.

dpgeorge · 2019-05-21T02:41:25Z

Ok, this was reworked and merged in ed2b6ea (it now stores the configurable timeout in a small array in i2c.c itself, which simplifies it a lot).

…n-main Translations update from Weblate

dpgeorge reviewed Apr 25, 2019

View reviewed changes

pi-anl force-pushed the stm32_i2c_timeout branch 2 times, most recently from cb45f3d to 1d4fc04 Compare April 26, 2019 06:23

stm32/i2c: Use timeout (in us) passed through from machine.I2C().

1586810

pi-anl force-pushed the stm32_i2c_timeout branch from 1d4fc04 to 1586810 Compare April 29, 2019 00:36

dpgeorge reviewed Apr 29, 2019

View reviewed changes

dpgeorge closed this May 21, 2019

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request May 6, 2021

Merge pull request micropython#4711 from weblate/weblate-circuitpytho…

1668e9c

…n-main Translations update from Weblate

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stm32/i2c: Use timeout passed through from machine.I2C() if set, else existing default of 50ms. #4711

stm32/i2c: Use timeout passed through from machine.I2C() if set, else existing default of 50ms. #4711

andrewleech commented Apr 23, 2019

dpgeorge Apr 25, 2019

dpgeorge Apr 25, 2019

dpgeorge Apr 25, 2019

dpgeorge Apr 25, 2019

dpgeorge Apr 25, 2019

dpgeorge Apr 25, 2019

dpgeorge commented Apr 25, 2019

andrewleech commented Apr 26, 2019

dpgeorge commented Apr 26, 2019

andrewleech commented Apr 29, 2019

dpgeorge left a comment

dpgeorge Apr 29, 2019

dpgeorge Apr 29, 2019

dpgeorge commented May 20, 2019

andrewleech commented May 20, 2019

dpgeorge commented May 21, 2019

		i2c_init(self->i2c, self->scl, self->sda, freq);
		if (self->us_timeout != NULL) {

stm32/i2c: Use timeout passed through from machine.I2C() if set, else existing default of 50ms. #4711

stm32/i2c: Use timeout passed through from machine.I2C() if set, else existing default of 50ms. #4711

Conversation

andrewleech commented Apr 23, 2019

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

dpgeorge commented Apr 25, 2019

andrewleech commented Apr 26, 2019

dpgeorge commented Apr 26, 2019

andrewleech commented Apr 29, 2019

dpgeorge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented May 20, 2019

andrewleech commented May 20, 2019

dpgeorge commented May 21, 2019