Skip to content

fixing beginTransaction() thread safety #6425

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.

Sign up for GitHub

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

Merged
merged 27 commits into from
May 9, 2022
Merged

fixing beginTransaction() thread safety #6425

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
534978d
fixing beginTransaction() thread safety
smarq8 Mar 13, 2022
957f818
fixing beginTransaction thread safety
smarq8 Mar 13, 2022
db409b9
fixing beginTransaction()
smarq8 Mar 13, 2022
3480fa9
Merge branch 'master' into master
smarq8 Mar 13, 2022
7c2d808
fixing beginTransaction() by protecting _freq and _div by additional …
smarq8 Mar 14, 2022
5f4bc38
Update esp32-hal-spi.h
smarq8 Mar 14, 2022
70431bb
Update SPI.cpp
smarq8 Mar 14, 2022
5b2e387
add missing code
smarq8 Mar 14, 2022
8be51a4
ensure to lock params for whole transaction
smarq8 Mar 14, 2022
41be2fc
fix incorrect mutex macro at spiSimpleTransaction
smarq8 Mar 29, 2022
2d1098f
added paramLock destructor
smarq8 Mar 29, 2022
15fcc21
move create/delete to contructor/destructor
smarq8 Mar 29, 2022
28943f4
moving paramLock' delete from end() to destructor
smarq8 Mar 29, 2022
5a30b8a
.
smarq8 Mar 29, 2022
262a000
Merge branch 'master' of https://github.com/smarq8/arduino-esp32
smarq8 Mar 29, 2022
eaca28f
paramLock assign to null
smarq8 Mar 29, 2022
d0cc215
Merge branch 'master' of https://github.com/smarq8/arduino-esp32
smarq8 Mar 29, 2022
cf1363a
fix type typo and include files
smarq8 Mar 29, 2022
3e42a87
added paramLock initializer
smarq8 Mar 29, 2022
b4a6329
fix type typo
smarq8 Mar 29, 2022
22e2881
missing include for log_e
smarq8 Mar 29, 2022
64cb9c8
fix destructor declaration
smarq8 Mar 29, 2022
d157e52
missing paranthases
smarq8 Mar 29, 2022
beda8cd
missing paranthases
smarq8 Mar 29, 2022
a4950f2
Merge branch 'dev1' of https://github.com/smarq8/arduino-esp32 into dev1
smarq8 Mar 29, 2022
e91e10a
Merge branch 'master' into master
me-no-dev May 4, 2022
1b3c34a
Fix indentation
me-no-dev May 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cores/esp32/esp32-hal-spi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@ void spiTransaction(spi_t * spi, uint32_t clockDiv, uint8_t dataMode, uint8_t bi
if(!spi) {
return;
}
SPI_MUTEX_LOCK();
SPI_MUTEX_LOCK();
spi->dev->clock.val = clockDiv;
switch (dataMode) {
case SPI_MODE1:
Expand Down Expand Up @@ -1059,7 +1059,7 @@ void spiSimpleTransaction(spi_t * spi)
if(!spi) {
return;
}
SPI_MUTEX_LOCK();
SPI_MUTEX_LOCK();
}

void spiEndTransaction(spi_t * spi)
Expand Down
46 changes: 42 additions & 4 deletions libraries/SPI/src/SPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@
*/

#include "SPI.h"
#include "esp32-hal-log.h"

#if !CONFIG_DISABLE_HAL_LOCKS
#define SPI_PARAM_LOCK() do {} while (xSemaphoreTake(paramLock, portMAX_DELAY) != pdPASS)
#define SPI_PARAM_UNLOCK() xSemaphoreGive(paramLock)
#else
#define SPI_PARAM_LOCK()
#define SPI_PARAM_UNLOCK()
#endif

SPIClass::SPIClass(uint8_t spi_bus)
:_spi_num(spi_bus)
Expand All @@ -32,7 +41,30 @@ SPIClass::SPIClass(uint8_t spi_bus)
,_div(0)
,_freq(1000000)
,_inTransaction(false)
{}
#if !CONFIG_DISABLE_HAL_LOCKS
,paramLock(NULL)
#endif
{
#if !CONFIG_DISABLE_HAL_LOCKS
if(paramLock==NULL){
paramLock = xSemaphoreCreateMutex();
if(paramLock==NULL){
log_e("xSemaphoreCreateMutex failed");
return;
}
#endif
}

SPIClass::~SPIClass()
{
end();
#if !CONFIG_DISABLE_HAL_LOCKS
if(paramLock!=NULL){
vSemaphoreDelete(paramLock);
paramLock = NULL;
}
#endif
}

void SPIClass::begin(int8_t sck, int8_t miso, int8_t mosi, int8_t ss)
{
Expand Down Expand Up @@ -106,19 +138,23 @@ void SPIClass::setHwCs(bool use)

void SPIClass::setFrequency(uint32_t freq)
{
//check if last freq changed
SPI_PARAM_LOCK();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protects "_div" from cross-thread modification. OK

//check if last freq changed
uint32_t cdiv = spiGetClockDiv(_spi);
if(_freq != freq || _div != cdiv) {
_freq = freq;
_div = spiFrequencyToClockDiv(_freq);
spiSetClockDiv(_spi, _div);
}
SPI_PARAM_UNLOCK();
}

void SPIClass::setClockDivider(uint32_t clockDiv)
{
_div = clockDiv;
SPI_PARAM_LOCK();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protects "_div" from cross-thread modification. OK

_div = clockDiv;
spiSetClockDiv(_spi, _div);
SPI_PARAM_UNLOCK();
}

uint32_t SPIClass::getClockDivider()
Expand All @@ -138,7 +174,8 @@ void SPIClass::setBitOrder(uint8_t bitOrder)

void SPIClass::beginTransaction(SPISettings settings)
{
//check if last freq changed
SPI_PARAM_LOCK();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protects "_div" from cross-thread modification. OK

//check if last freq changed
uint32_t cdiv = spiGetClockDiv(_spi);
if(_freq != settings._clock || _div != cdiv) {
_freq = settings._clock;
Expand All @@ -153,6 +190,7 @@ void SPIClass::endTransaction()
if(_inTransaction){
_inTransaction = false;
spiEndTransaction(_spi);
SPI_PARAM_UNLOCK(); // <-- Im not sure should it be here or right after spiTransaction()
}
}

Expand Down
6 changes: 6 additions & 0 deletions libraries/SPI/src/SPI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include <stdlib.h>
#include "pins_arduino.h"
#include "esp32-hal-spi.h"
#include "freertos/FreeRTOS.h"
#include "freertos/semphr.h"

#define SPI_HAS_TRANSACTION

Expand All @@ -49,11 +51,15 @@ class SPIClass
int8_t _ss;
uint32_t _div;
uint32_t _freq;
#if !CONFIG_DISABLE_HAL_LOCKS
SemaphoreHandle_t paramLock=NULL;
#endif
bool _inTransaction;
void writePattern_(const uint8_t * data, uint8_t size, uint8_t repeat);

public:
SPIClass(uint8_t spi_bus=HSPI);
~SPIClass();
void begin(int8_t sck=-1, int8_t miso=-1, int8_t mosi=-1, int8_t ss=-1);
void end();

Expand Down