cortex-m
cortex-m copied to clipboard
rust-embedded
Verify peripheral configuration support inline instead of seperate `has_` functions?
Throughout the crate I believe most (if not all) register-mutating functions are unchecked, meaning that the crate user should check for support before enabling and configurating a feature. I consider this reasonable for trivial functions such as DWT::has_exception_trace and DWT::enable_exception_tracing but less so for more complex functions like DWT::configure and ITM::configure which may enable and configure a whole set of features. For these functions it's not trivial to figure out which has_ functions must be called before configuration (presuming they are implemented in the first place).
I believe it would be a better idea to verify support in configure functions before registers are mutated (and return Err if the requested configuration cannot be applied) and offer some configure_unchecked variant when the user knows when a set of features are supported. I'm not sure if we should do the same to the more drivial functions.
Thoughts?
An edge-case to consider are RAZ/WI (Read-as-zero, writes ignored) register fields which do not necessarily have an associated flag bit to check for support. Per the standard one should try to configure these and if they subsequenty read zero (unless zero was written to disable the feature), the configuration failed. An example of this is ITM_TCR.GTSFREQ. Executing an ITM::has_global_timestamps could lead to unintended side-effects because one must:
- read the flag value and store it;
- write something non-zero to the flag location (all non-zero values having side-effects)
- read the flag value again;
- compare the old and new flag values;
- if flags are equal (and both thus zero), return false; or
- if flags differ, write the old value back and return true.
A more proper way to achieve this is to read back the GTSFREQ value in ITM::configure and return failure if the user requested global timestamp generation and it wasn't supported. This must be done before any other bits are flipped, so that we don't end up with a partially applied configuration. In cases where a potentially RAZ/WI field cannot be touched before another field is configured, setup should be split this into some unlock function (e.g. ITM::unlock) when it makes sense or document it. Fortunately I have yet to see any heavy nesting of this sort.
See above PR. Implementing an ITM::configure_unchecked would in this case lead to a lot of code duplication due to the lack of feature support flags.
I don't think the code duplication would be too bad. Just pulling out all of the write logic to *_unchecked and use that function in the safe version after doing all of the checking. You'd do a bit of double checking of the settings struct but it's probably not too bad and probably inlines away.
In general I agree that this proposal makes sense. We should check as much as possible for the user
and use [
*_unchecked] in the safe version after doing all of the checking
Only possible with features that have their own feature flag field, e.g. NOCYCCNT. GTSFREQ is RAZ/WI and must be checked after the register field has been written to. The same applies to RAZ and RAO (read-as-ones) fields.
Ah of course. I read that and then immediately forgot when thinking about this haha.
In that case I'm thinking unchecked variants should only be added if someone requests it or there's a clear use case, otherwise be safe and avoid the code duplication