Re: [PATCH rtems-littlevgl] Allow to pass custom lv_conf.h and lv_drv_conf.h.

[Christian Mauderer]

On 09/07/2020 20:52, Vijay Kumar Banerjee wrote:
> 
> 
> On Fri, Jul 10, 2020, 12:11 AM Christian Mauderer  > wrote:
> 
> Hello Vijay,
> 
> thanks for the review and the test.
> 
> On 09/07/2020 19:58, Vijay Kumar Banerjee wrote:
> > Hi,
> >
> > Thanks for the patch, I tested the patch and it's building fine. I
> > just had two questions which I have inlined below.
> >
> > On Thu, Jul 9, 2020 at 9:13 PM Christian Mauderer
> >  > wrote:
> >>
> >> ---
> >>  lv_conf.h => default_lv_conf.h         |  0
> >>  lv_drv_conf.h => default_lv_drv_conf.h |  0
> >>  lvgl.py                                | 19 ---
> >>  wscript                                | 13 +
> >>  4 files changed, 29 insertions(+), 3 deletions(-)
> >>  rename lv_conf.h => default_lv_conf.h (100%)
> >>  rename lv_drv_conf.h => default_lv_drv_conf.h (100%)
> >>
> >> diff --git a/lv_conf.h b/default_lv_conf.h
> >> similarity index 100%
> >> rename from lv_conf.h
> >> rename to default_lv_conf.h
> >> diff --git a/lv_drv_conf.h b/default_lv_drv_conf.h
> >> similarity index 100%
> >> rename from lv_drv_conf.h
> >> rename to default_lv_drv_conf.h
> >> diff --git a/lvgl.py b/lvgl.py
> >> index c154a5e..6d55db7 100644
> >> --- a/lvgl.py
> >> +++ b/lvgl.py
> >> @@ -73,6 +73,17 @@ def build(bld):
> >>      includes.append('.')
> >>      include_paths = []
> >>
> >> +    def write_stuff(stuff):
> >> +        def stuff_writer(task):
> >> +            task.outputs[0].write(stuff)
> >> +        return stuff_writer
> >> +
> >> +    lv_conf_h='lv_conf.h'
> >> +    lv_drv_conf_h='lv_drv_conf.h'
> >> +
> >> +    bld(rule=write_stuff(bld.env.LV_CONF), target=lv_conf_h)
> >> +    bld(rule=write_stuff(bld.env.LV_DRV_CONF), target=lv_drv_conf_h)
> >> +
> >>      for source in sources:
> >>          source_dir = os.path.dirname(source)
> >>          if source_dir not in include_paths:
> >> @@ -80,7 +91,7 @@ def build(bld):
> >>
> >>      bld.stlib(target = 'lvgl',
> >>                features = 'c',
> >> -              cflags = ['-O2', '-g'],
> >> +              cflags = ['-O2', '-g', '-DLV_CONF_INCLUDE_SIMPLE'],
> >>                includes = includes,
> >>                source = sources)
> >>
> >> @@ -96,6 +107,8 @@ def build(bld):
> >>      for include_path in include_paths:
> >>          files = os.listdir(include_path)
> >>          include_headers = [os.path.join(include_path, x) for x
> in files if (x[-2:] == '.h')]
> >> -        bld.install_files(os.path.join("${PREFIX}/" ,
> arch_inc_path, include_path),
> >> +        bld.install_files(os.path.join("${PREFIX}",
> arch_inc_path, include_path),
> >>                            include_headers)
> >> -    bld.install_files('${PREFIX}/' + arch_lib_path, ["liblvgl.a"])
> >> +    bld.install_files(os.path.join('${PREFIX}', arch_lib_path),
> ["liblvgl.a"])
> >> +    bld.install_files(os.path.join('${PREFIX}', arch_inc_path,
> include_path),
> >> +        [lv_conf_h])
> >
> > Should lv_drv_conf_h be installed as well?
> 
> I haven't seen that it is used but I can install it too. I didn't test
> all drivers.
> 
> One use case would be to check if USE_BSD_FBDEV (Or some other driver)
> is defined. The example apps assumed that both evdev and fbdev is
> defined since the conf was fixed. I think they would need some check to
> ensure it is defined.

I think it is OK for the example apps to use the default configuration
used in RTEMS. But it's a good point. I'll add the lv_drv_conf.h to the
installed headers too before I push the commit.

> 
> >
> >> diff --git a/wscript b/wscript
> >> index ae91daa..0e1a51d 100644
> >> --- a/wscript
> >> +++ b/wscript
> >> @@ -49,9 +49,22 @@ def options(opt):
> >>                     dest = "drivers",
> >>                     help = "Build without lv_drivers." +
> >>                            "Useful for building without libbsd.")
> >> +    opt.add_option("--lv-conf",
> >> +                   default = "default_lv_conf.h",
> >> +                   dest = "lv_conf",
> >> +                   help = "Use a custom lv_conf.h instead of the
> default one.")
> >> +    opt.add_option("--lv-drv-conf",
> >> +                   default = "default_lv_drv_conf.h",
> >> +                   dest = "lv_drv_conf",
> >> +                   help = "Use a custom lv_drv_conf.h instead of
> the default one.")
> >>
> >>  def configure(conf):
> >>      conf.env.DRIVERS = conf.options.drivers
> >> +    with open(conf.options.lv_conf, "rb") as lv_conf:
> >> +        conf.env.LV_CONF = lv_conf.read()
> 

Re: [PATCH rtems-littlevgl] Allow to pass custom lv_conf.h and lv_drv_conf.h.

[Vijay Kumar Banerjee]

On Fri, Jul 10, 2020, 12:11 AM Christian Mauderer  wrote:

> Hello Vijay,
>
> thanks for the review and the test.
>
> On 09/07/2020 19:58, Vijay Kumar Banerjee wrote:
> > Hi,
> >
> > Thanks for the patch, I tested the patch and it's building fine. I
> > just had two questions which I have inlined below.
> >
> > On Thu, Jul 9, 2020 at 9:13 PM Christian Mauderer
> >  wrote:
> >>
> >> ---
> >>  lv_conf.h => default_lv_conf.h |  0
> >>  lv_drv_conf.h => default_lv_drv_conf.h |  0
> >>  lvgl.py| 19 ---
> >>  wscript| 13 +
> >>  4 files changed, 29 insertions(+), 3 deletions(-)
> >>  rename lv_conf.h => default_lv_conf.h (100%)
> >>  rename lv_drv_conf.h => default_lv_drv_conf.h (100%)
> >>
> >> diff --git a/lv_conf.h b/default_lv_conf.h
> >> similarity index 100%
> >> rename from lv_conf.h
> >> rename to default_lv_conf.h
> >> diff --git a/lv_drv_conf.h b/default_lv_drv_conf.h
> >> similarity index 100%
> >> rename from lv_drv_conf.h
> >> rename to default_lv_drv_conf.h
> >> diff --git a/lvgl.py b/lvgl.py
> >> index c154a5e..6d55db7 100644
> >> --- a/lvgl.py
> >> +++ b/lvgl.py
> >> @@ -73,6 +73,17 @@ def build(bld):
> >>  includes.append('.')
> >>  include_paths = []
> >>
> >> +def write_stuff(stuff):
> >> +def stuff_writer(task):
> >> +task.outputs[0].write(stuff)
> >> +return stuff_writer
> >> +
> >> +lv_conf_h='lv_conf.h'
> >> +lv_drv_conf_h='lv_drv_conf.h'
> >> +
> >> +bld(rule=write_stuff(bld.env.LV_CONF), target=lv_conf_h)
> >> +bld(rule=write_stuff(bld.env.LV_DRV_CONF), target=lv_drv_conf_h)
> >> +
> >>  for source in sources:
> >>  source_dir = os.path.dirname(source)
> >>  if source_dir not in include_paths:
> >> @@ -80,7 +91,7 @@ def build(bld):
> >>
> >>  bld.stlib(target = 'lvgl',
> >>features = 'c',
> >> -  cflags = ['-O2', '-g'],
> >> +  cflags = ['-O2', '-g', '-DLV_CONF_INCLUDE_SIMPLE'],
> >>includes = includes,
> >>source = sources)
> >>
> >> @@ -96,6 +107,8 @@ def build(bld):
> >>  for include_path in include_paths:
> >>  files = os.listdir(include_path)
> >>  include_headers = [os.path.join(include_path, x) for x in
> files if (x[-2:] == '.h')]
> >> -bld.install_files(os.path.join("${PREFIX}/" , arch_inc_path,
> include_path),
> >> +bld.install_files(os.path.join("${PREFIX}", arch_inc_path,
> include_path),
> >>include_headers)
> >> -bld.install_files('${PREFIX}/' + arch_lib_path, ["liblvgl.a"])
> >> +bld.install_files(os.path.join('${PREFIX}', arch_lib_path),
> ["liblvgl.a"])
> >> +bld.install_files(os.path.join('${PREFIX}', arch_inc_path,
> include_path),
> >> +[lv_conf_h])
> >
> > Should lv_drv_conf_h be installed as well?
>
> I haven't seen that it is used but I can install it too. I didn't test
> all drivers.
>
One use case would be to check if USE_BSD_FBDEV (Or some other driver) is
defined. The example apps assumed that both evdev and fbdev is defined
since the conf was fixed. I think they would need some check to ensure it
is defined.

> >
> >> diff --git a/wscript b/wscript
> >> index ae91daa..0e1a51d 100644
> >> --- a/wscript
> >> +++ b/wscript
> >> @@ -49,9 +49,22 @@ def options(opt):
> >> dest = "drivers",
> >> help = "Build without lv_drivers." +
> >>"Useful for building without libbsd.")
> >> +opt.add_option("--lv-conf",
> >> +   default = "default_lv_conf.h",
> >> +   dest = "lv_conf",
> >> +   help = "Use a custom lv_conf.h instead of the
> default one.")
> >> +opt.add_option("--lv-drv-conf",
> >> +   default = "default_lv_drv_conf.h",
> >> +   dest = "lv_drv_conf",
> >> +   help = "Use a custom lv_drv_conf.h instead of the
> default one.")
> >>
> >>  def configure(conf):
> >>  conf.env.DRIVERS = conf.options.drivers
> >> +with open(conf.options.lv_conf, "rb") as lv_conf:
> >> +conf.env.LV_CONF = lv_conf.read()
> >> +with open(conf.options.lv_drv_conf, "rb") as lv_drv_conf:
> >> +conf.env.LV_DRV_CONF = lv_drv_conf.read()
> >> +conf.env.BUILDINCLUDE = 'build-include'
> >
> > It's not clear to me where is build-include being used. It builds fine
> > without this line, should it be removed from the patch?
> >
>
> That is a part of a dead end during writing the patch. I missed to
> remove it. Thanks for finding it.
>
> Best regards
>
> Christian
>
> >
> > Thanks for working on this feature, it was much needed.
> >
> > Best regards,
> > Vijay
> >
> >>  rtems.configure(conf)
> >>
> >>  def build(bld):
> >> --
> >> 2.26.2
> >>
> > ___
> > devel mailing list
> > devel@rtems.org
> > 

Re: [PATCH rtems-littlevgl] Allow to pass custom lv_conf.h and lv_drv_conf.h.

[Christian Mauderer]

Hello Vijay,

thanks for the review and the test.

On 09/07/2020 19:58, Vijay Kumar Banerjee wrote:
> Hi,
> 
> Thanks for the patch, I tested the patch and it's building fine. I
> just had two questions which I have inlined below.
> 
> On Thu, Jul 9, 2020 at 9:13 PM Christian Mauderer
>  wrote:
>>
>> ---
>>  lv_conf.h => default_lv_conf.h |  0
>>  lv_drv_conf.h => default_lv_drv_conf.h |  0
>>  lvgl.py| 19 ---
>>  wscript| 13 +
>>  4 files changed, 29 insertions(+), 3 deletions(-)
>>  rename lv_conf.h => default_lv_conf.h (100%)
>>  rename lv_drv_conf.h => default_lv_drv_conf.h (100%)
>>
>> diff --git a/lv_conf.h b/default_lv_conf.h
>> similarity index 100%
>> rename from lv_conf.h
>> rename to default_lv_conf.h
>> diff --git a/lv_drv_conf.h b/default_lv_drv_conf.h
>> similarity index 100%
>> rename from lv_drv_conf.h
>> rename to default_lv_drv_conf.h
>> diff --git a/lvgl.py b/lvgl.py
>> index c154a5e..6d55db7 100644
>> --- a/lvgl.py
>> +++ b/lvgl.py
>> @@ -73,6 +73,17 @@ def build(bld):
>>  includes.append('.')
>>  include_paths = []
>>
>> +def write_stuff(stuff):
>> +def stuff_writer(task):
>> +task.outputs[0].write(stuff)
>> +return stuff_writer
>> +
>> +lv_conf_h='lv_conf.h'
>> +lv_drv_conf_h='lv_drv_conf.h'
>> +
>> +bld(rule=write_stuff(bld.env.LV_CONF), target=lv_conf_h)
>> +bld(rule=write_stuff(bld.env.LV_DRV_CONF), target=lv_drv_conf_h)
>> +
>>  for source in sources:
>>  source_dir = os.path.dirname(source)
>>  if source_dir not in include_paths:
>> @@ -80,7 +91,7 @@ def build(bld):
>>
>>  bld.stlib(target = 'lvgl',
>>features = 'c',
>> -  cflags = ['-O2', '-g'],
>> +  cflags = ['-O2', '-g', '-DLV_CONF_INCLUDE_SIMPLE'],
>>includes = includes,
>>source = sources)
>>
>> @@ -96,6 +107,8 @@ def build(bld):
>>  for include_path in include_paths:
>>  files = os.listdir(include_path)
>>  include_headers = [os.path.join(include_path, x) for x in files if 
>> (x[-2:] == '.h')]
>> -bld.install_files(os.path.join("${PREFIX}/" , arch_inc_path, 
>> include_path),
>> +bld.install_files(os.path.join("${PREFIX}", arch_inc_path, 
>> include_path),
>>include_headers)
>> -bld.install_files('${PREFIX}/' + arch_lib_path, ["liblvgl.a"])
>> +bld.install_files(os.path.join('${PREFIX}', arch_lib_path), 
>> ["liblvgl.a"])
>> +bld.install_files(os.path.join('${PREFIX}', arch_inc_path, 
>> include_path),
>> +[lv_conf_h])
> 
> Should lv_drv_conf_h be installed as well?

I haven't seen that it is used but I can install it too. I didn't test
all drivers.

> 
>> diff --git a/wscript b/wscript
>> index ae91daa..0e1a51d 100644
>> --- a/wscript
>> +++ b/wscript
>> @@ -49,9 +49,22 @@ def options(opt):
>> dest = "drivers",
>> help = "Build without lv_drivers." +
>>"Useful for building without libbsd.")
>> +opt.add_option("--lv-conf",
>> +   default = "default_lv_conf.h",
>> +   dest = "lv_conf",
>> +   help = "Use a custom lv_conf.h instead of the default 
>> one.")
>> +opt.add_option("--lv-drv-conf",
>> +   default = "default_lv_drv_conf.h",
>> +   dest = "lv_drv_conf",
>> +   help = "Use a custom lv_drv_conf.h instead of the 
>> default one.")
>>
>>  def configure(conf):
>>  conf.env.DRIVERS = conf.options.drivers
>> +with open(conf.options.lv_conf, "rb") as lv_conf:
>> +conf.env.LV_CONF = lv_conf.read()
>> +with open(conf.options.lv_drv_conf, "rb") as lv_drv_conf:
>> +conf.env.LV_DRV_CONF = lv_drv_conf.read()
>> +conf.env.BUILDINCLUDE = 'build-include'
> 
> It's not clear to me where is build-include being used. It builds fine
> without this line, should it be removed from the patch?
> 

That is a part of a dead end during writing the patch. I missed to
remove it. Thanks for finding it.

Best regards

Christian

> 
> Thanks for working on this feature, it was much needed.
> 
> Best regards,
> Vijay
> 
>>  rtems.configure(conf)
>>
>>  def build(bld):
>> --
>> 2.26.2
>>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
> 
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH rtems-littlevgl] Allow to pass custom lv_conf.h and lv_drv_conf.h.

[Vijay Kumar Banerjee]

Hi,

Thanks for the patch, I tested the patch and it's building fine. I
just had two questions which I have inlined below.

On Thu, Jul 9, 2020 at 9:13 PM Christian Mauderer
 wrote:
>
> ---
>  lv_conf.h => default_lv_conf.h |  0
>  lv_drv_conf.h => default_lv_drv_conf.h |  0
>  lvgl.py| 19 ---
>  wscript| 13 +
>  4 files changed, 29 insertions(+), 3 deletions(-)
>  rename lv_conf.h => default_lv_conf.h (100%)
>  rename lv_drv_conf.h => default_lv_drv_conf.h (100%)
>
> diff --git a/lv_conf.h b/default_lv_conf.h
> similarity index 100%
> rename from lv_conf.h
> rename to default_lv_conf.h
> diff --git a/lv_drv_conf.h b/default_lv_drv_conf.h
> similarity index 100%
> rename from lv_drv_conf.h
> rename to default_lv_drv_conf.h
> diff --git a/lvgl.py b/lvgl.py
> index c154a5e..6d55db7 100644
> --- a/lvgl.py
> +++ b/lvgl.py
> @@ -73,6 +73,17 @@ def build(bld):
>  includes.append('.')
>  include_paths = []
>
> +def write_stuff(stuff):
> +def stuff_writer(task):
> +task.outputs[0].write(stuff)
> +return stuff_writer
> +
> +lv_conf_h='lv_conf.h'
> +lv_drv_conf_h='lv_drv_conf.h'
> +
> +bld(rule=write_stuff(bld.env.LV_CONF), target=lv_conf_h)
> +bld(rule=write_stuff(bld.env.LV_DRV_CONF), target=lv_drv_conf_h)
> +
>  for source in sources:
>  source_dir = os.path.dirname(source)
>  if source_dir not in include_paths:
> @@ -80,7 +91,7 @@ def build(bld):
>
>  bld.stlib(target = 'lvgl',
>features = 'c',
> -  cflags = ['-O2', '-g'],
> +  cflags = ['-O2', '-g', '-DLV_CONF_INCLUDE_SIMPLE'],
>includes = includes,
>source = sources)
>
> @@ -96,6 +107,8 @@ def build(bld):
>  for include_path in include_paths:
>  files = os.listdir(include_path)
>  include_headers = [os.path.join(include_path, x) for x in files if 
> (x[-2:] == '.h')]
> -bld.install_files(os.path.join("${PREFIX}/" , arch_inc_path, 
> include_path),
> +bld.install_files(os.path.join("${PREFIX}", arch_inc_path, 
> include_path),
>include_headers)
> -bld.install_files('${PREFIX}/' + arch_lib_path, ["liblvgl.a"])
> +bld.install_files(os.path.join('${PREFIX}', arch_lib_path), 
> ["liblvgl.a"])
> +bld.install_files(os.path.join('${PREFIX}', arch_inc_path, include_path),
> +[lv_conf_h])

Should lv_drv_conf_h be installed as well?

> diff --git a/wscript b/wscript
> index ae91daa..0e1a51d 100644
> --- a/wscript
> +++ b/wscript
> @@ -49,9 +49,22 @@ def options(opt):
> dest = "drivers",
> help = "Build without lv_drivers." +
>"Useful for building without libbsd.")
> +opt.add_option("--lv-conf",
> +   default = "default_lv_conf.h",
> +   dest = "lv_conf",
> +   help = "Use a custom lv_conf.h instead of the default 
> one.")
> +opt.add_option("--lv-drv-conf",
> +   default = "default_lv_drv_conf.h",
> +   dest = "lv_drv_conf",
> +   help = "Use a custom lv_drv_conf.h instead of the default 
> one.")
>
>  def configure(conf):
>  conf.env.DRIVERS = conf.options.drivers
> +with open(conf.options.lv_conf, "rb") as lv_conf:
> +conf.env.LV_CONF = lv_conf.read()
> +with open(conf.options.lv_drv_conf, "rb") as lv_drv_conf:
> +conf.env.LV_DRV_CONF = lv_drv_conf.read()
> +conf.env.BUILDINCLUDE = 'build-include'

It's not clear to me where is build-include being used. It builds fine
without this line, should it be removed from the patch?


Thanks for working on this feature, it was much needed.

Best regards,
Vijay

>  rtems.configure(conf)
>
>  def build(bld):
> --
> 2.26.2
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel