Skip to content

padding and reserve field cleanup #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 28, 2025
Merged

Conversation

Uthedris
Copy link
Contributor

Added code to default padding and reserve in some packed sturct definitions where they were missing.

Removed un-needed padding = 0 and reserver = 0 in register writes.

@Uthedris Uthedris marked this pull request as ready for review April 22, 2025 12:20
@tact1m4n3
Copy link
Collaborator

Should those reserved fields in cortex_m be set to zero by default? I think this isn't recommended to be done.

@Grazfather
Copy link
Collaborator

What should they be set to? The problem is that if you use .write you have to provide a value for the padding fields anyway, and if you use .modify, it's just going to read and write back whatever it read. I don't know of a system (that isn't buggy) where writing a 1 or a 0 to a padding or reserved field actually does anything.

@Uthedris
Copy link
Contributor Author

I suppose the code that parsed the .svd files could set the default for padding and filler by parsing the <reseValue>, but since I don't know of a case where writing zero is harmful there seems little point.

@mattnite
Copy link
Contributor

I'm open to this change for padding and reserved bits, we do need to change the default based off the reset value in the SVD, but also consider that across the entire repo this change is only saving 100 lines of code, so I don't think it's as big of a convenience as it may seem.

@ikskuh and I did discuss this in the past and we decided that the verbosity was worth it over potential hardware bugs, but @Grazfather does make a good point: I've also never seen hardware that misbehaves when writing 0 to padding/reserved bits. Felix what do you think of this change?

@Uthedris
Copy link
Contributor Author

The reason for my orignal PR (#429) suggesting setting these defaults wasn't to save space in the repo, but rather to not make the users of Microzig have to worry about setting unused bits every time they do an Mmio write.

I had wondered whether the write should work like modify, but instead of reading the register to get the base values the write would start with the reset value. That way the user would only have to specify the values that were not the default.

I didn't pursue that because there is value in code being clear and explicitly defining the values of every actual field when initializing a register, though I do think that having to define values for unused parts of the register is a bit much.

@mattnite
Copy link
Contributor

Okay I'm happy with that. Writes must be explicit for all real bitfields -- way too easy to shoot yourself in the foot if we use reset values as defaults, and padding and reserved will be defaulted to zero. I'll make this change in regz as well.

@mattnite mattnite enabled auto-merge (squash) April 25, 2025 14:53
@mattnite
Copy link
Contributor

Okay just need to fix merge conflicts and we're good to go

auto-merge was automatically disabled April 28, 2025 02:13

Head branch was pushed to by a user without write access

@Uthedris
Copy link
Contributor Author

I fixed the conflicts, but there was some wierdness with examples/espressif/esp/src/systimer.zig. I commented it out but someone more familiar with the expressif stuff might want to look at it.

@mattnite mattnite merged commit 7db5d59 into ZigEmbeddedGroup:main Apr 28, 2025
42 checks passed
@Uthedris Uthedris deleted the no_zeros branch April 28, 2025 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants