[RFC]Template prototype with updated edts

[evenl]

Goal

To propose the jinja2 template engine as an alternative to the COG
based solution proposed for code generation.

Rationale

The version of COG proposed to be used in Zephyr is a highly modified
version of the original COG implementation. Which means that we can
not leverage future improvements made to COG or contribute our own
modifications back to COG.

This commit evenl@d8c986c
shows what code can be removed by using a jinja instead of a COG base solution

The Jinja2 engine however can be used as a pip package dependency
without modifications.

COG is more or less straight forward python which means that a generator
must more or less be written for each case. Which means that even the
simplest things will need a lot of boiler code just to output something.

Example:

Data in is “dev_name”

Cog:

/**
 * @code{.codegen}
 *
 *     codegen.outl(“#ifndef {}_INCLUDE_H”.format(dev_name))
 *     codegen.outl(“#define {}_INCLUDE_H”.format(dev_name))
 *
 * @endcode{.codegen}
 */

Jinja2:

    #ifndef {{ dev_name }}_INCLUDE_H
    #define {{ dev_name }}_INCLUDE_H

TODO:

The Jinja2 implementation is on par with what is currently implemented using the COG
solution. But because the COG solution give the user the ability to directly write
python code directly in the template there may be a need to develop some custom filters/
helper functions for the jinja2 solution.

This code is based on this code https://github.com/galak/zephyr/tree/EDTS which is not
yet in master.

Other advantages:

    * Big active community
    * Standardized and documented (http://jinja.pocoo.org)
    * Has been in development for a long time with multiple stable releases
    * Extensive enough to be a one-stop-shop for all code generation in the Zephyr project

[codecov-io]

Codecov Report

Merging #10873 into topic-EDTS will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           topic-EDTS   #10873   +/-   ##
===========================================
  Coverage       53.27%   53.27%           
===========================================
  Files             215      215           
  Lines           25851    25851           
  Branches         5695     5695           
===========================================
  Hits            13773    13773           
  Misses           9762     9762           
  Partials         2316     2316

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb4269e...274ba16. Read the comment docs.

[carlescufi]

I assume we cannot have this file as part of i2c_ll_stm32_device_template.h and rename it to i2c_ll_stm32_device_template.h.jinja2?

I wouldn't mind having an .h.jinja2 that includes all the templating instead of having it as 2 files.

[erwango]

@carlescufi , if we're adding the .jinja2 extension, do we really need the "_template" suffix?
I think it'll be more user friendly if we can contain a bit the complete file name..

[evenl]

Because how jinja2 technically works cannot a template that inherit another template (by using the extends keyword) be rendered directly, but must be imported by another template. But I kind of think that separating out the metadata is cleaner anyway.

This is also related to how I decided to solve this particular use-case, it can be solved in many ways. I could for instance have made a similar solution to how the COG version do it by using the built-in replace filter, but that would resulted in generator within the generator. I could also have exposed python functions through a data object, and that is maybe something we should look into in the long term, but for now I wanted to show a less complex jinja2 only solution.

The naming scheme is maybe a bad adaptation of your request to avoid the .jinja2 extension. I needed a different name on the file than the outputted file so that's why I introduced the _template postfix. The outputted file is called i2c_ll_stm32_device.h

[erwango]

Some initial comments based on codegen experience.
Besides, since I'm not aware of .jinja2 way of working yet, some comments are more like a first time user feedback (which is more difficult to me to provide on codegen).

As discussed @ELC, this would benefit a to have a kind of high level API, to remove some "criptic" bits,
but I don't know if this is feasible with jinja2.

This is a good start at least.

[erwango]

Can you provide a more easy to use macro?
Ideally one should only call zephyr_library_sources_jinja2_ifdef(CONFIG_SPI_STM32 spi_ll_stm32.c)
Then jinja2 should generate spi_ll_stm32_devices.c/h from spi_ll_stm32_devices.c.jinja

[evenl]

well, you can omit CODEGEN_OUTPUT and SEARCH_PATH if you don't need them

CODEGEN_OUTPUT is to control the name of the rendered file which can be nice in many cases

SEARCH_PATH is set "include_paths", so if you have templates in other folders that you want to reuse in your template (with the include/import or extends keywords)

[erwango]

Do you still need to have jinja2 extension on this file?

[erwango]

I think i2c_ll_stm32_devices.h would make more sense.

Then, should we include .c or .h file?
I'm not sure to have any preference, but question should be asked.
@carlescufi ? @galak ?
Any preference?

[galak]

I go back and forth about this, but maybe .c, and adding a comment about so something like:

/* Include generated code */
#include "i2c_ll_stm32_device.c"

[evenl]

I kind of don't like either and would prefer to render the content directly into the driver file instead.

Like I did here https://github.com/evenl/zephyr/blob/843cfad25ea6c1c6347b77365f8e4d5e171c868d/drivers/spi/spi_ll_stm32.c.jinja2

But I know that I am probably in minority here ;)

[carlescufi]

@evenl
After thinking about this and discussing it with people, I really dislike the templated C file. I would like my .c file to be a .c file, clean of templating and containing only .c code.

[evenl]

Ok, I am anyway generating .h file as you requested in the first place.

[erwango]

You should be able to get rid of this block.
IRQ code generation sequence should be able to detect if nodes has irqs and then build ad-hoc function based on irq data extracted from EDTS.
Only input would be isr function prefix: stm32_i2c_isr.
Eventually, prefix might be replaced by "_isr"

[evenl]

Yes, sure, but I wanted to make it possible for the developer to decide what the ISR functions was called and not auto-generate because the template don't have control on the ISR generation.

[erwango]

We'll.. Since code generation concept is new in Zephyr, we're trying to get first to a most easy to use and constrained version, which should be used as is by majority of developers.

This should help in adoption by developers and users and avoid having heterogeneous versions of code fragmentation in various zephyr drivers. Once merged, I think we don't want to spend time in reviews arguing with developers that they should not do this and this in the code generation.

This is why a constrained version which support vast majority of cases but limits the individual freedom of each developer in this area is required, from my point of view, at least in its first iteration.

[evenl]

Ok, I have removed the irq_config_function_body block and the IRQ_CONNECT and irq_enable lines are now auto generated as in the codegen version.

[erwango]

For reader, it would be clearer to keep using CONFIG_I2C_STM32_INTERRUPT here.
Since we're templating the config_struct, only missing args should be the ones from EDTS.

[evenl]

I don't like to have "multiple truths".

If this file was copied and used as a template for different metadata file, or extended/changed, the developer only need to set the irq_flag one place.

[erwango]

Agreed, but this makes difficult to read and it is not strictly required.

In the end, solution might be to remove this element the structure and add an element in DEVICE_AND_API_INIT macro.

[evenl]

Ok, just because you asked for it, I keep my eyes shut when I push enter after writing 'git commit':D

[erwango]

Would you be able to use recently introduced get_reg functions ?
@galak
What about doing the formatting inside get_reg directly ?

[galak]

I agreed, can we use the edtsdevice get_reg() api.

@erwango hmm, I want to keep the api's as actual integers. Wonder if {{ hex(device['reg']['0']['address']) }} works

[evenl]

yes, I can make any python API available as a data object in the template or custom filters.

@galak If you see in template code, I use the format filter to turn an integer into a hex value no need to use an external function

[erwango]

@evenl , aim of my comment was to find a way provide a more simple syntax instead of

{{ "%x"|format(device['reg']['0']['address']) }}

@galak: what about get_hex_reg()?

[galak]

@erwango I was hoping to not add such APIs, and thus wondered if the following would work:
{{ hex(device['reg']['0']['address']) }}

[erwango]

@carlescufi , if we're adding the .jinja2 extension, do we really need the "_template" suffix?
I think it'll be more user friendly if we can contain a bit the complete file name..

[erwango]

It is not clear where "irq_config_function_name" is coming from.
For "data_struct_name", since we are populating a "data_struct", I can guess, even if maybe a data_struct.name would feel more "constructed" and so guessable.

But I don't see a "irq_config_function" function anywhere.

[evenl]

Check the comment on line 14, the block is "scoped" and inherit all variables in parent macro (in device_declare.jinja2). The irq_config_function_name is created in the original device macro in the parent file.

[galak]

Is there some other way to do this? Can we use a macro that we can pass these values to?

[evenl]

I would think so, but I will have to test it before I can definitely say it works. I can test it and update the pull-request if it works.

[evenl]

Seems like I still need the block to ensure the order of when the template is rendered, but I now have version with a set/get pair.

[carlescufi]

This is an .h file that should really be an .h.jinja2 file. See my comment here

[evenl]

I probably misunderstood you, but I though you wanted me to have the .h so that the IDE's would recognize it as .h file, but that was maybe for when I rendered the whole driver file and not this file that is included in the driver file?

[galak]

We need to refactor out the changes that we import from codegen. I'd like to see this PR not have codegen references. For example can we remove codegen.rst & template_flow.png files. Do we need options.py?

[b0661]

@evenl

Would be nice if you could use a different name (e.g. 'codejinja'/'gen_jinja' instead of codegen). By this the code generation could be used in parallel and my OOT work does not break.

I evaluated Jinja when I started codegen. I like it and use it in my project.

Why I did not choose Jinja:

• There was a general request to only have pure Python for code generation. Even complex C macros were rejected because they are too complex for a driver developer. Ninja templates require to learn another (simple) scripting 'language' addionally to Python.
• A driver writer that needs data that is not already precomputed has to extend Ninja by some form of Python function that may e.g. be called by find_undeclared_variables(). To do this some infrastructure where the driver can add the data generation has to be added. In this case the driver will have a Python part and a C part. Having this all in one file and getting it automatically executed looked less complex to me than having to teach a driver writer about how to do it with Jinja.

What is the real problem with code/ data generation:

• There are so many potential users and so many different opinions that probably a whole set of generation means will not please everybody.

COG is more or less straight forward python which means that a generator must more or less be written for each case.

This is nonsense. Codegen (not COG) does have the device_declare module that you can use for most of the drivers. It replaces placeholders in a Python string (using the Python string template engine) by values that are precomputed from the device tree, CMake variables, etc.. There are template header files that can just be include into the C source - that is how the pin controller driver is made.

Please don´t call it COG. COG is Ned´s tool. Codegen is something different.

The Jinja2 implementation is on par with what is currently implemented using the COG
solution. But because the COG solution give the user the ability to directly write python code directly in the template there may be a need to develop some custom filters/helper functions for the jinja2 solution.

Because codegen already has the "custom filters/helper functions" the Jinja solution is not on par. For simple placeholder replacements you can really use any template engine. The hard part is to get the "custom filters/helper functions" and the template syntax in a way that is acceptable to driver writers and other users. Also the implementation of complex drivers, like the pin controller driver, is another lackmus test.

I assume that the "custom filters/helper functions" will evolve to something similar to codegen with other means. Which will reduce the benefit of Jinja to some extend.

The version of COG proposed to be used in Zephyr is a highly modified version ... Which means that we can not leverage future improvements made to COG or contribute our own modifications back to COG.

COG has a different target and a very stable code base. Codegen used it as a starting point to avoid re-inventing the wheel. There was no intention to improve COG due to its different target.

The Jinja2 engine however can be used as a pip package dependency without modifications.

Yes and no. Jinja can be used as is but the driver infrastructure to use Jinja still has to be in Zephyr. This also will most probably never be upstreamed as it is in this case not the target of Jinja.

[evenl]

Just to make sure we are on the same page, I assume a data driven code generation scheme for zephyr. Based on the fact that there is already a lot of metadata available that can be used for code generation. So my choices in this proposal are based on that assumption.

Would be nice if you could use a different name (e.g. 'codejinja'/'gen_jinja' instead of codegen). By
this the code generation could be used in parallel and my OOT work does not break.

I though the word "codegen" was so ambiguous so I though It was represented the feature and not the implementation, but for sure, I can rename the files. I am not sure how much sense it makes to use multiple code generators though.

I evaluated Jinja when I started codegen. I like it and use it in my project.

Ok, I was not aware of this, I am fairly new to the Zephyr project, and there was already so many comments in the codegen related pull requests that I probably have missed many comments and discussions.

There was a general request to only have pure Python for code generation. Even complex C macros
were rejected because they are too complex for a driver developer. Ninja templates require to learn
another (simple) scripting 'language' addionally to Python.

This argument just does not make sense at all when there are multiple perfectly good engines out there for generating content. It is important always pick the right tool for the right job. But of course, Python has it's place in the zephyr project, but not for generating code directly.

A driver writer that needs data that is not already precomputed has to extend Ninja by some form of
Python function that may e.g. be called by find_undeclared_variables(). To do this some
infrastructure where the driver can add the data generation has to be added.

The thing is that by using an already feature full template engine, we can start attacking such problems directly. We do not need to spend time implementing and debugging the basics first, but if there is a need, the infrastructure that you have made for codegen can easily be made available for jinja2 templates. It's is however important to identify where the missing feature are missing, is it in the templates/template engine, in the data model or somewhere else.

Having this all in one file and getting it automatically executed looked less complex to me than
having to teach a driver writer about how to do it with Jinja.

The process of generating a jinja template and codegen template is the same. Codegen templates still have to be identified in the CMakeLists.txt files, and you still have to process the files with the codegen tool. It's just a matter of how you organize the files, I think it's cleaner to identify which files are templates and not. If you already know python, jinja2 should be a breeze, and if you don't, jinja2 should be a quicker learn than python.

Please don´t call it COG. COG is Ned´s tool. Codegen is something different.

Ok, sorry for that, but I see it as an extension of COG. It still works the way COG was intended, but you have added many features to it. This argument validates my argument that codegen (as implemented) will add a lot of extra development/maintenance work just to have a feature that we can get almost for free through engines like Jinja2. I am actually not arguing that it MUST be jinja, but that it should be an externally developed engine that we can get maintained/developed almost for free.

Jinja2 comes with good documentation and a big community which should make it easier to learn and get help.

For simple placeholder replacements you can really use any template engine. The hard part is to get
the "custom filters/helper functions" and the template syntax in a way that is acceptable to driver
writers and other users.

A template should preferably be kept simple and only contain simple logic and string replacement. This is the beauty with many template engine, they enforces a much stricter border between the view and the model. If driver A and driver B shares commonalities it will be easier to avoid re-inventing the wheel or in worst case model divergence with a template engine that forces you to update a common script or data model.

Jinja can be used as is but the driver infrastructure to use Jinja still has to be in Zephyr. This also will
most probably never be upstreamed as it is in this case not the target of Jinja.

We don't need to upstream anything to the Jinja2 project, the engine supports one template to import or extend another template (much like I do it in this pull request). But the engine it self is very also customization/extendable, either through custom filters or by making python function available in the templates.

Like this:

Custom filters:
https://stackoverflow.com/questions/22401959/how-to-create-jinja2-custom-filters-with-django-jinja-app

Python functions:
You could for instance use a data object called api, which you populate with python functions, this
would look something like this in jinja2 syntax

python code:
Both of these can easily be done straight in jinja, so meant as an example:

def print_hello_world():
    return "Hello World!"

def get_list_of_devices(compat_list):
    return data['devicetree']['compatibles'][compa_list]

api['print_hello_world'] = print_hello_world
api['get_list_of_devices'] = get_list_of_devices

env = Environment(loader=FileSystemLoader('/path/to/templates'))
template_handle = env.get_template(template_file)
rendered_content = template_handle.render(api=api)

Jinja2:

{{ api.print_hello_world() }}

{{ api.get_list_of_devices(['st,stm32-i2c-v1','st,stm32-i2c-v2']) }}

or I could populate the env.globals in the python script to avoid the 'api' namespace.

env.globals['print_hello_world'] = print_hello_world
env.globals['get_list_of_devices'] = get_list_of_devices

jinja2 template:

{{ print_hello_world() }}

{{ get_list_of_devices(['st,stm32-i2c-v1','st,stm32-i2c-v2']) }}

[b0661]

There was a general request to only have pure Python for code generation. Even complex C macros were rejected because they are too complex for a driver developer. Ninja templates require to learn another (simple) scripting 'language' addionally to Python.

This argument just does not make sense at all when there are multiple perfectly good engines out there for generating content. It is important always pick the right tool for the right job.

Well spoken!

But right is always debatable and depends on the view point. From my past experience knowledge of people is prime - you have to select the tool depending on the knowledge of the people. It is easier to find a tool that fits to the knowledge of the people than to find knowledged people for a tool or train them.

Though in theory I agree.

[b0661]

Just to make sure we are on the same page, I assume a data driven code generation scheme for zephyr. Based on the fact that there is already a lot of metadata available that can be used for code generation. So my choices in this proposal are based on that assumption.

Data driven code generation is still a litlle bit vague. For some of the OOT drivers and for the pin controller driver I had to add extra logic/ functionality to create the data that was necessary for code generation. In the strict sense this is not purely data driven as the available meta data was not sufficient. Codegen targets more than just data driven code generation.

As the transformation of the data can also be handled external to the template there will be always a way to do it. Using Codegen and Python can make these transformation functions part of the source code. A major benefit in my view. Also I had in mind to do some database requests to personalize the Zephyr images during build using inlined Python code. All this can be also done outside of the template. The opinions what is the best place are very diverse. Codegen is just more versatile in this respect.

To do this some infrastructure where the driver can add the data generation has to be added.

The thing is that by using an already feature full template engine, we can start attacking such problems directly. We do not need to spend time implementing and debugging the basics first, but if there is a need, the infrastructure that you have made for codegen can easily be made available for jinja2 templates. It's is however important to identify where the missing feature are missing, is it in the templates/template engine, in the data model or somewhere else.

Assume the feature is missing in the data model (like for the pin controller driver). You have to extend the data model - currently the EDTS database. This might be a viable way for in tree drivers. For OOT drivers this adds an extra patch to be applied and maintained. From my experience with the current driver base there will be a lot more OOT drivers created than in tree drivers. Partly because of the limited functionality of the in tree drivers, partly because you don´t want to disclose a special driver. Codegen provides a way to handle that in the same way as for the in tree drivers.

I though the word "codegen" was so ambiguous so I though It was represented the feature and not the implementation, but for sure, I can rename the files.

For me 'codegen' is just a name for a tool that describes the task of the tool. I´m not a native speaker, sorry I don´t see the ambiguity.

I am not sure how much sense it makes to use multiple code generators though.

I don´t know either, but if you use the same interface I can not use the interface for my development. Keep the tools distinct as they are different. When one is merged the other can go on without breaking the OOT usage. I think this a fair deal. This relates to the tools names, documentation and also the build system interfaces (aka. zephyr_sources_codegen_xx should not be used or be extended to select the code generator).

A template should preferably be kept simple and ...

Sorry their was a lot of discussion what a template/ inline code generation should be. The opinions are diverse. Also the opionions when something is 'simple', ... are very diverse. I learned the hard way not to jump into that again. For me Codegen fulfills all these characteristics and requirements.

But the engine it self is very also customization/extendable, either through custom filters or by making python function available in the templates.

Yes, and when you use this feature you get a Zephyr specific part that has to be developed and maintained "within" Zephyr. The big question that really nobody knows is how much extension will be necessary in the future.

I am actually not arguing that it MUST be jinja, but that it should be an externally developed engine that we can get maintained/developed almost for free.

You got Codegen developed for free - Ned did the basics for free, I extended it for free. The basic engine was also developed and maintained externally for a long time and has a very stable code base. The Zephyr specific additions/ extensions are done by me. You may question my involvement in the maintenance of this, but this also applies to the maintanence of the Jinja templates/ additions that you might maintenance be doing. It´s not a really technical question but a matter of trust. In the end it is open source and if there is a need in the future all is prepared.

I hope the "almost for free" argument in an open source development is not meant as it sounds.

[carlescufi]

@SebastianBoe could you look into the build system integration in this PR please? Or was it you who wrote it in the first place?

[galak]

@SebastianBoe could you look into the build system integration in this PR please? Or was it you who wrote it in the first place?

I was wondering if we should be generate the files as part of the same phase that we call extract_dts_includes.py?

[carlescufi]

@SebastianBoe could you look into the build system integration in this PR please? Or was it you who wrote it in the first place?

I was wondering if we should be generate the files as part of the same phase that we call extract_dts_includes.py?

We discussed this with @erwango the other day at ELCE. I think the 2 options we have are:

1. Generate all the .c/.h files at once at CMake generation time (i.e. cmake -G time). This could be another add_custom_target perhaps.
2. Generate them as part of compiling a .c files that requires a template (i.e. at ninja time)

I would really like to hear @SebastianBoe opinion on this.

[carlescufi]

@carlescufi , if we're adding the .jinja2 extension, do we really need the "_template" suffix?
I think it'll be more user friendly if we can contain a bit the complete file name..

@erwango
No, we don't. I am fine with removing it.

[galak]

Why do we need the ifdef protection, this file should only ever be included once?

[evenl]

Hehe, that more or less happens by it self when I start to write something called .h :) But for sure, we do not need it.

[galak]

I'm sure I missed this, but why do we have both i2c_ll_stm32.meta.jinja2 & i2c_ll_stm32_device.h.jinja2?

[SebastianBoe]

I would really like to hear @SebastianBoe opinion on this.

Unless there is a good reason to do otherwise, you want to do work at build time (ninja) and not Configure time (CMake). This is for build-time performance reasons. Build time work will be done in parallel, configure time work will be done serially.

Build-time performance is important for usability, a tool that responds within 0.2 seconds is easier to use than one that responds within 2 seconds. (UI turn-around time).

[carlescufi]

I would really like to hear @SebastianBoe opinion on this.

Unless there is a good reason to do otherwise, you want to do work at build time (ninja) and not Configure time (CMake). This is for build-time performance reasons. Build time work will be done in parallel, configure time work will be done serially.

Build-time performance is important for usability, a tool that responds within 0.2 seconds is easier to use than one that responds within 2 seconds. (UI turn-around time).

Thanks for the feedback. Should we then add "Render at ninja time" as a requirement at the bottom of this issue description? #10904
currently it is asking the question with no conclusion

[SebastianBoe]

Should we then add "Render at ninja time" as a requirement at the bottom of this issue description? #10904
currently it is asking the question with no conclusion

Yes. Note that this has been discussed and implemented earlier.

[evenl]

Just pushed an update that expose the EDTSDatabase API to the template through the edts_api object/namespace. I thought it was cleaner to do it through a object/namespace, but I the functions can be exposed without the namespace.

My whole device_declare.jinja2 template and the i2c_ll_stm32.meta.jinja2 file is now updated to only use the API and no direct JSON data access.