FPGA C Generator generating invalid C Code with Clusters of Clusters Of Fixed Point Numbers

[thad.house]

I am building an FPGA program that I want to interface with C, and am running into a few issues with the generated code. I have a few controls that are a cluster of clusters, with the inner clusters containing fixed point numbers. When the generator creates the typedefs for the fixed point numbers, it creates all of them with the same name, causing compilation errors. It needs to include the inner cluster name as part of the generated name, in order to not collide. I have attached a minimal reproduction VI, along with the built FPGA file and the generated c files. 

It generates the following 3 times, missing the inner cluster name. Because this is a struct definition, this causes a compilation failure.

```

const NiFpga_FxpTypeInfo NiFpga_GenFailureVI_ControlCluster_Cluster_FixedNumber_TypeInfo =

{

   1,

   16,

   16

};

```

This is using LabVIEW 2019 with the 2019 version of the C generator as well.

[Terry_ALE]

Need some clarification.

Are you using LabVIEW FPGA?

Which FPGA board are you using?

Can you spell out the intended workflow in more detail? 

[thad.house]

Yes, I am using LabVIEW FPGA with a roboRIO as the target. 

The intended workflow is to have some properties to be able to configure one of my blocks in the FPGA from C/C++ code. It's not a problem with the FPGA code itself, that works perfectly. It's entirely in the C generator, and it generating invalid C code for a valid FPGA image. The example I posted is just a minimal project, with only whats needed to reproduce the issue. 

[Michael_Strain]

Thanks for the clear and easily reproducible bug report.  I filed bug 946334 (if you contact support, they can use that number to get the status of it).  It is targeted to be fixed in the 20.0 release of the C API generator.

[thad.house]

Awesome, thanks for the update. I noticed another issue, where those structs, along with the Resource definitions and the PackedSizeInBytes definitions also need to be made static, otherwise ODR violations occur.

I work on the C++ team fro FRC, and am playing around with writing my own FPGA code, so if you ever want to test, I am happy to test before general release to ensure any bugs are worked out. The generated headers would be really nice to compile with `-pedantic -Wall -Wextra`, but currently they don't for a few other reasons. I would be happy to help attempt to fix some of these issues to make the C generator better and more compliant with C and C++ standards.

[Michael_Strain]

Thanks for pointing that out.  We will make the static fix as well.

In general we try to support -Wall -Wextra -Werror (and warning level 4 with msvc), but occasionally people using different compiler versions will see warnings we don't.  Pedantic can be a bit over zealous at times and IIRC doesn't play well with dlsym, so we generally don't use it.  Let us know if you have any other comments.

We've considered putting NiFpga.h and .c up on github for easy contributions and might do so sometime next year.  We've considered the same for the C API generator, but its use of internal dependencies makes it more difficult. 

[thad.house]

Makes sense. Would definitely be cool to see those on GitHub, I would be happy to contribute.

pedantic definitely is hard. Right now what I currently do is use pragmas to disable the pedantic warning specifically only around dysym and the IRQ enum, since gcc 7 on a 19.0 device warns on those. 

Just want to double check, through an internal contact I submitted an issue about FXP code causing aliasing errors. I got an updated header from him that fixed the issue, I just want to make sure that made it in.

Those are the only issues I see currently through my testing. It would be cool to see the whole generator too on GitHub if possible, but I do understand if that is harder.

[Michael_Strain]

I hadn't considered using pragma disable.  That would be an easy enough fix for the dlsym part.

The aliasing fix for the FXP code in the header did make it in and will also ship with 20.0.  Thanks for reporting that one also.