Add new methods to Wire library #131

ghost · 2017-10-11T09:27:02Z

This solves issue #124
The library can now use an external buffer to write and read data in master mode.
The internal buffer size limitation (BUFFER_LENGTH) is no more a problem (#120) because we don't use in this case the internal buffers.

Note: when you use requestFrom() with an external buffer, do not use available() and read() functions to read the data. requestFrom() will return a not null value if data are available and the data are stored in the external buffer.

I took advantage of the fix to upgrade the library with the Arduino libraries rules.

LMESTM · 2017-10-11T12:38:23Z

cores/arduino/stm32/twi.c

+  if(size > I2C_TXRX_BUFFER_SIZE) {
+    size = I2C_TXRX_BUFFER_SIZE;
+  }
+


Shouldn't we rather return en error?
Otherwise application would get an incomplete frame and will not get an error ...

I quickly patched a possible memory leak so I kept the function "void".
I can return 0 in case of error if you rather.

What I mean is that you need to properly manage errors.

LMESTM · 2017-10-11T12:42:10Z

libraries/Wire/src/Wire.cpp

-      write(data[i]);
-    }
+    txBufferLength = quantity;
+    pWriteData = (uint8_t*)pdata;


before the patch, write() was called and transmission happens.
Now only 2 variables are updated - how does transmission actually start ?

I think you confused master and slave mode. I only change the master mode.
The transmission only happens when endtransmission() is called (before and now).

well what I see here is that you remove the lines:

for(size_t i = 0; i < quantity; ++i){

write(data[i]);

}
and replace them by txBufferLength and pWriteData assignment ... it looks like changing the dynamic

well what I see here is that you remove the lines:
for(size_t i = 0; i < quantity; ++i){
write(data[i]);
}
and replace them by txBufferLength and pWriteData assignment ... it looks like changing the dynamic

Of course I changed the dynamic because I pass directly the data buffer pWriteData to the endTransmission() function (and so to the HAL) and I don't use anymore the internal buffer (low size) which is filled in the write() function.

Ok - I got confused by write() function as I thought it would actually transmit data on I2C lines. Thanks for the clarification.

In fact, @LMESTM is right. the dynamic is not the same and so the txbuffer construction is broken.
I've seen the issue with test on i2C eeprom. See code below:

Wire.beginTransmission(ctrlByte); if (_nAddrBytes == 2) Wire.write( (byte) (addr >> 8) ); //high addr byte Wire.write( (byte) addr ); //low addr byte Wire.write(values, nWrite); txStatus = Wire.endTransmission();

The 2 first calls of Wire.write() put the bytes in the internal txBuffer but when Wire.write(values, nWrite); is called, it initialize pWriteData.
Finally when Wire.endTransmission(); is called:

// Select the buffer where are stored the data to send if(pWriteData != NULL) { data = pWriteData; } else { data = txBuffer; }

the txBuffer datas are ignored....

There are 2 methods completely different now. You can't mix them as you see.

But maybe I found a solution to mix them and keep all older sketches compatibles.
I look at this.

ghost · 2017-11-07T08:38:18Z

Wire library updated to keep the write() method available as required by @fpistm .
I had to create a new method to allow to pass a pointer of buffer. I couldn't modify the write() prototype because it is a virtual method.
Finally I think it is better to add a method and keep the library as close as possible of the Arduino specifications.
@fpistm, @LMESTM is it OK for you?

LMESTM

not sure about the proposal - I have posted comments and I would like to have @fpistm feedback as well.
EDIT: the good point is to have a separate API, this means that it will not break existing libraries and we may live with this proposal until we possibly find a better one ...

LMESTM · 2017-11-07T08:46:47Z

libraries/Wire/README.md

+called only once after _beginTransmission()_. _endTransmission()_ must be called
+immediately after. No other _write()_ method have to be called between
+_beginTransmission()_ and _endTransmission()_ when _writeBuffer()_ is used.
+


I understand the idea and this would work ok - but I am not sure that this requirement (call only once, call endTRansmission immediately after ...) is acceptable - Isn't there an alternative that is more compliant with arduino defaut API ? Coudln't we simply increase internal buffer size ? Or maybe manage malloc & free of extra buffers in case size becomes > 255 then in endTransmission you would send the default buffers + extra ones ? ... other feedbacks are welcome of course

We have discussed with @fprwi6labs about that. Changing the static buffer to a dynamic one will consume memory space. If several I2C instances are used and extra buffer size is required this will reach quickly the maximum space.
Proposal was to kept the current write function and add a new one with the usage restriction.

LMESTM · 2017-11-07T08:47:35Z

libraries/Wire/README.md

+* **requestFrom(uint8_t address, uint16_t quantity, uint8_t \*pdata)**: this
+function allows to read data and store them directly in pdata buffer.
+Do not call _available()_ and _read()_ after this function because data are
+directly stored in pdata buffer.


Same here, if new methods are not easy to understand, I am not sure whether we should offer them

fpistm · 2017-11-07T09:24:30Z

libraries/Wire/src/Wire.cpp

+  * @param  quantity: number of bytes to write
+  * @retval number of bytes read. If > 0 then data are present in pdata buffer.
+  */
+size_t TwoWire::writeBuffer(uint8_t *pdata, size_t quantity)


Add a warning about the pdata to ensure user do not freed or changed this buffer before endTransmission()

fpistm

Ok for me.
Wait NFC test and @LMESTM feedback

LMESTM · 2017-11-07T11:35:21Z

@fpistm @fprwi6labs as said above, ok with me as it will not break existing libraries and we may live with this proposal until we possibly find a better one - but I would not encourage a wide use of this API as long as it is not robust for begin/end and other APIs.

Signed-off-by: fpr <[email protected]>

ghost · 2017-11-08T14:20:53Z

Updated to use dynamic memory allocation. No new method added. Compatibility kept.
In master, the internal buffer size is now limited to the heap size.

fpistm

API compatibility is preserved.
tested with i2C eeprom and its ok.

fpistm · 2017-11-08T15:44:54Z

libraries/Wire/src/Wire.cpp

@@ -97,6 +99,10 @@ void TwoWire::begin(int address)

 void TwoWire::end(void)
 {
+  free(txBuffer);


Check if pointer is null before free it

free() accepts null pointer

If ptr is a null pointer, the function does nothing.

ok, i've got a doubt ;)

fpistm · 2017-11-08T15:45:07Z

libraries/Wire/src/Wire.cpp

@@ -97,6 +99,10 @@ void TwoWire::begin(int address)

 void TwoWire::end(void)
 {
+  free(txBuffer);
+  txBuffer = nullptr;
+  free(rxBuffer);


fpistm · 2017-11-08T15:46:07Z

libraries/Wire/src/Wire.cpp

@@ -109,6 +115,13 @@ uint8_t TwoWire::requestFrom(uint8_t address, uint8_t quantity, uint32_t iaddres
 {
  UNUSED(sendStop);
  if (master == true) {
+    rxBuffer = allocateBuffer(rxBuffer, quantity);
+    // error if not memory block available to allocate the buffer


fpistm · 2017-11-08T16:01:29Z

libraries/Wire/src/Wire.cpp

-    // don't bother if buffer is full
-    if(txBufferLength >= BUFFER_LENGTH){
+    txBuffer = allocateBuffer(txBuffer, txBufferLength + 1);
+    // error if not memory block available to allocate the buffer


…ayer Signed-off-by: fpr <[email protected]>

fpistm · 2017-11-13T08:22:11Z

Hi @fprwi6labs,
could you have a look at this commit to enhance buffer usage:
fpistm@c4b9f24

ghost · 2017-11-14T08:26:59Z

Hi @fpistm

could you have a look at this commit to enhance buffer usage:
fpistm/Arduino_Core_STM32@c4b9f24

It is OK for me.

fpistm · 2017-11-14T09:49:39Z

Ok thanks. Waiting @LMESTM feedback

LMESTM

Few comments to be addressed, but overall I like the idea of not chaging the standard API.
I think the PR should be renamed now because "Add new methods to wire lib" does not make sense anymore. Same for commits titles and commit messages, they need to be updated accordingly.

LMESTM · 2017-11-14T21:47:52Z

libraries/Wire/src/Wire.cpp

+}
+
+/**
+  * @brief  Reset the buffer. Reduce is size if greater than BUFFER_LENGTH.


typo: is size / its size
comment says that it reduce the size only if greater than BUFFER_LENGHT but code seems to do it unconditionnally

Change in the enhancement

LMESTM · 2017-11-14T21:49:26Z

libraries/Wire/src/Wire.cpp

+  buffer = (uint8_t *)realloc(buffer, BUFFER_LENGTH * sizeof(uint8_t));
+  if(buffer != nullptr) {
+    memset(buffer, 0, BUFFER_LENGTH);
+  }


in case of nullptr, we need to raise an error (no memory ...).
I 'm not sure this is checked after every call to resetBuffer ...

Checked in the R/W function

LMESTM · 2017-11-14T21:49:56Z

libraries/Wire/src/Wire.cpp

@@ -66,9 +66,11 @@ void TwoWire::begin(uint8_t address)
 {
  rxBufferIndex = 0;
  rxBufferLength = 0;
+  rxBuffer = resetBuffer(rxBuffer);


if null raise en error ?

Error is raised later if rx/txBuffer is NULL

LMESTM · 2017-11-14T21:50:04Z

libraries/Wire/src/Wire.cpp


  txBufferIndex = 0;
  txBufferLength = 0;
+  txBuffer = resetBuffer(txBuffer);


Error is raised later if rx/txBuffer is NULL

LMESTM · 2017-11-14T21:50:21Z

libraries/Wire/src/Wire.cpp

@@ -216,9 +224,15 @@ uint8_t TwoWire::endTransmission(uint8_t sendStop)
    break;
    }

+    //Reduce buffer size to free memory in case of large memory use
+    if(txBufferLength > BUFFER_LENGTH) {
+      txBuffer = resetBuffer(txBuffer);


raise an errror if null ?

Will be removed in my PR.

LMESTM · 2017-11-14T21:54:16Z

libraries/Wire/src/Wire.cpp

@@ -241,8 +255,9 @@ size_t TwoWire::write(uint8_t data)
 {
  if(transmitting){
  // in master transmitter mode
-    // don't bother if buffer is full
-    if(txBufferLength >= BUFFER_LENGTH){
+    txBuffer = allocateBuffer(txBuffer, txBufferLength + 1);


maybe not efficient to allocate +1 byte each time - maybe you could go by steps (allocateBuffer(txBuffer, txBufferLength + BUFFER_LENGTH) each time it is needed) because reallocation can be costly in performances?
In that way BUFFER_LENGHT would be used as the "unit" of buffer allocation.

Mainly call to set first bytes of the data (ex: address).
See fpistm@c4b9f24
In this commit the min size is BUFFER_LENGTH.

fpistm · 2017-11-15T16:10:29Z

This PR has been replaced by #152

ghost mentioned this pull request Oct 11, 2017

Remove I2C buffer length limitation required by M24SR64-Y library #120

Closed

fpistm assigned fpistm and ghost and unassigned fpistm Oct 11, 2017

fpistm self-requested a review October 11, 2017 12:38

LMESTM reviewed Oct 11, 2017

View reviewed changes

ghost mentioned this pull request Oct 12, 2017

Add source files stm32duino/M24SR64-Y#1

Merged

ghost mentioned this pull request Oct 25, 2017

Add new method to Wire library #124

Closed

ghost requested a review from LMESTM November 7, 2017 08:38

LMESTM reviewed Nov 7, 2017

View reviewed changes

fpistm reviewed Nov 7, 2017

View reviewed changes

ghost requested a review from fpistm November 7, 2017 10:58

fpistm reviewed Nov 7, 2017

View reviewed changes

fpr added 2 commits November 8, 2017 09:34

Update Wire library properties

4ec461d

Signed-off-by: fpr <[email protected]>

Add src folder to Wire library

f0a930a

Signed-off-by: fpr <[email protected]>

ghost requested review from LMESTM and fpistm November 8, 2017 14:21

fpistm requested changes Nov 8, 2017

View reviewed changes

Add new methods to Wire library to pass user buffer directly to I2C l…

8cbdc0b

…ayer Signed-off-by: fpr <[email protected]>

LMESTM reviewed Nov 14, 2017

View reviewed changes

fpistm mentioned this pull request Nov 15, 2017

Change Wire Rx/Tx buffers managements #152

Merged

fpistm closed this Nov 15, 2017

fpistm mentioned this pull request Jun 6, 2023

gh pr list do not show some PR cli/cli#7538

Closed

Add new methods to Wire library #131

Add new methods to Wire library #131

Conversation

ghost commented Oct 11, 2017

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

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

ghost commented Nov 7, 2017

LMESTM left a comment • edited Loading

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

fpistm left a comment

Choose a reason for hiding this comment

LMESTM commented Nov 7, 2017

ghost commented Nov 8, 2017

fpistm left a 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

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

fpistm commented Nov 13, 2017

ghost commented Nov 14, 2017

fpistm commented Nov 14, 2017

LMESTM left a 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

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

Choose a reason for hiding this comment

fpistm Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

fpistm commented Nov 15, 2017

LMESTM left a comment •

edited

Loading

fpistm Nov 15, 2017 •

edited

Loading