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

revolution: Fix WS2811 conflict. #1845

Closed
wants to merge 1 commit into
base: next
from

Conversation

Projects
None yet
2 participants
@glowtape
Copy link
Member

glowtape commented Jun 24, 2017

The DMAShot code checks and waits for the TC flag to be set for every DMA stream in the update trigger. Works just fine, until there's a channel conflict, like what happens with WS2811 on the revo. Whenever there's a race between the two, it might get cleared due to STM32F4 internal magic (even though they're on separate DMA streams), and when said wait gets hit, it'll block until the watchdog starts barking.

However the TC flag wait shouldn't be necessary. We don't run at update rates high enough to ever collide updates at even DShot300 and hit still going updates under normal circumstances (that includes the eventual 3.2KHz mode for Seppuku/BMI160). And even if jitter is high enough for it to happen anyway (might occasionally happen with targets with I2C gyros*, or generally being at 99% CPU load), the update trigger will explicitly stop ongoing DMA transfers before setting up new updates, anyway, so an update might just not make it through entirely, which should be noticed by the ESC (either via timing error or mismatching checksum).

(*: Actually, the jittery mess of the OG Brain, that was kind of blamed on I2C, got fixed by the same changes that fixed Seppuku jitter, it looks pretty fine on the scope.)

@glowtape glowtape force-pushed the glowtape:glowtape-revough branch from c417f1a to ffe93f5 Jun 24, 2017

@glowtape

This comment has been minimized.

Copy link
Member

glowtape commented Jun 24, 2017

After @tracernz pointed out on IRC that it might be fishy, I check for the error flags and disable DMA for the current update instead. Appears to work and is more correct, I guess.

@mlyle

This comment has been minimized.

Copy link
Member

mlyle commented Jun 24, 2017

This doesn't make sense to me-- streams are completely independent. Channel 1 on stream 1 has nothing to do with channel 1 on stream 2.

@glowtape

This comment has been minimized.

Copy link
Member

glowtape commented Jun 24, 2017

Documentation says: More than one enabled DMA stream must not serve the same peripheral request.

Where the peripheral requests are the DMA channels, as per the stream/channel table. I've always understood the documentation that the streams and channels form sort of a crossbar.

It looks like WS2811 can interrupt/does stop the DMAShot DMA request and cause the TC flag to never be set (WS2811 DMA ISR clears it). Interestingly, when starting DMA (presumably while WS2811 is going) the error flag gets set, otherwise the current patch wouldn't even work, so there's something like this happening.

Seems like the DMA arbiter favors WS2811 on the Revo. Both WS2811 and DMAShot use the VeryHigh priority, and in this case, the lower stream gets favored. WS2811 is on stream 4, DMAShot on stream 5.

@glowtape

This comment has been minimized.

Copy link
Member

glowtape commented Jun 24, 2017

Jesus Christ, I can't repro it anymore. Earlier, enabling WS2811 and then setting pin 3 to DShot actually caused reboots. The patch actually caused updates to be skipped, i.e. they were missing on the scope. I've disabled said patch for a test, now the thing runs merrily without a bother.

@glowtape

This comment has been minimized.

Copy link
Member

glowtape commented Jun 24, 2017

Possibly chasing ghosts.

@glowtape glowtape closed this Jun 24, 2017

@glowtape glowtape deleted the glowtape:glowtape-revough branch Jan 21, 2018

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