I2C implementation for STM32

[Giovanni]

"Bit" of a nazi? The list above sounds like dictatorship to me

Probably you are right, probably I should be more tactful with somebody is volunteering to make things better, I must apologize. I hope you understand that I am not trying to do a "me too" product and I am really concerned about the real and perceived quality., it can make the difference between a winner and another overambitious project that did not last.

Allow me to underline how much impulse barthess gave and is giving to the project: two full complex drivers and implementations, all the ideas and the countless small fixes and improvements, not joking, hundreds of commits. He is fast, at times I had to hit the brake or a new stable version would never materialize.

Giovanni

[Badger]

I see that i2cMasterTransmit is now i2cMasterTransmitTimeout. Can we have i2cMasterTransmit() as a macro and with TIME_INFINITE as the timeout? Just like how chIQGet() is a wrapper to chIQGetTimeout(). Same would apply to i2cMasterRead().

Looking forward to the API stabalising; great work barthess

[barthess]

1) i2c_lld_reset(0 function is not used.

Not sure, is function need at all?

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.

fixed

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

I know, how to use VTs in regular threads but never used they in driver context. Are there any examples in other drivers?

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 ?

Yes, correct because there is some time between releasing bus wires (clearing BUSY bit) and
STOP bit clearing. Both checks are needed because line may be busy
in case of hardware errors. By the way, I moved this macro into functions
to improve readability.

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.

fixed

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.

It is architecture dependant features, so please move it correct himself.

8) The field txbytes of the driver structure is never used.

fixed

9) Could the field rxbytes of the driver structure be eliminated with some rework ? (not sure)

Not sure too. In ISR we need correct pointer to receive buffer to realize
read through write behavior. Is there more elegant way to pass it to ISR without separate field
in driver?

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

fixed

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.

Moved this lock from macro to low level functions. Yes, it is better
to do that in one place, but how to realize atomic state transitions
in I2C driver?

12) The macro i2c_lld_error_wakeup_isr() assigns a critical pointer outside the critical zone.

fixed in i2c_lld_error_wakeup_isr() and i2c_lld_wakeup_isr()

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.

Please move they to doxygen documentation.

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

fixed

Changes quickly tested on F1x and F4x with simple tests from testhal without any extreme loads. Works as expected.
rev. 3713
--------------------------------------------------------------------------------

@DrunkenDonkey

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?

Not always true. If slave "kick the bucket" and pull one (both) of wire down than you lose the bus at all.
Also, look in testhal\STM32F1xx\I2C\fake.c

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?

But what byte from 2 return? It is also slave dependent feature.

The list above sounds like dictatorship to me

I think successful projects must have one and only one dictator to be successful (for example Python, Linux). If you want to contribute code you must keep rules of the project. If not agree than make your own fork "with blackjack and whores". All is simple.

[barthess]

Can we have i2cMasterTransmit() as a macro and with TIME_INFINITE as the timeout? Just like how chIQGet() is a wrapper to chIQGetTimeout(). Same would apply to i2cMasterRead().

Good idea, added in rev. 3715

[DrunkenDonkey]

I was kidding about the dictatorship guys, I love how clean and easy to use the os/hal layer is, checked few others before chibi, and you can easily tell the difference.

[Giovanni]

Not sure, is function need at all?

I don't know, I just saw it is not invoked anywhere. What is it supposed to do?

I know, how to use VTs in regular threads but never used they in driver context. Are there any examples in other drivers?

No other examples, I need to think about this. Probably while loops will have to check for the VT status and return with a timeout if it becomes not armed.

It is architecture dependant features, so please move it correct himself.

I will reorder the fields and adjust doxygen comments.

Not sure too. In ISR we need correct pointer to receive buffer to realizeread through write behavior. Is there more elegant way to pass it to ISR without separate field in driver?

I don't know yet, probably not, this is not an important issue anyway, the driver is already much more simple than before.

how to realize atomic state transitions in I2C driver?

State transitions are all done in the high level driver, there is no need to do transitions in the low level. In general state transition should reside in a critical zone.

Please move they to doxygen documentation.

Will do.

Giovanni

[Giovanni]

Hi,

I made some rework to the I2C LLD driver (attached in the forum), mostly reformatting and documentation-related changes but I also made some small changes because potential race conditions and small optimizations.

Could you please verify it? if it is OK please commit the changes.

Probably I also found a way to remove the rxbytes and rxbuf fields by moving the RX DMA initialization in the transmit function instead of the ISR but I didn't change it because it is a big modification and I am not sure it is right.

Giovanni

[barthess]

Added calls to reset function to i2c_lld_start() and i2c_lld_stop() functions just to be safe.

About timeouts. Using VT here is bad idea because its resolution much lover than needed timeouts (generally ones of uS). So I add timeouts based on simple counters.

I made some rework to the I2C LLD driver

Holly shit, how much copypaste errors was in my code, thanks. Code merged and quick checks done with testahl programs. Rev. 3719.

One issue I can not resolve. Transfer functions correct acquire data but always return RDY_TIMEOUT message. I do something wrong in threading switching macros. Could you please check macros in i2c_lld.h?

Probably I also found a way to remove the rxbytes and rxbuf fields by moving the RX DMA initialization in the transmit function instead of the ISR but I didn't change it because it is a big modification and I am not sure it is right.

IMO better to release in 2.4 not very optimized driver but stable. Also I have idea to improve DMA usage in next version of OS. Drivers acquire DMA channel in lld_start() functions and do not release it until lld_stop() function called. My desire is acquire/release DMA in transceiving functions to improve sharing possibilities. My be add synchronization primitives in DMA helper to use it in the same manner as i2cAcquireBus().

[Giovanni]

I will give the driver another look tomorrow about that issue. Few comments:

About timeouts, being this a communication timeout the resolution is not that important, it is just a way to not remain into the driver forever in case something goes wrong. On the job I am working on a ISO26262 related project so I am becoming fixated with similar safety-related solutions, my idea is to make the HAL more and more safety-friendly after the release 2.4.0.

Anyway your comment gave me an idea for a realtime counter abstraction driver that could be used for delays defined with the resolution of clock ticks and for execution time measurements, I already did something like this on PPC platforms for another project so this one should be quick. It will be a super new feature and I want it in 2.4, stay tuned

About optimizations, don't be afraid and optimize where you can, if it does not go in 2.4.0 then it will have to wait 2.6.0 and that is more than one year away. Anyway my idea was not about DMA acquisition and release but just about DMA registers initialization, you don't have to wait the ISR to do that, you can prepare the transfer in the transmit and in the ISR just start the already initialized stream.

Giovanni

[Giovanni]

I made some more documentation related changes and implemented that optimization, when you have some time do yo mind verifying it? if everything is OK I will then look into the whole timeout issue.

Thanks,
Giovanni