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.

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

Conversation

ser-plu
Copy link

@ser-plu ser-plu commented Jun 26, 2023

No description provided.

Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

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

Thanks! I'd like to know what the reason for this change is. Is it only an optimization or did you have some trouble with transfers not completing?

Comment on lines 128 to 131
} else {
Dma::TxChannel::setMemoryAddress(uint32_t(&dmaDummy));
Dma::TxChannel::setMemoryIncrementMode(false);
}
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.

@@ -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

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.

Comment on lines +86 to +87
static inline bool dmaTransmitComplete { true };
static inline bool dmaReceiveComplete { true };
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants