Add _iter and _len methods to block and fieldset arrays

[Wassasin]

Arrays in blocks and fieldsets result in accessors being generated with an index argument. This argument is then checked in an assert-block, which is only checked in debug builds typically. The amount of elements though is not emitted as of yet. Hence it requires HAL implementors to define a constant themselves. If for whatever reason the amount of elements changes, the code thus is incorrect.

We could add methods like _len to denote the number of elements. We could also add _iter to conveniently allow the user to iterate over every element. For fieldsets an iterator is not a straightforward API. I considered adding a callback interface for this, but for now I did not add it.

Unsure whether this is actually in line with what we want from chiptool, as we tend to favor 'light and simple' code per my understanding. I am very happy to discuss what is a good API to achieve similar results.

[diondokter]

Implementation looks good

[diondokter]

Should these be inline always?
Not sure. We could make them #[inline] too which only gives a strong signal to the compiler to inline it, but not force it.

But maybe it should be always inlined...

[Dirbaio]

I do agree we need something like this, i've also been annoyed with having to redefine lengths in HALs.

The approach of generating name_len, name_iter methods is a bit ugly though. I wonder if we could make an Array<T> type in common.rs that contains pointer+stride+len. It could implement the [] operator and have .len(), .iter() etc methods, implement IntoIter, whatever. Feels much more idiomatic.

The downside is it is a syntax breaking change, but that's perhaps okay? We just made the enum casing breaking change which is much bigger.

This argument is then checked in an assert-block, which is only checked in debug builds typically

btw this is not true. assert! is always checked therefore is OK to rely on for soundness, debug_assert! is only checked in debug builds.

[Wassasin]

I am unsure if Array<T> would have very negative effects on codegen with those values not being constant anymore, especially with monomorphization. But we can try it out and check.

If we want the concept to apply to register fields as well, we'll probably need a separate type for that.

[inferiorhumanorgans]

The approach of generating name_len, name_iter methods is a bit ugly though. I wonder if we could make an Array<T> type in common.rs that contains pointer+stride+len. It could implement the [] operator and have .len(), .iter() etc methods, implement IntoIter, whatever. Feels much more idiomatic.

Is it reasonable to include index checking for holey/sparse arrays or is that veering too much towards bloat?

[Wassasin]

Using core::ops::Index is not really going to work as that always returns a reference, so we cannot construct say a Reg<T, A> and return it.

I'll update this PR with a proposal and then we can discuss further.

[Wassasin]

Pushed a proposal for how the array for ptrs (registers & blocks) could look like. Still need to consider how this would look for fieldsets. I am not too hot on the FromPtr trait being in the public API. Let me know what you think,

[Wassasin]

The approach of generating name_len, name_iter methods is a bit ugly though. I wonder if we could make an Array<T> type in common.rs that contains pointer+stride+len. It could implement the [] operator and have .len(), .iter() etc methods, implement IntoIter, whatever. Feels much more idiomatic.

Is it reasonable to include index checking for holey/sparse arrays or is that veering too much towards bloat?

We could also add that, but perhaps it might be better to add it as a separate PR later.

[Wassasin]

I played around with arrays for fields and have come to the conclusion that it (currently) is infeasible.

My approach basically meant having a FieldElement<SET, VALUE> where SET is the fieldset, and VALUE is the enum or value type of the FieldElement. For each applicable combination of these a getter and setter would be implemented as both SET and VALUE have their own underlying integer data. On the fieldset accessing the field would yield a FieldArray or FieldCursedArray. Each would implement

• fn get(&mut self, n: usize) -> FieldElem<SET, VALUE>
• pub const fn len(&self) -> usize
• pub fn into_iter(self) -> ....

Accessing field that is an array basically requires owning a mutable reference to the underlying fieldset so that the value in the fieldset can be updated. This is problematic as:

• For iterators this would require something like lending iterators, which we could do, but never will truly be a real iterator (we would need to use the while let-syntax instead of for)
• If you do not want to change the field, we would need a non-mutable reference. (like when using defmt to format the fieldset) To implement this, code would need to be duplicated between mutable and non-mutable variants. The accessor trait like for Reg<T, A: Access> would not work here as we need to own the mutable reference instead of being able to magically create it out of thin air like we can for registers and their pointers.
• Currently we have bit widths for fields and enums, separately. They do not need to be identical. Hence the arrays and FieldElem would need to store the field bit width (or mask) resulting in storing an extra word.
• Supporting discontinuous fields makes this feature a whole new level of complex.

Currently I would rather just add _len to fields and call it a day until at least lending iterators are a thing and this feature enables the user to write more idiomatic code.

[datdenkikniet]

I'm just thinking out loud (so feel free to ignore/dismiss), but am wondering: what (major) benefits does this bring over wrapping array accessors in const blocks in the places where one really cares about it, i.e.

let my_reg = const { MY_PERIPHERAL.array(2) };

which will fail to build if n is out of bounds? Both the approach in this PR and "put it in a const block" require a bit of work from whoever is using the PAC, and neither are "super" straightforward (IMO).

The main use-case I can think of is that you can have a const { MY_PERIPHERAL.array().len() != THE_LENGTH_IT_WAS_ORIGINALLY }, but I don't know enough to have any ideas for what that might be useful for.

The current behaviour of panicking at runtime on out-of-bounds accesses isn't really improved by this beyond that the HAL can catch it now, but what will it do with that information beyond also panicking?

I'd also be quite interested (because I haven't encountered it) to see what one could use these iterators/lengths for. Any example use cases that you already have in mind?

[Wassasin]

I'd also be quite interested (because I haven't encountered it) to see what one could use these iterators/lengths for. Any example use cases that you already have in mind?

Typically you would want to update all registers and fields in one go. For Trustzone for example, large sets of memory regions need to be set to secure (and parts optionally non-secure). Forgetting a single register is a security issue.

This PR will make it possible to express the following (which is not const):

for rom_mem_rule in ahb_secure_ctrl.rom_mem_rule() {
    rom_mem_rule.write(|w| {
        for j in 0..w.rule_len() {
            w.set_rule(j, Rule::SecurePrivUserAllowed);
        }
    });
}

[inferiorhumanorgans]

The other TZ example that comes to mind are interrupts. The NVIC and ICU expose interrupts as 32-bit registers and it'd be nice to reduce the boilerplate code to a (const?) iterator over each (NVIC,ICU) instead of a mess of cfg directives.