search

ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

Regression in SVDConv omits bitfield-mask definitions #726

Closed mangoa01 closed 4 years ago

mangoa01 commented 4 years ago

In updating my DFP tooling, to use SVDConv version 3.3.27, I noticed that the bitfield-mask definitions for registers defined as an array are missing in the generated header file. The registers involved use the [%s] placeholder in the name and have a <dim> defined for them. I have attached a zip file containing a mimimal SVD file that demonstrates the behavior as well as the generated header files for versions 3.3.18 and 3.3.27. A simple diff between the header files shows the missing _Pos and _Msk preprocessor definitions.

svd-omission.zip

JonatanAntoni commented 4 years ago

Thanks for raising this. We'll take a look into your reproducer and let you know what we can do.

mangoa01 commented 4 years ago

Sorry to be a pest, but I'd appreciate knowing the status of your review. This particular regression breaks code and I need to decide what I'll do for a fix.

thorstendb-ARM commented 4 years ago

Hi Andrew,

can you please test SVDConv v3.3.31? I have fixed and tested the issue.

BR, Th. de Buhr SVDConv 3-3-31.zip

mangoa01 commented 4 years ago

Version 3.3.31 indeed has _Pos and _Msk defines back in, but it is still not quite right. Now there are _Pos and _Msk definitions for each member of the array of registers. Since the registers form an array and array members are intended to be of the same type, making separate position and mask defines for each element is redundant. For example, where 3.3.18 produced defines like:

#define REG_STATUS_STAT0_Pos
#define REG_STATUS_STAT0_Msk
#define REG_STATUS_STAT1_Pos
#define REG_STATUS_STAT1_Msk

version 3.3.31 produces:

#define REG_STATUS0_STAT0_Pos
#define REG_STATUS0_STAT0_Msk
#define REG_STATUS0_STAT1_Pos
#define REG_STATUS0_STAT1_Msk

#define REG_STATUS1_STAT0_Pos
#define REG_STATUS1_STAT0_Msk
#define REG_STATUS1_STAT1_Pos
#define REG_STATUS1_STAT1_Msk

using the example SVD file in the original report. The intent is to define an array (of 2 in the example) of registers named "STATUS", each register having two bit fields.

Having separate position and mask defines for each element of the array still breaks code and introduces redundant definitions that one must, somewhat arbitrarily, choose which set to use.

Sorry, I don't think version 3.3.31 improves the situation

thorstendb-ARM commented 4 years ago

Agreed. I first had to test scenarios of dim-ed register groups with the same name first, e.g.: TIMERA-C, TIMERE-F, TIMERX-Z, TIMER[n]. In this case just the generics cannot be created because the names are equal.

Please test v3.3.32: SVDConv 3-3-32.zip

I tested using the following snippet, which should cover all cases (double defined Reg6 is an error condition):

    <peripheral>
      <name>PartlyDimDefinedRegisters</name>
      <description>Registers defined in parts</description>
      <baseAddress>0x400C5002</baseAddress>
      <alternatePeripheral>PeriAltNameEqual</alternatePeripheral>
      <registers>
        <register>
          <dim>4</dim>
          <dimIncrement>0x04</dimIncrement>
          <dimIndex>1,2,3,4</dimIndex>
          <name>Reg%s</name>
          <description>register name 1-4</description>
          <addressOffset>0x00</addressOffset>
          <size>32</size>
          <fields>
            <field>
              <name>FIELD</name>
              <description>Field</description>
              <bitOffset>0</bitOffset>
              <bitWidth>1</bitWidth>
              <access>read-write</access>
            </field>
          </fields>
        </register>
        <register>
          <name>Reg5</name>
          <description>register name 5</description>
          <addressOffset>0x10</addressOffset>
          <size>32</size>
          <fields>
            <field>
              <name>FIELD</name>
              <description>Field</description>
              <bitOffset>0</bitOffset>
              <bitWidth>1</bitWidth>
              <access>read-write</access>
            </field>
          </fields>
        </register>
        <register>
          <name>Reg6</name>
          <description>register name 6, conflict</description>
          <addressOffset>0x20</addressOffset>
          <size>32</size>
          <fields>
            <field>
              <name>FIELD</name>
              <description>Field</description>
              <bitOffset>0</bitOffset>
              <bitWidth>1</bitWidth>
              <access>read-write</access>
            </field>
          </fields>
        </register>
        <register>
          <dim>4</dim>
          <dimIncrement>0x04</dimIncrement>
          <dimIndex>6,7,8,9</dimIndex>
          <name>Reg%s</name>
          <description>register name 6-9, address conflict to 1-4</description>
          <addressOffset>0x00</addressOffset>
          <size>32</size>
          <fields>
            <field>
              <name>FIELD</name>
              <description>Field</description>
              <bitOffset>0</bitOffset>
              <bitWidth>1</bitWidth>
              <access>read-write</access>
            </field>
          </fields>
        </register>
        <register>
          <dim>4</dim>
          <dimIncrement>0x04</dimIncrement>
          <name>Reg[%s]</name>
          <description>register array</description>
          <addressOffset>0x100</addressOffset>
          <size>32</size>
          <fields>
            <field>
              <name>FIELD</name>
              <description>Field</description>
              <bitOffset>0</bitOffset>
              <bitWidth>1</bitWidth>
              <access>read-write</access>
            </field>
          </fields>
        </register>
      </registers>
    </peripheral>

Result:

/* =================================================================================================
/* ================                               ARM_PartlyDimDefinedRegisters                     
/* =================================================================================================
/*=========================================================  Reg1  ================================
#define PartlyDimDefinedRegisters_Reg1_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg1_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg2  ================================
#define PartlyDimDefinedRegisters_Reg2_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg2_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg3  ================================
#define PartlyDimDefinedRegisters_Reg3_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg3_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg4  ================================
#define PartlyDimDefinedRegisters_Reg4_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg4_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg5  ================================
#define PartlyDimDefinedRegisters_Reg5_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg5_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg6  ================================
#define PartlyDimDefinedRegisters_Reg6_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg6_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg6  ================================
#define PartlyDimDefinedRegisters_Reg6_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg6_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg7  ================================
#define PartlyDimDefinedRegisters_Reg7_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg7_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg8  ================================
#define PartlyDimDefinedRegisters_Reg8_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg8_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* =========================================================  Reg9  ================================
#define PartlyDimDefinedRegisters_Reg9_FIELD_Pos (0UL)              /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg9_FIELD_Msk (0x1UL)            /*!< FIELD (Bitfield-Mask: 0x01)
/* ==========================================================  Reg  ================================
#define PartlyDimDefinedRegisters_Reg_FIELD_Pos (0UL)               /*!< FIELD (Bit 0)              
#define PartlyDimDefinedRegisters_Reg_FIELD_Msk (0x1UL)             /*!< FIELD (Bitfield-Mask: 0x01)
mangoa01 commented 4 years ago

Version 3.3.32 of SVDConv fixes the problem I had. Thanks for your help. I look forward to its release as I need Linux executables also.

mangoa01 commented 4 years ago

It has been two months now that SVDConv 3.3.32 fixed this problem. However, as far as I can see, there have not been any new official releases of SVDConv. As I said in my previous comment, I need a Linux version of the SVDConv.

So, I'm wondering when we will see new releases of SVDConv?

JonatanAntoni commented 4 years ago

Hi @mangoa01,

We will include a new version of SVDConv in the upcoming CMSIS Release 5.7.0 probably end of February.

Cheers, Jonatan

thorstendb-ARM commented 4 years ago

Official release: please see SVDConv v3.3.35

stephanosio commented 4 years ago

We will include a new version of SVDConv in the upcoming CMSIS Release 5.7.0 probably end of February.

@JonatanAntoni Do you have an estimated date for CMSIS 5.7.0 release?

JonatanAntoni commented 4 years ago

Hi @stephanosio,

we are in stabilization, now. We should be able to finalize in April time frame. The version of SVDConv 3.3.35 that is going to be part of the release is already on develop branch, see in ./CMSIS/Utilities/.

Please let us know if you have any concerns.

Cheers, Jonatan