Short Feedback about ada_drivers_library HOT 9 CLOSED

Thanks for your feedback Emanuel! I think we need a little bit of time to process all that ;)

from ada_drivers_library.

Hello Emanuel,

First of all, many thanks for your detailed feedbacks. It's both very helpful and very constructive :) Here is some elements answering partially your remarks, on the GPIO front:

1. I removed the need for in out mode recently (yesterday), as indeed this does not really make sense in real life, even though from a theoretical point of view and from a software point of view it would seem logical to have the GPIO point instance modified after a call to set, clear or toggle.

   But practically and from someone used to drivers programming, this is indeed unexpected and limitating.

2. On the limited type side for peripheral, note that those limited types are all aliased, which means that you can pass on access values when you need to reference them.

Having limited types is useful sometimes when we need to add values to the peripheral descriptor (such as peripheral states), whose values are not directly derived from the hardware registers. We need to ensure that there's only one instance describing the peripheral in such case. So for homogeneity purpose (as well as allowing this in future updates), we decided to have all peripherals described as limited aliased types.
3. Now on the name clashes, I removed the GPIO enum type, as it was a bit useless. The only useful situation is to retrieve the GPIO port number to setup external interrupts.

New answers to come later on your other points ;)
Best regards,

• Jerome

from ada_drivers_library.

About GPIO 1. (in out paramters)

I think I already explained why I'm against removing the "in out" descriptors, but here are my arguments:

• We should use the language features to make the best possible interface. Using the 'out' descriptor shows that setting or clearing and IO may modify it. It's an information that we provide to the user of that interface.
• You may want to give a read-only 'access' to some part of the code. If we remove the 'out' descriptor we lose that control. Setting or clearing an IO means changing its state, which requires the right to modify it.
• For IO expenders like the MCP23008, one may want to keep an internal representation of the IO to avoid some costly communication with the device. This internal state requires modification rights. Example:

PA0.Clear; --  Clear the IO
PA0.Set; -- Set the IO
PA0.Set; -- Redundant,  this operation can be optimized, i.e. do nothing because the IO is already set

For your return value case @elgor, I would return a "not null GPIO_Point_Ref".

from ada_drivers_library.

My feeling is that such caching issue should be performed then at the GPIO_Port level, not at the GPIO_Point. In GPIO_Point, you have an access to the GPIO Port, so can do whatever you want there, ensuring coherent read/write (as the GPIO_Port is a limited type), as opposed to the GPIO_Point that should really be non-limited here.

I would have:

package HAL.GPIO is 

   type GPIO_Point is interface;
   prodecure Set (Point : GPIO_Point) is abstract;
   ...

package STM32.GPIO is

   type GPIO_Port is limited private;
   type GPIO_Pin is range 0 .. 15;

   type GPIO_Point is new HAL.GPIO.GPIO_Point with record
      Port : access GPIO_Port;
      Pin  : GPIO_Pin;
   end record;

from ada_drivers_library.

Indeed that's one way to implement it, it doesn't mean we should restraint other ways and it's still hiding the fact that there's a modification of internal state.

On the other hand, I don't think there's a good reason to remove the 'out' descriptor.

from ada_drivers_library.

Well, to me you're manipulating an identifier when you manipulate a GPIO_Point, not an actual representation of the hardware pin. In such case, one may expect all manipulations to be 'in', and the type being non-limited (such as me but your mileage may vary here of course). Although the GPIO point is a record type, I really see it as an enum value: just a label.

On the more practical side: having GPIO_Point as non-limited read-only values would allow their storage in structures you don't always have write access to, more flexibility in their use in general.

from ada_drivers_library.

Considering GPIO_Point as identifier you are missing point 1 and 2 of my arguments and you are not using the nice features of the language (which is an important goal of this project).

Note that I'm mostly interested in HAL.GPIO.GPIO_Point here. GPIO_Point is an interface type so you can't put it directly a record anyway.

from ada_drivers_library.

Well, having enum identifiers is one of the nice features of the language ;)

Next, it turns out that when you have the actual GPIO points (e.g. you have an application that knows perfectly well on what hardware it runs on, so have GPIO point as a constrained type), you really need those points to be identifiers: so they should not interfere with your own access rights you want to define for your device.

You don't want read operations to be performed in "in out" mode just because we decided that pins should be "in out" whatever the use case, imho, but the read operation needs sets/clears to be performed on pins. In particular when we all know that it's not needed at all from an implementation point of view.

from ada_drivers_library.

Sure, enums have their own benefits, but what you describe is a project specific and not portable handling of IO. You define an enum with meaningful identifiers for the IO in the context of a specific project. It doesn't apply in this library because we want to favor code reuse and portability with the HAL. Enums don't work in HAL.

from ada_drivers_library.