Wire: improve code, add safety checks, add basic logging#171
Wire: improve code, add safety checks, add basic logging#171JZimnol wants to merge 6 commits intozephyrproject-rtos:nextfrom
Conversation
JZimnol
requested review from
DhruvaG2000,
beriberikix,
soburi and
szczys
as code owners
|
We are considering integrating this part with Arduino, so could you please wait a little while? |
|
We are using this repository as our intent is to use Arduino libraries within Zephyr application. Therefore we would like to have it in this repository |
Yes, we are aware of the issue. |
@soburi please could you elaborate on what you mean by this part? Are you saying these fixes are already part of the Arduino fork or you're going to send a PR with these fixes there first? |
libraries/Wire/Wire.cpp
Outdated
| return (int)buf[0]; | ||
|
|
||
| if(!ring_buf_get(&rxRingBuffer.rb, buf, 1)) { | ||
| printk("\n\nERR: buff empty\n\n\n"); |
There was a problem hiding this comment.
oo many unnecessary \n's .
Just keep it as printk("ERR: buff empty");
Also, elaborate more on the fix itself in the commit "Wire: fix the behavior of the arduino::ZephyrI2C::read()"
libraries/Wire/Wire.cpp
Outdated
| /* Use stack-allocated buffer for reading */ | ||
| uint8_t readbuff[sizeof(rxRingBuffer.buffer)]; | ||
| if (len > sizeof(readbuff)) { | ||
| printk("\n\nERR: requested length (%zu) exceeds buffer size (%zu) \n\n\n", |
There was a problem hiding this comment.
Again, too many unnecessary \n's ...
| /* Flush the receive buffer so another read() call returns the correct data */ | ||
| flush(); | ||
| ret = ring_buf_put(&rxRingBuffer.rb, rxRingBuffer.buffer, len); | ||
| ret = ring_buf_put(&rxRingBuffer.rb, readbuff, len); |
There was a problem hiding this comment.
Can you elaborate more on the reason in the commit "Wire: don't use rxRingBuffer.buffer directly for i2c_read() calls"
| } | ||
| if (!ring_buf_peek(&rxRingBuffer.rb, buf, 1)){ | ||
| // No data available | ||
| return -1; |
There was a problem hiding this comment.
The commit "Wire: fix the behavior of the arduino::ZephyrI2C::peek()" could also use some more explanation in it's body
|
|
||
| if(!ring_buf_get(&rxRingBuffer.rb, buf, 1)) { | ||
| printk("\n\nERR: buff empty\n\n\n"); | ||
| LOG_ERR("buffer is empty"); |
There was a problem hiding this comment.
Ah in the last commit you're changing everything to LOG_ERR. Fine, then you need to drop the printk changes from all your earlier commits because then it's just noise.
There was a problem hiding this comment.
@DhruvaG2000 would you like me to squash all those commits into one with a proper name and description then? In the first commits I've been doing some fixes only with the printks being unchanged, that's why it might look misleading. I can squash everything if you want.
There was a problem hiding this comment.
@JZimnol no, don't squash. I like how you've partitioned all the fixes. I would just like to see an explanation for each in their own commits, and also those commits need to contain only the fix, you can just remove the printk's from those places, or else just use LOG_ERR in those commits itself.
There was a problem hiding this comment.
@DhruvaG2000 I've added required changes + descriptions to some of the commits.
Implemented flush() function to clear the rx buffer. The buffer needs to be flushed so the next read() call returns the proper data. Signed-off-by: Jakub Zimnol <zimnol.jakub@gmail.com>
Removed unecessary ring buffer peek. Additionally added negative value as a return value in case of an error. Signed-off-by: Jakub Zimnol <zimnol.jakub@gmail.com>
Removed a bug from requestFrom() function that caused ring buffer corruption by using rxRingBuffer.buffer directly in i2c_read() calls. Signed-off-by: Jakub Zimnol <zimnol.jakub@gmail.com>
Added a negative return value to peek() in case there's no data to read. Signed-off-by: Jakub Zimnol <zimnol.jakub@gmail.com>
Signed-off-by: Jakub Zimnol <zimnol.jakub@gmail.com>
Signed-off-by: Jakub Zimnol <zimnol.jakub@gmail.com>
JZimnol
force-pushed
the
wire_lib_improvements
branch
from
73880fb to
80c15be
Compare
4 participants
This PR fixes and changes a few things related to the
wireport of the Arduino library. All changes are done in separate commits for readability. The most important functions changed are thearduino::ZephyrI2C::requestFrom()andarduino::ZephyrI2C::read().Additionally, the
ARDUINO_APIlog module/levels have been added enabling an user to control the log level of logs in this repository. As an example,printk()calls have been switched with theLOG_*API.