lcgamboa / picsimlab

PICsimLab - Programmable IC Simulator Laboratory
GNU General Public License v2.0
442 stars 85 forks source link

Issue with qemu i2c bitbang #96

Closed fariouche closed 7 months ago

fariouche commented 10 months ago

Hello,

So, I had some time to continue on the i2c implementation on my esp32 board. I've found an error, were the i2c send does not work. There is a small mistake in the code: in picsimlab_i2c_event(), (bsim_qemu.cc), case I2C_READ, the return should be "datas" and not "datar"

With that modification, i can send and receive i2c packet just fine.

By the way, I now understand better what you meant back then by saying that because of the synchronous i2c implementation, you are blocking the simulation until the i2c is read or written.

I have good news: the version of qemu used by esp32 supports async i2c. https://patchwork.ozlabs.org/project/qemu-devel/patch/20220606150732.2282041-18-clg@kaod.org/

I didn't check the stm32 qemu. The calls are a little different in async mode, and i do not know yet how to modify the code to implement this.

lcgamboa commented 10 months ago

Hi,

the bsim_qemu use only the bitbang_i2c_ctrl (I2C Controller/Master). The "datas" (data to send) is not used in the controller implementation. The "datas" is only used by I2C peripherals (bitbang_i2c). The "datar" means data received. With your modification, the examples and tests that use I2C don´t work anymore.

To implement your display communication you should add an I2C peripheral or check the I2C address of the picsimlab_i2c_event callback and do the communication directly without using the I2C controller (and bit-bang), implementing your own switch/case for the address of your display.

I know that Qemu now supports async I2C, but the Espressif ESP32 doesn´t yet. I believe that adding this support to Qemu/PICSimLab will not improve the simulation speed significantly.

fariouche commented 10 months ago

oh, so I misunderstood then how to do it. What I've done is create an i2c device, and call bitbang_i2c from it from a function named (xxx_io) (bitbang_i2c_io, bitbang_i2c_send and bitbang_i2c_get_status) (I basically copied the behavior of rtc_pfc8563) This xxx_io function is called by my board from inside Run_CPU_ns...

What I noticed is that the bitbang_i2c_send call was never received by my firmware application. After changing the bitbang_i2c_ctrl, it started to work. (I was confused by the fact that datas is not used anywhere except as a return status in bitbang_i2c... so I don't understand how this send function can work)

here is the modification I've done. What example are you talking about? (because I checked all the qemu boards and I didn't see any breakage, but they do not use i2c)

index 941e74f7..1007fab7 100644
--- a/src/boards/bsim_qemu.cc
+++ b/src/boards/bsim_qemu.cc
@@ -160,7 +160,7 @@ static int picsimlab_i2c_event(const uint8_t id, const uint8_t addr, const uint1
             g_board->timer.last += 72000;
             g_board->Run_CPU_ns(72000);
             dprintf("<== recv addr=0x%02x value=0x%02x\n", addr, g_board->master_i2c[id].datar);
-            return g_board->master_i2c[id].datar;
+            return g_board->master_i2c[id].datas;
             break;
     }
     return 0;
lcgamboa commented 10 months ago

From the description you are on the correct path, but from your result you must be using just one bitbang_i2c_t object shared between the controller and peripheral (which doesn't work) or you are using two bitbang_i2c_t objects but passing one as an exchanged parameter in the I2C functions.

Are you using one structure like the board K16F uses for PCF8563? Note that pre-treatment must be done on the pins used for I2C for the bus to work. (You can use VCD dump with pulseview I2C decoder to verify)

Theoretically, placing the I2C peripheral update at the end of the if that contains the MStep method call (which updates the I2C controller) should work.

The example is for BM280. The automated tests (in folder tests) uses the BM280 for test I2C of ESP32 too. With your modification, the example and test failed to read data from BM280.

lcgamboa commented 10 months ago

Observs that in the code of pre-treatment when the pins are accessed directly from pins array the index is subtracted from one. For example, pin number 3 is accessed as pins[2], and the functions use the number 3 directly.

fariouche commented 10 months ago

Yes, I've noticed the minus one when accessing the pins directly.

About my device, yes it has its own bitbang_i2c_t struct. And the bsim_qemu has an array of bitbang_i2c_t named master_i2c[] I've used vcdump and pulseview and I did see the I2C signal.

I will check the example to understand how it's done.

Since my device is directly used inside my board, I may have overlooked something. Most likely the access to the pins (however, I know that the pin values are correctly reflecting the I2C transfert since I'm using MGetPin(scl-1) and MGetPin(sda-1) in my device Io function) I will also push my code in my fork too very soon.

lcgamboa commented 10 months ago

Hi @fariouche ,

I have implemented a simple test adding a bmp280 I2C sensor to the devkit board. It works without problems. The modified code and one workspace to test are in the annexed zip file. (It uses the same code as the original bm280 example). You can use it as a base to discover the problem in your implementation.

builtin_bmp280.zip

fariouche commented 10 months ago

Hi @lcgamboa ,

Thanks for the sample. I know understand why my modification was wrong and also why it worked by chance in my use case. I've found the reason my send was not working.... I had to do something similar than SpareParts.SetPullupBus, like so: pins[22 - 1].value &= io_axp192_io(&axp192, MGetPin(23)/scl/, MGetPin(22)/sda/);

It's a bit ugly, as I was not able to use the SpareParts function. Most likely because my device is not a spare part but directly called from inside my board?

lcgamboa commented 10 months ago

For historical reasons, the spare parts simulation is isolated from the board simulation. In the beginning, there was only the board simulation. Spare parts are created after. In the future, perhaps the board could become one spare part. But now I believe you are on the right path.

fariouche commented 10 months ago

it does not shock me to have them separate. One evolution that would be helpful is to hide this "pullup" code so that it is transparent. Maybe inside bitbang_i2c_io?

lcgamboa commented 10 months ago

I'll put this on the to-do list. Lately, I haven't had time to work on PICSimLab.