Closed dwagenk closed 6 years ago
just for reference: mcux i2c driver seems to be unable to handle messages that lack the I2C_MSG_STOP flag all together. Tried to get some reference on how other boards handle these cases with the frdm_kw41z board I have.
@DWagenk I started working on the issue on my personal repository (https://github.com/ydamigos/zephyr/tree/stm32-i2c). Now the stm32 I2C driver uses the I2C API flags, but there are still some loose ends to tie up to make the API's i2c_burst_write functions to work properly.
@ydamigos I just pushed a version to fix this problem in v2 driver to my personal repo. Functionality is tested in non-interrupt mode on stm32f3_disco and works good. See https://github.com/DWagenk/zephyr/commit/f274f36c425fbbada98931e3ff02bf7074b94412 or https://github.com/DWagenk/zephyr/tree/stm32_i2c_adapt_driver_v2_to_match_API Lets compare and see which approach we want to take.
Since I don't like reusing the flags with different meaning I pass flags_next_msg into the functions, so both are available: this messages flags msg->flags next messages flags flags_next_message The rest of the code looks pretty straightforward this way.
functionality is tested in interrupt mode as well: works good, I just don't like the while loops in msg_done when we could use interrupts, but that's nothing to change here/now.
@DWagenk I agree with you and https://github.com/ydamigos/zephyr/commit/76ffd76f86f370cf794295a6e2dbb7c3d7496750 just uses the I2C API flags. The only trick the code does is to set the I2C_MSG_RESTART flag on the first message in order to create the start condition. When the message doesn't have a I2C_MSG_STOP and it is a I2C_MSG_WRITE it enables the RELOAD mode. I tested the non-interrupt mode for v1/v2 and it worked. Interrupt mode works on v1 (I want to test it a littler more) and I didn't have the time to check it on v2.
@ydamigos I looked into your code and the trick with the I2C_MSG_RESTART flag for first message looks good to me. I don't know if we get by, without having the knowledge inside the driver, whether next message starts with a restart condition or without one. If we don't make any assumptions based on message direction inside the driver (like your decision to enable reload mode in i2c_ll_stm32_v2.c line 34 is based on), the API can be used as flexible as in the example code attached, glueing messages together, without having to prepare a single buffer in the app. For v2 driver this worked with my fork. Even Messages with 300+ bytes and no start/stop/restart condition in between, as long as every single message was less than 255 bytes long.
If we don't make any assumptions based on message direction inside the driver (like your decision to enable reload mode in i2c_ll_stm32_v2.c line 34 is based on)
@DWagenk I didn't mean to check message direction, just if it has a I2C_MSG_STOP flag. I must have mixed something during a rebase. I will check it a little bit more to see if we could avoid the knowledge, whether next message starts with a restart condition or without one. We should use the same i2c_ll_stm32.c file without ifdefs regarding stm32 I2C version.
@ydamigos ydamigos
We should use the same i2c_ll_stm32.c file without ifdefs regarding stm32 I2C version.
yes, definitely, I just started on v2 and didn't wan't to break v1 before working on it, so I left the old version in place for v1 driver.
I will check it a little bit more to see if we could avoid the knowledge, whether next message starts with a restart condition or without one.
Since this problem should be not only relevant for stm32 we could think of reserving 1 bit (or 2 if we can think of yet another use case where it might be needed) of i2c_msg->flags for driver internal use in general. Would be a bigger change, but might be worth it. Then we could define I2C_MSG_NEXT_IS_RESTART in i2c_ll_stm32.h and avoid passing around additional flag-bytes.
@DWagenk I merged both approaches in the following https://github.com/ydamigos/zephyr/commit/7a112a0c9def4c809b2344ae6bdd5163f49fe496 keeping us both as authors. Unfortunately, I didn't find the time to check it on hardware. If it is ok with you, let's work on this commit as a base.
@ydamigos I tested v2 driver with stm32f3_disco board and adapted sample i2c_fujitsu_fram file https://gist.github.com/DWagenk/8260d1958e114f4104656190c121fbcf which also contains a long write, that is composed of multiple messages that need to be concatenated by the driver.
interrupt mode works now, see https://github.com/ydamigos/zephyr/commit/7a112a0c9def4c809b2344ae6bdd5163f49fe496#commitcomment-25177689
@DWagenk Thanks for checking it. I hope I will find time to check it with stm32 I2C v1 hardware before creating a PR.
PR #4572 was merged.
Discussion for this issue started in #4429
with samples/drivers/i2c_fujitsu_fram adapted to use I2C_1 as I2C_DEV or with a sample app using i2c_burst_write16 function there's the following issue.
Expected behaviour (S = start, P = stop): S - SLAVE_ADDR|W - FRAM_ADDR_HIGH - FRAM_ADDR_LOW - BYTE_0 - [...] BYTE_n - P There should be no STOP or REPEATED start condition between the FRAM address and the data.
Behaviour of stm32-i2c drivers: S - SLAVE_ADDR|W - FRAM_ADDR_HIGH - FRAM_ADDR_LOW - P - S - SLAVE_ADDR|W - BYTE_0 - [...] BYTE_n - P