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

STM32 DMA Interrupts don’t allow priority control #218

Closed simonjwright closed 6 years ago

simonjwright commented 7 years ago

As in the title: to mimic the Certyflie DMA setup I need control of the DMA transfer to the NRF51.

I tried modifying STM32.DMA.Interrupts to

   protected type DMA_Interrupt_Controller
     (Controller : not null access DMA_Controller;
      Stream     : DMA_Stream_Selector;
      ID         : Ada.Interrupts.Interrupt_ID;
      Priority   : System.Interrupt_Priority)
   is
      pragma Interrupt_Priority (Priority);
      ...

(well, to be honest, I tried providing a default for Priority, but that would mean providing defaults for all).

This then leads to a problem with STM32.Device: it declares DMA_Interrupt_Controllers, whose priority would be fixed (and probably not to the one I need!). And of course I can’t declare my own, because that would mean attempting to attach two handlers to the same interrupt.

I’d think we could achieve this by declaring subtypes and leaving it up to the user to declare instances as required, from the provided subtype if the defaults are OK.

   subtype DMA2_Stream6_Type is DMA_Interrupt_Controller
     (Controller => DMA_2'Access,
      Stream     => Stream_6,
      ID         => Ada.Interrupts.Names.DMA2_Stream6_Interrupt,
      Priority   => System.Interrupt_Priority'Last);
Fabien-Chouteau commented 6 years ago

Sorry @simonjwright I've missed this one.

And of course I can’t declare my own, because that would mean attempting to attach two handlers to the same interrupt.

Did you check that? Because it's possible that if the DMA_Interupt_Controller defined in STM32.Device is not used, it will not be included in the final binary and therefore not attached to the interrupt.

Otherwise you are right, this is not flexible enough.

simonjwright commented 6 years ago

I found out because I got a runtime error. However, I was using my cortex-gnat-rts which makes attempting to attach a second handler to an interrupt an error. I just chased through the GNAT GPL 2017 ravenscar-full-stm32f4 code and I think that that RTS doesn’t treat this as an error? I will give it a try.

simonjwright commented 6 years ago

I tried attaching another handler to the DMA2_Stream6_Interrupt and GNAT GPL 2017 ravenscar-full-stm32f4 didn’t complain. I think this is quite a serious bug in the RTS (which handler gets used depends on elaboration order).

Did you check that? Because it's possible that if the DMA_Interupt_Controller defined in STM32.Device is not used, it will not be included in the final binary and therefore not attached to the interrupt.

Looking at the map file, I see

                0x0000000020010c20       0x30 /Users/simon/adacore/certyflie/Ada_Drivers_Library/arch/ARM/STM32/lib/stm32f407/libstm32f407.a(stm32-device.o)
                0x0000000020010c20                stm32__device__dma2_stream0
 .bss.stm32__device__dma2_stream1
                0x0000000020010c50       0x30 /Users/simon/adacore/certyflie/Ada_Drivers_Library/arch/ARM/STM32/lib/stm32f407/libstm32f407.a(stm32-device.o)
                0x0000000020010c50                stm32__device__dma2_stream1
 .bss.stm32__device__dma2_stream2
...

and I think what’s happening is that the elaboration code for STM32.Device creates all the DMA_Interrupt_Controllers, with the data in the appropriate slot, which creates references, so the BSS component gets linked. There won’t be separate function sections for the elaboration of the separate DMA_Interrupt_Controller instances.

lambourg commented 6 years ago

I tried attaching another handler to the DMA2_Stream6_Interrupt and GNAT GPL 2017 ravenscar-full-stm32f4 didn’t complain. I think this is quite a serious bug in the RTS (which handler gets used depends on elaboration order).

Hi Simon,

Here is my view on this:

In the RM C.3.1, 10/3 only talks about runtime reserved interrupts.

Moreover, 12/3 describes how handlers are restored after having been overwritten. and 14.1/3 explicitly allows attaching multiple handlers to interrupts.

Of course, with the ravenscar profile, 12/3 never happens as there's no termination of protected objects, but the elaboration of protected objects still apply, so 14.1/3 still partially applies.

I agree that it's not ideal, and maybe an extra pragma would be needed to prevent this behavior, but my conclusion is that we shouldn't raise a Program_Error upon multiple Attach_Handler to an interrupt.

Now to solve this specific issue, I'm pretty sure that using the proper pragmas to control the elaboration order would solve your issue and would ensure that the proper handler is always used, no matter what.

simonjwright commented 6 years ago

Yes, but ... the person who implemented Ada.Interrupts for that RTS clearly thought PE was appropriate in some circumstances, since all the subprogram bodies raise it!

Not sure you’d need a pragma; this could be an implementation restriction.

lambourg commented 6 years ago

The explicit use of Ada.Interrupts is forbidden in Ravenscar, so that's expected:

No_Dynamic_Attachment: There is no call to any of the operations defined in package Ada.Interrupts (Is_Reserved, Is_Attached, Current_Handler, Attach_Handler, Exchange_Handler, Detach_Handler, and Reference).

Here I'm refering to the dynamic semantics of the Attach_Handler aspect that 14.1/3 covers.

Fabien-Chouteau commented 6 years ago

and I think what’s happening is that the elaboration code for STM32.Device creates all the DMA_Interrupt_Controllers, with the data in the appropriate slot, which creates references, so the BSS component gets linked. There won’t be separate function sections for the elaboration of the separate DMA_Interrupt_Controller instances.

So they end up in the binary even with --gc-sections, this is another problem with this approach.

I wanted to pre-declare all the DMA_Interrupt_Controller in STM32.Device to make it easier for users but it looks like there's too many drawbacks.

Fabien-Chouteau commented 6 years ago

Also done in #264