Topic/unsigned

[reznikmm]

In Ada, data types are not used based on their physical properties. Unlike C, where uint8, uint32 are used everywhere a corresponding range of values is required, in Ada, types are created where the type represents semantic value. This is achieved by the fact that Ada has advanced means of specifying the physical properties of components and variables, and these properties do not depend on the type used. You can always force the compiler to allocate, for example, 2 bits for an Integer type by specifying a range constraint and representation specifier for a specific field. Since it is not possible to know which fields of the SVD description correspond to which semantic types, there is no need to create multiple types for the fields. These created types do not carry useful information and only force the user to insert type conversions to stop the compiler from complaining. It would be more logical to use a single unsigned integer type and use it to describe all components in SVD record types.

This patch adds the --use-unsigned-type=<UInt32> command line option. Specifying this option forces svd2ada to use this type (from the package of --base-types-package=) for the declaration of all components of records and arrays. Corresponding range constraints range 0 .. 2**W-1 are added, where W is the number of bits.

The SVD packages created in this way do not depend on HAL, which simplifies splitting into crates.

CC @godunko @ohenley

[CLAassistant]

All committers have signed the CLA.

[Irvise]

I personally think this is a good idea, if it is "hidden" within a flag, which it is. So I would like to see it within SVD2Ada. It gives another option, while not being the default.

[Fabien-Chouteau]

The problem is that there is still a type (even if anonymous) because you restrict the range. And you are introducing implicit conversions to this anonymous type; this is error-prone.

In Ada, data types are not used based on their physical properties. Unlike

Here we want the physical property because that's the only thing SVD is giving us.

[reznikmm]

Consider two launch of svd2ada:

./bin/svd2ada --package=Root --base-types-package=HAL --boolean -o old --gen-uint-always CMSIS-SVD/ST/STM32F40x.svd
# vis
./bin/svd2ada --package=Root --base-types-package=Interfaces --boolean -o new --use-unsigned-type=Unsigned_32 --gen-uint-always CMSIS-SVD/ST/STM32F40x.svd 

Comparing Root.ADC packages we will see:

-   subtype CR1_AWDCH_Field is HAL.UInt5;
+   subtype CR1_AWDCH_Field is Interfaces.Unsigned_32 range 0 .. 31;

If we have usage like

Root.ADC.ADC1_Periph.CR1.AWDCH := 99;

In the first case compiler output will be:

svd_test.adb:5:38: error: value not in range of type "CR1_AWDCH_Field" defined at root-adc.ads:47
svd_test.adb:5:38: error: static expression fails Constraint_Check

In the second case:

svd_test.adb:5:38: warning: value not in range of type "CR1_AWDCH_Field" defined at root-adc.ads:47 [enabled by default]
svd_test.adb:5:38: warning: Constraint_Error will be raised at run time [enabled by default]

So, compiler spots this in both cases.

[kevlar700]

I think I like this. Minimum sizes are still required for use record (component clause). The only perhaps minor issue I have found so far is that with e.g. HAL.UInt7 you can get a warning such as:

component size overrides size clauses for "HAL.UInt7" [-gnatw.s].

When the last range of a records components in the use record (component clause) is too large.

[kevlar700]

It would also seem to provide for ranges that are more accurate such as values 0 .. 64 in 8 bits (where enumerations are not most appropriate) at the cost of lesser clarity in how many bits each register represents. Though this clarity issue could perhaps be overcome by using byte offsets other than 0. Though I think that would be a bit of an implementation pain when a register spans two registers (a non micro device I am currently working on without SVD does this). I'm also not sure if spanning registers partially is SVD compliant anyway.

Register_1 at 0 range 0 .. 7;
Register_2 at 1 range 0 .. 7;
Register_3 at 2 range 0 .. 7;
Register_4 at 3 range 0 .. 6;
Register_5 at 3 range 7 .. 15;
Register_6 at 4 range 0 .. 7;

[kevlar700]

There is a higher chance of a runtime exception though. If a procedures parameter is for some accidental reason Unsigned_32 and 256 gets passed through to an Unsigned_32 with range 0 .. 255. SPARK would probably catch that but Ada would have an exception at runtime.

[Fabien-Chouteau]

In the first case compiler output will be:

The compiler will only detect this for basic cases on assigning with a constant. My point holds, this is just hiding potential bug.

[pat-rogers]

I believe that the existing bit-specific types are better, because mistakes are compile-time errors that cannot be ignored. (I don't like the subtypes anyway, eg CR1_AWDCH_Field , but that's another story.)
Why not have SVD2Ada define types like those defined in the HAL package (but also the 8|16|32-bit types), and thus not depend on the HAL package?

[kevlar700]

I don't like the subtypes anyway

Could you explain a little if you don't mind? I think that my current stance is that they aid readability.

[pat-rogers]

On 21-Feb-25 9:58 AM, Kevin Chadwick wrote: I don't like the subtypes anyway Could you explain a little if you don't mind? I think that my current stance is that they aid readability.

Ordinarily subtypes would indeed enhance readability, but in this case I cannot see how they do so. In this case they only provide another name for the unsigned type that reflects the structural size of the record component, so they don't add anything and slightly hide the size. It would be simpler to just use the unsigned type directly, and wouldn't lose anything.

[kevlar700]

Ordinarily subtypes would indeed enhance readability, but in this case I cannot see how they do so. In this case they only provide another name for the unsigned type that reflects the structural size of the record component, so they don't add anything and slightly hide the size. It would be simpler to just use the unsigned type directly, and wouldn't lose anything.

I find that having e.g. CR1_AWDCH_Field in a procedures parameter signature helps for looking up the register needed either in the reference manual or in the SVD. Though I do create non volatile versions. Though this is an enum in the SVD anyway. AWDCH is an enum too in the latest STM32 chips SVDs. I correct their naming from B_0x0, B_0x1 etc. to Input, Output, Alternate, Analogue in the GPIO case within the SVD xml though as I require functionality. It is too much work otherwise.

package Non_Volatile is
   package GPIO_MODER_MODE0 is
         type Field is new STM32_SVD.GPIO.MODER_MODER0_Field;
   end GPIO_MODER_MODE0;
end Non_Volatile;

[pat-rogers]

On 25-Feb-25 5:58 AM, Kevin Chadwick wrote: Ordinarily subtypes would indeed enhance readability, but in this case I cannot see how they do so. In this case they only provide another name for the unsigned type that reflects the structural size of the record component, so they don't add anything and slightly hide the size. It would be simpler to just use the unsigned type directly, and wouldn't lose anything. I find that having e.g. CR1_AWDCH_Field in a procedures parameter signature helps for looking up the register needed either in the reference manual or in the SVD.

I see. Do you write code at the register level most of the time, rather than using the abstract data types?

[kevlar700]

I see. Do you write code at the register level most of the time, rather than using the abstract data types?

I try to. The SVDs often need checking/fixing against the reference manual anyway and moving to a new chip in the ST family means that the process is straight forward. Just make sure the SVD is right and the Gnat compiler highlights the issues in any case statements etc.. Often there are some generated/svd name changes but nothing to have to think about most of the time. Last week I found an instance where being tediously verbose with Ada case statements feels wrong but pays dividends vs being concise with enum position arithmetic such as with EXTICR1..4 on STM32U073.