Re: [PATCH V6 3/8] libgpiod: Add rust wrapper crate

[Kent Gibson <warthog618@gmail.com>]

On Fri, Oct 14, 2022 at 04:25:31PM +0200, Bartosz Golaszewski wrote:
> On Fri, Oct 14, 2022 at 11:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 14-10-22, 11:45, Bartosz Golaszewski wrote:
> > > Maybe also add chained mutators everywhere? To be able to do
> > > settings.set_direction().set_edge() etc.?
> >
> > Based on Kent's suggestion earlier, what I have implemented is
> > set_prop(), to which one can pass all settings and it will apply them
> > in a loop.
> >
> >     pub fn set_prop(&mut self, values: &[SettingVal]) -> Result<()> {
> >         for value in values {
> >             match value {
> >                 SettingVal::Direction(val) => self.set_direction(*val)?,
> >                 SettingVal::EdgeDetection(val) => self.set_edge_detection(*val)?,
> >                 SettingVal::Bias(val) => self.set_bias(*val)?,
> >                 SettingVal::Drive(val) => self.set_drive(*val)?,
> >                 SettingVal::ActiveLow(val) => self.set_active_low(*val),
> >                 SettingVal::DebouncePeriod(val) => self.set_debounce_period(*val),
> >                 SettingVal::EventClock(val) => self.set_event_clock(*val)?,
> >                 SettingVal::OutputValue(val) => self.set_output_value(*val)?,
> >             }
> >         }
> >
> >         Ok(())
> >     }
> >
> > I think that replaces the need of nested ones ? And if we want to add
> > those later, we can always come back and add them. But I am not sure
> > it would be required.
> >
> 
> I cannot find Kent's comment on that - what was the reasoning behind this?
> 

The comment was:

    Add a Setting enum that has a variant for each setting.
    Then you only need 3 mutators total.
    And the user can define configs as a list of Settings.
    So perhaps the mutators should accept &[Setting].
    And &[offsets] rather than just offset.

    Similarly, gets can be consolidated into:

      get_prop_offset(self, offset, SettingKind) -> Result<Setting>

    and
      get_prop_default(self, SettingKind) -> Result<Setting>

    (simplified signatures)


Prior to the line_settings change there was a veritable forest of
mutators, so the idea was to consolidate them where possible, cos in Rust
you can.  But behind the scenes the implementation is just fanning them
out again, so I'm not sure I see the benefit if that is the case.

If the mutators for each field still exist they may as well be pub.

And they should return Result<&mut Self> so they can be chained, as you
suggest.

Wrt the values param (which I would prefer was called props), the idea
was that the config could be built and passed around as pure Rust
objects.  The set_prop() applied that list to the C line config, at the
time using one of the default/offset/subset mutators.  So it decoupled
the settings from the scope they were to be applied to.  Not such an
issue now - the scope is always line.
Might still be useful, might not.

> > > And I would still love a thorough API review from someone who actually
> > > knows rust too. :(
> >
> > Well, Kent did a very good job earlier. I am not sure if he has extra
> > cycles to review this once again, though not a lot has changed since
> > last time.
> >
> 
> Yeah sorry Kent, I forgot we're at v6 already and you did review the
> previous iterations. :)
> 

My review was of v4.  Things have changed a bit since then.
And I agree with your original point - it would be good to get a review
from someone that actually uses Rust in anger - I only toy with it.

Cheers,
Kent.