Add support for enums #10
Conversation
|
How we doing on this one. It is holding up some development on a chip support package for me. |
|
@amykyta3 Is there any plan to integrate this into main? |
|
Is anyone still working on this? We need to resolve the conflicts and get this merge done. |
|
Yep. I'm catching up to some backlog. I will merge this and other updates today/tomorrow |
|
woohoo!!! THANK YOU!!! |
amykyta3
left a comment
amykyta3
left a comment
There was a problem hiding this comment.
I think a few changes are warranted here before we can merge this.
- Generating enum typedefs is useful, so I want to keep that, but they really ought to be generated "next to" the register that it is associated with and not in a totally different part of the header.
- Use of the enum types in the bit-field structs is dangerous and will not work in all cases. Existing implementation also has unresolved runtime issues. I would recommend removing
- If above things are resolved, I'm comfortable making this feature always enabled and therefore the command-line option can be removed.
|
|
||
| def get_field_type(field: FieldNode) -> str: | ||
| encode = field.get_property("encode") | ||
| return f"uint{regwidth}_t" if not self.ds.generate_enums or encode is None else \ |
There was a problem hiding this comment.
This code will cause a runtime error. The variable regwidth is not set.
| self.write(f"uint{regwidth}_t :{field.low - current_offset:d};\n") | ||
| current_offset = field.low | ||
| self.write(f"uint{regwidth}_t {kwf(field.inst_name)} :{field.width:d};\n") | ||
| self.write(f"{get_field_type(field)} {kwf(field.inst_name)} :{field.width:d};\n") |
There was a problem hiding this comment.
I'm not sure it is generically safe to use the enum type in C bitfields.
C enums are interpreted as type int which does not have a guaranteed bit-width like the stdint types.
This may work ok for compilers that use 32-bit ints, and register maps that use 32-bit regs, but beyond that the bit-packing may fail. (what if a design uses registers that are 8 or 16-bits wide, or the target GCC architecture uses 64-bit ints?)
I would recommend removing this change, and only generate the enum typedefs on the side without actually using them in the struct definition. C is weakly typed in this sense, so the utility of having the enum definition still benefits the user.
|
|
||
| for node in top_nodes: | ||
| self.root_node = node | ||
| RDLWalker().walk(node, Listener()) |
There was a problem hiding this comment.
Please do not nest another listener/walker here.
I think it would be cleaner to have enum typedefs be emitted alongside the registers that use them in the existing listener.
Recommend emitting enums in an enter_Reg() callback in the HeaderGenerator listener. That will result in enums be defined right next to the register they belong to, as well as not result in the tool traversing the entire design hierarchy a second time.
| help=""" | ||
| Enable generation of enum definitions. | ||
| """ | ||
| ) |
There was a problem hiding this comment.
I think we can remove the option, and have enums always be part of the output.
|
Concur on not using enums inside register structs for the reasons you state. We really do need enums for bit fields that are actually enumerated values in the design and in the description. We all hate bare numeric constants in code we are reading/maintaining. I like your idea of putting them right next to the register structure that will use them. When you are tracing back to a dot h file you can easily find the values being applied to a field in the struct even without it being declared enum. I hope these changes can be made quickly and a new MR generated soon. |
|
I started making the changes, but then I realized I would basically need to change everything. Given that this is a relatively small change and that you have a much better knowledge of the corner cases, I think it is best if you handle it. On my side, I've stopped using PeakRDL-cheader main branch last year because I wanted more features (C++ header & support for custom user properties), so I don't have any SystemRDL code that would work to test the changes. |
|
No problem, and agree - I'll use this PR as a reference and implement the changes on my end. |
This comment was marked as off-topic.
This comment was marked as off-topic.
4 participants
Adds an option
--generate-enumsto generate enum typedefs at the top of the header and use them in struct fields.I didn't manage to run the tests so I haven't touched that part.