-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Raspberry Pi Pico I2C Interface Implementation #20677
base: master
Are you sure you want to change the base?
Conversation
we can run vcnl4040 from there
… read reg probably still not working
…r with 0xFF mask instead of 0xF and also cmd not being put in IC_DATA_CMD[8]
… understands what the reg parameter is meant to do, needs write_bytes to work now
…us tests to find error with device ID, should be 0x20?
…upported in riot anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @studiosoftdev
Thanks for this PR.
Before going into a more in-depth review, I noticed a few things. See inline comments.
I also see that your I2C implementation is a GPIO bit banging one. Do you have any reason not using the I2C peripheral instead ?
In addition, if you have question, feel free that ask here or on matrix.
You might also find useful information regarding contribution here
@@ -1,2 +1,2 @@ | |||
DIRS = $(RIOTBOARD)/rpi-pico | |||
DIRS = $(RIOTBOARD)/rpi-pico-w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rpi-pico-w
inherits its configuration rpi-pico
so I am wondering why you need this ?
Did you encounter any issue with rpi-pico-w
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I remember, I think I just thought the pico-w was without a sufficient board definition
/** | ||
* @brief Number of I2C interfaces | ||
*/ | ||
#define I2C_NUMOF _periph_numof_is_unsigned_0() | ||
#define I2C_NUMOF 2 //_periph_numof_is_unsigned_0() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We strongly encourage the use of ARRAY_SIZE()
macro here. This way, there is no need to update this define everytime we modify an entry inside the i2c_config struct
Moreover, it would be better to also include the I2C PIO for those who need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I will modify this soon when I have some more time for this.
* @brief I2C configuration options | ||
*/ | ||
typedef struct { | ||
uint32_t* dev; //device pointer should have better type probably |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use C Style /* */ only.
You can have a look at our coding convention here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I will fix this.
@@ -128,7 +128,7 @@ class SerCmd(cmd.Cmd): | |||
|
|||
# initialize class members | |||
cmd.Cmd.__init__(self) | |||
self.port = port | |||
self.port = "/dev/tty0" #supposed to just be 'port' (no quotes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unrelated.
I'm guessing you are on Windows, probably using WSL2.
You may want to try to use PORT=/dev/tty0 BOARD=rpi-pico-w make -C tests/leds flash term
for instance.
You can change the default serial port configured by the build system using PORT=...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not supposed to be in the version that got the PR! This was a hack I had used (because I didn't know about PORT=...
) but I had thought I had changed that back some time ago ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first look -> this needs some work
_i2c_bus[dev].cmd = 0; // send write cmd | ||
*(baseaddr + IC_DATA_CMD) = _i2c_bus[dev].cmd; | ||
|
||
uint8_t tx_buffer[256] = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit exessive ( 256 byte buffer to store 3 )
int reg_len = 0; //know register length in bytes | ||
//if in 16-bit register addressing mode, split across tx bytes | ||
if(flags & I2C_REG16){ | ||
tx_buffer[0] = reg >> 8; | ||
tx_buffer[1] = reg & 0xFF; | ||
reg_len = 2; | ||
} | ||
else { | ||
tx_buffer[0] = reg; | ||
reg_len = 1; | ||
} | ||
|
||
tx_buffer[reg_len] = data; | ||
int retstatus = i2c_write_bytes(dev, addr, tx_buffer, 3, flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KISS
int reg_len = 0; //know register length in bytes | |
//if in 16-bit register addressing mode, split across tx bytes | |
if(flags & I2C_REG16){ | |
tx_buffer[0] = reg >> 8; | |
tx_buffer[1] = reg & 0xFF; | |
reg_len = 2; | |
} | |
else { | |
tx_buffer[0] = reg; | |
reg_len = 1; | |
} | |
tx_buffer[reg_len] = data; | |
int retstatus = i2c_write_bytes(dev, addr, tx_buffer, 3, flags); | |
//if in 16-bit register addressing mode, split across tx bytes | |
if(flags & I2C_REG16){ | |
tx_buffer[0] = reg >> 8; | |
tx_buffer[1] = reg & 0xFF; | |
tx_buffer[2] = data | |
int retstatus = i2c_write_bytes(dev, addr, tx_buffer, 3, flags); | |
} | |
else { | |
tx_buffer[0] = reg; | |
tx_buffer[1] = data; | |
int retstatus = i2c_write_bytes(dev, addr, tx_buffer, 2, flags); | |
} |
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for double newlines ( the CI will also and it will complain)
} | ||
|
||
//copy data into txbuffer after the reg | ||
memcpy(&tx_buffer[reg_len], data, len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensure you do not overflow buffers with memcpy
(len > 256 will for sure)
DEBUG("i2c write %d bytes", len); | ||
|
||
//prepare i2c bus | ||
long* baseaddr = (long *) I2C0_BASE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like you are using I2C -hardware and next 40 lines
} | ||
memcpy(txbuffer, data, len); | ||
|
||
//init pins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and next 60 looks like you are doing bitbanging
for(int i = 0; i < 256; i++){ | ||
txbuffer[i] = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be a memset
but since there is a memcpy in the next line it might not be needed
Contribution description
Pi Pico I2C support has an implementation now although it is not fully working - it is a baseline implementation for improvement.
Testing procedure
Testing has been implemented by a new example program (pi_i2c) which uses a VCNL4040 device to test the functionality in a simple real-world application. Currently, the device does not report back a correct device ID read and sets GPIO 11 to high.
Issues/PRs references