| Message ID | DB4PR08MB8102C882795C5B8725105A0F9E93A@DB4PR08MB8102.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | kernel-fitImage: allow overwrite the configuration dtb label | expand |
On Fri May 16, 2025 at 2:32 PM CEST, Oliver ROHE via lists.openembedded.org wrote: > This allows to provide a custom label for each dtb > without having to overwrite the whole function. > > Signed-off-by: Oliver Rohe <oliver.rohe@wago.com> > --- Hi Oliver, Thanks for your patch. Just a first note about it: it looks like tabs in the patch were replaced by spaces, so this is a bit painful to apply on our side. It might come from your mailer setup. Are you using git send-email command? I've corrected the spacing on my side for this patch, but may you have a look at your configuration for the next time? Thanks Mathieu
> I've corrected the spacing on my side for this patch, but may you have a > look at your configuration for the next time? Thanks a lot Mathieu, unfortunately my company uses exchange, but I'll make sure it's properly formatted next time! Internal
Hi Oliver On Fri, 2025-05-16 at 12:32 +0000, Oliver ROHE via lists.openembedded.org wrote: > This allows to provide a custom label for each dtb > without having to overwrite the whole function. There is also a patch-set pending which does a complete rewrite of this code in Python: https://lists.openembedded.org/g/openembedded-core/message/216836. I guess your change will potentially be simpler if it will would done in Python later on. What do you think? With the current implementation of the kernel-fitimage.bbclass many details are hard-coded to "one configuration node gets generated per dtb". Are you sure that we can change the labels without breaking other things here? What's your exact use case for this? > > Signed-off-by: Oliver Rohe <oliver.rohe@wago.com> > --- > meta/classes-recipe/kernel-fitimage.bbclass | 26 ++++++++++++++----- > -- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/meta/classes-recipe/kernel-fitimage.bbclass > b/meta/classes-recipe/kernel-fitimage.bbclass > index 07786647e1..f770d65087 100644 > --- a/meta/classes-recipe/kernel-fitimage.bbclass > +++ b/meta/classes-recipe/kernel-fitimage.bbclass > @@ -344,6 +344,15 @@ symlink_points_below() { > echo "$realpath" > } > > +# > +# Simple function to allow overwriting the configuration node label > +# > +# $1 ... DTB image name > +fitimage_emit_section_config_dtb_label() { > + dtb_img="$1" > + echo "${FIT_CONF_PREFIX}${dtb_img}" > +} The convention here is that functions starting with fitimage_emit_section_... emits a section. However, this function is just concatenating 2 variables. Thank you for the patch Adrian > + > # > # Emit the fitImage ITS configuration section > # > @@ -404,7 +413,7 @@ fitimage_emit_section_config() { > # conf node name is selected based on dtb ID if it is > present, > # otherwise its selected based on kernel ID > if [ -n "$dtb_image" ]; then > - conf_node=$conf_node$dtb_image > + conf_node=$(fitimage_emit_section_config_dtb_label > $dtb_image) > else > conf_node=$conf_node$kernel_id > fi > @@ -442,13 +451,14 @@ fitimage_emit_section_config() { > # default node is selected based on dtb ID if it is > present, > # otherwise its selected based on kernel ID > if [ -n "$dtb_image" ]; then > - # Select default node as user specified dtb > when > - # multiple dtb exists. > - if [ -n "$default_dtb_image" ]; then > - default_line="default = > \"${FIT_CONF_PREFIX}$default_dtb_image\";" > - else > - default_line="default = > \"${FIT_CONF_PREFIX}$dtb_image\";" > - fi > + # Select default node as user specified dtb > when > + # multiple dtb exists. > + if [ -n "$default_dtb_image" ]; then > + img=$default_dtb_image > + else > + img=$dtb_img > + fi > + default_line="default = > \"$(fitimage_emit_section_config_dtb_label $img)\";" > else > default_line="default = > \"${FIT_CONF_PREFIX}$kernel_id\";" > fi > -- > 2.43.0 > > Internal > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#216750): > https://lists.openembedded.org/g/openembedded-core/message/216750 > Mute This Topic: https://lists.openembedded.org/mt/113144105/3616858 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: > https://lists.openembedded.org/g/openembedded-core/unsub [ > adrian.freihofer@siemens.com] > -=-=-=-=-=-=-=-=-=-=-=- >
Hi guys,
I've just quickly looked at the code and when I see something like
${variable} in bitbake, my control LED starts blinking. Please try to
avoid using the ${ } for shell variables in bitbake' context.
Some day some developer creates a bitbake's variable like: dtb_img =
"my-machine.dtb" and your function will be in troubles.
Just my 5cents.
Regards,
Andy
On 19.05.2025 19:05, Adrian Freihofer via lists.openembedded.org wrote:
> Hi Oliver
>
> On Fri, 2025-05-16 at 12:32 +0000, Oliver ROHE via
> lists.openembedded.org wrote:
>> This allows to provide a custom label for each dtb
>> without having to overwrite the whole function.
> There is also a patch-set pending which does a complete rewrite of this
> code in Python:
> https://lists.openembedded.org/g/openembedded-core/message/216836. I
> guess your change will potentially be simpler if it will would done in
> Python later on. What do you think?
>
> With the current implementation of the kernel-fitimage.bbclass many
> details are hard-coded to "one configuration node gets generated per
> dtb". Are you sure that we can change the labels without breaking other
> things here? What's your exact use case for this?
>
>> Signed-off-by: Oliver Rohe <oliver.rohe@wago.com>
>> ---
>> meta/classes-recipe/kernel-fitimage.bbclass | 26 ++++++++++++++-----
>> --
>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/meta/classes-recipe/kernel-fitimage.bbclass
>> b/meta/classes-recipe/kernel-fitimage.bbclass
>> index 07786647e1..f770d65087 100644
>> --- a/meta/classes-recipe/kernel-fitimage.bbclass
>> +++ b/meta/classes-recipe/kernel-fitimage.bbclass
>> @@ -344,6 +344,15 @@ symlink_points_below() {
>> echo "$realpath"
>> }
>>
>> +#
>> +# Simple function to allow overwriting the configuration node label
>> +#
>> +# $1 ... DTB image name
>> +fitimage_emit_section_config_dtb_label() {
>> + dtb_img="$1"
>> + echo "${FIT_CONF_PREFIX}${dtb_img}"
>> +}
> The convention here is that functions starting with
> fitimage_emit_section_... emits a section. However, this function is
> just concatenating 2 variables.
>
> Thank you for the patch
> Adrian
>
>> +
>> #
>> # Emit the fitImage ITS configuration section
>> #
>> @@ -404,7 +413,7 @@ fitimage_emit_section_config() {
>> # conf node name is selected based on dtb ID if it is
>> present,
>> # otherwise its selected based on kernel ID
>> if [ -n "$dtb_image" ]; then
>> - conf_node=$conf_node$dtb_image
>> + conf_node=$(fitimage_emit_section_config_dtb_label
>> $dtb_image)
>> else
>> conf_node=$conf_node$kernel_id
>> fi
>> @@ -442,13 +451,14 @@ fitimage_emit_section_config() {
>> # default node is selected based on dtb ID if it is
>> present,
>> # otherwise its selected based on kernel ID
>> if [ -n "$dtb_image" ]; then
>> - # Select default node as user specified dtb
>> when
>> - # multiple dtb exists.
>> - if [ -n "$default_dtb_image" ]; then
>> - default_line="default =
>> \"${FIT_CONF_PREFIX}$default_dtb_image\";"
>> - else
>> - default_line="default =
>> \"${FIT_CONF_PREFIX}$dtb_image\";"
>> - fi
>> + # Select default node as user specified dtb
>> when
>> + # multiple dtb exists.
>> + if [ -n "$default_dtb_image" ]; then
>> + img=$default_dtb_image
>> + else
>> + img=$dtb_img
>> + fi
>> + default_line="default =
>> \"$(fitimage_emit_section_config_dtb_label $img)\";"
>> else
>> default_line="default =
>> \"${FIT_CONF_PREFIX}$kernel_id\";"
>> fi
>> --
>> 2.43.0
>>
>> Internal
>>
>>
>>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#216864): https://lists.openembedded.org/g/openembedded-core/message/216864
> Mute This Topic: https://lists.openembedded.org/mt/113144105/3619876
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [andrej.v@skyrain.eu]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Hi Adrian, > There is also a patch-set pending which does a complete rewrite of this > code in Python: > https://urldefense.com/v3/__https://lists.openembedded.org/g/ > openembedded-core/message/216836__;!!B6Ltl4Q! > sx1j_QU1O83_wMeOlorBivh3C9HD06dwOTkdj_Mvw5g0JkiLC3BEVzuC8rMFPlyIDdqYv5r1ln2lPqCb7AhxUktc8So-$ <https://urldefense.com/v3/__https://lists.openembedded.org/g/openembedded-core/message/216836__;!!B6Ltl4Q!sx1j_QU1O83_wMeOlorBivh3C9HD06dwOTkdj_Mvw5g0JkiLC3BEVzuC8rMFPlyIDdqYv5r1ln2lPqCb7AhxUktc8So-$>. I > guess your change will potentially be simpler if it will would done in > Python later on. What do you think? Cool, looks very promising. Especially that the fitimage now builds from the image recipe. I will check and try it out once I have a spare minute. > With the current implementation of the kernel-fitimage.bbclass many > details are hard-coded to "one configuration node gets generated per > dtb". Are you sure that we can change the labels without breaking other > things here? What's your exact use case for this? My patch does not change the default behavior and keeps the dtb name as label for each entry. It just allows to provide a custom key if you want to. From my understanding, the reason for using formats like fitimage/blspec/extlinux is, so that you don’t need to know what kernel/dtb/initramfs combination you want to load, especially if you support multiple variants. We are using a unique device id written into rom to know what combination to boot from the bootloader. Internal
On Tue May 20, 2025 at 10:51 AM CEST, Oliver ROHE via lists.openembedded.org wrote: > Hi Adrian, > > +AD4- There is also a patch-set pending which does a complete rewrite of this > +AD4- code in Python: > +AD4- https://urldefense.com/v3/+AF8AXw-https://lists.openembedded.org/g/ > +AD4- openembedded-core/message/216836+AF8AXwA7ACEAIQ-B6Ltl4Q+ACE- > +AD4- sx1j+AF8-QU1O83+AF8-wMeOlorBivh3C9HD06dwOTkdj+AF8-Mvw5g0JkiLC3BEVzuC8rMFPlyIDdqYv5r1ln2lPqCb7AhxUktc8So-+ACQ- +ADw-https://urldefense.com/v3/+AF8AXw-https://lists.openembedded.org/g/openembedded-core/message/216836+AF8AXwA7ACEAIQ-B6Ltl4Q+ACE-sx1j+AF8-QU1O83+AF8-wMeOlorBivh3C9HD06dwOTkdj+AF8-Mvw5g0JkiLC3BEVzuC8rMFPlyIDdqYv5r1ln2lPqCb7AhxUktc8So-+ACQAPg-. I > +AD4- guess your change will potentially be simpler if it will would done in > +AD4- Python later on. What do you think? > > Cool, looks very promising. Especially that the fitimage now builds from the image recipe. > I will check and try it out once I have a spare minute. > Hi Oliver, Just a quick update: Adrian series on "FIT image improvements" is most likely going to be merged soon: can you rewrite your patch on top of it? Thanks!
> Just a quick update: Adrian series on "FIT image improvements" is most > likely going to be merged soon: can you rewrite your patch on top of it? Yes, absolutely. Will do. Thanks allot! Internal
diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass index 07786647e1..f770d65087 100644 --- a/meta/classes-recipe/kernel-fitimage.bbclass +++ b/meta/classes-recipe/kernel-fitimage.bbclass @@ -344,6 +344,15 @@ symlink_points_below() { echo "$realpath" } +# +# Simple function to allow overwriting the configuration node label +# +# $1 ... DTB image name +fitimage_emit_section_config_dtb_label() { + dtb_img="$1" + echo "${FIT_CONF_PREFIX}${dtb_img}" +} + # # Emit the fitImage ITS configuration section # @@ -404,7 +413,7 @@ fitimage_emit_section_config() { # conf node name is selected based on dtb ID if it is present, # otherwise its selected based on kernel ID if [ -n "$dtb_image" ]; then - conf_node=$conf_node$dtb_image + conf_node=$(fitimage_emit_section_config_dtb_label $dtb_image) else conf_node=$conf_node$kernel_id fi @@ -442,13 +451,14 @@ fitimage_emit_section_config() { # default node is selected based on dtb ID if it is present, # otherwise its selected based on kernel ID if [ -n "$dtb_image" ]; then - # Select default node as user specified dtb when - # multiple dtb exists. - if [ -n "$default_dtb_image" ]; then - default_line="default = \"${FIT_CONF_PREFIX}$default_dtb_image\";" - else - default_line="default = \"${FIT_CONF_PREFIX}$dtb_image\";" - fi + # Select default node as user specified dtb when + # multiple dtb exists. + if [ -n "$default_dtb_image" ]; then + img=$default_dtb_image + else + img=$dtb_img + fi + default_line="default = \"$(fitimage_emit_section_config_dtb_label $img)\";" else default_line="default = \"${FIT_CONF_PREFIX}$kernel_id\";" fi
This allows to provide a custom label for each dtb without having to overwrite the whole function. Signed-off-by: Oliver Rohe <oliver.rohe@wago.com> --- meta/classes-recipe/kernel-fitimage.bbclass | 26 ++++++++++++++------- 1 file changed, 18 insertions(+), 8 deletions(-) -- 2.43.0 Internal