Re: [PATCH v2] boot: add support for button commands

[Dragan Simic <dsimic@manjaro.org>]

Hello Caleb and Michael,

On 2024-01-11 10:38, Michael Walle wrote:
>>> This is simply awesome, but I see one possible issue -- the need to 
>>> have
>>> proper environment variables defined for a particular board or 
>>> device,
>>> to make the buttons work as expected.  Obviously, those environment
>>> variables can be absent or can become missing for numerous reasons.
>> 
>> Is CFG_EXTRA_ENV_SETTINGS not persistent enough?
> 
> IMHO no. Because a user might accidentially mess up the environment
> variables.

I apologize for my delayed response.

Using CONFIG_EXTRA_ENV_SETTINGS should be good enough to provide
the fallback defaults.  However, the users can still mess the things up,
but again, they can do that already in many places.

>>> I think that we should have an additional mechanism in place that
>>> defines the buttons and the associated commands even if no 
>>> environment
>>> variables are found.  Like a set of fallback defaults for a 
>>> particular
>>> board or device, built into the U-Boot image.  For example, Rockchip
>>> boards have those defaults pretty well defined.
>> 
>> A programmatic API for register button/cmd mapping from
>> board_late_init() (for example) sounds sensible to me.
> 
> I don't know if it has to be that complex, or if it will be enough
> to just have some compile-time constants like CONFIG_BUTTON_CMD_N.

Having compile-time constants is also a viable option, IMHO.  We should
just need some fallback mechanism, and the simpler the better.

>> Is this really an issue that invalidates the implementation proposed
>> here though? It feels much more like a nice-to-have addition that 
>> maybe
>> we could leave out for now?
> 
> Agreed. Looks like one can add it to get_button_cmd() later.

I also agree, but it should be added eventually.

>> It also has a MUCH wider scope imo - should board override env or vice
>> versa? What about triggering default AND custom actions for one button
>> press? What if a board wants to register a callback function instead 
>> of
>> running a command?) - these are questions I don't want to answer with
>> this patch.

Yes, all this opens quite a few questions.  IMHO, we should simply have
some fallback mechanism, for which using CONFIG_EXTRA_ENV_SETTINGS seems
fine to me, and leave everything else to as it already is.  Sure, the
users could mess it up, but that can already happen in many places.

> One use case I have is restoring the default environment *in any case*;
> regardless what the state of the board is. In this case, the 
> environment
> must not override the button settings. This can be a compiled-in
> command, which always takes precedence.

To me, having an option to restore the default environment isn't a bad
idea, but not automatically and always.  Perhaps some other mechanism 
that
triggers the restoration should be devised.

> If you want to be able to overwrite a button command, then I guess you
> can already do that with the environment setting. Provide sane defaults
> via CFG_EXTRA_ENV_SETTINGS and a user can then overwrite it.

I agree.

> In summary, the registered (compiled-in) command should always take
> precedence. If one wants to supply a default command which can be
> changed later, that can go via the (compiled-in) default environment.

Sorry, this is a bit confusing to me.  Didn't you write above that
the users should be able to change the associated commands through
the environment variables?