FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

STM32: SPI with DMA - more reliable detection of transfer completed by ser-plu · Pull Request #1043 · modm-io/modm · GitHub

/ modm Public

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

Open
ser-plu wants to merge 1 commit into
modm-io:developfrom
ser-plu:stm32-spi-dma-fix
Open

ser-plu wants to merge 1 commit into
modm-io:developfrom
ser-plu:stm32-spi-dma-fix

Conversation

ser-plu commented Jun 26, 2023

Copy link
Copy Markdown

No description provided.

chris-durand left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Dma::TxChannel::setMemoryAddress(uint32_t(tx));
Dma::TxChannel::setMemoryIncrementMode(true);
Dma::TxChannel::setDataLength(length);
dmaTransmitComplete = false;

chris-durand Jun 26, 2023
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL