search

AdaCore / Ada_Drivers_Library

Ada source code and complete sample GNAT projects for selected bare-board platforms supported by GNAT.
BSD 3-Clause "New" or "Revised" License
240 stars 141 forks source link

Allow control of interrupts to be enabled in DMA transfer. #274

Closed simonjwright closed 5 years ago

simonjwright commented 5 years ago

I had a problem with Certyflie in that after a considerable time (an hour, maybe) I’d get an assertion failure in the postcondition of the Clear_Status call at the end of DMA_Interrupt_Controller.Interrupt_Handler, because the Half_Transfer_Complete_Interrupt hadn’t cleared. (I do question whether it’s OK to use assertions as a check on whether some external hardware behaved as expected? on the other hand, what else to do?)

I made this change, then changed the Certyflie code to enable all interrupts except Half_Transfer_Complete_Interrupt, and the drone ran without error for two hours. (The Crazyflie code only enabled Transfer_Complete last time I looked).

You might wonder whether to eliminate STM32.DMA.Start_Transfer_With_Interrupts and simply give Start_Transfer the same Enabled_Interrupts parameter? I could add that to this request if considered helpful.

pat-rogers commented 5 years ago

On 9/9/2018 10:04 AM, Simon Wright wrote:

I had a problem with Certyflie in that after a considerable time (an hour, maybe) I’d get an assertion failure in the postcondition of the |Clear_Status| call at the end of |DMA_Interrupt_Controller.Interrupt_Handler|, because the |Half_Transfer_Complete_Interrupt| hadn’t cleared. (I do question whether it’s OK to use assertions as a check on whether some external hardware behaved as expected? on the other hand, what else to do?)

Yes indeed, sometimes postconditions cannot be used because the hardware does something unexpected, but in general I think they are a good thing here too. During development, they helped me determine that what I thought would happen really did happen. And they serve as good documentation.

I don't know why the Half_Transfer_Complete status flag didn't clear though. To me this is a good reason for the postconditions, in that something in need of exploration is noticed.

I made this change, then changed the Certyflie code to enable all interrupts except |Half_Transfer_Complete_Interrupt|, and the drone ran without error for two hours. (The Crazyflie code only enabled |Transfer_Complete| last time I looked).

You might wonder whether to eliminate |STM32.DMA.Start_Transfer_With_Interrupts| and simply give |Start_Transfer| the same |Enabled_Interrupts| parameter? I could add that to this request if considered helpful.

I have some preference for the original approach with two distinct routines, one with interrupts and one without. It seemed a good distinction at the time anyway.

Pat

simonjwright commented 5 years ago

On 10 Sep 2018, at 23:35, Pat Rogers notifications@github.com wrote:

On 9/9/2018 10:04 AM, Simon Wright wrote:

I had a problem with Certyflie in that after a considerable time (an hour, maybe) I’d get an assertion failure in the postcondition of the |Clear_Status| call at the end of |DMA_Interrupt_Controller.Interrupt_Handler|, because the |Half_Transfer_Complete_Interrupt| hadn’t cleared. (I do question whether it’s OK to use assertions as a check on whether some external hardware behaved as expected? on the other hand, what else to do?)

Yes indeed, sometimes postconditions cannot be used because the hardware does something unexpected, but in general I think they are a good thing here too. During development, they helped me determine that what I thought would happen really did happen. And they serve as good documentation.

I have to agree!

I don't know why the Half_Transfer_Complete status flag didn't clear though. To me this is a good reason for the postconditions, in that something in need of exploration is noticed.

I'm fairly baffled about this, too. I wondered whether it was related to a very short transfer; I managed to check the status flags in the hardware register on one occasion while chasing this, and found both Transfer_Complete and Half_Transfer_Complete set. Perhaps the hardware won't clear HTIF if TCIF is set?

I made this change, then changed the Certyflie code to enable all interrupts except |Half_Transfer_Complete_Interrupt|, and the drone ran without error for two hours. (The Crazyflie code only enabled |Transfer_Complete| last time I looked).

I've been having real trouble chasing this down. At one point it was repeatable (failed within a minute or two), now I can't get it to fail at all. Something to do with optimisation levels? There's certainly an issue with comms to the nrf51, related I suspect to failing to pause DMA transfers to the nrf51 when requested (40 us deadline!) and therefore overrunning its buffers.

OK, just retried with PLATFORM_BUILD set to Production, ADL_BUILD to Debug, and got the error: the transfer count at the start of the transfer was 3, and the status flags were FEIF, HTIF, TCIF. Of course the hardware had had plenty of time to change the status flags after the breakpoint (in gnat_last_chance_handler) was hit.

FIFO error? Hmm, probably just the nrf51 couldn't read, since the FIFO wasn't enabled.

(Later) I made a more extensive patch, to only process a status flag if the corresponding interrupt is enabled; and then ran with just Transfer_Complete_Interrupt enabled. Looking good.

You might wonder whether to eliminate |STM32.DMA.Start_Transfer_With_Interrupts| and simply give |Start_Transfer| the same |Enabled_Interrupts| parameter? I could add that to this request if considered helpful.

I have some preference for the original approach with two distinct routines, one with interrupts and one without. It seemed a good distinction at the time anyway.

Would you prefer this patch if it adopted the same approach?

pat-rogers commented 5 years ago

On 9/11/2018 3:15 PM, Simon Wright wrote:

(Later) I made a more extensive patch, to only process a status flag if the corresponding interrupt is enabled; and then ran with just Transfer_Complete_Interrupt enabled. Looking good.

Interesting!

You might wonder whether to eliminate |STM32.DMA.Start_Transfer_With_Interrupts| and simply give |Start_Transfer| the same |Enabled_Interrupts| parameter? I could add that to this request if considered helpful.

I have some preference for the original approach with two distinct routines, one with interrupts and one without. It seemed a good distinction at the time anyway.

Would you prefer this patch if it adopted the same approach?

Yes, sounds good.

Thanks Simon, good sleuthing!

Pat

Fabien-Chouteau commented 5 years ago

Solved in #276