Skip to content
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

STM32: SPI with DMA - more reliable detection of transfer completed #1043

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+14 −18
Open

STM32: SPI with DMA - more reliable detection of transfer completed #1043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 src/modm/platform/spi/stm32/spi_master_dma.hpp.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ private:
handleDmaTransmitComplete();

static inline bool dmaError { false };
static inline bool dmaTransmitComplete { false };
static inline bool dmaReceiveComplete { false };
static inline bool dmaTransmitComplete { true };
static inline bool dmaReceiveComplete { true };
Comment on lines +86 to +87
Copy link
Member

Choose a reason for hiding this comment

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

Why is this required?
If you do a transfer with TX and RX, both dmaTransmitComplete and dmaReceiveComplete are set to false before it is started. If you only transfer in one direction waiting for the interrupt is now bypassed as mentioned above. It seems to me there is another issue somewhere else.

Copy link
Author

Choose a reason for hiding this comment

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

So the scenario is to send the SPI data with low frequency. It's not a real high-frequency poll, so in general the previous packet would have been sent before the next is ready, but I'd like to have a guard for this case as well.

Before I send a packet, I check, whether SPI is ready to send:

    if (SpiMaster::transfer(nullptr, nullptr, 0).getState() == modm::rf::Running) {
      return false;
    }
    SpiMaster::transfer(reinterpret_cast<uint8_t*>(data_.data()), nullptr, sizeof(data_));
    return true;

If nothing has been sent before this function is called, the transfer always return Running and the real send never occurs.

So there is a way to overcome this by sending some dummy at the initialization stage, but I thought it could be also nice to reorganize the code to handle the general case as well.

Also, for the initialisation values: dmaTransmitComplete generally marks, that no transfers are active, so i the false value could be valid here.

In general, I don't think this PR is important and can be simply deleted.


// needed for transfers where no RX or TX buffers are given
static inline uint8_t dmaDummy { 0 };
Expand Down
28 changes: 12 additions & 16 deletions src/modm/platform/spi/stm32/spi_master_dma_impl.hpp.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,26 +122,24 @@ modm::platform::SpiMaster{{ id }}_Dma<DmaChannelRx, DmaChannelTx>::transfer(
if (tx) {
Dma::TxChannel::setMemoryAddress(uint32_t(tx));
Dma::TxChannel::setMemoryIncrementMode(true);
Dma::TxChannel::setDataLength(length);
dmaTransmitComplete = false;
Copy link
Member

@chris-durand chris-durand Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't this introduce a bug? In case an RX only transaction follows a TX only one dmaTransmitComplete is not reset and remains true. The condition if (not dmaTransmitComplete and not dmaReceiveComplete) will immediately evaluate to false and not wait for DMA transfer completion.

There is another potential issue here. dmaTransmitComplete is neither volatile nor std::atomic. The compiler can arbitrarily reorder the store to dmaTransmitComplete and Dma::TxChannel::start(). It's unlikely to generate code that would cause actual trouble in practice but it would be a valid optimization to move dmaTransmitComplete = false far behind starting the channel. Making dmaTransmitComplete and dmaReceiveComplete volatile would fix this.

Dma::TxChannel::start();
} else {
Dma::TxChannel::setMemoryAddress(uint32_t(&dmaDummy));
Dma::TxChannel::setMemoryIncrementMode(false);
}
Comment on lines 128 to 131
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this else-branch when we don't enable the channel at all? No transfer will be started. Thus, there should be no need to configure it.

Also in line 119-120 we always enable both TX and RX DMA requests even when the respective channel is not enabled. I am wondering if we should really do this after that change.

if (rx) {
Dma::RxChannel::setMemoryAddress(uint32_t(rx));
Dma::RxChannel::setMemoryIncrementMode(true);
Dma::RxChannel::setDataLength(length);
dmaReceiveComplete = false;
Dma::RxChannel::start();
} else {
Dma::RxChannel::setMemoryAddress(uint32_t(&dmaDummy));
Dma::RxChannel::setMemoryIncrementMode(false);
}

Dma::RxChannel::setDataLength(length);
dmaReceiveComplete = false;
Dma::RxChannel::start();

Dma::TxChannel::setDataLength(length);
dmaTransmitComplete = false;
Dma::TxChannel::start();

while (true)
{
if (dmaError) break;
Expand Down Expand Up @@ -180,26 +178,24 @@ modm::platform::SpiMaster{{ id }}_Dma<DmaChannelRx, DmaChannelTx>::transfer(
if (tx) {
Dma::TxChannel::setMemoryAddress(uint32_t(tx));
Dma::TxChannel::setMemoryIncrementMode(true);
Dma::TxChannel::setDataLength(length);
dmaTransmitComplete = false;
Dma::TxChannel::start();
} else {
Dma::TxChannel::setMemoryAddress(uint32_t(&dmaDummy));
Dma::TxChannel::setMemoryIncrementMode(false);
}
if (rx) {
Dma::RxChannel::setMemoryAddress(uint32_t(rx));
Dma::RxChannel::setMemoryIncrementMode(true);
Dma::RxChannel::setDataLength(length);
dmaReceiveComplete = false;
Dma::RxChannel::start();
} else {
Dma::RxChannel::setMemoryAddress(uint32_t(&dmaDummy));
Dma::RxChannel::setMemoryIncrementMode(false);
}

Dma::RxChannel::setDataLength(length);
dmaReceiveComplete = false;
Dma::RxChannel::start();

Dma::TxChannel::setDataLength(length);
dmaTransmitComplete = false;
Dma::TxChannel::start();

[[fallthrough]];

default:
Expand Down