RJRP44 / VL53L8CX-Library

💽 A vl53l8cx library for esp32 using the esp-idf framework
Other
4 stars 2 forks source link

I2C driver update #1

Open Martin12350 opened 6 months ago

Martin12350 commented 6 months ago

Any plans to update I2C for the new driver? Currently, everything works, and thanks for this lib, but the annoying warning shows up:

W (xxx) i2c: This driver is an old driver, please migrate your application code to adapt driver/i2c_master.h

If I am right only affected file should be platform.c with those functions:

illusionfield commented 6 months ago

Hi there!

I noticed that a similar issue has been raised concerning the VL53L5CX library, which can be found here: #7.

It's important to note that although the two libraries are not updated simultaneously, any solutions developed can be easily adapted between them. Therefore, I recommend keeping an eye on the other discussion as well, as any progress there might be beneficial to us. Please be aware that while a solution might quickly become available in one library, implementing it in the other may occur with a slight delay.

Martin12350 commented 6 months ago

Hi, and thanks for the notice. As said here and even there, it still works fine and everything is ok. I am just fearing the day when the IDF will say: "Not anymore, we warned you about it" ;)

RJRP44 commented 5 months ago

I just published a new version vl53l8cx v2.0.1. It should work as well as the old one !

illusionfield commented 5 months ago

Thank you so much for the work you've put into this library!

Martin12350 commented 5 months ago

Wow, did not expect so fast release. Definitely will check asap. Even examples updated. Thanks a lot. 👍

Martin12350 commented 5 months ago

Everything works as expected, without any issue and any warning :) However, I have a question about the buffer choice. Would not be better to dynamically create the buffer based on the Read/Write size in the function? Because now we waste a lot of memory, which is potentially needed to be so big only for FW init. And since I am using only one sensor, another half of the buffer is totally unusable. Am I right? Also, can I continue on this thread, or should I close it and create a new one?

Memory utilization difference report (v1.2.1 -> v2.0.1)

DATA_FLASH: -1212 INSTR_FLASH: +2356 INSTR_RAM: -4184 DATA_RAM: +64K (25% on my ESP)

illusionfield commented 5 months ago

I updated the project today and it works great!

RJRP44 commented 5 months ago

Everything works as expected, without any issue and any warning :) However, I have a question about the buffer choice. Would not be better to dynamically create the buffer based on the Read/Write size in the function? Because now we waste a lot of memory, which is potentially needed to be so big only for FW init. And since I am using only one sensor, another half of the buffer is totally unusable. Am I right? Also, can I continue on this thread, or should I close it and create a new one?

Memory utilization difference report (v1.2.1 -> v2.0.1)

DATA_FLASH: -1212 INSTR_FLASH: +2356 INSTR_RAM: -4184 DATA_RAM: +64K (25% on my ESP)

That was my main concern while implementing this new update, I chosen the use of a static buffer for the simplicity of use. Although dynamically allocated buffer work well they require increasing the main stack size or using threads witch is not that hard I grant you that. So may I suggest something. What if the user could decide between the ease of use with the static buffer and the optimization with the dynamic one via the menuconfig?

Martin12350 commented 5 months ago

I was trying to find a way to avoid the necessity of the buffer at all, because we actually already have all the data in p_values. But was unable to find a way to push in RegisterAdress (typo with capital letter? haha), digging in the I2C driver is not a good way to go, and changing all the code, because of this is also not a good idea. RdMulti we can fix very easily, because there are always only 2 bytes. But that WrMulti is a tough one and only "clever" solution I figured out was dynamic allocation. When playing with the code, I also noticed that the timeout is set to -1, while in the previous version you had 1s. Should it not be wise to have the same timeout?

Edit: That is what I was looking for: i2c_master_prefixed_transmit ESP-IDF API feat, which would solve our issue.

RJRP44 commented 5 months ago

Oh this PR is exactly what I wanted to do yesterday in the lib but I soon realized that it would be impossible because everything is static. It would be nice if this ultimately get merged, it would avoid a lot of headaches. For the WrMulti I am going to try dynamic allocation with segmentation into small chunks to use less heap. And yes it could be a good idea to set the timeout to 1s.

PS : I am not responsible of this terrible naming on the functions

Martin12350 commented 5 months ago

I really hope it will get merged soon and then will knock on your door again :D Currently I am not limited by RAM, so I am fine with the current release, the only issue I can think of is the timeout, which maybe could be in the settings.

And one more small feature, could be inverted reset, or some settings flag. Now is from 0->1, but in my case I need 1->0. To be precise, the sensor reset procedure is as follows (from STM doc #4.2):

Reset the I²C communication by toggling the SPI_I2C_N pin.

Because PWR_EN is only on the SATEL module, not the sensor itself.

And that is pretty much all I could think of. One more time - thank You for the time and the lib :)

RJRP44 commented 5 months ago

This is not the sensor reset, this is, as you said the I²C communication reset. I had an issue, sometimes the sensor refused to apply a new configuration so the only way I found is to power it down in order to reset it, because the I²C communication reset doesn't reset the sensor itself.

Sensor reset management

To reset the device, the following pins need to be toggled:

  1. Set pins VDDIO, AVDD, and CORE_1V8 to low.
  2. Wait 10 ms.
  3. Set pins VDDIO, AVDD, and CORE_1V8 to high.

UM3109 #4.2

The reset function needs to be implemented by the user on the power supply of the sensor for a custom circuit. But I agree, more customization via the settings could be nice.

It is a pleasure to see your interest in this lib

Martin12350 commented 5 months ago

You are correct, not a reset like a reset :) But could be a bit more flexible. Honestly, did not have the need to reset, after start (setting the pin 0) and init, all works fine even for days. But I am trying to think ahead, what would happen if the sensor got stucked, reset would be handy instead of a whole ESP restart.

RJRP44 commented 5 months ago

image Let me know what you think about this.

And by the way I tried the i2c_master_prefixed_transmit PR and it works flawlessly !

Martin12350 commented 5 months ago

That is awesome :) Now we need to push the ESP guys to merge it hehe.

RJRP44 commented 5 months ago

It seems to have been added to the idf af1524e 🎉🎉

illusionfield commented 5 months ago

This is awesome, I can't wait to see which release it will appear in.

Martin12350 commented 5 months ago

Ohh, they even made it better, now you can send multiple buffers in one go. The example with LCD looks very cool. Can't wait for release :)