BossHobby / QUICKSILVER

Flight Controller Firmware
MIT License
171 stars 40 forks source link

dshot needs a "guard" for dma2 ahb transactions #25

Closed NotFastEnuf closed 3 years ago

NotFastEnuf commented 4 years ago

As per the stm errata on f4, we have been fighting corruption on gpio pins due to dshot and the dma2 hardware bug with concurrent ahb and apb transactions. IIn the feature-looptime-autodetect branch, Dshot has been moved to a single stream, adc has been taken off dma2, and there is a guard in place on any dma2 spi transaction which makes it wait for dshot to be done. For compete coverage - it may be concievable that some day we will have dma2 spi transaction which is initiated before dshot or overlaps into the beginning call to dshot. In this case we will need a guard which makes dshot wait. Any impact to looptime will be handled by the upcoming merge of looptime autodetect feature and this guard will help to set the fastest looptime a fc can run. Hanfer has created such a guard system in the blackbox branch, so once all these features are merged in - we need only remember to utilize his system to "guard" dshot dma transaction.

bkleiner commented 4 years ago

I extracted the changes from the blackbox branch and finally put the change in place here. https://github.com/NotFastEnuf/Guano/commit/11a1b0c82dfc427e675288663e285c2854d55db4 I will update the blackbox branch accordingly.

NotFastEnuf commented 4 years ago

I think this is close to what we want here but maybe we should go further in modifyingour original guard. If I am not visualizing this properly then please correct me. The intention of the loop time autodetect system is such that we wouldn't ever want to skip a dshot command sequence anymore. I think we want to wait for spi1 to be done and for the last command to complete and allow a series of blown loops which then let's the loop time detection code engage to adjust looptime according to the best possible fit to solve the conflict. What are your thoughts on dropping the timer on this?

bkleiner commented 3 years ago

of course we should test it, but my thinking here is: we are waiting a max of one looptime in this function. we did however already spend a bit of time in this loop and will continue to spend more further down that same loop. so if the spi is actually blocking here, the loop will be blown by a good margin and loop time detect will trigger. meaning the return here will speed up the process of finding the new looptime, because once it blows the current one, it will be like "nope, ok, i wont make it in time"

NotFastEnuf commented 3 years ago

Autodetect however is working off of an average of a sequence of loops. Cutting some short could skew the data it is using to determine a fit. I suppose it could just trigger to run again if a good fit wasn't found in the first attempt... however i do wonder if there isnts some oddball situation where a fit is found that somehow has enough room to always skip the motor command but never had enough room for it. Maybe I am worried about nothing

bkleiner commented 3 years ago

thing is, i worry about situations eg during boot where full block would prevent the fc from ever completing or even reaching a loop. i guess you concern is valid, we could cover that by adding a bigger margin (eg. looptime * 2) to the time check in dshot guard.

NotFastEnuf commented 3 years ago

This might be worth not forgetting just in case, but has never seemed to cause any problems. Marking it closed for nw