I2C implementation for STM32

[marobi]

The workaround is to insert a small delay before calling a I2C function, for example 2mS (this guarantees a delay between 1 and 2 milliseconds), this makes sure that the polled part is skipped immediately.

That's the approach I already took. What I observed is that as long as you address the same device, you can Tx/Rx as fast as you want to that device0. However when you change device-destination (another address), you have to insert a 1 ms delay (or more) before Tx-ing again to the new device.

Rien

[barthess]

Agree with all except:

3) Added a type i2xaddr_t to be defined in the low level, some implementations may want the 10 bits addressing and would define that as a 16 bits value.

because:
1) 10-bit addressing is not so simple, as it seems. It require additional code to handle it.
2) 10-bit slaves are too rare for a moment and I have no one to perform tests for future support.
So I propose to realize 7-bit only driver.

[Giovanni]

I agree to have a 7 bits only driver, the point is to not preclude a 10 bits driver by forcing an uint8_t type as address. This change would not impact the current implementation.

Giovanni

[barthess]

Ok, I will go ahead and realize changes on this weekend in trunk.

[barthess]

Hi Giovanni
Almost all changes done in rev.3692. Some remarks.

I removed const qualifier from pointer to txbuf in transmit function because it
is convenient to have some different buffers in case of using EEPROM slave.

How to wake up thread with RDY_RESET status from ISR? Is it possible at all?

Is it good idea to use following code?

Code: Select all

 chSysLock();
  (i2cp)->id_thread = chThdSelf();
  rdymsg = chSchGoSleepTimeoutS(THD_STATE_SUSPENDED, timeout);
  chSysUnlock();

In my opinion it locks kernel for the whole time of thread sleeping.
Am I right?

Data acquisition works on STM32F1x testhal application but receive
function always return RDY_TIMEOUT. I have no ideas why.
In other words - I need your help in thread handling parts of the driver.

One drawback occured. Now transaction functions require little more RAM for
call stack because thread state handling doing in low level functions. Now
simple polling thread require at least 132 bytes of working space.

[Giovanni]

I will give it a look tomorrow.

Some notes:
- I don't understand the problem with "const", it just informs the compiler that the function does not write in the buffer an allows optimizations.
- You can't use "inline", it is C99. The compiler does inlining by itself when it is convenient to do.
- Why you check the error in order to return RDY_RESET? you should simply wakeup the thread with RDY_RESET as message in case of error, there is no need for that.

From ISR for example:

Code: Select all

  chSysLockFromIsr();
  tp->p_u.rdymsg = RDY_RESET;
  chSchReadyI(tp);
  chSysUnlockFromIsr();

Giovanni

[barthess]

- I don't understand the problem with "const", it just informs the compiler that the function does not write in the buffer an allows optimizations.

That is my mistake. I mixed up the structure and function. Fixed.

- You can't use "inline", it is C99. The compiler does inlining by itself when it is convenient to do.

Fixed.

From ISR for example:
chSysLockFromIsr();
tp->p_u.rdymsg = RDY_RESET;
chSchReadyI(tp);
chSysUnlockFromIsr();

that is exactly I need.
Rev. 3694

[Giovanni]

Hi,

It seems there are two I2C drivers under configuration is the one under I2Cv1 still used? if not please remove it. I hope to go through a the I2C driver tomorrow and then I will post my findings.

Giovanni

[Giovanni]

Hi,

This is the first part of my analysis on the status of the I2C driver implementation for STM32.

The following points should be addressed, note very few are functional, mostly style or questions (which I will fix myself):
1) i2c_lld_reset(0 function is not used.
2) i2c_lld_set_clock() and i2c_lld_set_opmode() can be static because the HLL does not uses them anymore.
3) i2c_lld_master_transceive() can be static and cannot use the keyword "inline", you could inline it directly because it is used only one time.
3) There are while loop not protected by timeout in i2c_lld_master_receive_timeout() (OK, here I should explain how to do that using VTs).
4) In i2c_lld_master_receive_timeout() and i2c_lld_master_transmit_timeout() is it correct to call first i2c_lld_wait_bus_free() and then wait for I2C_CR1_STOP ?
5) Use of __NOP() is forbidden, it should not even needed.
6) Some fields of the driver structures have "id_" prefixes, this is obsolete, those prefixes have been removed from all drivers.
7) The id_thread field (and several others) cannot be in the "mandatory" part of the driver structure because are not mandated by the HLL driver.
8) The field txbytes of the driver structure is never used.
9) Could the field rxbytes of the driver structure be eliminated with some rework ? (not sure)
10) Driver fields should not be prefixed with the __IO macro, those are not I/O registers and "volatile" should not be necessary unless there are fields you access from ISRs and out of IRSs outside critical zones (and probably this would be an error anyway).
11) The macro i2c_lld_wait_bus_free() hides a lock, this is generally not a good idea for code readability. Better do it in the same place where the unlock() is.
12) The macro i2c_lld_error_wakeup_isr() assigns a critical pointer outside the critical zone.
13) The driver contains non standard sections like "Knowledge base". This kind of documentation should not be in the code. It must be either private or in the user documentation using Doxygen.
14) #define inside function bodies are not allowed. Definitions must go in the proper section and do not have private scope.
15) Compile time checks must be done in the header file (clock checks).
16) Text beyond column 80 is not allowed and various other formatting issues, not "wrong" code but things done using a different style. This will be addressed at the end using the Eclipse reformatter, no need to do anything.

Good ideas I found:
1) Use of chDbgChecks() on configuration parameters, probably this should be extended to other drivers.
2) Except for style considerations (and forgive me if I am a bit of a nazi here ) the driver code itself looks much cleaner than previous versions.

The list is long but really important points are very few, please address just the functional points, I will address style, formatting and documentation issues at the very end. I will also look into the timeout thing later.

Giovanni

[DrunkenDonkey]

"Bit" of a nazi? The list above sounds like dictatorship to me
Fun aside, is there a way to tell if some i2c slave is alive at all? If it is not responding the state will be defined as NAK on the address write right?
Also, shouldn't the stm32 quirk with inability to receive a single byte be workarounded inside the low level driver implementation by receiving 2 and return 1? This will make the higher level, program code universal.