Closed nathanPPS closed 1 year ago
I don't have a clear opinion about this.
On the one hand this example looks incompatible with CMSYS SVD specification.
On the other hand I don't see correct way to describe such registers. In my view there should derivedFrom
be used instead of array.
I looked through the CMSIS SVD specification and couldn't find anything saying the description used for the R7FA2E1A9.svd ELSR* registers is invalid. In fact running the SVD file through the CMSIS SVDConv tool generates the following C struct:
typedef struct { /*!< (@ 0x40041000) ELC Structure */
__IOM uint8_t ELCR; /*!< (@ 0x00000000) Event Link Controller Register */
__IM uint8_t RESERVED;
__IOM uint8_t ELSEGR0; /*!< (@ 0x00000002) Event Link Software Event Generation Register 0 */
__IM uint8_t RESERVED1;
__IOM uint8_t ELSEGR1; /*!< (@ 0x00000004) Event Link Software Event Generation Register 1 */
__IM uint8_t RESERVED2;
__IM uint16_t RESERVED3[5];
__IOM uint16_t ELSR0; /*!< (@ 0x00000010) Event Link Setting Register 0 */
__IM uint16_t RESERVED4;
__IOM uint16_t ELSR1; /*!< (@ 0x00000014) Event Link Setting Register 1 */
__IM uint16_t RESERVED5;
__IOM uint16_t ELSR2; /*!< (@ 0x00000018) Event Link Setting Register 2 */
__IM uint16_t RESERVED6;
__IOM uint16_t ELSR3; /*!< (@ 0x0000001C) Event Link Setting Register 3 */
__IM uint16_t RESERVED7[9];
__IOM uint16_t ELSR8; /*!< (@ 0x00000030) Event Link Setting Register 8 */
__IM uint16_t RESERVED8;
__IOM uint16_t ELSR9; /*!< (@ 0x00000034) Event Link Setting Register 9 */
__IM uint16_t RESERVED9[5];
__IOM uint16_t ELSR12; /*!< (@ 0x00000040) Event Link Setting Register 12 */
__IM uint16_t RESERVED10[3];
__IOM uint16_t ELSR14; /*!< (@ 0x00000048) Event Link Setting Register 14 */
__IM uint16_t RESERVED11;
__IOM uint16_t ELSR15; /*!< (@ 0x0000004C) Event Link Setting Register 15 */
__IM uint16_t RESERVED12[5];
__IOM uint16_t ELSR18; /*!< (@ 0x00000058) Event Link Setting Register 18 */
} ELC_Type; /*!< Size = 90 (0x5a) */
Which takes the same approach as option 2 above, albeit without svd2rust's typing of registers and fields. Granted in this example the array would have to be expanded anyway because the the register size is smaller than the dimIncrement
. Nevertheless, the proposed feature now applies a common type to each ELSR* register.
My opinion is usually to let my tools do the most work they can. In this case the alternative would be to manually go back and patch each ELSR* register present with derivedFrom
. Or I can have the tool infer this for me. The output should be the same, but letting the tool do the work for me saves me time. What do you think @burrbull?
How SVDConv
would convert this block if register size
equal to dimIncrement
? Does it use C arrays? Would it use C arrays for ELSR0-3?
Strangely enough, after looking through a few other examples SVDConv
doesn't seem to implement C arrays at all for registers specified as such in the SVD (even though I would have expected it to do so in a few cases I looked at). Regardless, I think the main point is the SVD file is valid according to SVDConv
which checks against the specification before generating C headers. That being the case, I think the svd2rust
tool should make an effort to do something intelligent with a valid SVD, even if that SVD would be better written using derivedFrom
. Would you agree?
I'm not opposite to support this, specifically option 2. I just afraid of big growing of codebase. Even now it is not easy to support it.
In total this feature adds about 350 lines of code which is mostly contained to a single function check_erc_reprs
. I work in embedded systems professionally and would love to use Rust more, but many parts don't yet have PAC support and this feature would help provide less barrier to entry for more parts, specifically all parts from the Renesas RA series. I understand code bloat should be avoided, but I don't think this falls into that category.
I'll leave this open until CI tests and register contents compare be implemented.
I've run into an svd file where a set of registers is allocated linearly in memory, but only specific registers can be used. For example the span 0-31 is allocated for a register type, but only 0-3, 8, 9, 12, 14, 15 and 18 can be used. See ELSR* registers in R7FA2E1A9.svd from the Renesas RA DFP. When parsed by svd2rust version 0.25.1, this creates new types for each register grouping, often causing name collisions later when compiling the crate. I can see two ways to handle this:
I lean towards option two for the safety reasons, but am interested in other options for handling this use case.