[driver] driver for ST vl53l5, vl53l7, vl53l8 #1296
Conversation
hshose
force-pushed
the
feature/vl53
branch
2 times, most recently
from
d5c4350 to
1d8b904
Compare
chris-durand
left a comment
chris-durand
left a comment
There was a problem hiding this comment.
Very nice! I don't have the time for a full review because I'll be on a dive boat in the Andaman sea in 2 hours ;)
Putting ST's code into ext is the right way to go. Getting it from the website is also fine because it's BSD licensed and you are providing the automated update script. @salkinium or @rleh can probably help with making a repo in modm-ext.
hshose
force-pushed
the
feature/vl53
branch
2 times, most recently
from
b2b4199 to
b2d643f
Compare
|
All examples work in HW now. If you set up the ext repo, I can add the submodule and move the ST files there... |
|
I created https://github.com/modm-ext/vl53-partial and granted you access. |
|
Adding submodule done. |
hshose
force-pushed
the
feature/vl53
branch
2 times, most recently
from
84974a0 to
a2d2cda
Compare
salkinium
left a comment
salkinium
left a comment
There was a problem hiding this comment.
Thanks, just some minor improvements needed.
hshose
force-pushed
the
feature/vl53
branch
2 times, most recently
from
b5d0896 to
b24a4ac
Compare
|
Thank you @salkinium for the detailed review, I tried to address most of the issues, only the stack and example include questions remain. I left the stack as is because the I2C examples seem to need this, I changed the SPI examples stack to 16kB. |
ext/vl53/vl53_transport_impl.hpp
Outdated
|
|
||
| modm::this_fiber::poll([&] { return this->acquireMaster(); }); | ||
| Cs::reset(); | ||
| SpiMaster::transfer(&tx_buffer[0], nullptr, data_size); |
There was a problem hiding this comment.
Why not do it in two transfers?
uint8_t addr_buffer[2];
addr_buffer[0] = SPI_WRITE_MASK(temp) >> 8;
addr_buffer[1] = SPI_WRITE_MASK(temp) & 0xFF;
SpiMaster::transfer(addr_buffer, nullptr, sizeof(addr_buffer));
SpiMaster::transfer(p_values, nullptr, size);that saves the memcpy and the scratch buffer
There was a problem hiding this comment.
I used to do this, and it works with the SpiMaster2.
However, with the SpiMaster2_Dma it doesn't work and I don't really know why. Any ideas?
When I have the separate TX Buffer and memcopy the write content into that Buffer, it also works with the SpiMaster2_Dma.
There was a problem hiding this comment.
Not sure, but the DMA buffers need to be 4B aligned. If you make the addr_buffer[0] a member it should be 4B aligned.
There was a problem hiding this comment.
Making addr_buffer a member doesn't help. How would i know that p_values is aligned properly?
There was a problem hiding this comment.
Not sure, but the DMA buffers need to be 4B aligned. If you make the
addr_buffer[0]a member it should be 4B aligned.
Is that really required? The SPI DMA only performs byte accesses and it should even support unaligned access on normal memory. The only references on DMA alignment requirements I could find were on device and strongly-ordered memory which need to be properly aligned.
However, when you use cacheable memory DMA buffers need to be aligned to the cache line size for cache maintenance operations to sensibly work. modm doesn't support that anyway. @hshose Just to make sure, you didn't manually enable the data cache, right?
There was a problem hiding this comment.
I wouldn't even know how to turn this on 😄 Also making the addr_buffer a class member doesn't help.
There was a problem hiding this comment.
I pushed a workaround to a common cache bug on Cortex-M7. It probably won't fix the DMA issue, but it's something we need to fix in modm anyways.
There was a problem hiding this comment.
Erratum 1259864 only applies if a memory region is configured in write-through mode. In the default memory map all internal cacheable RAM and almost all external RAM is write-back write-allocate. At least in the 3 H7 manuals I had on my laptop. Only external RAM connected via QUAD/OCTOSPI would be write-through by default. We don't support that, so we should be fine.
ext/vl53/vl53_transport_impl.hpp
Outdated
|
|
||
| modm::this_fiber::poll([&] { return this->acquireMaster(); }); | ||
| Cs::reset(); | ||
| SpiMaster::transfer(&tx_buffer[0], nullptr, data_size); |
There was a problem hiding this comment.
Not sure, but the DMA buffers need to be 4B aligned. If you make the addr_buffer[0] a member it should be 4B aligned.
|
Yes, the macOS CI is super slow (~2h), I'm trying to fix it over at #1301. |
4 participants
STs Ultra Light Driver for VL53L5 (I2C), VL53L7 (I2C), and VL53L8 (I2C, SPI) 8x8 Pixel Time-of-Flight Sensors
As the interaction with the sensor is pretty complicated (many undocumented registers and huge binary blobs), I use the ultra light drivers (ULD) for the sensors as provided by ST.
If you think this is a good way of integrating support for these sensors, I suggest to add these ULDs as a submodule under ext (see temp commit here for proposed structure). Does this work for y'all?
The VL53L5 driver exists undre STs namespace on github, the VL53LMZ driver for VL53L7 and VL53L8 only on STs website, I thus decided on STs website as datasorce for all drivers; I hope this is fine?
The
update.pyscript in the future submodule automatically extracts the files, fixes some typos and bugs in their C-API, prefixes global functions with "VL53_", and unifies the use of theVL53_Platformstruct among both drivers and their examples.modm transport
I added the modm transport SPI and I2C classes in the
ext/vl53folder, is this the right place or should they go in thesrc/modm/driver/positionfolder?Examples
The VL53L8 sensor supports SPI and I2C, the VL53L7 sensor (same driver as the VL53L8 but wider field of view) supports only I2C, the VL53L5 supports only I2C.
Examples should cover these cases.
Todos:
Future plans: