Re: [PATCH v7 05/10] rust: io: add IoLoc and IoWrite types

[Gary Guo]

On Mon Mar 2, 2026 at 1:44 AM GMT, Alexandre Courbot wrote:
> On Mon Mar 2, 2026 at 12:11 AM JST, Gary Guo wrote:
>> On Sat Feb 28, 2026 at 12:33 AM GMT, Alexandre Courbot wrote:
>>> On Sat Feb 28, 2026 at 3:02 AM JST, Gary Guo wrote:
>>> <snip>
>>>>> +/// A pending I/O write operation, bundling a value with the [`IoLoc`] it should be written to.
>>>>> +///
>>>>> +/// Created by [`IoLoc::set`], [`IoLoc::zeroed`], [`IoLoc::default`], [`IoLoc::init`], or
>>>>> +/// [`IoLoc::init_default`], and consumed by [`Io::write`] or [`Io::try_write`] to perform the
>>>>> +/// actual write.
>>>>> +///
>>>>> +/// The value can be modified before writing using [`IoWrite::update`] or [`IoWrite::try_update`],
>>>>> +/// enabling a builder pattern:
>>>>> +///
>>>>> +/// ```ignore
>>>>> +/// io.write(REGISTER.init(|v| v.with_field(x)));
>>>>> +/// ```
>>>>
>>>> Thinking about this again, I think we might still want to write
>>>>
>>>>     io.write(REGISTER, value)
>>>
>>> This was the original design, but as you point out below this makes the
>>> very common case of writing a register value built from scratch more
>>> verbose than it needs. Real-world examples are significantly worse, e.g:
>>>
>>>     bar.write(
>>>     regs::NV_PFALCON_FALCON_DMATRFMOFFS::of::<E>()
>>>         .try_init(|r| r.try_with_offs(load_offsets.dst_start + pos))?,
>>>     );
>>
>> My main dissatisfaction with this is with the function call
>>
>> I wonder if we can just have
>>
>>     io.write(loc, value)
>>     io.write_with(loc, updater)
>>
>> where the latter simply is a function that does
>>
>>     io.write(loc, updater(T::zeroed()))
>>
>> then the example would be
>>
>>     bar.write_with(
>>         regs::NV_PFALCON_FALCON_DMATRFMOFFS::of::<E>(),
>>         |r| r.try_with_offs(load_offsets.dst_start + pos)
>>     )
>
> That should be doable. Note that we currently support `zeroed` and
> `default` as initializers, so having the same level of coverage would
> require two `write` variants. I'd like to hear what Danilo thinks.

I looked at current Nova changes, it looks like they all just use the zeroed
version.

I wonder if just providing a single version that starts with
`Default::default()` should be sufficient? For most users, zeroed version is the
default version anyway. For those where default is not zero, it perhaps makes
more sense to start with default anyway; if explicitly zeroing is needed they
can always do an explicit `::zeroed()`.

I think with this we can make the API look nice for the common case, removing
most of API complexity that the current design have, and still preserve the
ability do full custom things, with perhaps just a little more verbosity in
code.

>
>>
>> [ Note: it's possible to have write_with that works with both fallible and
>> non-fallible callbacks. You can define a trait like a monad (well, not fully
>> a monad, because this just has a map, not a return and a bind):
>>
>> trait Map<T> {
>>     type Mapped<U>;
>>     
>>     fn map<U>(self, f: impl FnOnce(T) -> U) -> Self::Mapped<U>;
>> }
>>
>> impl<T> Map<T> for T {
>>     type Mapped<U> = U;
>>     
>>     fn map<U>(self, f: impl FnOnce(T) -> U) -> Self::Mapped<U> {
>>         f(self)
>>     }
>> }
>>
>> impl<T, E> Map<T> for Result<T, E> {
>>     type Mapped<U> = Result<U, E>;
>>     
>>     fn map<U>(self, f: impl FnOnce(T) -> U) -> Self::Mapped<U> {
>>         Ok(f(self?))
>>     }
>> }
>>
>> impl Io {    
>>     fn write_with<R: IoLoc<T>, U: Map<T>, T: Zeroable, F: FnOnce(T) -> U>(&self, loc: R, f: F) -> U::Mapped<()> {
>>         f(T::zeroed).map(|x| self.write(loc, x))
>>     }
>> }
>>
>> then this returns `()` if closure cannot fail, and returns `Result<()>` if it
>> fails).
>
> This approach look like it could also be used for I/O in general - right
> now we do not handle bus errors, but we definitely should.
>
>>
>> end of note ]
>>
>> BTW, I am also not very happy with the `::<E>()` syntax. I think with the I/O
>> projection upcoming, we might be able to get rid of relative offseting
>> completely by requiring user to first project into `View<'_, .., FalconBase>` and then have
>> some registers that can only be used on `Io<Type = FalconBase>` but nothing else.
>
> That's something we can always update once I/O projection is available
> if it can indeed be applied (using a two-steps approach if necessary). I
> can already hear the pitchforks if we don't deliver some basic register
> support for 7.1. :)

Yeah, don't treat the above as "I don't like this so please fix before merge". I
am just pointing out things that can be in general improvements and there's no
need to make everything perfect from the get go.

Best,
Gary