Raspberry Pi Pico I2C Interface Implementation #20677

studiosoftdev · 2024-05-17T00:31:35Z

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

we can run vcnl4040 from there

…go no init

… 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

…t now

…, pin 10 lit

…us tests to find error with device ID, should be 0x20?

…upported in riot anyway)

dylad

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

dylad · 2024-05-17T16:10:24Z

boards/rpi-pico-w/Makefile

@@ -1,2 +1,2 @@
-DIRS = $(RIOTBOARD)/rpi-pico
+DIRS = $(RIOTBOARD)/rpi-pico-w


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 ?

Not that I remember, I think I just thought the pico-w was without a sufficient board definition

.vscode/settings.json

dylad · 2024-05-17T16:15:05Z

boards/rpi-pico/include/periph_conf.h

 /**
 *  @brief  Number of I2C interfaces
 */
-#define I2C_NUMOF       _periph_numof_is_unsigned_0()
+#define I2C_NUMOF       2 //_periph_numof_is_unsigned_0()


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.

Good idea, I will modify this soon when I have some more time for this.

dylad · 2024-05-17T16:16:06Z

cpu/rpx0xx/include/periph_cpu.h

+ * @brief   I2C configuration options
+ */
+typedef struct {
+    uint32_t* dev; //device pointer should have better type probably


Please use C Style /* */ only.
You can have a look at our coding convention here

Apologies, I will fix this.

dylad · 2024-05-17T16:18:31Z

dist/tools/pyterm/pyterm

@@ -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)


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=...

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 ...

drivers/include/periph/i2c.h

kfessel

first look -> this needs some work

kfessel · 2024-06-03T18:39:55Z

cpu/rpx0xx/periph/i2c.c

+    _i2c_bus[dev].cmd = 0; // send write cmd
+    *(baseaddr + IC_DATA_CMD) = _i2c_bus[dev].cmd;
+
+    uint8_t tx_buffer[256] = {};


this seems a bit exessive ( 256 byte buffer to store 3 )

kfessel · 2024-06-03T18:42:08Z

cpu/rpx0xx/periph/i2c.c

+    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);


KISS

Suggested change

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);

}

kfessel · 2024-06-03T18:44:28Z

cpu/rpx0xx/periph/i2c.c

+
+

check for double newlines ( the CI will also and it will complain)

Suggested change

kfessel · 2024-06-03T18:46:30Z

cpu/rpx0xx/periph/i2c.c

+    }
+
+    //copy data into txbuffer after the reg
+    memcpy(&tx_buffer[reg_len], data, len);


ensure you do not overflow buffers with memcpy

(len > 256 will for sure)

kfessel · 2024-06-03T18:49:26Z

cpu/rpx0xx/periph/i2c.c

+    DEBUG("i2c write %d bytes", len);
+
+    //prepare i2c bus
+    long* baseaddr = (long *) I2C0_BASE;


this looks like you are using I2C -hardware and next 40 lines

kfessel · 2024-06-03T18:50:51Z

cpu/rpx0xx/periph/i2c.c

+    }
+    memcpy(txbuffer, data, len);
+
+    //init pins


this and next 60 looks like you are doing bitbanging

kfessel · 2024-06-03T18:51:51Z

cpu/rpx0xx/periph/i2c.c

+    for(int i = 0; i < 256; i++){
+        txbuffer[i] = 0;
+    }


this might be a memset

but since there is a memcpy in the next line it might not be needed

studiosoftdev added 16 commits March 15, 2024 23:21

added all the new files with the in progress vcnl4040 script

75f673b

testing pi_test code to first check blinking ability and then seeing if

2f73642

we can run vcnl4040 from there

blink test now working, next to implement vcnl4040 testing

3d409f2

full looped blink test

5a0dbfb

first (non-working) test of ambient light read

4b8e360

testing whether init actually works, status to 35, nothing showing er…

dc12360

…go no init

current delivery was issue in vcnl40x0_params, now can see pin again,…

61f2811

… read reg probably still not working

rewriting reads because not following process, wrong write to dataadd…

1c5cfe8

…r with 0xFF mask instead of 0xF and also cmd not being put in IC_DATA_CMD[8]

implemented write_reg(s) functions in a manner that i think correctly…

53bb9ca

… understands what the reg parameter is meant to do, needs write_bytes to work now

implementing write_bytes, fixed i2c_config for pico, need i2c_transmi…

2f5d1ad

…t now

implementation of i2c_transmit

6812dd3

build that SEEMS like it outputs some non-0 value on amb light sensor…

79727ce

…, pin 10 lit

nvm ... it was status == -2 causing gpio 10 to light ... rewrote stat…

433cef6

…us tests to find error with device ID, should be 0x20?

pre recombobulating i2c_transmit into read/write bytes

5e561a6

added rpi-pico-w board support (same as rpi-pico as wifi module not s…

d7f93b0

…upported in riot anyway)

final pre-project deadline commit: reintroducing IC_TAR reg control

4342927

studiosoftdev requested a review from MrKevinWeiss as a code owner May 17, 2024 00:31

github-actions bot added Area: drivers Area: Device drivers Area: tools Area: Supplementary tools Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications labels May 17, 2024

dylad self-requested a review May 17, 2024 07:13

benpicco requested review from maribu and fabian18 May 17, 2024 09:44

dylad reviewed May 17, 2024

View reviewed changes

kfessel reviewed Jun 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raspberry Pi Pico I2C Interface Implementation #20677

Raspberry Pi Pico I2C Interface Implementation #20677

studiosoftdev commented May 17, 2024

dylad left a comment

dylad May 17, 2024

studiosoftdev May 21, 2024

dylad May 17, 2024

studiosoftdev May 21, 2024

dylad May 17, 2024

studiosoftdev May 21, 2024

dylad May 17, 2024 •

edited

studiosoftdev May 21, 2024

kfessel left a comment

kfessel Jun 3, 2024

kfessel Jun 3, 2024

kfessel Jun 3, 2024

kfessel Jun 3, 2024

kfessel Jun 3, 2024

kfessel Jun 3, 2024

kfessel Jun 3, 2024

		@@ -1,2 +1,2 @@
		DIRS = $(RIOTBOARD)/rpi-pico
		DIRS = $(RIOTBOARD)/rpi-pico-w

Raspberry Pi Pico I2C Interface Implementation #20677

Are you sure you want to change the base?

Raspberry Pi Pico I2C Interface Implementation #20677

Conversation

studiosoftdev commented May 17, 2024

Contribution description

Testing procedure

Issues/PRs references

dylad 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

dylad May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfessel 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

dylad May 17, 2024 •

edited