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 9 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_UNLOCK();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possible bug _LOCK() changed to _UNLOCK(). I will do a more in depth study of the code this weekend.

Copy link
Member

Choose a reason for hiding this comment

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

it should be LOCK as that is another way to start transaction

}

void spiEndTransaction(spi_t * spi)
Expand Down
26 changes: 23 additions & 3 deletions libraries/SPI/src/SPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@

#include "SPI.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)
,_spi(NULL)
Expand Down Expand Up @@ -48,6 +56,12 @@ void SPIClass::begin(int8_t sck, int8_t miso, int8_t mosi, int8_t ss)
if(!_spi) {
return;
}
#if !CONFIG_DISABLE_HAL_LOCKS
if(paramLock==NULL){
paramLock = xSemaphoreCreateMutex();
if(paramLock==NULL) return; // fail to create mutex so abort initialization
}
#endif

if(sck == -1 && miso == -1 && mosi == -1 && ss == -1) {
#if CONFIG_IDF_TARGET_ESP32S2
Expand Down Expand Up @@ -106,19 +120,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 +156,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 +172,7 @@ void SPIClass::endTransaction()
if(_inTransaction){
_inTransaction = false;
spiEndTransaction(_spi);
SPI_PARAM_UNLOCK();
}
}

Expand Down
7 changes: 7 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 @@ -25,6 +25,10 @@
#include "pins_arduino.h"
#include "esp32-hal-spi.h"

#if !CONFIG_DISABLE_HAL_LOCKS
#include "FreeRTOS.h"
#endif

#define SPI_HAS_TRANSACTION

class SPISettings
Expand All @@ -49,6 +53,9 @@ class SPIClass
int8_t _ss;
uint32_t _div;
uint32_t _freq;
#if !CONFIG_DISABLE_HAL_LOCKS
xSemaphoreHandle paramLock=NULL;
#endif
bool _inTransaction;
void writePattern_(const uint8_t * data, uint8_t size, uint8_t repeat);

Expand Down