diff mbox series

[RFC] Provide mechanism for build-time default env entries

Message ID 20200119073738.29189-1-mrjoel@lixil.net
State Needs Review / ACK
Delegated to: Tom Rini
Headers show
Series [RFC] Provide mechanism for build-time default env entries | expand

Commit Message

Joel Johnson Jan. 19, 2020, 7:37 a.m. UTC
This enables the building user to specify environment values to be
included in the static default_environment with an image. This is
useful to build multiple otherwise like configured images, varying
by environment unique entries.

---

I expected something like this to already be present, but couldn't
find such a mechanism. I also assumed that something similar may
have been proposed previously, but also couldn't find anything via
searching.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>
---
 env/Kconfig           | 19 +++++++++++++++----
 include/env_default.h |  3 +++
 2 files changed, 18 insertions(+), 4 deletions(-)

Comments

Wolfgang Denk Jan. 20, 2020, 4:51 p.m. UTC | #1
Dear Joel,

In message <20200119073738.29189-1-mrjoel@lixil.net> you wrote:
> This enables the building user to specify environment values to be
> included in the static default_environment with an image. This is
> useful to build multiple otherwise like configured images, varying
> by environment unique entries.
>
> ---
>
> I expected something like this to already be present, but couldn't
> find such a mechanism. I also assumed that something similar may
> have been proposed previously, but also couldn't find anything via
> searching.
...

> +config USER_ENV_SETTINGS
> +	string "User build-time additional environment entries"
> +	help
> +	  This value is reserved for the building user to provide custom
> +	  environment entries to be added to the default environment. Care must
> +	  be taken to not break the environment, incompatible entries may cause
> +	  failure to compile, or failure of the target board to properly
> +	  initialize or boot. The format is key=value pairs, with entries
> +	  separated by C-style escaped null terminated values, such as:
> +	    "key1=valueA\0key2=valueB\0key3=valueC".

Something in your descripotion must be missing, or this change makes
no sense to me.

>  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
>  	CONFIG_EXTRA_ENV_SETTINGS
> +#endif
> +#ifdef	CONFIG_USER_ENV_SETTINGS
> +	CONFIG_USER_ENV_SETTINGS
>  #endif

What exactly is the difference between CONFIG_EXTRA_ENV_SETTINGS and
CONFIG_USER_ENV_SETTINGS for you, i. e. what can you do with
CONFIG_USER_ENV_SETTINGS that cannot be done in exatly the same way
with CONFIG_EXTRA_ENV_SETTINGS?

Maybe it would help if you include any code that is actually using
this feature?

Best regards,

Wolfgang Denk
Joel Johnson Jan. 20, 2020, 5:09 p.m. UTC | #2
On 2020-01-20 09:51, Wolfgang Denk wrote:
> Dear Joel,
> 
> In message <20200119073738.29189-1-mrjoel@lixil.net> you wrote:
>> This enables the building user to specify environment values to be
>> included in the static default_environment with an image. This is
>> useful to build multiple otherwise like configured images, varying
>> by environment unique entries.
>> 
>> ---
>> 
>> I expected something like this to already be present, but couldn't
>> find such a mechanism. I also assumed that something similar may
>> have been proposed previously, but also couldn't find anything via
>> searching.
> ...
> 
>> +config USER_ENV_SETTINGS
>> +	string "User build-time additional environment entries"
>> +	help
>> +	  This value is reserved for the building user to provide custom
>> +	  environment entries to be added to the default environment. Care 
>> must
>> +	  be taken to not break the environment, incompatible entries may 
>> cause
>> +	  failure to compile, or failure of the target board to properly
>> +	  initialize or boot. The format is key=value pairs, with entries
>> +	  separated by C-style escaped null terminated values, such as:
>> +	    "key1=valueA\0key2=valueB\0key3=valueC".
> 
> Something in your descripotion must be missing, or this change makes
> no sense to me.
> 
>>  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
>>  	CONFIG_EXTRA_ENV_SETTINGS
>> +#endif
>> +#ifdef	CONFIG_USER_ENV_SETTINGS
>> +	CONFIG_USER_ENV_SETTINGS
>>  #endif
> 
> What exactly is the difference between CONFIG_EXTRA_ENV_SETTINGS and
> CONFIG_USER_ENV_SETTINGS for you, i. e. what can you do with
> CONFIG_USER_ENV_SETTINGS that cannot be done in exatly the same way
> with CONFIG_EXTRA_ENV_SETTINGS?
> 
> Maybe it would help if you include any code that is actually using
> this feature?
> 
> Best regards,
> 
> Wolfgang Denk

The main different to me is that many boards set/replace 
CONFIG_EXTRA_ENV_SETTINGS, making it unusable as a user build-time 
configuration value. As a result, I can actually specify values for 
CONFIG_USER_ENV_SETTINGS and have them included in the resulting 
environment instead of overridden by the board configuration.

There is intentionally no code using this feature, as meant in the 
description it is intended to be reserved for the building user's usage. 
The specific use case for which I'm using it is to ease building of 
multiple otherwise identical images, embedding a fixed and well known 
set of ethernet MAC addresses into each unique image for execution in 
environments with no environment storage. It is meant to be used for any 
other values changed or added in the default image environment though - 
if there is a better way to accomplish this I'd be all for it, but I 
couldn't find anything, thus the RFC patch.

Joel
Tom Rini Jan. 20, 2020, 5:15 p.m. UTC | #3
On Mon, Jan 20, 2020 at 10:09:21AM -0700, Joel Johnson wrote:
> On 2020-01-20 09:51, Wolfgang Denk wrote:
> > Dear Joel,
> > 
> > In message <20200119073738.29189-1-mrjoel@lixil.net> you wrote:
> > > This enables the building user to specify environment values to be
> > > included in the static default_environment with an image. This is
> > > useful to build multiple otherwise like configured images, varying
> > > by environment unique entries.
> > > 
> > > ---
> > > 
> > > I expected something like this to already be present, but couldn't
> > > find such a mechanism. I also assumed that something similar may
> > > have been proposed previously, but also couldn't find anything via
> > > searching.
> > ...
> > 
> > > +config USER_ENV_SETTINGS
> > > +	string "User build-time additional environment entries"
> > > +	help
> > > +	  This value is reserved for the building user to provide custom
> > > +	  environment entries to be added to the default environment. Care
> > > must
> > > +	  be taken to not break the environment, incompatible entries may
> > > cause
> > > +	  failure to compile, or failure of the target board to properly
> > > +	  initialize or boot. The format is key=value pairs, with entries
> > > +	  separated by C-style escaped null terminated values, such as:
> > > +	    "key1=valueA\0key2=valueB\0key3=valueC".
> > 
> > Something in your descripotion must be missing, or this change makes
> > no sense to me.
> > 
> > >  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
> > >  	CONFIG_EXTRA_ENV_SETTINGS
> > > +#endif
> > > +#ifdef	CONFIG_USER_ENV_SETTINGS
> > > +	CONFIG_USER_ENV_SETTINGS
> > >  #endif
> > 
> > What exactly is the difference between CONFIG_EXTRA_ENV_SETTINGS and
> > CONFIG_USER_ENV_SETTINGS for you, i. e. what can you do with
> > CONFIG_USER_ENV_SETTINGS that cannot be done in exatly the same way
> > with CONFIG_EXTRA_ENV_SETTINGS?
> > 
> > Maybe it would help if you include any code that is actually using
> > this feature?
> > 
> > Best regards,
> > 
> > Wolfgang Denk
> 
> The main different to me is that many boards set/replace
> CONFIG_EXTRA_ENV_SETTINGS, making it unusable as a user build-time
> configuration value. As a result, I can actually specify values for
> CONFIG_USER_ENV_SETTINGS and have them included in the resulting environment
> instead of overridden by the board configuration.
> 
> There is intentionally no code using this feature, as meant in the
> description it is intended to be reserved for the building user's usage. The
> specific use case for which I'm using it is to ease building of multiple
> otherwise identical images, embedding a fixed and well known set of ethernet
> MAC addresses into each unique image for execution in environments with no
> environment storage. It is meant to be used for any other values changed or
> added in the default image environment though - if there is a better way to
> accomplish this I'd be all for it, but I couldn't find anything, thus the
> RFC patch.

I think part of the problem is that CONFIG_EXTRA_ENV_SETTINGS needs to
be migrated to Kconfig.  And that in turn shows that some amount of
things need to be taken out of the CONFIG namespace and moved in to
distinct headers to be included / keyed-in-to.  What we have in
include/environment/ti/ is a start to that but far from complete.
Joel Johnson Jan. 20, 2020, 5:30 p.m. UTC | #4
On January 20, 2020 5:15:37 PM UTC, Tom Rini <trini@konsulko.com> wrote:
>On Mon, Jan 20, 2020 at 10:09:21AM -0700, Joel Johnson wrote:
>> On 2020-01-20 09:51, Wolfgang Denk wrote:
>> > Dear Joel,
>> > 
>> > In message <20200119073738.29189-1-mrjoel@lixil.net> you wrote:
>> > > This enables the building user to specify environment values to
>be
>> > > included in the static default_environment with an image. This is
>> > > useful to build multiple otherwise like configured images,
>varying
>> > > by environment unique entries.
>> > > 
>> > > ---
>> > > 
>> > > I expected something like this to already be present, but
>couldn't
>> > > find such a mechanism. I also assumed that something similar may
>> > > have been proposed previously, but also couldn't find anything
>via
>> > > searching.
>> > ...
>> > 
>> > > +config USER_ENV_SETTINGS
>> > > +	string "User build-time additional environment entries"
>> > > +	help
>> > > +	  This value is reserved for the building user to provide
>custom
>> > > +	  environment entries to be added to the default environment.
>Care
>> > > must
>> > > +	  be taken to not break the environment, incompatible entries
>may
>> > > cause
>> > > +	  failure to compile, or failure of the target board to
>properly
>> > > +	  initialize or boot. The format is key=value pairs, with
>entries
>> > > +	  separated by C-style escaped null terminated values, such as:
>> > > +	    "key1=valueA\0key2=valueB\0key3=valueC".
>> > 
>> > Something in your descripotion must be missing, or this change
>makes
>> > no sense to me.
>> > 
>> > >  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
>> > >  	CONFIG_EXTRA_ENV_SETTINGS
>> > > +#endif
>> > > +#ifdef	CONFIG_USER_ENV_SETTINGS
>> > > +	CONFIG_USER_ENV_SETTINGS
>> > >  #endif
>> > 
>> > What exactly is the difference between CONFIG_EXTRA_ENV_SETTINGS
>and
>> > CONFIG_USER_ENV_SETTINGS for you, i. e. what can you do with
>> > CONFIG_USER_ENV_SETTINGS that cannot be done in exatly the same way
>> > with CONFIG_EXTRA_ENV_SETTINGS?
>> > 
>> > Maybe it would help if you include any code that is actually using
>> > this feature?
>> > 
>> > Best regards,
>> > 
>> > Wolfgang Denk
>> 
>> The main different to me is that many boards set/replace
>> CONFIG_EXTRA_ENV_SETTINGS, making it unusable as a user build-time
>> configuration value. As a result, I can actually specify values for
>> CONFIG_USER_ENV_SETTINGS and have them included in the resulting
>environment
>> instead of overridden by the board configuration.
>> 
>> There is intentionally no code using this feature, as meant in the
>> description it is intended to be reserved for the building user's
>usage. The
>> specific use case for which I'm using it is to ease building of
>multiple
>> otherwise identical images, embedding a fixed and well known set of
>ethernet
>> MAC addresses into each unique image for execution in environments
>with no
>> environment storage. It is meant to be used for any other values
>changed or
>> added in the default image environment though - if there is a better
>way to
>> accomplish this I'd be all for it, but I couldn't find anything, thus
>the
>> RFC patch.
>
>I think part of the problem is that CONFIG_EXTRA_ENV_SETTINGS needs to
>be migrated to Kconfig.  And that in turn shows that some amount of
>things need to be taken out of the CONFIG namespace and moved in to
>distinct headers to be included / keyed-in-to.  What we have in
>include/environment/ti/ is a start to that but far from complete.

That sounds like a good path indeed, but wouldn't that still result in the Kconfig value being required to contain the full extra contents? The nice thing about a separate config entry is that it can serve as an overlay of just selected overrides and/or custom entries, instead of requiring following and updating user configs for in-source changes which are unrelated, but still captured in board level EXTRA

Either way works, I'd still be in favor of a separate overlay, thus the RFC patch. If you'd like it all eventually consolidated in EXTRA then I'll just carry the patch locally.

Joel
Tom Rini Jan. 20, 2020, 6:27 p.m. UTC | #5
On Mon, Jan 20, 2020 at 05:30:57PM +0000, Joel Johnson wrote:
> 
> 
> On January 20, 2020 5:15:37 PM UTC, Tom Rini <trini@konsulko.com> wrote:
> >On Mon, Jan 20, 2020 at 10:09:21AM -0700, Joel Johnson wrote:
> >> On 2020-01-20 09:51, Wolfgang Denk wrote:
> >> > Dear Joel,
> >> > 
> >> > In message <20200119073738.29189-1-mrjoel@lixil.net> you wrote:
> >> > > This enables the building user to specify environment values to
> >be
> >> > > included in the static default_environment with an image. This is
> >> > > useful to build multiple otherwise like configured images,
> >varying
> >> > > by environment unique entries.
> >> > > 
> >> > > ---
> >> > > 
> >> > > I expected something like this to already be present, but
> >couldn't
> >> > > find such a mechanism. I also assumed that something similar may
> >> > > have been proposed previously, but also couldn't find anything
> >via
> >> > > searching.
> >> > ...
> >> > 
> >> > > +config USER_ENV_SETTINGS
> >> > > +	string "User build-time additional environment entries"
> >> > > +	help
> >> > > +	  This value is reserved for the building user to provide
> >custom
> >> > > +	  environment entries to be added to the default environment.
> >Care
> >> > > must
> >> > > +	  be taken to not break the environment, incompatible entries
> >may
> >> > > cause
> >> > > +	  failure to compile, or failure of the target board to
> >properly
> >> > > +	  initialize or boot. The format is key=value pairs, with
> >entries
> >> > > +	  separated by C-style escaped null terminated values, such as:
> >> > > +	    "key1=valueA\0key2=valueB\0key3=valueC".
> >> > 
> >> > Something in your descripotion must be missing, or this change
> >makes
> >> > no sense to me.
> >> > 
> >> > >  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
> >> > >  	CONFIG_EXTRA_ENV_SETTINGS
> >> > > +#endif
> >> > > +#ifdef	CONFIG_USER_ENV_SETTINGS
> >> > > +	CONFIG_USER_ENV_SETTINGS
> >> > >  #endif
> >> > 
> >> > What exactly is the difference between CONFIG_EXTRA_ENV_SETTINGS
> >and
> >> > CONFIG_USER_ENV_SETTINGS for you, i. e. what can you do with
> >> > CONFIG_USER_ENV_SETTINGS that cannot be done in exatly the same way
> >> > with CONFIG_EXTRA_ENV_SETTINGS?
> >> > 
> >> > Maybe it would help if you include any code that is actually using
> >> > this feature?
> >> > 
> >> > Best regards,
> >> > 
> >> > Wolfgang Denk
> >> 
> >> The main different to me is that many boards set/replace
> >> CONFIG_EXTRA_ENV_SETTINGS, making it unusable as a user build-time
> >> configuration value. As a result, I can actually specify values for
> >> CONFIG_USER_ENV_SETTINGS and have them included in the resulting
> >environment
> >> instead of overridden by the board configuration.
> >> 
> >> There is intentionally no code using this feature, as meant in the
> >> description it is intended to be reserved for the building user's
> >usage. The
> >> specific use case for which I'm using it is to ease building of
> >multiple
> >> otherwise identical images, embedding a fixed and well known set of
> >ethernet
> >> MAC addresses into each unique image for execution in environments
> >with no
> >> environment storage. It is meant to be used for any other values
> >changed or
> >> added in the default image environment though - if there is a better
> >way to
> >> accomplish this I'd be all for it, but I couldn't find anything, thus
> >the
> >> RFC patch.
> >
> >I think part of the problem is that CONFIG_EXTRA_ENV_SETTINGS needs to
> >be migrated to Kconfig.  And that in turn shows that some amount of
> >things need to be taken out of the CONFIG namespace and moved in to
> >distinct headers to be included / keyed-in-to.  What we have in
> >include/environment/ti/ is a start to that but far from complete.
> 
> That sounds like a good path indeed, but wouldn't that still result in the Kconfig value being required to contain the full extra contents? The nice thing about a separate config entry is that it can serve as an overlay of just selected overrides and/or custom entries, instead of requiring following and updating user configs for in-source changes which are unrelated, but still captured in board level EXTRA
> 
> Either way works, I'd still be in favor of a separate overlay, thus the RFC patch. If you'd like it all eventually consolidated in EXTRA then I'll just carry the patch locally.

It's really hard to say how much of what's in CONFIG_EXTRA_ENV_SETTINGS
today should stay that way vs being put in to a common header that can
be re-used.  That in fact might lead to making it easier to include an
additional file of environment contents as while we can put in
escape-sequenced strings via Kconfig (and do for mtdparts related stuff)
it's really not user-friendly and another case of where we're encoding
information via Kconfig that could be done more nicely.
Wolfgang Denk Jan. 20, 2020, 6:59 p.m. UTC | #6
Dear Joel,

In message <7cac871967a1317cea251e78d67a30ac@lixil.net> you wrote:
>
> >> +config USER_ENV_SETTINGS
> >> +	string "User build-time additional environment entries"
> >> +	help
> >> +	  This value is reserved for the building user to provide custom
> >> +	  environment entries to be added to the default environment. Care 
> >> must
> >> +	  be taken to not break the environment, incompatible entries may 
> >> cause
> >> +	  failure to compile, or failure of the target board to properly
> >> +	  initialize or boot. The format is key=value pairs, with entries
> >> +	  separated by C-style escaped null terminated values, such as:
> >> +	    "key1=valueA\0key2=valueB\0key3=valueC".
> > 
> > Something in your descripotion must be missing, or this change makes
> > no sense to me.
> > 
> >>  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
> >>  	CONFIG_EXTRA_ENV_SETTINGS
> >> +#endif
> >> +#ifdef	CONFIG_USER_ENV_SETTINGS
> >> +	CONFIG_USER_ENV_SETTINGS
> >>  #endif
> > 
> > What exactly is the difference between CONFIG_EXTRA_ENV_SETTINGS and
> > CONFIG_USER_ENV_SETTINGS for you, i. e. what can you do with
> > CONFIG_USER_ENV_SETTINGS that cannot be done in exatly the same way
> > with CONFIG_EXTRA_ENV_SETTINGS?
...

> The main different to me is that many boards set/replace 
> CONFIG_EXTRA_ENV_SETTINGS, making it unusable as a user build-time 
> configuration value. As a result, I can actually specify values for 
> CONFIG_USER_ENV_SETTINGS and have them included in the resulting 
> environment instead of overridden by the board configuration.

But what exactly is the difference between editing some include
file to change CONFIG_EXTRA_ENV_SETTINGS or editing some include
file to change CONFIG_USER_ENV_SETTINGS ?

To me this makes no sense.

Also, you probably cannot do what you want anyway.  Assume
CONFIG_EXTRA_ENV_SETTINGS has a setting of BOOTDELAY to 5 seconds,
and you want to change it to 1 second only.  with your code you have
the same variable definition twice in your default environment.  Do
you know what the environment importer will do?

> There is intentionally no code using this feature, as meant in the 
> description it is intended to be reserved for the building user's usage. 

I think you are following a wrong approach.  Such user specific
settings should IMHO loaded separately, either over serial like
(should be fast enough for a few hundred bytes) or from external
storage. "env import" is your friend.

> The specific use case for which I'm using it is to ease building of 
> multiple otherwise identical images, embedding a fixed and well known 
> set of ethernet MAC addresses into each unique image for execution in 
> environments with no environment storage. It is meant to be used for any 
> other values changed or added in the default image environment though - 

Editing files and building new images seems not the right method to
me.  If each image is different, you will never ever be able to
reproduce which image has been installed where.

> if there is a better way to accomplish this I'd be all for it, but I 
> couldn't find anything, thus the RFC patch.

There are many ways to provision code new hardware; depending on
hardware features more or less.  You can always import envrionment
blobs from any device you can read from, including serial line
and (if available) network. You may be able to boot the system over
USB, you may use DFU, etc. 

Changing the code and building a new image for each and every board
is pretty much pessimal.

Best regards,

Wolfgang Denk
Joel Johnson Jan. 21, 2020, 5:20 p.m. UTC | #7
On 2020-01-20 11:59, Wolfgang Denk wrote:
> Dear Joel,
> 
> In message <7cac871967a1317cea251e78d67a30ac@lixil.net> you wrote:
>> 
>> >> +config USER_ENV_SETTINGS
>> >> +	string "User build-time additional environment entries"
>> >> +	help
>> >> +	  This value is reserved for the building user to provide custom
>> >> +	  environment entries to be added to the default environment. Care
>> >> must
>> >> +	  be taken to not break the environment, incompatible entries may
>> >> cause
>> >> +	  failure to compile, or failure of the target board to properly
>> >> +	  initialize or boot. The format is key=value pairs, with entries
>> >> +	  separated by C-style escaped null terminated values, such as:
>> >> +	    "key1=valueA\0key2=valueB\0key3=valueC".
>> >
>> > Something in your descripotion must be missing, or this change makes
>> > no sense to me.
>> >
>> >>  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
>> >>  	CONFIG_EXTRA_ENV_SETTINGS
>> >> +#endif
>> >> +#ifdef	CONFIG_USER_ENV_SETTINGS
>> >> +	CONFIG_USER_ENV_SETTINGS
>> >>  #endif
>> >
>> > What exactly is the difference between CONFIG_EXTRA_ENV_SETTINGS and
>> > CONFIG_USER_ENV_SETTINGS for you, i. e. what can you do with
>> > CONFIG_USER_ENV_SETTINGS that cannot be done in exatly the same way
>> > with CONFIG_EXTRA_ENV_SETTINGS?
> ...
> 
>> The main different to me is that many boards set/replace
>> CONFIG_EXTRA_ENV_SETTINGS, making it unusable as a user build-time
>> configuration value. As a result, I can actually specify values for
>> CONFIG_USER_ENV_SETTINGS and have them included in the resulting
>> environment instead of overridden by the board configuration.
> 
> But what exactly is the difference between editing some include
> file to change CONFIG_EXTRA_ENV_SETTINGS or editing some include
> file to change CONFIG_USER_ENV_SETTINGS ?
> 
> To me this makes no sense.

The primary difference is in how we store the changes - include file 
changes are necessarily in-tree source changes, whereas setting the 
Kconfig value can be easily done by script obtaining the needed values 
from an external storage mechanism.

> Also, you probably cannot do what you want anyway.  Assume
> CONFIG_EXTRA_ENV_SETTINGS has a setting of BOOTDELAY to 5 seconds,
> and you want to change it to 1 second only.  with your code you have
> the same variable definition twice in your default environment.  Do
> you know what the environment importer will do?

This is a valid concern and the biggest gap in the approach. I don't 
have a full and proper solution for it currently, but have started 
looking into a couple of ideas. In the meantime, I tried to draw 
attention to it in the help text, essentially saying "here be dragons", 
be careful.

>> There is intentionally no code using this feature, as meant in the
>> description it is intended to be reserved for the building user's 
>> usage.
> 
> I think you are following a wrong approach.  Such user specific
> settings should IMHO loaded separately, either over serial like
> (should be fast enough for a few hundred bytes) or from external
> storage. "env import" is your friend.

I'm intentionally trying to have no persistent environment separate from 
boot image.

>> The specific use case for which I'm using it is to ease building of
>> multiple otherwise identical images, embedding a fixed and well known
>> set of ethernet MAC addresses into each unique image for execution in
>> environments with no environment storage. It is meant to be used for 
>> any
>> other values changed or added in the default image environment though 
>> -
> 
> Editing files and building new images seems not the right method to
> me.  If each image is different, you will never ever be able to
> reproduce which image has been installed where.
> 
>> if there is a better way to accomplish this I'd be all for it, but I
>> couldn't find anything, thus the RFC patch.
> 
> There are many ways to provision code new hardware; depending on
> hardware features more or less.  You can always import envrionment
> blobs from any device you can read from, including serial line
> and (if available) network. You may be able to boot the system over
> USB, you may use DFU, etc.
> 
> Changing the code and building a new image for each and every board
> is pretty much pessimal.
> 
> Best regards,
> 
> Wolfgang Denk

Joel
Wolfgang Denk Jan. 21, 2020, 5:49 p.m. UTC | #8
Dear Joel,

In message <ac347e2e978aa082fc516c789d0f9048@lixil.net> you wrote:
>
> > I think you are following a wrong approach.  Such user specific
> > settings should IMHO loaded separately, either over serial like
> > (should be fast enough for a few hundred bytes) or from external
> > storage. "env import" is your friend.
>
> I'm intentionally trying to have no persistent environment separate from 
> boot image.

In which way does this prevent you from following my suggestion?
such a separate environment block (in any format, either as plain
ASCII text, or binary, or as checksummed blob) can easily be
attached to the U-Boot image and handled as a combined unit.  Then
you just import the additional environment from this well-known
address.  This gives you all the possibilities you have for handling
the environment, conflict free.

Best regards,

Wolfgang Denk
Joel Johnson Jan. 21, 2020, 9:35 p.m. UTC | #9
On 2020-01-21 10:49, Wolfgang Denk wrote:
> Dear Joel,
> 
> In message <ac347e2e978aa082fc516c789d0f9048@lixil.net> you wrote:
>> 
>> > I think you are following a wrong approach.  Such user specific
>> > settings should IMHO loaded separately, either over serial like
>> > (should be fast enough for a few hundred bytes) or from external
>> > storage. "env import" is your friend.
>> 
>> I'm intentionally trying to have no persistent environment separate 
>> from
>> boot image.
> 
> In which way does this prevent you from following my suggestion?
> such a separate environment block (in any format, either as plain
> ASCII text, or binary, or as checksummed blob) can easily be
> attached to the U-Boot image and handled as a combined unit.  Then
> you just import the additional environment from this well-known
> address.  This gives you all the possibilities you have for handling
> the environment, conflict free.
> 
> Best regards,
> 
> Wolfgang Denk

Ah, I didn't catch the gist of your suggestion then since you references 
serial and storage.

Doing something like you suggest sounds like it would be a reasonable 
option, but as far as I'm aware there isn't anything currently built-in 
to provide such a separate capability. Plus, if I'm understanding your 
suggestion, it would still require an execution of "env import" to load 
from the separate block instead of being seeded automatically on boot. 
The default environment is already stored in some embedded location, so 
my goal would be to store the defaults in the existing environment 
segment. Ideally that would have a compile-time processing check to 
deduplicate entries based on specified precedence, but the simple fix 
for my use case worked as was better than what I could find already 
provided.

Joel
Wolfgang Denk Jan. 22, 2020, 9:43 a.m. UTC | #10
Dear Joel,

In message <da8de88ab6c98da726e3548367e09dd9@lixil.net> you wrote:
>
> Ah, I didn't catch the gist of your suggestion then since you references 
> serial and storage.

These were examples just to demonstrate the flexibility.

> Doing something like you suggest sounds like it would be a reasonable 
> option, but as far as I'm aware there isn't anything currently built-in 
> to provide such a separate capability.

But as a plus, it does not require any code changes, and you can
still use the same binary U-Boot image for all your boards.

Doing some "dd" + "cat" to concatenate the two files with proper
alignment is trivial; this can and should be done independent from
the U-Boot build as you don't have to build a new U-Boot image for
each of your boards any more - you just provide a new environment
blob to concat with the u-boot.bin .

> Plus, if I'm understanding your 
> suggestion, it would still require an execution of "env import" to load 
> from the separate block instead of being seeded automatically on boot. 

There is no "instead".  Yes, it requires to run such an "env import"
command, but this also can be added without code change but just
defining such a command with CONFIG_PREBOOT.  The it will
automatically do that on every boot before doing anything else.

> The default environment is already stored in some embedded location, so 
> my goal would be to store the defaults in the existing environment 
> segment.

As discussed, this is a bad idea, as this would result in
board-specific images, which is a maintenance nightmare.

> Ideally that would have a compile-time processing check to 
> deduplicate entries based on specified precedence, but the simple fix 
> for my use case worked as was better than what I could find already 

Such a deduplication is non-trivial.  You would need something that
extrcts the preprocessor-generated string, parses it according to the
internal structure of the environment, and rewrites it.  And just to
do something your way, while other (much more flexible) ways exist?

My general thinking at uch things is: if you can script something or
do it in other ways that don't require code changes, then it is
worth having a closer look in those other ways.

Best regards,

Wolfgang Denk
diff mbox series

Patch

diff --git a/env/Kconfig b/env/Kconfig
index ed12609f6a..5049cb78be 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -556,14 +556,25 @@  config SYS_RELOC_GD_ENV_ADDR
 	  Relocate the early env_addr pointer so we know it is not inside
 	  the binary. Some systems need this and for the rest, it doesn't hurt.
 
+config USER_ENV_SETTINGS
+	string "User build-time additional environment entries"
+	help
+	  This value is reserved for the building user to provide custom
+	  environment entries to be added to the default environment. Care must
+	  be taken to not break the environment, incompatible entries may cause
+	  failure to compile, or failure of the target board to properly
+	  initialize or boot. The format is key=value pairs, with entries
+	  separated by C-style escaped null terminated values, such as:
+	    "key1=valueA\0key2=valueB\0key3=valueC".
+
 config USE_DEFAULT_ENV_FILE
 	bool "Create default environment from file"
 	help
 	  Normally, the default environment is automatically generated
-	  based on the settings of various CONFIG_* options, as well
-	  as the CONFIG_EXTRA_ENV_SETTINGS. By selecting this option,
-	  you can instead define the entire default environment in an
-	  external file.
+	  based on the settings of various CONFIG_* options, combined with values
+	  of board specific CONFIG_EXTRA_ENV_SETTINGS and user provided
+	  CONFIG_USER_ENV_SETTINGS. By selecting this option, you can instead
+	  define the entire default environment in an external file.
 
 config DEFAULT_ENV_FILE
 	string "Path to default environment file"
diff --git a/include/env_default.h b/include/env_default.h
index 56a8bae39a..9396a34715 100644
--- a/include/env_default.h
+++ b/include/env_default.h
@@ -109,6 +109,9 @@  const uchar default_environment[] = {
 #endif
 #ifdef	CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
+#endif
+#ifdef	CONFIG_USER_ENV_SETTINGS
+	CONFIG_USER_ENV_SETTINGS
 #endif
 	"\0"
 #else /* CONFIG_USE_DEFAULT_ENV_FILE */