Problematic enum names are passed through unchecked

[roysmeding]

trafficstars

Hi,

I've been tinkering with the SVD for the STM32F334x microcontrollers, adding enums to make the generated API more useful. At some point, rustfmt started throwing errors and it took me a little bit of effort to figure out why. It turned out I'd named an enum value push-pull, which isn't a valid Rust identifier.

While it makes sense that this breaks, and it was trivial to fix, it would be helpful if svd2rust recognised this sort of error in the SVD file, rather than generating output that then results in a non-obvious compiler error. A similar check could have caught the duplicate enum value names in #112 too.

I'm pretty new to Rust but I'll try to have a look at the code to see how easy this would be to implement. Just thought I'd make an issue first to see what other people think and prevent duplicate effort. :)

[japaric]

It turned out I'd named an enum value push-pull, which isn't a valid Rust identifier.

It's true that 'push-pull' is not a valid Rust identifier but I think it's a valid, although uncommon, enumeratedValue name as defined by the SVD specification. So I think this may actually be a svd2rust bug where it should have normalized that name to uppercase.

it would be helpful if svd2rust recognised this sort of error in the SVD file

IMO this kind of verification should be in the SVD parser. I'm reluctant about doing such verification though, specially if we want 100% compliance to the SVD standard, because no vendor SVD file I have seen passes cleanly a verification of the SVD schema.

[roysmeding]

My reasoning for addressing it here was that this is an issue that doesn't stem from parsing the file, but from turning it into Rust code. In going over the schema, though, I found that enumeratedValue names are identifierType which is defined as the pattern [_A-Za-z0-9]*, so it's invalid according to the spec too.

I'm inclined to say "validate the files and have people fix them if they don't conform" but I realise that might be a bit idealistic… This specific case definitely seems worth addressing, though, since it results in code that won't compile anyway. Detecting the issue in svd2rust (or the parser) makes it possible to refer the user directly to the problem with the SVD file, instead of them having to deduce this from the compiler error.

(P.S. this SVD file for the STM32F334 chips seems to validate in xmllint w/ the XSD you linked)

[thenewwazoo]

I believe that another apparent example of this is in the LPC43xx SVD:

                                <enumeratedValue>
                                    <name>RESUME_DETECTED/DRIV</name>
                                    <description>Resume detected/driven on port.</description>
                                    <value>1</value>
                                </enumeratedValue>

The slash is passed without being sanitized:

pub enum FPRR {
    # [ doc = "No resume (K-state) detected/driven on port." ]
    NO_RESUME_K_STATE_ ,
    # [ doc = "Resume detected/driven on port." ]
    RESUME_DETECTED/DRIV
}

[jaycle]

I encountered something similar today when using svd2rust on a file generated from msp430_svd. The error came ultimately from rustfmt where I got:

error: expected `where`, `{`, `(`, or `;` after struct name, found `-`

due to

pub struct REAL-TIME_CLOCK

I must say I agree with @roysmeding. svd2rust should parse any valid svd file and create valid rust code. Finding out from rustfmt that it wasn't valid is non-ideal. The svd generators shouldn't have to account for any possible language and their identifier rules, IMO.

[Emilgardis]

These SVD files do not follow the spec as the names have to be ANSI C compatible (according to spec)

The spec has varying requirements on identifiers (which can be found here) but the more general statement is

All name tags must comply with the ANSI C identifier naming restrictions. In particular, they must not contain any spaces or special characters. This is necessary to support the generation of device header files thus providing consistency between the names being shown by the debugger and the symbols being used in the CMSIS compliant target software. [link]

edited as I missed a comment which mentioned this

push-pull is not an valid enumeratedValue name. enumeratedValue is defined as such

  <xs:complexType name="enumeratedValueType">
    <xs:sequence>
      <!-- name is a ANSI C indentifier representing the value (C Enumeration) -->
      <xs:element name="name" type="identifierType"/>
      <!-- description contains the details about the semantics/behavior specified by this value -->
      <xs:element name="description" type="stringType" minOccurs="0"/>
      <xs:choice>
        <xs:element name="value" type="enumeratedValueDataType"/>
        <!-- isDefault specifies the name and description for all values that are not
             specifically described individually -->
        <xs:element name="isDefault" type="xs:boolean"/>
      </xs:choice>
    </xs:sequence>
  </xs:complexType>

and the type identifierType is defined as

  <xs:simpleType name="identifierType">
    <xs:restriction base="xs:string">
      <xs:pattern value="[_A-Za-z0-9]*"/>
    </xs:restriction>
  </xs:simpleType>

---

However, the fact that many SVDs are not compliant puts a lot of effort on us as we'll have to decide whether we make svd-parser know of these common errors or demand the user to patch their SVDs. I'm not sure which one would be the best alternative between these two paths.

[pftbest]

@jaycle Please file an issue at msp430_svd

[jamesmunns]

I might suggest that we consider "fixing" bad names, but trigger a WARNING once we have some kind of Logging infrastructure (see https://github.com/japaric/svd2rust/issues/183).

Normal name transforms to meet Rust variable naming standards could be an INFO/DEBUG level log.

[burrbull]

Closing as should be fixed in svd-parser with ValidationLevel::Strict