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?