wrong error management in i2C functions - part 2

[camelator]

Well...
I don't know how the tests are done as master, but to me, it may work with fast MCU but not with CPU as I2S slave:

1/ To me, the HAL functions are not correct and for that STM32Duino must deliver / correct the bad error management from HAL.
Example in function HAL_StatusTypeDef HAL_I2C_Master_Seq_Receive_IT
This function has a bad mix of error management and state and return value.
Then, it is possible (and because of other errors in STM32duino) to have this function returning HAL_OK but reading nothing (for example, if the line is busy)
=> STM32Duino should not only manage the return value of the HAL functions but as well error codes and state values, for each call o HAL function, mainly in master mode.

2/ In master mode, The philosophy of the requestFrom is we have to send a request, wait the slave to be ready, and read the data and check errors during the process.
2a/ There is no error check after the TX in function requestFrom
2b/
There is no waiting code in STM32Duino resulting errors returned, or random results, or 0 data, or n random data.
There is something that look like waiting code in function i2c_master_read but it is not well managed as the errors, states and return values from HAL functions are too bad.
For example it is clearly possible that the RX become ready after a while but before the timeout. In this condition, the reading is not repeated resulting wrong results and random behaviors like this one:
https://community.st.com/s/question/0D53W00000EnOPYSA3/stm32l4-hali2cstatebusyrx-issue
but it is not for a short time, when it happens it is stay.

I am not sure part 1/ is related to master as the code on my slave can't see the errors on the line.

I am sure there are other mistakes as I can see sometime strange state values with strange logic state on SCL and SDA with a logic analyzer.

[camelator]

Discussion related to my comment:
My understanding:

• The Arduino class is Wire.h/.cpp
• The file twi.c contains main calls to HAL functions => This is the bridge between the Arduino class and the HAL functions.
• Twi is working in blocking mode for master and in non blocking mode for slave
• STM32Duino is not sticky to blocking mode or not but it should stick to the Arduino way
• It means: Wire.h/.cpp is not sticky to blocking mode or not but it should stick to the Arduino way, and not depend on how twi is working (hal bridge).

=> Based on my understanding (may be wrong), my opinion (may be wrong too):

twi.c is a bad word in this situation. Twi is a protocol very close to I2C protocol. word was defined for copyright reasons. But here the architecture of this file is not related to a dedicated protocol. To me, a better name should be hal_wire_bridge or something like that.

For simplicity reasons Twi should work in the same mode (non blocking) for both slave and master:

• it should manage errors related to I/O functions and hardware lines. Delay management, tempo, etc... Should not be managed here.
• Wire should manage errors related to messages,,delay, tempo, etc... and manage I/O errors coming from twi. Wire can/should work in blocking mode.

To me, the current situation (a mix between non blocking mode for slave, blocking mode for master in file twi) is driving complexity to maintain the code.

delay management in twi may be moved to wire for easy maintenance, and clear separation of tasks.

[ABOSTM]

@camelator,
Few remarks:

I don't know how the tests are done as master, but to me, it may work with fast MCU but not with CPU as I2S slave:
I don't catch the point, I guess you mean I2C slave instead of I2S slave (note that I2S bus also exist).
I don't understand why you oppose fast MCU to CPU: by CPU you mean host computer ? to me generally MCU are slower that CPU.
At the end I tested 2 STM32 one as master(Nucleo-STM32l476RG), one as slave (Nucleo-F103RB and Nucleo-G474RE), and it works fine.

1/ Aim of Stm32duino is not to fix HAL bad error management:
If there are bug in HAL, please report them to HAL official github like:
https://github.com/STMicroelectronics/STM32CubeF7
(by the way I don't know which board/MCU you are using)

Then, about function `HAL_I2C_Master_Seq_Receive_IT()

Then, it is possible (and because of other errors in STM32duino) to have this function returning HAL_OK but reading nothing

It looks normal to me, ..._IT means we use interrupt mode, so aim of this function is not to receive data, it will be handle by IRQ handler.

2a/
At the end, Arduino API: https://www.arduino.cc/reference/en/language/functions/communication/wire/requestfrom/
doesn't offer ability to return an error code to application. Everything is well working when expected read bytes correspond to effective read bytes, else there is probably an error (potentially within the application).

2b/

There is no waiting code in STM32Duino ....
For example it is clearly possible that the RX become ready after a while but before the timeout. 

Hardware clock stretching is there for that purpose: Slave keeps clock low until data is ready to be send. No need for extra software. If Slave doesn't use stretch capability, the master will not wait.
Note: stretch mode is configurable at begin():
void begin(uint8_t, bool generalCall = false, bool NoStretchMode = false);

For simplicity reasons Twi should work in the same mode (non blocking) for both slave and master:

I disagree: aim is not only simplicity, but also efficiency, and respect of Arduino API.
This is necessary for slave to work in non blocking, because it rely on master to decide when it will initiate a transfer.
And Arduino API onreceive() and onrequest() clearly works with callback and non blocking mode.
So, Wire cannot work in blocking for slave.
Whereas in master mode, user expect data to be effectively send when returning from endTransmission(). And it is mandatory as Arduino API expect to return the status of the transmission.

Please note that I convert this thread to discussion, as I see no clear failing use case.
But if you have one, please report it with details to reproduce.

[camelator]

hi,
many thanks for the answer,
few comments about what you said:
2A/
Here is what is written on the Arduino Doc for the return value of function RequestFrom:
byte : the number of bytes returned from the peripheral device.

Consequently Stm32duino should return 0 in case of 0 bytes red. But as I said there are situations it returns for example 8 and data are not consistent instead of returning 0.
as I wrote It is very difficult to know the fail cases because of the architecture design, but I will try to find one in part 2B/

And I am not sure this line is helping:
read = quantity;
How sure are you all the bytes are red? Where did you count them? Are you sure it is compliant to
"byte : the number of bytes returned from the peripheral device." ????

Please see below one of potential source of error:

2B/
Arduino should work in stretch mode or not. This is not a problem.
I think here you didn't catch the point, maybe because I was not clear.

Please have a look in this function:
HAL_I2C_Master_Seq_Receive_IT
Please note what's happen if the I2C line is not ready:
if (hi2c->State == HAL_I2C_STATE_READY)
......
it returns HAL_I2C_BUSY

please come back to the call stack, in function i2c_master_read

We just said HAL_I2C_Master_Seq_Receive_IT returned HAL_I2C_BUSY ( note there are no error code in hi2c->ErrorCode)

in this situation i2c_master_read returns HAL_OK (is it normal ??????)

Please continue to the call stack, in the last function requestFrom. Now we can be sure there are at least two or threee problems here....
As I said there is no TX error management in this function. So the function assuming endTransmission is working fine... hmm.... are you sure???? (but maybe if the line is busy it is because there are problems????, and why trying to continue reading if the TX is not good?????) . Imagine for many reasons, the sending is wrong and you receive data, nothing is saying that data resulting from the bytes you sent!!!! Definitely here is a big mistake in the efficiency....

But let's continue....
Then within our example I2C_master_read returns HAL_OK (but the line was busy and nothing was red!) and requestFrom returns read = quantity ..... without having read anything..... (and problaby without sending anything as well... lol)

and because of no errors management, if the line is broken it remains broken. You can call the function requestFrom 10 times if you want...

Please excuse me for that, I have no falling use case, because it is really hard to understand the exact root cause to reproduce that. But it can happen very often, and as I said, there is no way to come back in the right state when it is broken.

I suspect it is because CPU are not dedicated on single task and it may have a kind of jitter coming from the master, the CPU may not be answered instantaneously and the problem are beginning...

With some complex program and a logic analyzer it is possible to see the problem.
And what we see with a logic analyzer, is since the line enter in a blocked state it remains blocked because requestFrom returns bad values. That's it.
Without a logic analyzer it is not easy:
The function requestFrom will always return the number of bytes and the buffer contains the old values, if it fails.
Assume we made a logic in the slave that is known by the master, and with this logic the two first bytes are different from the previous packet received. Now we can see more errors, because in two calls of RequestFrom It should not have the same buffer values, but as I demonstrated this situation can happened.

etc.... for the list of possible not managed errors.

This is why with this architecture I am asking myself how you can manage master errors in blocking mode? Where / and How you manage timeout between the write and the receive? And How sure you can be sure to match the Arduino description for RequestFrom:
"Returns
byte : the number of bytes returned from the peripheral device."
What happen if the slave return 4 bytes instead of 8 ? To me you return always 8 so it is not compliant to Arduino, and not very efficient.

I don't know if your way is more efficient.... But for sure it is more difficult to debug and find the root cause of the bugs.... Isn't it?

Actually, the only way I found to byepass the bad I2C error management in Stm32Duino was to implement the following error management:

• On the master side: a retry system : 5 consecutive times with strange returns from RequestFrom => Reset the board (to reset the I2C Line)
• On the slave slide: Having 5 seconds without requests in the interrupt functions => Reset the board (for the same reasons)

by this way I am sure to reach a synchronization . Maybe there are other ways but I don't know how to.

[ABOSTM]

2A/
As said, RequestFrom() as defined by Arduino API cannot handle error.
Nevertheless, this API calls i2c_master_read() and this is the place where error handler occurs, like for example:

Arduino_Core_STM32/libraries/Wire/src/utility/twi.c

Line 959 in 7230e09

ret = I2C_BUSY;

Arduino_Core_STM32/libraries/Wire/src/utility/twi.c

Line 980 in 7230e09

ret = I2C_TIMEOUT;

Arduino_Core_STM32/libraries/Wire/src/utility/twi.c

Line 983 in 7230e09

ret = I2C_NACK_DATA;

If i2c_master_read() returns I2C_OK, then RequestFrom() returns the number of bytes read thanks to read = quantity; (so yes it is useful).
on the contrary, if i2c_master_read() returns is different from I2C_OK, then RequestFrom() returns read initial value aka 0.
Conclusion, in case of error detected by i2c_master_read(), then RequestFrom() returns 0.

How sure are you all the bytes are red? Where did you count them?

We don't count, Hardware do it for us: As soon as slave is ready (slave don't stretch the clock), I2C hardware (master) will initiate read operation with the number of bytes requested. And either there is an error detected and it will be handle (see link below) then i2c_master_read() will return different from I2C_OK, and RequestFrom() returns 0. If there is no error, then hardware succeeded to read the requested amount of bytes. No need to count.

2B/
Please tell me which board/MCU/variant you are using, otherwise, I don't know which STM32 series to use for HAL code analyzing.

Please have a look in this function:
HAL_I2C_Master_Seq_Receive_IT
Please note what's happen if the I2C line is not ready:
if (hi2c->State == HAL_I2C_STATE_READY)
......
it returns HAL_I2C_BUSY

I think there is a confusion here:
HAL_I2C_STATE_READY is not the state of the bus, it is the internal state of the HAL I2C driver.

We just said HAL_I2C_Master_Seq_Receive_IT returned HAL_I2C_BUSY but no error code here :hi2c->ErrorCode
in this situation i2c_master_read returns HAL_OK (is it normal ??????)

When HAL_I2C_Master_Seq_Receive_IT() returns HAL_I2C_BUSY (with a timeout) , then i2c_master_read() returns I2C_BUSY

Arduino_Core_STM32/libraries/Wire/src/utility/twi.c

Line 959 in 7230e09

ret = I2C_BUSY;

Note: I am based on latest commit from Core repo but according to your comment I am not sure you have patch #1775

And what we see with a logic analyzer, is since the line enter in a blocked state it remains blocked because requestFrom returns bad values. That's it.

Please details your setup, what board, what MCU, which Stm32duino core version, what is CPU you are talking about, ...
and if you have analyser screenshot, don't hesitate to share them here.

Where / and How you manage timeout between the write and the receive? And How sure you can be sure to match the Arduino description for RequestFrom:

There is no timeout to manage:

• either slave stretchs the SCL clock line Low, meaning it is not ready to provide data, hardware I2C master will wait until line is released.
• or slave does not stretch the line, (or release the line after a stretch period) and master initiates immediately the read sequence

This is automatically managed by hardware.

As I understand at some point your bus is stuck, to go further in the analysis, you should identify which device is blocking the line: slave or master ?
Implementing your patches (board reset), on master only first, then on slave only could help to identify faulty device.