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

[ws2812] resumable write and dma example #879

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

hshose
Copy link
Contributor

@hshose hshose commented Jul 10, 2022

Tested in hardware

@rleh rleh marked this pull request as draft July 11, 2022 09:20
@rleh rleh added this to the 2022q3 milestone Jul 11, 2022
@salkinium
Copy link
Member

The driver was explicitly written as blocking, because any pause in the SPI transfers will break the timing protocol of the LEDs. See the caveats.
This should be implemented using DMA SPI ideally.

@chris-durand
Copy link
Member

This should be implemented using DMA SPI ideally.

That seems to be the intention of this change. If you look at the example you'll see DMA SPI is used. It won't work without DMA anymore, though.

@salkinium
Copy link
Member

Ah, you're right, I only looked at the implementation. 🙈
If we could if constexpr on the DMA version of the SPI, then we can implement both versions, ie. non-blocking for DMA on STM32, and blocking for non-DMA on everything else?

@rleh
Copy link
Member

rleh commented Jul 12, 2022

I just don't see a nice way to determine in a driver if the SPI master uses DMA, so I would suggest adding a (constexpr) function bool usesDma() to all SPI master implementations and then we should be able to use something like the following code:

template<typename T = void>
std::enable_if_t<SpiMaster::usedDma(), modm::ResumableResult<void>>
write()
{
	return SpiMaster::transfer(data, nullptr, length+1);
}

template<typename T = void>
std::enable_if_t<! SpiMaster::usedDma(), void>
write()
{
	for (const auto value : data) {
		while (not SpiMaster::Hal::isTransmitRegisterEmpty()) ;
		SpiMaster::Hal::write(value);
	}
}

if constexpr would read nicer, but the return type of the function has to change...

@salkinium
Copy link
Member

I think we should keep the function signature as RF for both implementations, otherwise you cannot write platform-independent code here.

@chris-durand
Copy link
Member

@rleh In C++20 you could write that code without std::enable_if:

modm::ResumableResult<void>
write() requires SpiMaster::usesDma
{}

or something similar.

I agree with @salkinium, that we should keep the resumable function signature for both versions to allow platform independent code.

@rleh
Copy link
Member

rleh commented Jul 12, 2022

In C++20 you could write that code without std::enable_if: [...] or something similar.

Something similar is actually:

template<typename T = void>
static void
write()
requires (SpiMaster::usesDma())
{}

See details in compiler explorer.
I wanted to solve this with concepts but I couldn't figure it out earlier...


I think we should keep the function signature as RF for both implementations, otherwise you cannot write platform-independent code here.

Agree 👍🏽
(And then if constexpr works fine.)

@chris-durand
Copy link
Member

You don't need the template<typename T = void> and in case usesDma is a variable parentheses are also not required. The code works exactly as proposed, see Compiler Explorer 😉.

@salkinium salkinium removed this from the 2022q3 milestone Oct 1, 2022
@rleh rleh added the stale ♾ label Apr 9, 2023
@hshose hshose force-pushed the feature/ws2812dma branch 2 times, most recently from 63fd13c to 3d40be4 Compare May 26, 2023 22:23
@hshose
Copy link
Contributor Author

hshose commented May 26, 2023

Code works with DMA and non-DMA SPI (see both examples). Not sure, if you want to keep the copy of the F411/ws2812 for the F469 board. I used this for testing the non-dma version because I don't have an F411 board around...

@hshose
Copy link
Contributor Author

hshose commented May 26, 2023

LOL I think I don't get @chris-durand suggestion to actually use the DMA version...

modm::ResumableResult<void>
write() requires (SpiMaster::usesDma())
{}

The parenthesis seem to be neccessary?

@chris-durand
Copy link
Member

The parenthesis seem to be neccessary?

If you defined a variable static constexpr bool usesDma{true}; instead of a function it would work. You can leave it as is if you prefer the function.

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

Successfully merging this pull request may close these issues.

4 participants