Psoc ino i2c #57
Psoc ino i2c #57
Conversation
IFX-Anusha
force-pushed
the
psoc-ino-i2c
branch
from
b0c07e9 to
ac5c77c
Compare
Signed-off-by: IFX-Anusha <[email protected]>
Signed-off-by: IFX-Anusha <[email protected]>
Signed-off-by: IFX-Anusha <[email protected]>
IFX-Anusha
force-pushed
the
psoc-ino-i2c
branch
from
ac5c77c to
fea6507
Compare
Signed-off-by: IFX-Anusha <[email protected]>
There was a problem hiding this comment.
Good job 🔨 🦾 A few things we can further discuss.
In general, this Wire API I don´t feel is the most intuitive... But this is nothing we can do, unless we change the standard 😃 ⚔️
The API mix basic low level I2C functions START - transfer - STOP, with a atomic transfer functions.
For example, for writing something master-slave we will beginTransmission() and write(), and finally the real transfer is only happening in endTransmission()...
But as said, it is what it is🎈
| @@ -0,0 +1,3 @@ | |||
| ####################################### | |||
| # Syntax Coloring Map For Wire | |||
There was a problem hiding this comment.
This is empty? Will be completed in a another ticket?
| void onRequest(void (*function)(void)); | ||
|
|
||
| private: | ||
| static TwoWire *instances[I2C_HOWNMANY]; |
There was a problem hiding this comment.
Do we need to keep a list of TwoWire instances statically?
Same for the cyhal_ic2_t ?
Isn´t it simpler that each class instance keeps its own struct of cyhal_i2c_t?
| uint8_t instance; | ||
| bool is_master; | ||
| uint16_t slave_address; | ||
| static uint8_t rxBuffer[BUFFER_LENGTH]; |
There was a problem hiding this comment.
If the buffer are static, what would happen when we have 2 instances of the Wire class?
They are shared between the 2 instances? How would that work?
| TwoWire(cyhal_gpio_t sda, cyhal_gpio_t scl, uint8_t instance = 0); | ||
|
|
||
| void begin(); | ||
| void begin(uint16_t address); |
There was a problem hiding this comment.
All these overloaded functions for begin() required?
Wouldn´t uint8_t cover any standard use case address range?
As far as I understand, the I2c standard considers 7-bit addresses. + W/R bit.
And now reading the Wikipedia, I find some oddities like up to 10 bits 🤯
Anyhow, that configuration is not even possible in this API, neither in the cyhal i2c API...
|
|
||
| #include "Wire.h" | ||
|
|
||
| #define I2C_DEFAULT_FREQ 100000 |
There was a problem hiding this comment.
Preferably we do not use macros in C++.
It could be a static const uint16_t?
https://stackoverflow.com/questions/1637332/static-const-vs-define and other reasons.
I also have a few macros to reconvert in the wifi class 😛
There was a problem hiding this comment.
By the way, since we are mixing this anyhow with endless C files, we won´t be able to avoid some of the potential troubles 🤷♂️
|
|
||
| #define I2C_DEFAULT_FREQ 100000 | ||
|
|
||
| uint8_t TwoWire::rxBuffer[BUFFER_LENGTH]; |
There was a problem hiding this comment.
It would be nice if this ring buffer implementation is encapsulated in a minimal class and functions.
For example:
https://gist.github.com/jhschwartz/a7577efd60393fb15bce877282bdceb5#file-ringbuffer-hpp
Or as they did in MicroPython I2S:
https://github.com/Infineon/micropython/blob/ports-psoc6-main/extmod/machine_i2s.c#L111-L115
It should not be a reusable, generic, fully featured ring buffer lib. Just the minimal set of encapsulated features.
The maintenance of this will improve and its readability.
| } | ||
|
|
||
| void TwoWire::begin() { | ||
| end(); |
There was a problem hiding this comment.
What happens if you try to free() without init() ?
I would expect the user to be in charge of calling explicitly to end().
Or any reason to end() when calling begin() ?
Single and Multi board tests in arduino core tests are working