-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix STM32 SPI 3-wire (synchronous API) #14981
Conversation
@vznncv, thank you for your changes. |
targets/TARGET_STM/stm_spi_api.c
Outdated
@@ -602,6 +606,24 @@ static const uint32_t baudrate_prescaler_table[] = {SPI_BAUDRATEPRESCALER_2, | |||
SPI_BAUDRATEPRESCALER_256 | |||
}; | |||
|
|||
static uint8_t spi_get_baudrate_prescaler_rank(uint32_t value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you aling {
to a new line - style consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
This pull request has automatically been marked as stale because it has had no recent activity. @jeromecoutant, @LMESTM, please complete review of the changes to move the PR forward. Thank you for your contributions. |
@vznncv thanks for your great efforts in sharing this code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions or comments
I'd like to be able to review the change in a way that it impacts only the 3-wire mode. Maybe this would require to split into 2 commits, 1 for refactoring the code with inline functions. Another one for introducing the one_wire_write function
targets/TARGET_STM/stm_spi_api.c
Outdated
static uint8_t spi_get_baudrate_prescaler_rank(uint32_t value) | ||
{ | ||
#if TARGET_STM32H7 | ||
return value >> 28 & 0x07; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see HAL defines rather than hard coded values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with switch...case statement.
targets/TARGET_STM/stm_spi_api.c
Outdated
#if TARGET_STM32H7 | ||
return value >> 28 & 0x07; | ||
#else /* TARGET_STM32H7 */ | ||
return value >> 3 & 0x07; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
SPI_HandleTypeDef *handle = &(spiobj->handle); | ||
|
||
int freq = spi_get_clock_freq(obj); | ||
uint8_t baudrate_rank = spi_get_baudrate_prescaler_rank(handle->Init.BaudRatePrescaler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that possible to re-use LL macros like LL_SPI_GetBaudRatePrescaler or HAL equivalent ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LL_SPI_GetBaudRatePrescaler
returns some prescaler constant, but not prescaler value itself. So extra code that converts this constant to prescaler rank itself is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok !
targets/TARGET_STM/stm_spi_api.c
Outdated
/** | ||
* Check if SPI is busy in master mode. | ||
*/ | ||
static inline int msp_busy(spi_t *obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does msp mean ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The are some slave SPI mode related functions with ssp_
prefix (ssp_readable
, ssp_writable
, etc.). Probably it's abbreviation of slave SPI peripheral (or something like that), so I add msp_
prefix (master SPI peripheral) for helper functions of master SPI mode.
LL_SPI_StartMasterTransfer(SPI_INST(obj)); | ||
#endif /* TARGET_STM32H7 */ | ||
|
||
/* Transmit data */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred that only the 3-wires case is modified.
Or maybe create a first commit that introduce new inline functions, then introduce the 3-wires changes ... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The changes are splitted into multiple commits (inline functions, 3-wires changes for synchronous API, some improvements for 3-wire with asynchronous API)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
targets/TARGET_STM/stm_spi_api.c
Outdated
* @param rx_length number of bytes to read, may be zero | ||
* @return number of transmitted and received bytes or negative code in case of error. | ||
*/ | ||
static int spi_master_one_wire_write(spi_t *obj, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should actually be called "one_wire_transfer" ? as it does manage tx and rx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Yes, "_transfer" suffix is more meaningful.
note: I used "_write" suffix for consistency with mbed function spi_master_block_write
function (it has _write
suffix and manages rx and tx).
0f85516
to
e1b5fc2
Compare
Yes, my changes don't fix all 3-wire SPI issues:
|
This pull request has automatically been marked as stale because it has had no recent activity. @jeromecoutant, please complete review of the changes to move the PR forward. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this new version, this really helps review.
Previous comments have been covered, I have a few new ones now
SPI_HandleTypeDef *handle = &(spiobj->handle); | ||
|
||
int freq = spi_get_clock_freq(obj); | ||
uint8_t baudrate_rank = spi_get_baudrate_prescaler_rank(handle->Init.BaudRatePrescaler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok !
LL_SPI_StartMasterTransfer(SPI_INST(obj)); | ||
#endif /* TARGET_STM32H7 */ | ||
|
||
/* Transmit data */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
targets/TARGET_STM/stm_spi_api.c
Outdated
@@ -866,15 +866,6 @@ int spi_master_write(spi_t *obj, int value) | |||
const int bitshift = datasize_to_transfer_bitshift(handle->Init.DataSize); | |||
MBED_ASSERT(bitshift >= 0); | |||
|
|||
#if defined(LL_SPI_RX_FIFO_TH_HALF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change / commit here is an optimization and is not directly related to the SPI 3 wire change. I would prefer this to be a separate Pull Request and to have a full non regression testing before accepting such change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ , the commit is removed from this Pull Request
@@ -1364,10 +1375,15 @@ void spi_abort_asynch(spi_t *obj) | |||
NVIC_DisableIRQ(irq_n); | |||
|
|||
// clean-up | |||
__HAL_SPI_DISABLE(handle); | |||
LL_SPI_Disable(SPI_INST(obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you come up with this fix ? Is spi_abort_asynch being used in your tests ?
or did you plan to modify init_spi function ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is spi_abort_asynch being used in your tests ?
Yes, I have updated demo project to invoke SPI::abort_all_transfers
between sensor API calls. It's invoked when no actual transfers ongoing, but it shouldn't have any side effects. Although my test shows that this call causes data corruption of the subsequent transmissions, so I found and fixed the error in the spi_abort_asynch
. Demo project fragment:
// call SPI:spi_abort_all_transfers to check that it has no side effects
api->call_spi_abort_all_transfers();
// transmit data to BMX160
err_write = api->register_write(base_reg, out_buf, buf_len);
// receive data from BMX160
err_read = api->register_read(base_reg, in_buf, buf_len);
void call_spi_abort_all_transfers()
{
return _spi.abort_all_transfers();
}
did you plan to modify init_spi function ?
Unlike spi_abort_asynch
the init_spi
function already has correct code that doesn't enable SPI in 3 wire mode:
mbed-os/targets/TARGET_STM/stm_spi_api.c
Lines 139 to 146 in d2a88f4
/* In case of standard 4 wires SPI,PI can be kept enabled all time | |
* and SCK will only be generated during the write operations. But in case | |
* of 3 wires, it should be only enabled during rd/wr unitary operations, | |
* which is handled inside STM32 HAL layer. | |
*/ | |
if (handle->Init.Direction == SPI_DIRECTION_2LINES) { | |
__HAL_SPI_ENABLE(handle); | |
} |
The only one thing that I have added to init_spi
- explicit RX buffer/register cleanup:
/* In some cases after SPI object re-creation SPI overrun flag may not
* be cleared, so clear RX data explicitly to prevent any transmissions errors */
spi_flush_rx(obj);
The necessity of these lines I also discovered by my test project (https://github.com/vznncv/mbed-os-stm32-spi-3-wire-demo/blob/40d1556b049a46e82fc2ed214fd4d5340e1e6814/src/main.cpp). The demo re-creates SPI
object for each test run with different parameters (spi frequency, api type). So If previous test run fails, it may left some data in the RX buffer/register that causes failure of subsequent test runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok clear !
Hi My comment from a non-SPI expert :-)
|
e1b5fc2
to
8eeee1b
Compare
Hi
It looks interesting. I'll check it later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks @vznncv for the efforts in contributing !
@jeromecoutant please run the SPI test suite again as the PR has been updated
OK, let's merge this one. I will then rebase #15028 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic SPI non regression tests OK with all STM32 families
Hi @0xc0170 Please check CI/CD. It seems that scancode doesn't work in the current environment. Some investigation:
|
@vznncv If you can rebase to the latest master, it should be all green and I'l start our CI |
- move a code that waits readable SPI state from `spi_master_write` function to inline functions `msp_writable` and `msp_wait_writable` - move a code that waits writeable SPI state from `spi_master_write` function to inline functions `msp_readable` and `msp_wait_readable` - move a code that writes data to SPI from `spi_master_write` function to inline function `msp_write_data` - move a code that reads data from SPI from `spi_master_write` function to inline function `msp_read_data`
All STM32 families except STM32H7 has the following 3-wire SPI peculiarity in master receive mode: SPI continuously generates clock signal till it's disabled by a software. It causes that a software must disable SPI in time. Otherwise, "dummy" reads will be generated. Current STM32 synchronous SPI 3-wire implementation relies on HAL library functions HAL_SPI_Receive/HAL_SPI_Transmit. It performs some SPI state checks to detect errors, but unfortunately it isn't fast enough to disable SPI in time. Additionally, a multithreading environment or interrupt events may cause extra delays. This commit contains the custom transmit/receive function for SPI 3-wire mode. It uses critical sections to prevents accidental interrupt event delays, disables SPI after each frame receiving and disables SPI during frame generation. It adds some delay between SPI frames (~700 ns), but gives reliable 3-wire SPI communications.
- add RX cleanup after SPI re-initialization, as it isn't implemented in the `HAL_SPI_Init` - cancel SPI enabling for 3-wire mode
`HAL_SPI_Receive_IT` HAL function causes dummy reads in 3-wire mode, that causes data corruption in RX FIFO/register. It isn't possible to fix it without signification refactoring, but we may prevent data corruption with the following fixes: - RX buffer/register cleanup after asynchronous transfer in 3-wire mode - Explicit RX buffer/register cleanup after SPI initialization (for cases if we re-create SPI object).
8eeee1b
to
7bc773b
Compare
Pull request has been modified.
This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged. |
Let's start CI ? |
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Summary of changes
Fixes #14774
Problem
Almost all STM32 MCU families (except STM32H7) continuously generates clock signal, till SPI is disabled in 3-wire
receiving mode. It means that SPI should be disabled by software during last byte receiving.
It imposes the following limitations of the code that receive SPI data:
in time.
If SPI isn't be disabled in time, it starts generating next clock signal for next frame, and you get "dummy" read.
In some cases "dummy" reads don't hinder communication with devices/sensors, but in other cases it's crucial. For
example data reading from sensor FIFO. In this case any "dummy" reads cause data lost.
Current 3-Wire implementation uses STM HAL library functions
HAL_SPI_Receive
/HAL_SPI_Transmit
that do manyerror/state checks, but unfortunately they aren't fast enough to prevent dummy reads.
Fix
This pull request contains implements the following algorithm for SPI data receiving in 3-wire mode:
Notes:
Unlike
HAL_SPI_Receive
, that disables SPI after byte receiving, this approach disables SPI during byte receiving,so next frame isn't generated and we don't get dummy read.
Asynchronous SPI API still causes dummy reads. I tried to implement some approaches, but didn't get any success.
Tests
Currently, I have only tested this fix using demo project https://github.com/vznncv/mbed-os-stm32-spi-3-wire-demo with
BMX160 sensor and the following MCUs:
port: https://github.com/vznncv/TARGET_BLACKPILL_F411CE)
port: https://github.com/vznncv/TARGET_WEACT_H743VI)
The demo project simply reads and writes to BMX160 register (including burst reads/writes) to check that SPI works
correctly. Additionally, it counts SPI clock cycles using STM32 timer ETR input to detect "dummy" reads.
Test project results
STM32F103C8T6
Output logs:
Results
STM32F411CEU6
Output logs:
Results
STM32H743VIT6
Output logs:
Results
Note: due current STM32 SPI/FDCAN clock configuration maximal SPI frequency is 5 MHz.
Logic analyzer data
Additionally, I have check communication with sigrock
and logical analyzer.
The configuration:
gives the following results:
sigrock session files: target_blackpillf411ce_release_3125000Hz.zip
frame example:
According logical analyzer, the fix overhead adds small delay between frame receiving (about ~700 ns), but it's
acceptable.
Possible SPI test
FPGA CI TEST shield (https://github.com/ARMmbed/mbed-os/tree/master/hal/tests/TESTS/mbed_hal_fpga_ci_test_shield).
Probably it's good place for SPI 3-wire tests, and it seems that FPGA shield support half-duplex mode
(https://github.com/ARMmbed/mbed-os/blob/master/features/frameworks/COMPONENT_FPGA_CI_TEST_SHIELD/include/fpga_ci_test_shield/SPIMasterTester.h#L73)
, but:
support (https://github.com/ARMmbed/mbed-os/blob/master/hal/include/hal/spi_api.h#L71), so any test with SPI
3-wire mode will cause failure of devices that doesn't support it.
CI TEST shield (https://github.com/ARMmbed/ci-test-shield)
Probably loopback pins and STM32 ETR timer inputs can be used for some SPI tests (ETR inputs can be used to count
clock cycles of SCK and to check MOSI data indirectly), but:
So currently there is no possibility to implement 3-wire SPI test. At least with existed CI test shields.
Implementation notes:
I have implemented buffer logic, that is based on existed SPI 4-wire implementation:
3-wire implementation:
But I'm not sure that it's correct, as it seems that it doesn't handle data correctly if SPI frame size isn't 8 bits.
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers
@LMESTM @jeromecoutant