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

DShot: updated driver to use NuttX DMA functions #13233

Merged
merged 5 commits into from Oct 25, 2019
Merged

DShot: updated driver to use NuttX DMA functions #13233

merged 5 commits into from Oct 25, 2019

Conversation

Igor-Misic
Copy link
Member

@Igor-Misic Igor-Misic commented Oct 19, 2019

Switch from bare-metal driver to NuttX DMA driver.
The code looks cleaner and it is easier to port to other boards by using NuttX drivers.
DShot will use NuttX DMAMAP naming as other components.

Know issue explained:
The problem was happening because of timer peripheral was running freely. That caused unsynchronised the first DShot bits while DMA started filling the DMAR register. Sometimes there was a bit missing sometime there was an extra bit.

image
image

Solution:
In burst mode, the timer trigger should be enabled after the DMA module. This is done with enabling UDE (Update DMA request enable) flag in timer after DMA is enabled. After DMA transfer is done UDE is disabled with the callback function because it needs to be disabled for the next cycle.

@bkueng bkueng self-requested a review October 21, 2019 07:49
@dagar dagar requested a review from davids5 October 21, 2019 15:32
@davids5
Copy link
Member

davids5 commented Oct 21, 2019

@Igor-Misic - Is DShot like one shot where there is only one pulse train sent per output cycle?

@Igor-Misic
Copy link
Member Author

@davids5, yes. DShot drivers should be triggered externally (for example from the PID loop), but frequently enough that the ESC does not disarm motors.

@davids5
Copy link
Member

davids5 commented Oct 21, 2019

@Igor-Misic - in that case I would expect to use the timers synchronously to the DMA not in repeat mode. Is that what is being done?

@Igor-Misic
Copy link
Member Author

@davids5 - timers are used in burst mode (check TIMx_DMAR) and internal mechanisms take care that DMA is in synchronization with timer. We are only preparing data length, content and then we are enabling DMA and timer flag for DMA transaction. DMA then starts loading data to timer registers (CCR1 ... CCR4) over the DMAR register.

All PWMs (DShot bits) are triggered at once. If there are 4 channels per timer it means it will be triggered 4x16 PWMs at once.

@davids5
Copy link
Member

davids5 commented Oct 21, 2019

@Igor-Misic If the bust is completed, why is the fix for the missing/extra pulse to use a call back as opposed to just clear the UDE flag prior to the DMA set up/start call?

@Igor-Misic
Copy link
Member Author

Igor-Misic commented Oct 21, 2019

@davids5 DMA is not able to put the data in the timer before UDE is set. Setting UDE will start DMA transfer and timers at the same time. If we clean UDE in callback when transfer is complete we can be sure that next time when we call DShot again, timer and DMA will be in sync.

Edit: yeah I got your point now. I think it will work even without a callback, sorry on delay xD
Let me test it.

@Igor-Misic
Copy link
Member Author

@davids5 It works. It's a much cleaner solution.

bkueng
bkueng previously approved these changes Oct 24, 2019
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Tested, didn't see any telemetry timeouts. Thanks for the simplifications @Igor-Misic
Can you rebase and update the modalai/fc-v1 board as well (#13259)?

@Igor-Misic
Copy link
Member Author

@bkueng - I wasn't able to rebase because changes on the repo at commit 45c320 so I used Merge branch 'master' into pr-dshot-nuttx-dma.

@bkueng bkueng merged commit f4ee914 into PX4:master Oct 25, 2019
@julianoes
Copy link
Contributor

@bkueng do you consider this to be safe to go in to the release as well?

@bkueng
Copy link
Member

bkueng commented Oct 25, 2019

It should be, but as it's only a refactoring, there's no benefit and only risk in adding it.

@julianoes
Copy link
Contributor

Yer, I'm asking because I don't think we have a release branch yet, so we need to check.

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

Successfully merging this pull request may close these issues.

None yet

5 participants