Hackathon: Increase coverage of the SPI master FPGA test #12153
Conversation
Add setting if block transfer allows different sizes of rx/tx buffers.
|
@mprse, thank you for your changes. |
mprse
force-pushed
the
spi_fpga_test_extend
branch
from
c87e147 to
62f8e9d
Compare
mprse
changed the title
|
@ARMmbed/mbed-os-hal could you please review |
| bool hw_cs_handle; /**< If true, in SPI master mode Chip Select can be handled by hardware. */ | ||
| bool async_mode; /**< If true, in async mode is supported. */ | ||
|
|
||
| bool tx_rx_buffers_equal_length; /**< If true, rx and tx buffers must have the same length. */ |
There was a problem hiding this comment.
Why is this needed? isn't this always supported ? How this relates to the hw capabilities?
Often SPI transfer function has one length (both equal) - not our case at least for async. But were asked previously that it would simplify it.
Looking at the test - 2 platforms have this capability.
There was a problem hiding this comment.
Maybe the name is misleading. Should be the oposit tx_rx_diffrent_lenght. The problem is that on some platforms we can execute spi transfer like:
spi_master_block_write(&spi, p_tx, 10, p_rx, 5, 0xF5);
spi_master_block_write(&spi, p_tx, 5, p_rx, 10, 0xF5);
Unfortunatelly on some platform this won't work. The example is K64F - will take max from rx/tx lenghts for transfer.
On the feature branch, we tried to handle this and this was quite tricky or even impossible (in case of async mode), so we decided to add a requirement that in async mode rx/tx length must be equal.
Here, I didn't have time to update the drivers, so I added this capability. This way I can test different sizes only on targets that support this. From my point of view allowing different tx/rx lengths is redundant, but we can't simply change the API.
|
@mprse Who could help reviewing this one ? |
|
|
/jenkins-ci build |
|
@maciejbocianski @fkjagodzinski @jamesbeyond please review |
| * | ||
| * Given board provides SPI-Master support. | ||
| * When SPI transmission is performed using different settings. | ||
| * Then data is successfully transfered. |
There was a problem hiding this comment.
| * Then data is successfully transfered. | |
| * Then data is successfully transferred. |
| // Initialize mbed SPI pins | ||
| // Manually handle SS pin | ||
| if (!auto_ss) { | ||
| ss = new DigitalOut(ssel); |
There was a problem hiding this comment.
| ss = new DigitalOut(ssel); | |
| ss = new DigitalOut(ssel, SS_DEASSERT); |
|
@mprse could you look at the review comments please ? |
There was a problem hiding this comment.
I am quite happy to see this change. This brings a lot of valuable tests from the feature branch, that we spent quite a lot of efforts! well done @mprse !!
Increase coverage of the SPI master FPGA test: - check supported frequencies (based on the device capabilities), - add support for manual CS handling and test cases, - add test cases for rx/tx buffers with different length (based on the device capabilities), - add test case for one symbol transmission, - add test cases for different symbol sizes (based on the device capabilities).
mprse
force-pushed
the
spi_fpga_test_extend
branch
from
62f8e9d to
d298f96
Compare
mergify
bot
dismissed
jamesbeyond’s stale review
Pull request has been modified.
|
Addressed review comments. |
|
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
|
This PR does not contain release version label after merging. |
0xc0170
added
release-version: 6.0.0-alpha-2
|
I've fixed the version: Set to 6.0.0-alpha-2 |
Summary of changes
Increase coverage of the SPI master FPGA test:
Update default SPI capabilities and target specific capabilities (
STM, NFR32x).TEST RESULTS:
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers