Complete SPI API, added display ILI9341, added spi bme280, updated i2c bme280 #116
Conversation
There was a problem hiding this comment.
Nicely done! I've left a few comments for your enoyment :)
I'll try to make some time to look at this in an actual code editor during the weekend and also make sure the the SPI version still works. Any reason you didn't update the top level CMakeList.txt with these added tests?
Updated tag to char*, updated res in read_configuration, read_trimming_parametersand read_measurements to put && on next line.
added additional comments pertaining to heap corruption and dummy byte
Not sure what the best approach for alignas replacement
|
**From esp32 API- spi master**
Normally, the data that needs to be transferred to or from a Device will
be read from or written to a >chunk of memory indicated by the members
rx_buffer and tx_buffer of the structure spi_transaction_t
<https://docs.espressif.com/projects/esp-idf/en/latest/api-reference/peripherals/spi_master.html#_CPPv417spi_transaction_t>.
If DMA is >enabled for transfers, the buffers are required to be:
1. Allocated in DMA-capable internal memory. If external PSRAM is enabled
<https://docs.espressif.com/projects/esp-idf/en/latest/api-reference/system/mem_alloc.html#dma-capable-memory>,
this means using pvPortMallocCaps(size, MALLOC_CAP_DMA).
2. 32-bit aligned (staring from a 32-bit boundary and having a length of
multiples of 4 bytes).
If these requirements are not satisfied, the transaction efficiency will
be affected due to the >allocation and copying of temporary buffers.
Since we are using the SPI device with DMA I would like to make a class
like your FixedBuffer maybe call it DMAFixedBuffer that would use
heaps_caps_malloc(size, MALLOC_CAP_DMA). I would make this class do static
assert to make sure size is modulus of 4. I think I would need to
implement free()? So this will take me a few days to figure out. This type
of buffer (DMAFixedBuffer) would be used when the SPI device used dma.
Does this sound like a good approach?
…On Sat, Nov 9, 2019 at 6:47 AM Per Malmberg ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/smooth/application/io/spi/BME280SPI.cpp
<#116 (comment)>:
> + // The spi rx buffer need to be length of multiples of 32 bits to avoid heap corruption.
+ // see esp-idf/components/driver/spi_master.c line 865
+ //static_assert(&rxdata % 4 == 0, "Error:c rxdata not 32 bit aligned ");
+
You can do it like this:
using BuffType = std::array<uint8_t, 4>;
BuffType foo{};
static_assert(sizeof(BuffType::value_type) * foo.size() % 4 == 0, "Buffer must have a length of multiple of 32 bits, i.e. 4 bytes");
You can then place the static assert at a single place where BuffType is
used to declare a member. Just make sure to name the type better than my
example :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#116?email_source=notifications&email_token=AKLXR7355HARUG3BVSLVUMDQS25O5A5CNFSM4JKSIZUKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCK74UAI#pullrequestreview-314558977>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKLXR76ZNE4AS2SRMUHG5L3QS25O5ANCNFSM4JKSIZUA>
.
|
|
I does sound like a good idea. Re. free(), there should be a heap_caps_free or similar for you to use. I suggest you do it RAII-style, i.e. allocate the buffer in the constructor and deallocate in the destructor so that the user doesn't have to think about memory allocations. I do wonder how to ensure that the buffer is aligned on a 32-bit boundary since alignas() isn't yet supported. This should be done in the SPI master code (the temporary buffers the text mentions), but I can't see it. |
|
Not sure why it can't find the file? error: esp32/rom/lldesc.h: No such file or directory |
|
Not sure why mock-idf can't find the file? error: esp32/rom/lldesc.h: No such file or directory |
|
I think you've misunderstood the purpose of mock-idf. Instead of copying the entire file from esp-idf, only add (mostly)empty implementations of the functions actually being used, with return values that won't cause an application calling them to crash. The idea is that you should be able to compile your application on Linux and debug parts that are not depending on hardware etc. So I think you can remove 90% of the file as well as the lldesc.h file. |
|
You know that you can run the CI-chain locally, right? No need to push to GitHub for each try. From the root-folder of Smooth: Requires a Linux environment with docker installed. |
|
There's one last small thing - add yourself to the contributors.md file :) I'll be running a few tests on the I2C parts for the BME280 before merging, unless you already have done that? |
|
Added my name to contributors but assumed you add the PR # and the rest. |
- Renamed hello_world to starter_example since it clashes with IDF's component name. - Changed NULL to nullptr. - Use operator[] instead of at() since there should be no need to do index-checking. - Use std::for_each instead of indexed loops. - Updated README.md.
* Minor adjustment after merging PR #116. - Renamed hello_world to starter_example since it clashes with IDF's component name. - Changed NULL to nullptr. - Use operator[] instead of at() since there should be no need to do index-checking. - Use std::for_each instead of indexed loops. - Updated README.md. * Size of variable instead of type. * Use constant to ensure same length of arrays. Use operator[] instead of at(). * Removed comment that does not apply. Use constant.
Also added an spi test and an i2c test in test folder but did not update CMakeLists,txt