diff mbox series

[1/1] Lua: Add support for retrieving hardware boardname/revision

Message ID 20220720162310.3079881-1-james.hilliard1@gmail.com
State Superseded, archived
Headers show
Series [1/1] Lua: Add support for retrieving hardware boardname/revision | expand

Commit Message

James Hilliard July 20, 2022, 4:23 p.m. UTC
If one wants to make use of the same lua script for different
boards/revisions one needs a way to fetch that information from
within the lua script.

This adds the following functions:
swupdate.get_hw_boardname()
swupdate.get_hw_revision()

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Stefano Babic July 21, 2022, 7:34 a.m. UTC | #1
Hi James,

On 20.07.22 18:23, James Hilliard wrote:
> If one wants to make use of the same lua script for different
> boards/revisions one needs a way to fetch that information from
> within the lua script.
> 
> This adds the following functions:
> swupdate.get_hw_boardname()
> swupdate.get_hw_revision()
> 

Ok - but what about to implement a more "Lua" function ? Lua can return 
multiple value, and we always need both boardname and revision, so we 
could have just pushing both on the Lua's stack:

	boardname, revision = swupdate.get_hw()

Best regards,
Stefano

> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>   corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> index dd1be9e..dd962cf 100644
> --- a/corelib/lua_interface.c
> +++ b/corelib/lua_interface.c
> @@ -923,6 +923,36 @@ static int l_get_selection(lua_State *L) {
>   	return 2;
>   }
>   
> +static int l_get_hw_boardname(lua_State *L)
> +{
> +	struct swupdate_cfg *cfg = get_swupdate_cfg();
> +
> +	if (get_hw_revision(&cfg->hw) < 0)
> +		goto l_get_hw_boardname_exit;
> +
> +	lua_pushstring(L, cfg->hw.boardname);
> +	return 1;
> +
> +l_get_hw_boardname_exit:
> +	lua_pushnil(L);
> +	return 1;
> +}
> +
> +static int l_get_hw_revision(lua_State *L)
> +{
> +	struct swupdate_cfg *cfg = get_swupdate_cfg();
> +
> +	if (get_hw_revision(&cfg->hw) < 0)
> +		goto l_get_hw_revision_exit;
> +
> +	lua_pushstring(L, cfg->hw.revision);
> +	return 1;
> +
> +l_get_hw_revision_exit:
> +	lua_pushnil(L);
> +	return 1;
> +}
> +
>   
>   #ifdef CONFIG_HANDLER_IN_LUA
>   static int l_get_tmpdir(lua_State *L)
> @@ -1003,6 +1033,8 @@ static const luaL_Reg l_swupdate[] = {
>           { "umount", l_umount },
>           { "getroot", l_getroot },
>           { "getversion", l_getversion },
> +        { "get_hw_boardname", l_get_hw_boardname },
> +        { "get_hw_revision", l_get_hw_revision },
>           { "progress", l_notify_progress },
>           { NULL, NULL }
>   };
Storm, Christian July 21, 2022, 8:52 a.m. UTC | #2
Hi,

> > If one wants to make use of the same lua script for different
> > boards/revisions one needs a way to fetch that information from
> > within the lua script.
> > 
> > This adds the following functions:
> > swupdate.get_hw_boardname()
> > swupdate.get_hw_revision()
> > 

... and don't forget to add it to the new Lua Handler interface specification
in handlers/swupdate.lua ;)


> Ok - but what about to implement a more "Lua" function ? Lua can return
> multiple value, and we always need both boardname and revision, so we could
> have just pushing both on the Lua's stack:
> 
> 	boardname, revision = swupdate.get_hw()

Well, I'd opt for returning a Table, then you can pack in more information
later on without changing the signature (think of, e.g., DMI data or
something else). You can wrap it like

  boardname, revision = table.unpack(swupdate.get_hw())

to get Stefano's proposal. Or you may use a "dict-like" Table with
proper key/value pairs but then the table.unpack() doesn't work...



Kind regards,
   Christian
James Hilliard July 21, 2022, 9:01 a.m. UTC | #3
On Thu, Jul 21, 2022 at 1:34 AM Stefano Babic <sbabic@denx.de> wrote:
>
> Hi James,
>
> On 20.07.22 18:23, James Hilliard wrote:
> > If one wants to make use of the same lua script for different
> > boards/revisions one needs a way to fetch that information from
> > within the lua script.
> >
> > This adds the following functions:
> > swupdate.get_hw_boardname()
> > swupdate.get_hw_revision()
> >
>
> Ok - but what about to implement a more "Lua" function ? Lua can return
> multiple value, and we always need both boardname and revision, so we
> could have just pushing both on the Lua's stack:

Maybe I've missed something but multi-function returns seem kind of
annoying since you can't really use them in line easily without an assignment.

I find they also make the code less readable, especially for something
that's not likely to get changed much like an installer script as the function
name is less descriptive since you have to look up docs and such to verify
what the return values mean.

>
>         boardname, revision = swupdate.get_hw()

Maybe as a third option for cases where one actually needs to query both
values in the same place would make sense?

>
> Best regards,
> Stefano
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >   corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++
> >   1 file changed, 32 insertions(+)
> >
> > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> > index dd1be9e..dd962cf 100644
> > --- a/corelib/lua_interface.c
> > +++ b/corelib/lua_interface.c
> > @@ -923,6 +923,36 @@ static int l_get_selection(lua_State *L) {
> >       return 2;
> >   }
> >
> > +static int l_get_hw_boardname(lua_State *L)
> > +{
> > +     struct swupdate_cfg *cfg = get_swupdate_cfg();
> > +
> > +     if (get_hw_revision(&cfg->hw) < 0)
> > +             goto l_get_hw_boardname_exit;
> > +
> > +     lua_pushstring(L, cfg->hw.boardname);
> > +     return 1;
> > +
> > +l_get_hw_boardname_exit:
> > +     lua_pushnil(L);
> > +     return 1;
> > +}
> > +
> > +static int l_get_hw_revision(lua_State *L)
> > +{
> > +     struct swupdate_cfg *cfg = get_swupdate_cfg();
> > +
> > +     if (get_hw_revision(&cfg->hw) < 0)
> > +             goto l_get_hw_revision_exit;
> > +
> > +     lua_pushstring(L, cfg->hw.revision);
> > +     return 1;
> > +
> > +l_get_hw_revision_exit:
> > +     lua_pushnil(L);
> > +     return 1;
> > +}
> > +
> >
> >   #ifdef CONFIG_HANDLER_IN_LUA
> >   static int l_get_tmpdir(lua_State *L)
> > @@ -1003,6 +1033,8 @@ static const luaL_Reg l_swupdate[] = {
> >           { "umount", l_umount },
> >           { "getroot", l_getroot },
> >           { "getversion", l_getversion },
> > +        { "get_hw_boardname", l_get_hw_boardname },
> > +        { "get_hw_revision", l_get_hw_revision },
> >           { "progress", l_notify_progress },
> >           { NULL, NULL }
> >   };
>
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================
James Hilliard July 21, 2022, 9:09 a.m. UTC | #4
On Thu, Jul 21, 2022 at 2:52 AM Christian Storm
<christian.storm@siemens.com> wrote:
>
> Hi,
>
> > > If one wants to make use of the same lua script for different
> > > boards/revisions one needs a way to fetch that information from
> > > within the lua script.
> > >
> > > This adds the following functions:
> > > swupdate.get_hw_boardname()
> > > swupdate.get_hw_revision()
> > >
>
> ... and don't forget to add it to the new Lua Handler interface specification
> in handlers/swupdate.lua ;)

In here?:
https://github.com/sbabic/swupdate/tree/master/handlers

I'm not seeing that file.

>
>
> > Ok - but what about to implement a more "Lua" function ? Lua can return
> > multiple value, and we always need both boardname and revision, so we could
> > have just pushing both on the Lua's stack:
> >
> >       boardname, revision = swupdate.get_hw()
>
> Well, I'd opt for returning a Table, then you can pack in more information
> later on without changing the signature (think of, e.g., DMI data or
> something else). You can wrap it like
>
>   boardname, revision = table.unpack(swupdate.get_hw())
>
> to get Stefano's proposal. Or you may use a "dict-like" Table with
> proper key/value pairs but then the table.unpack() doesn't work...

Um, is there a better way than this to say lookup a key?:

function type_str(root)
  for k, v in pairs(swupdate.ROOT_DEVICE) do
    if v == root.type then return k end
  end
  return nil
end

function root_type()
  local root = swupdate.getroot()
  local type = type_str(root)
  local value = root.value
  return value, type
end

This API seems not ideal(or maybe I'm missing something obvious here).

>
>
>
> Kind regards,
>    Christian
>
> --
> Dr. Christian Storm
> Siemens AG, Technology, T CED SES-DE
> Otto-Hahn-Ring 6, 81739 München, Germany
>
> --
> You received this message because you are subscribed to the Google Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20220721085242.egwhxwazjv6cqnuc%40cosmos.fritz.box.
Storm, Christian July 21, 2022, 9:53 a.m. UTC | #5
Hi,

> > > > If one wants to make use of the same lua script for different
> > > > boards/revisions one needs a way to fetch that information from
> > > > within the lua script.
> > > >
> > > > This adds the following functions:
> > > > swupdate.get_hw_boardname()
> > > > swupdate.get_hw_revision()
> > > >
> >
> > ... and don't forget to add it to the new Lua Handler interface specification
> > in handlers/swupdate.lua ;)
> 
> In here?:
> https://github.com/sbabic/swupdate/tree/master/handlers
> 
> I'm not seeing that file.

Ah, yes, my fault, it hasn't been merged yet... Here's the patch:
https://groups.google.com/g/swupdate/c/gOxZGH1gbLA/m/g2l82YAkAAAJ


> > > Ok - but what about to implement a more "Lua" function ? Lua can return
> > > multiple value, and we always need both boardname and revision, so we could
> > > have just pushing both on the Lua's stack:
> > >
> > >       boardname, revision = swupdate.get_hw()
> >
> > Well, I'd opt for returning a Table, then you can pack in more information
> > later on without changing the signature (think of, e.g., DMI data or
> > something else). You can wrap it like
> >
> >   boardname, revision = table.unpack(swupdate.get_hw())
> >
> > to get Stefano's proposal. Or you may use a "dict-like" Table with
> > proper key/value pairs but then the table.unpack() doesn't work...
> 
> Um, is there a better way than this to say lookup a key?:
> 
> function type_str(root)
>   for k, v in pairs(swupdate.ROOT_DEVICE) do
>     if v == root.type then return k end
>   end
>   return nil
> end
> 
> function root_type()
>   local root = swupdate.getroot()

This gives you a Table with these fields (copied from the interface spec):

--- @field type   number  Root device type, one of `swupdate.ROOT_DEVICE`'s values
--- @field value  string  Root device path
--- @field path   string  Full root device path, if identified, else nil

>   local type = type_str(root)

Currently, it's this (again copied from the interface spec):

--- @class swupdate.ROOT_DEVICE
--- Lua equivalent of `root_dev_type` as in `include/lua_util.h`.
--- @type  table<string, number>
swupdate.ROOT_DEVICE = {
    PATH      = 0,
    UUID      = 1,
    PARTUUID  = 2,
    PARTLABEL = 3
}

To cover your use case, one would add a reverse lookup to swupdate.ROOT_DEVICE,
so that swupdate.ROOT_DEVICE[root.type] gives you the string value.
Now, either you can monkey-patch swupdate.ROOT_DEVICE in your handler or
you can extend corelib/lua_interface.c to provide this reverse lookup.

In essence something along the lines I have done here:
https://gitlab.com/cip-project/cip-sw-updates/swupdate-handler-roundrobin/-/blob/master/swupdate_handlers_roundrobin.lua#L56


Kind regards,
   Christian
James Hilliard July 22, 2022, 1:41 a.m. UTC | #6
On Thu, Jul 21, 2022 at 3:53 AM Christian Storm
<christian.storm@siemens.com> wrote:
>
> Hi,
>
> > > > > If one wants to make use of the same lua script for different
> > > > > boards/revisions one needs a way to fetch that information from
> > > > > within the lua script.
> > > > >
> > > > > This adds the following functions:
> > > > > swupdate.get_hw_boardname()
> > > > > swupdate.get_hw_revision()
> > > > >
> > >
> > > ... and don't forget to add it to the new Lua Handler interface specification
> > > in handlers/swupdate.lua ;)
> >
> > In here?:
> > https://github.com/sbabic/swupdate/tree/master/handlers
> >
> > I'm not seeing that file.
>
> Ah, yes, my fault, it hasn't been merged yet... Here's the patch:
> https://groups.google.com/g/swupdate/c/gOxZGH1gbLA/m/g2l82YAkAAAJ
>
>
> > > > Ok - but what about to implement a more "Lua" function ? Lua can return
> > > > multiple value, and we always need both boardname and revision, so we could
> > > > have just pushing both on the Lua's stack:
> > > >
> > > >       boardname, revision = swupdate.get_hw()
> > >
> > > Well, I'd opt for returning a Table, then you can pack in more information
> > > later on without changing the signature (think of, e.g., DMI data or
> > > something else). You can wrap it like
> > >
> > >   boardname, revision = table.unpack(swupdate.get_hw())
> > >
> > > to get Stefano's proposal. Or you may use a "dict-like" Table with
> > > proper key/value pairs but then the table.unpack() doesn't work...
> >
> > Um, is there a better way than this to say lookup a key?:
> >
> > function type_str(root)
> >   for k, v in pairs(swupdate.ROOT_DEVICE) do
> >     if v == root.type then return k end
> >   end
> >   return nil
> > end
> >
> > function root_type()
> >   local root = swupdate.getroot()
>
> This gives you a Table with these fields (copied from the interface spec):
>
> --- @field type   number  Root device type, one of `swupdate.ROOT_DEVICE`'s values
> --- @field value  string  Root device path
> --- @field path   string  Full root device path, if identified, else nil
>
> >   local type = type_str(root)
>
> Currently, it's this (again copied from the interface spec):
>
> --- @class swupdate.ROOT_DEVICE
> --- Lua equivalent of `root_dev_type` as in `include/lua_util.h`.
> --- @type  table<string, number>
> swupdate.ROOT_DEVICE = {
>     PATH      = 0,
>     UUID      = 1,
>     PARTUUID  = 2,
>     PARTLABEL = 3
> }
>
> To cover your use case, one would add a reverse lookup to swupdate.ROOT_DEVICE,
> so that swupdate.ROOT_DEVICE[root.type] gives you the string value.
> Now, either you can monkey-patch swupdate.ROOT_DEVICE in your handler or
> you can extend corelib/lua_interface.c to provide this reverse lookup.

I mean my type_string local function does work, but it's just not a
great interface IMO.

For something like fetching revision/boardname I'm not really seeing the
advantage to using a table as it just seems to introduce excessive complexity.

>
> In essence something along the lines I have done here:
> https://gitlab.com/cip-project/cip-sw-updates/swupdate-handler-roundrobin/-/blob/master/swupdate_handlers_roundrobin.lua#L56

Hmm, so I'm not actually using a lua handler feature AFAIU.

I'm just using a lua script like this:
https://sbabic.github.io/swupdate/sw-description.html#lua

What I'm trying to do is make it so that I can have a single lua script with
some conditional boardname/revision branching logic for properly handling
a few minor differences between boards/revisions.

I'm not really trying to implement a full separate handler.

>
>
> Kind regards,
>    Christian
>
> --
> Dr. Christian Storm
> Siemens AG, Technology, T CED SES-DE
> Otto-Hahn-Ring 6, 81739 München, Germany
>
> --
> You received this message because you are subscribed to the Google Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20220721095357.kz4nyxqdedikpjuk%40cosmos.fritz.box.
Stefano Babic July 23, 2022, 9:42 a.m. UTC | #7
Hi James,

On 21.07.22 11:09, James Hilliard wrote:
> On Thu, Jul 21, 2022 at 2:52 AM Christian Storm
> <christian.storm@siemens.com> wrote:
>>
>> Hi,
>>
>>>> If one wants to make use of the same lua script for different
>>>> boards/revisions one needs a way to fetch that information from
>>>> within the lua script.
>>>>
>>>> This adds the following functions:
>>>> swupdate.get_hw_boardname()
>>>> swupdate.get_hw_revision()
>>>>
>>
>> ... and don't forget to add it to the new Lua Handler interface specification
>> in handlers/swupdate.lua ;)
> 
> In here?:
> https://github.com/sbabic/swupdate/tree/master/handlers
> 
> I'm not seeing that file.
> 
>>
>>
>>> Ok - but what about to implement a more "Lua" function ? Lua can return
>>> multiple value, and we always need both boardname and revision, so we could
>>> have just pushing both on the Lua's stack:
>>>
>>>        boardname, revision = swupdate.get_hw()
>>
>> Well, I'd opt for returning a Table, then you can pack in more information
>> later on without changing the signature (think of, e.g., DMI data or
>> something else). You can wrap it like
>>
>>    boardname, revision = table.unpack(swupdate.get_hw())
>>
>> to get Stefano's proposal. Or you may use a "dict-like" Table with
>> proper key/value pairs but then the table.unpack() doesn't work...
> 
> Um, is there a better way than this to say lookup a key?:
> 
> function type_str(root)
>    for k, v in pairs(swupdate.ROOT_DEVICE) do
>      if v == root.type then return k end
>    end
>    return nil
> end
> 
> function root_type()
>    local root = swupdate.getroot()

     local type = root.type
     local value = root.value

Best regards,
Stefano Babic

>    local type = type_str(root)
>    local value = root.value
>    return value, type
> end
> 
> This API seems not ideal(or maybe I'm missing something obvious here).
> 
>>
>>
>>
>> Kind regards,
>>     Christian
>>
>> --
>> Dr. Christian Storm
>> Siemens AG, Technology, T CED SES-DE
>> Otto-Hahn-Ring 6, 81739 München, Germany
>>
>> --
>> You received this message because you are subscribed to the Google Groups "swupdate" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20220721085242.egwhxwazjv6cqnuc%40cosmos.fritz.box.
>
James Hilliard July 23, 2022, 9:49 a.m. UTC | #8
On Sat, Jul 23, 2022 at 3:42 AM Stefano Babic <sbabic@denx.de> wrote:
>
> Hi James,
>
> On 21.07.22 11:09, James Hilliard wrote:
> > On Thu, Jul 21, 2022 at 2:52 AM Christian Storm
> > <christian.storm@siemens.com> wrote:
> >>
> >> Hi,
> >>
> >>>> If one wants to make use of the same lua script for different
> >>>> boards/revisions one needs a way to fetch that information from
> >>>> within the lua script.
> >>>>
> >>>> This adds the following functions:
> >>>> swupdate.get_hw_boardname()
> >>>> swupdate.get_hw_revision()
> >>>>
> >>
> >> ... and don't forget to add it to the new Lua Handler interface specification
> >> in handlers/swupdate.lua ;)
> >
> > In here?:
> > https://github.com/sbabic/swupdate/tree/master/handlers
> >
> > I'm not seeing that file.
> >
> >>
> >>
> >>> Ok - but what about to implement a more "Lua" function ? Lua can return
> >>> multiple value, and we always need both boardname and revision, so we could
> >>> have just pushing both on the Lua's stack:
> >>>
> >>>        boardname, revision = swupdate.get_hw()
> >>
> >> Well, I'd opt for returning a Table, then you can pack in more information
> >> later on without changing the signature (think of, e.g., DMI data or
> >> something else). You can wrap it like
> >>
> >>    boardname, revision = table.unpack(swupdate.get_hw())
> >>
> >> to get Stefano's proposal. Or you may use a "dict-like" Table with
> >> proper key/value pairs but then the table.unpack() doesn't work...
> >
> > Um, is there a better way than this to say lookup a key?:
> >
> > function type_str(root)
> >    for k, v in pairs(swupdate.ROOT_DEVICE) do
> >      if v == root.type then return k end
> >    end
> >    return nil
> > end
> >
> > function root_type()
> >    local root = swupdate.getroot()
>
>      local type = root.type

I think that just returns the type integer value rather than the string/enum.

>      local value = root.value
>
> Best regards,
> Stefano Babic
>
> >    local type = type_str(root)
> >    local value = root.value
> >    return value, type
> > end
> >
> > This API seems not ideal(or maybe I'm missing something obvious here).
> >
> >>
> >>
> >>
> >> Kind regards,
> >>     Christian
> >>
> >> --
> >> Dr. Christian Storm
> >> Siemens AG, Technology, T CED SES-DE
> >> Otto-Hahn-Ring 6, 81739 München, Germany
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "swupdate" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20220721085242.egwhxwazjv6cqnuc%40cosmos.fritz.box.
> >
Storm, Christian July 25, 2022, 11:02 a.m. UTC | #9
Hi,

> > > > > Ok - but what about to implement a more "Lua" function ? Lua can return
> > > > > multiple value, and we always need both boardname and revision, so we could
> > > > > have just pushing both on the Lua's stack:
> > > > >
> > > > >       boardname, revision = swupdate.get_hw()
> > > >
> > > > Well, I'd opt for returning a Table, then you can pack in more information
> > > > later on without changing the signature (think of, e.g., DMI data or
> > > > something else). You can wrap it like
> > > >
> > > >   boardname, revision = table.unpack(swupdate.get_hw())
> > > >
> > > > to get Stefano's proposal. Or you may use a "dict-like" Table with
> > > > proper key/value pairs but then the table.unpack() doesn't work...
> > >
> > > Um, is there a better way than this to say lookup a key?:
> > >
> > > function type_str(root)
> > >   for k, v in pairs(swupdate.ROOT_DEVICE) do
> > >     if v == root.type then return k end
> > >   end
> > >   return nil
> > > end
> > >
> > > function root_type()
> > >   local root = swupdate.getroot()
> >
> > This gives you a Table with these fields (copied from the interface spec):
> >
> > --- @field type   number  Root device type, one of `swupdate.ROOT_DEVICE`'s values
> > --- @field value  string  Root device path
> > --- @field path   string  Full root device path, if identified, else nil
> >
> > >   local type = type_str(root)
> >
> > Currently, it's this (again copied from the interface spec):
> >
> > --- @class swupdate.ROOT_DEVICE
> > --- Lua equivalent of `root_dev_type` as in `include/lua_util.h`.
> > --- @type  table<string, number>
> > swupdate.ROOT_DEVICE = {
> >     PATH      = 0,
> >     UUID      = 1,
> >     PARTUUID  = 2,
> >     PARTLABEL = 3
> > }
> >
> > To cover your use case, one would add a reverse lookup to swupdate.ROOT_DEVICE,
> > so that swupdate.ROOT_DEVICE[root.type] gives you the string value.
> > Now, either you can monkey-patch swupdate.ROOT_DEVICE in your handler or
> > you can extend corelib/lua_interface.c to provide this reverse lookup.
> 
> I mean my type_string local function does work, but it's just not a
> great interface IMO.

Yes, I agree.

> For something like fetching revision/boardname I'm not really seeing the
> advantage to using a table as it just seems to introduce excessive complexity.

Well, Tables are a "basic" data type in Lua and cheap to use. You want
to return two values (as of now), as in your example
  boardname, revision = swupdate.get_hw()

Now, you can (1) either place those in a Table and return this table to
transparently add more values in the future or you can (2) return more
(direct) values from the function. In the end, it's a matter of taste.

However, IMO, the latter quickly becomes tedious if the number of
possibly returned values ever increases as they're position-dependent.
It's common practice to "void" non-used (return) values by a `_` which 
then may become 
  boardname, _, revision, _, cpu = swupdate.get_hw()
or something alike if swupdate.get_hw() ever gets extended.
But again, it's a matter of taste...


> > In essence something along the lines I have done here:
> > https://gitlab.com/cip-project/cip-sw-updates/swupdate-handler-roundrobin/-/blob/master/swupdate_handlers_roundrobin.lua#L56
> 
> Hmm, so I'm not actually using a lua handler feature AFAIU.
> 
> I'm just using a lua script like this:
> https://sbabic.github.io/swupdate/sw-description.html#lua
> 
> What I'm trying to do is make it so that I can have a single lua script with
> some conditional boardname/revision branching logic for properly handling
> a few minor differences between boards/revisions.
> 
> I'm not really trying to implement a full separate handler.

Well, this is a general technique in Lua to have Tables with reverse
lookup and is not specific to a SWUpdate Handler.



Kind regards,
   Christian
Stefano Babic July 25, 2022, 11:14 a.m. UTC | #10
Hi Christian, James,

On 25.07.22 13:02, Christian Storm wrote:
> Hi,
> 
>>>>>> Ok - but what about to implement a more "Lua" function ? Lua can return
>>>>>> multiple value, and we always need both boardname and revision, so we could
>>>>>> have just pushing both on the Lua's stack:
>>>>>>
>>>>>>        boardname, revision = swupdate.get_hw()
>>>>>
>>>>> Well, I'd opt for returning a Table, then you can pack in more information
>>>>> later on without changing the signature (think of, e.g., DMI data or
>>>>> something else). You can wrap it like
>>>>>
>>>>>    boardname, revision = table.unpack(swupdate.get_hw())
>>>>>
>>>>> to get Stefano's proposal. Or you may use a "dict-like" Table with
>>>>> proper key/value pairs but then the table.unpack() doesn't work...
>>>>
>>>> Um, is there a better way than this to say lookup a key?:
>>>>
>>>> function type_str(root)
>>>>    for k, v in pairs(swupdate.ROOT_DEVICE) do
>>>>      if v == root.type then return k end
>>>>    end
>>>>    return nil
>>>> end
>>>>
>>>> function root_type()
>>>>    local root = swupdate.getroot()
>>>
>>> This gives you a Table with these fields (copied from the interface spec):
>>>
>>> --- @field type   number  Root device type, one of `swupdate.ROOT_DEVICE`'s values
>>> --- @field value  string  Root device path
>>> --- @field path   string  Full root device path, if identified, else nil
>>>
>>>>    local type = type_str(root)
>>>
>>> Currently, it's this (again copied from the interface spec):
>>>
>>> --- @class swupdate.ROOT_DEVICE
>>> --- Lua equivalent of `root_dev_type` as in `include/lua_util.h`.
>>> --- @type  table<string, number>
>>> swupdate.ROOT_DEVICE = {
>>>      PATH      = 0,
>>>      UUID      = 1,
>>>      PARTUUID  = 2,
>>>      PARTLABEL = 3
>>> }
>>>
>>> To cover your use case, one would add a reverse lookup to swupdate.ROOT_DEVICE,
>>> so that swupdate.ROOT_DEVICE[root.type] gives you the string value.
>>> Now, either you can monkey-patch swupdate.ROOT_DEVICE in your handler or
>>> you can extend corelib/lua_interface.c to provide this reverse lookup.
>>
>> I mean my type_string local function does work, but it's just not a
>> great interface IMO.
> 
> Yes, I agree.
> 
>> For something like fetching revision/boardname I'm not really seeing the
>> advantage to using a table as it just seems to introduce excessive complexity.
> 
> Well, Tables are a "basic" data type in Lua and cheap to use. You want
> to return two values (as of now), as in your example
>    boardname, revision = swupdate.get_hw()
> 
> Now, you can (1) either place those in a Table and return this table to
> transparently add more values in the future or you can (2) return more
> (direct) values from the function. In the end, it's a matter of taste.
> 
> However, IMO, the latter quickly becomes tedious if the number of
> possibly returned values ever increases as they're position-dependent.

That is correct. My point to return both (independently how, it is fine 
in atable of course) is that they are bound together. If SWUpdate have 
to check for hardware-software compatibility, both board name and 
revision are important. If for a project, the revision does not care, 
the value can be ignored. If it is in a table, this is straightforward 
and the user just accesses via keys to the needed data.

> It's common practice to "void" non-used (return) values by a `_` which
> then may become
>    boardname, _, revision, _, cpu = swupdate.get_hw()
> or something alike if swupdate.get_hw() ever gets extended.
> But again, it's a matter of taste...
> 
> 
>>> In essence something along the lines I have done here:
>>> https://gitlab.com/cip-project/cip-sw-updates/swupdate-handler-roundrobin/-/blob/master/swupdate_handlers_roundrobin.lua#L56
>>
>> Hmm, so I'm not actually using a lua handler feature AFAIU.
>>
>> I'm just using a lua script like this:
>> https://sbabic.github.io/swupdate/sw-description.html#lua
>>
>> What I'm trying to do is make it so that I can have a single lua script with
>> some conditional boardname/revision branching logic for properly handling
>> a few minor differences between boards/revisions.
>>
>> I'm not really trying to implement a full separate handler.
> 
> Well, this is a general technique in Lua to have Tables with reverse
> lookup and is not specific to a SWUpdate Handler.

Regards,
Stefano

> 
> 
> 
> Kind regards,
>     Christian
>
James Hilliard July 26, 2022, 6:02 p.m. UTC | #11
On Mon, Jul 25, 2022 at 5:02 AM Christian Storm
<christian.storm@siemens.com> wrote:
>
> Hi,
>
> > > > > > Ok - but what about to implement a more "Lua" function ? Lua can return
> > > > > > multiple value, and we always need both boardname and revision, so we could
> > > > > > have just pushing both on the Lua's stack:
> > > > > >
> > > > > >       boardname, revision = swupdate.get_hw()
> > > > >
> > > > > Well, I'd opt for returning a Table, then you can pack in more information
> > > > > later on without changing the signature (think of, e.g., DMI data or
> > > > > something else). You can wrap it like
> > > > >
> > > > >   boardname, revision = table.unpack(swupdate.get_hw())
> > > > >
> > > > > to get Stefano's proposal. Or you may use a "dict-like" Table with
> > > > > proper key/value pairs but then the table.unpack() doesn't work...
> > > >
> > > > Um, is there a better way than this to say lookup a key?:
> > > >
> > > > function type_str(root)
> > > >   for k, v in pairs(swupdate.ROOT_DEVICE) do
> > > >     if v == root.type then return k end
> > > >   end
> > > >   return nil
> > > > end
> > > >
> > > > function root_type()
> > > >   local root = swupdate.getroot()
> > >
> > > This gives you a Table with these fields (copied from the interface spec):
> > >
> > > --- @field type   number  Root device type, one of `swupdate.ROOT_DEVICE`'s values
> > > --- @field value  string  Root device path
> > > --- @field path   string  Full root device path, if identified, else nil
> > >
> > > >   local type = type_str(root)
> > >
> > > Currently, it's this (again copied from the interface spec):
> > >
> > > --- @class swupdate.ROOT_DEVICE
> > > --- Lua equivalent of `root_dev_type` as in `include/lua_util.h`.
> > > --- @type  table<string, number>
> > > swupdate.ROOT_DEVICE = {
> > >     PATH      = 0,
> > >     UUID      = 1,
> > >     PARTUUID  = 2,
> > >     PARTLABEL = 3
> > > }
> > >
> > > To cover your use case, one would add a reverse lookup to swupdate.ROOT_DEVICE,
> > > so that swupdate.ROOT_DEVICE[root.type] gives you the string value.
> > > Now, either you can monkey-patch swupdate.ROOT_DEVICE in your handler or
> > > you can extend corelib/lua_interface.c to provide this reverse lookup.
> >
> > I mean my type_string local function does work, but it's just not a
> > great interface IMO.
>
> Yes, I agree.
>
> > For something like fetching revision/boardname I'm not really seeing the
> > advantage to using a table as it just seems to introduce excessive complexity.
>
> Well, Tables are a "basic" data type in Lua and cheap to use. You want
> to return two values (as of now), as in your example
>   boardname, revision = swupdate.get_hw()

Well I'm not actually using both values in the same place hence why
my patch so far fetched them independently:
swupdate.get_hw_boardname()
swupdate.get_hw_revision()

Fetching them at the same time is probably slightly faster if one needs
to use both but this isn't really a hot-path so I figured an extra
query wouldn't
really be an issue.

>
> Now, you can (1) either place those in a Table and return this table to
> transparently add more values in the future or you can (2) return more
> (direct) values from the function. In the end, it's a matter of taste.
>
> However, IMO, the latter quickly becomes tedious if the number of
> possibly returned values ever increases as they're position-dependent.
> It's common practice to "void" non-used (return) values by a `_` which
> then may become
>   boardname, _, revision, _, cpu = swupdate.get_hw()
> or something alike if swupdate.get_hw() ever gets extended.
> But again, it's a matter of taste...

Yeah, that's why I avoided multiple returns and separated them like:
swupdate.get_hw_boardname()
swupdate.get_hw_revision()

Which is trivially extendable via adding a new function like:
swupdate.get_hw_cpu()

Going to experiment more with the table option.

>
>
> > > In essence something along the lines I have done here:
> > > https://gitlab.com/cip-project/cip-sw-updates/swupdate-handler-roundrobin/-/blob/master/swupdate_handlers_roundrobin.lua#L56
> >
> > Hmm, so I'm not actually using a lua handler feature AFAIU.
> >
> > I'm just using a lua script like this:
> > https://sbabic.github.io/swupdate/sw-description.html#lua
> >
> > What I'm trying to do is make it so that I can have a single lua script with
> > some conditional boardname/revision branching logic for properly handling
> > a few minor differences between boards/revisions.
> >
> > I'm not really trying to implement a full separate handler.
>
> Well, this is a general technique in Lua to have Tables with reverse
> lookup and is not specific to a SWUpdate Handler.

It just seems to make things more complex for this particular use case IMO.

I don't normally use lua in general so I'm not really familiar with
typical conventions.

>
>
>
> Kind regards,
>    Christian
>
> --
> Dr. Christian Storm
> Siemens AG, Technology, T CED SES-DE
> Otto-Hahn-Ring 6, 81739 München, Germany
>
> --
> You received this message because you are subscribed to the Google Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20220725110233.epbntoud2p4j7uad%40MD1ZFJVC.ad001.siemens.net.
diff mbox series

Patch

diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
index dd1be9e..dd962cf 100644
--- a/corelib/lua_interface.c
+++ b/corelib/lua_interface.c
@@ -923,6 +923,36 @@  static int l_get_selection(lua_State *L) {
 	return 2;
 }
 
+static int l_get_hw_boardname(lua_State *L)
+{
+	struct swupdate_cfg *cfg = get_swupdate_cfg();
+
+	if (get_hw_revision(&cfg->hw) < 0)
+		goto l_get_hw_boardname_exit;
+
+	lua_pushstring(L, cfg->hw.boardname);
+	return 1;
+
+l_get_hw_boardname_exit:
+	lua_pushnil(L);
+	return 1;
+}
+
+static int l_get_hw_revision(lua_State *L)
+{
+	struct swupdate_cfg *cfg = get_swupdate_cfg();
+
+	if (get_hw_revision(&cfg->hw) < 0)
+		goto l_get_hw_revision_exit;
+
+	lua_pushstring(L, cfg->hw.revision);
+	return 1;
+
+l_get_hw_revision_exit:
+	lua_pushnil(L);
+	return 1;
+}
+
 
 #ifdef CONFIG_HANDLER_IN_LUA
 static int l_get_tmpdir(lua_State *L)
@@ -1003,6 +1033,8 @@  static const luaL_Reg l_swupdate[] = {
         { "umount", l_umount },
         { "getroot", l_getroot },
         { "getversion", l_getversion },
+        { "get_hw_boardname", l_get_hw_boardname },
+        { "get_hw_revision", l_get_hw_revision },
         { "progress", l_notify_progress },
         { NULL, NULL }
 };