Generated driver instantiation prototype #8561

erwango · 2018-06-26T14:08:19Z

Goal

Aim of this work is to demonstrate a prototype of driver instantiation leveraging codegen library provided by @b0661.

Concept

Device tree is converted to a json database with help of yaml bindings.
Codegen is able to parse this database and provides an easy access from user to the database
Codegen expose a simplified device_declare API to generate driver instantiation code

Oustanding points

Database

Differentiation point compared to initial proposal (#6762 and #5799) is to introduce an intermediate generated json database result of device tree treatment along with yaml bindings. This database exposes a flatten list of device nodes enabled in device tree, grouped by compatible. Each device node contains device tree properties augmented thanks to yaml bindings.
This database aims at providing a unique source of information on the target and could be reused for other purposes than code generation, such as CI, Kconfig generation, ...
Database structure could be refined after acceptance of this prototype, specially to add specific node information for advanced post-treatment.
json format was chosen over pickle alternative due to the text format.

Device declaration API

A special effort has been made to provide and easy to use code generation interface.
A device_declare function is provided which should be filled by driver developer once. It will then be processed before compilation and generate code for all device nodes instances matching the driver compatible and not disabled. If no node matching compatible, driver is not compiled.

device_declare will drive the whole device instances code generation (generic to all/most drivers) with device_data and device_config will help writing dedicated _data and _config structs that can depend on each driver.
Here is and example of device_declare usage applied to STM32 SPI driver.

  device_data = ( 'spi_stm32_data',
  """
  	SPI_CONTEXT_INIT_LOCK(st_stm32_spi_${node_index}_data, ctx),
  	SPI_CONTEXT_INIT_SYNC(st_stm32_spi_${node_index}_data, ctx),
  """
  )

  device_config = ( 'spi_stm32_config',
  """
       .spi = (SPI_TypeDef *)${@reg/0},
       .pclken.bus = ${@clock/bus},
       .pclken.enr = ${@clock/bits},
  #ifdef CONFIG_SPI_STM32_INTERRUPT
       .irq_config = st_stm32_spi_${node_index}_config_irq,
  #endif
 )
"""
 
  codegen.device_declare('st,stm32-spi-fifo',
  'CONFIG_SPI_INIT_PRIORITY',
  'POST_KERNEL',
  {'irq_func':'spi_stm32_isr','irq_flag': 'CONFIG_SPI_STM32_INTERRUPT'},
  'spi_stm32_init',
  'api_funcs',
  device_data,
  device_config
  )

Generated code looks as follows (for instance 'SPI_1'):

 #ifdef CONFIG_SPI_1

 DEVICE_DECLARE(st_stm32_spi_spi_1);
 static void st_stm32_spi_spi_1_config_irq(struct device *dev)
 {
 	IRQ_CONNECT(35,
 		5,
 		spi_stm32_isr,
 		DEVICE_GET(st_stm32_spi_spi_1),
 		0);
 	irq_enable(35);
 }

 static const struct spi_stm32_config st_stm32_spi_spi_1_config = {
      .spi = (SPI_TypeDef *)0x40013000,
      .pclken.bus = 3,
      .pclken.enr = 4096,
 #ifdef CONFIG_SPI_STM32_INTERRUPT
      .irq_config = st_stm32_spi_spi_1_config_irq,
 #endif
 };

 static struct spi_stm32_data st_stm32_spi_spi_1_data = {
 	SPI_CONTEXT_INIT_LOCK(st_stm32_spi_spi_1_data, ctx),
 	SPI_CONTEXT_INIT_SYNC(st_stm32_spi_spi_1_data, ctx),
 };

 DEVICE_AND_API_INIT(st_stm32_spi_spi_1,
 	"SPI_1",
 	spi_stm32_init,
 	&st_stm32_spi_spi_1_data,
 	&st_stm32_spi_spi_1_config,
 	POST_KERNEL,
 	CONFIG_SPI_INIT_PRIORITY,
 	&api_funcs);

 #endif /* CONFIG_SPI_1 */

device_data and device_config should be filled with help specific tags:

@dts_path
'node_index': could be used to insert device index in the string

Even if working fine with current example, this work could be further improved by:

need to add some checks to detect malformed expressions
...

EDIT:
Since there are 2 compatibles for STM32 spi, duplicate drivers files to have one file per compatible.
Only file matching compatible will be compiled.
For drivers offering quite small differences between 2 different 'compatible', this could be improved in a second step to enable the ability for a driver file to be compatible with 2 dts 'compatibles'

EDIT (06/29):

Rebased onto latest scripts: add Zephyr inline code generation with Python #6762 proposal. Pushed declare.py into codegen/modules.
Used python string.template to propose a simplest API. Example provided in a new declare_template.py module and used in drivers/spi/spi_ll_stm32.c
Reworked PR description
Added multiple irq support (will require changes in driver code to support appropriate _isr function name)

EDIT (07/03):

Updated edevicetree to support definition of multiple compatibles by a single driver.
Several minor improvements
Generate STM32 I2C instantiation

EDIT (07/04):

Various minor improvments
Moved sensor LSM6DSL to code generation to demonstrate several possible uses of API, cf commit 2597c24da5967e40c1d726de1873682117a14b28

EDIT (07/06)

Added doc for device_declare API

EDIT (08/01)

Rebased on latest master
Removed alternate module API proposal, focusing on the easiest to use (initialy named 'declare_template', renamed to 'declare'.
Focus changes on drivers to minimal changes, to feature ready to merge examples.

Next steps

Based on current propotype reception, following points should be refined:

Review database generation (generation method and database specification)
Add sanity checks
Any point that should be raised during review of this RFC

codecov-io · 2018-06-26T14:30:02Z

Codecov Report

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

@@             Coverage Diff             @@
##           topic-EDTS    #8561   +/-   ##
===========================================
  Coverage       53.11%   53.11%           
===========================================
  Files             218      218           
  Lines           26871    26871           
  Branches         5952     5952           
===========================================
  Hits            14273    14273           
  Misses          10163    10163           
  Partials         2435     2435

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 c18a56e...043c929. Read the comment docs.

dbkinder · 2018-06-26T15:52:17Z

CONTRIBUTING.rst

@@ -329,8 +329,11 @@ On Linux systems, you can install uncrustify with
 For Windows installation instructions see the `sourceforge listing for
 uncrustify <https://sourceforge.net/projects/uncrustify>`_.

+Best coding practises


practices

dbkinder · 2018-06-26T15:54:45Z

CONTRIBUTING.rst

+
+Keep the code as simple as possible.
+
+Zephyr allows to generate code by tools. Keep this to a minimum and use the tool


How about this:

Code-generation preprocessing tools provide a convenient way to simplify some repetitive or parameterized coding tasks. While Zephyr development allows use of such tools, we recommend keeping this use to a minimum and when it provides an appropriate and simple coding solution that follows these rules:

dbkinder · 2018-06-26T16:03:08Z

doc/subsystems/codegen/codegen.rst

+tailored and tuned to a specific project configuration.
+
+The Zephyr project supports a code generating tool that processes embedded
+Python "snippets" inlined in your source files. It can be used, for example,


should you mention Python2 or Python3 here?

No not relevant here and Zephyr anyway is restricted to Python 3 only.

dbkinder · 2018-06-26T16:05:12Z

doc/subsystems/codegen/codegen.rst

+Python scripts. All Python snippets in a source file and all Python snippets of
+included template files are treated as a python script with a common set of
+global Python variables. Global data created in one snippet can be used in
+another snippet that is processed later on. This feature is e.g. used to


Change to:

This feature could be used, for example, to customize included template files.

dbkinder · 2018-06-26T16:06:18Z

doc/subsystems/codegen/codegen.rst

+An inlined Python snippet can always access the codegen module. The codegen
+module encapsulates and provides all the functions to retrieve information
+(options, device tree properties, CMake variables, config properties) and to
+put out the generated code.


change put out to output

dbkinder · 2018-06-26T16:46:18Z

doc/subsystems/codegen/codegen.rst

+
+.. function:: codegen.if_device_tree_controller_compatible(device_type_name, compatible)
+
+    Stop code generation if there is no activated device that is compatible.


What does "stop" mean here? Just that no output for that chunk is generated or the whole code generation processing stops?

Whole code processing stops

dbkinder · 2018-06-26T16:48:55Z

doc/subsystems/codegen/codegen.rst

+    The codegen.error function.
+
+    Instead of raising standard python errors, codegen generators can use
+    this function.  It will display the error without a scary Python


Again, I'd avoid using "scary Python traceback", so just write this as:

Instead of raising standard Python errors, codegen generators can use this function to write the error message to the log.

dbkinder · 2018-06-26T16:49:58Z

doc/subsystems/codegen/codegen.rst

+Logging
+-------
+
+.. function:: codegen.log(message [, message_type=None] [, end="\n"] [, logonly=True])


You're missing descriptions and parameter descriptions for these functions.

Where does logging output get written?

dbkinder · 2018-06-26T16:53:27Z

doc/subsystems/codegen/codegen.rst

+Code generation in the build process
+************************************
+
+Code generation has to be invoked as part of the build process. Zephyr uses


I'm assuming then that the doc generating (doxygen) processing won't see the codegen output. I suspect this is OK but I'm curious how the API documentation will be affected

dbkinder · 2018-06-26T16:56:04Z

doc/subsystems/codegen/codegen.rst

+
+.. function:: codegen.prerr(s [, end="\n"])
+
+Code generation in the build process


After reading all these codegen function definitions, it would be great to have some practical examples of their use included in the document to better show why we're provide this codegen capability.

carlescufi · 2018-06-26T17:00:06Z

Thanks for the PR @erwango. I am copying @evenl here since he has some ideas on code generation and templating in general.

carlescufi · 2018-06-26T17:02:47Z

scripts/codegen/cogapp/cogapp.py

@@ -0,0 +1,781 @@
+# coding: utf8


This is present on PyPi: https://pypi.org/project/cogapp/. Do we really need to include it here or could we add it to requirements.txt?

cogapp.py is the orginal import from PyPi. It was not changed and is treated as an external source. Does not make sense to include it to requirements.txt.

what do you mean @b0661? Why do you need to import it from PyPi and make a local copy of it? if you put cogapp in requirements.txt the module will be installed automatically when the user sets up the development environment and the Zephyr scripts will always have access to the latest version. I don't understand why we wouldn't do the same thing we do with all other Python modules we require, which is to install them via PyPi. Having local copies is always a maintenance burden.

The local copy of the sources of cogapp is reduced to the bare minimum. Major parts are already re-written, only very litlle is used anymore. The rest will be replaced over time. There is no extra burden as there is no benefit for codegen to follow cogapp. It is vice versa; it would be an extra burden to follow cogapp.

OK, that was not what I understood originally when you said "cogapp.py is the original import from PyPi. It was not changed...". Fine then, if we want to avoid following upstream to do our own thing I'm OK with that.

carlescufi · 2018-06-26T17:05:08Z

@erwango one of the outstanding points that hasn't been listed yet is the obvious "should we mix .c code with templated pseudo-code"? I personally lean towards not mixing both together, but it's not based on actual real-life experience. I know that @SebastianBoe prefers not to mix those, so I'd like to hear everybody else's opinion on this.

b0661

We have a lot of review comments that target codegen. Please make clear what part of the code is just used by your solution (such as codegen from #6762) and what are the additions/ changes you are doing.

I will try to solve all comments that are related to the original codegen sources in #6762 providing an updated version to be used here. Some of the comments are already solved in the current version of #6762, your copy is outdated - please rebase your code or cherry-pick from #6762. You should pick at least:

scripts: prepare Zephyr inline code generation
scripts: add Zephyr inline code generation
doc: subsystems: add inline code generation documentation

Comments on the API:

device_data = ( ['spi_stm32_data',
       {'string': ['SPI_CONTEXT_INIT_LOCK(st_stm32_spi_fifo_', 'index', '_data, ctx)']},
       {'string': ['SPI_CONTEXT_INIT_SYNC(st_stm32_spi_fifo_', 'index', '_data, ctx)']}]
  )

  device_config = ( ['spi_stm32_config',
        {'field': '.spi', 'value': '@reg/0', 'cast': '(SPI_TypeDef *)'},
        {'field': '.pclken.bus', 'value': '@clock/bus'},
        {'field': '.pclken.enr', 'value': '@clock/bits'},
        {'string': ['.irq_config = st_stm32_spi_fifo_', 'index', '_config_irq'], 'flag': 'CONFIG_SPI_STM32_INTERRUPT'}]
 )

You are specifying data strings in Python that a C programmer can easily specify in C syntax. Python provides a string template class that I used in my solution and that should be used instead. By this the programmer does not have to learn a new syntax. She has only to remember some macro names. The macros are replaced by the appropriate values for the driver instances.

See https://github.com/b0661/zephyr/blob/1e68884e8b1505cb236b5af595a91fc55712f337/drivers/spi/spi_stm32.c as an example:

/**
 * @code{.codegen}
 * ...
 * device_data = ('struct spi_stm32_data',
 * """
 *         SPI_CONTEXT_INIT_LOCK(${DEVICE_DATA}, ctx),
 *         SPI_CONTEXT_INIT_SYNC(${DEVICE_DATA}, ctx),
 * """)
 * device_config_info = ('struct spi_stm32_config',
 * """
 *         .spi = (SPI_TypeDef *)$BASE_ADDRESS,
 *         .pclken.bus = $CLOCK_0_BUS,
 *         .pclken.enr = $CLOCK_0_BITS,
 * #if CONFIG_SPI_STM32_INTERRUPT
 *         .config_irq = ${DEVICE_CONFIG_IRQ},
 * #endif
 * """)
 * device_config_irq = (
 * """
 * #if CONFIG_SPI_STM32_INTERRUPT
 * DEVICE_DECLARE(${DEVICE_NAME});
 * static void ${DEVICE_CONFIG_IRQ}(struct device *dev)
 * {
 *      IRQ_CONNECT(${IRQ_0}, ${IRQ_0_PRIORITY}, spi_stm32_isr, \\
 *                  DEVICE_GET(${DEVICE_NAME}), 0);
 *      irq_enable(${IRQ_0});
 * }
 * #endif
 * """)
 * codegen.out_include('templates/drivers/simple_tmpl.c')
 * @endcode{.codegen}
 */
/** @code{.codeins}@endcode */

/** @} device_driver_spi_stm32 */
"""

b0661 · 2018-06-26T18:22:25Z

scripts/codegen/codegen.py

+from .include import IncludeMixin
+from .log import LogMixin
+
+class CodeGenerator(OptionsMixin, GenericMixin, ConfigMixin,


@erwango

You are using an old version of codegen. Please rebase to the actual #6762. There are major changes compared to the version you use here.

b0661 · 2018-06-26T18:29:13Z

doc/subsystems/codegen/codegen.rst

+tailored and tuned to a specific project configuration.
+
+The Zephyr project supports a code generating tool that processes embedded
+Python "snippets" inlined in your source files. It can be used, for example,


No not relevant here and Zephyr anyway is restricted to Python 3 only.

b0661 · 2018-06-26T18:32:04Z

doc/subsystems/codegen/codegen.rst

+put out the generated code.
+
+Codegen transforms files in a very simple way: it finds chunks of Python code
+embedded in them, executes the Python code, and inserts its output back into


Yes, hello will be put in your code if you use codegen.out("hello"). Print is another story, it will usually go to stdout.

b0661 · 2018-06-26T18:46:52Z

doc/subsystems/codegen/codegen.rst

+.. function:: codegen.error(msg)
+
+    Raises an exception with msg as the text. No traceback is included, so
+    that non-Python programmers using your code generators won’t be scared.


Wrong anyway - will delete.

b0661 · 2018-06-26T18:50:13Z

scripts/codegen/cogapp/cogapp.py

@@ -0,0 +1,781 @@
+# coding: utf8


cogapp.py is the orginal import from PyPi. It was not changed and is treated as an external source. Does not make sense to include it to requirements.txt.

tbursztyka · 2018-06-27T07:33:34Z

@b0661 The issue with :

 * device_data = ('spi_stm32_data',
 * """
 *         SPI_CONTEXT_INIT_LOCK(${DEVICE_DATA}, ctx),
 *         SPI_CONTEXT_INIT_SYNC(${DEVICE_DATA}, ctx),
 * """)

is that you are forcing devs to follow a certain way of naming their private data variable. And that you cannot do it. Up to code generation to provide enough flexibility to adapt to any kind of naming a dev would like to follow, not the other way round.

Maybe there is another way, mixing both ideas in one: we could have a definition of the base name of the data variable, and the rest would be about the same as yours:

 * device_data = ('spi_stm32_data',
* ['spi_stm32_dev_data_', @dev_index],
 * """
 *         SPI_CONTEXT_INIT_LOCK(${DEVICE_DATA}, ctx),
 *         SPI_CONTEXT_INIT_SYNC(${DEVICE_DATA}, ctx),
 * """)

2nd parameter would be the one dictating the base name of the private data name. It's always a string so no need to precise the type, and it's just an array of string/@dts_generated_vars.

@erwango what do you think?

We of course would apply the same to device_config().

Personally I like the fact dev does not have to manager #ifdefs by himself and use the 'flag' introduced in this PR used as a condition to generate code. Thouhg I don't know of to merge it in above proposal.

Another one is the macro names your are talking about: these are totally unrelated to DTS. $BASE_ADDRESS, $CLOCK_0_BITS, ... These are hard to relate to DTS db, and that's something we would like to avoid. Idea is that the relationship with DTS and code generation should be straight.

It's not visible yet in this PR because it requires quite some work again, but things like @reg/0 will disappear to @base_address for instance (DTS db will provide nicer names/aliases). In clock area it's already quite nice because it reuses the flag names from the yaml file for instance.

For the rest, we have to stick with device_declare(), fully hiding all the DEVICE_* macros, the irq config function stuff etc... We really want that to disappear.

b0661 · 2018-06-27T08:15:58Z

@tbursztyka

is that you are forcing devs to follow a certain way of naming their private data variable. And that you cannot do it

@erwango and you should really come up with the goals you wan´t to achieve. Simplicity and ease of use was the goal @erwango postulated. To be simple some restrictions must be there - otherwise you always can use pure inline python and not a template.

By the way you copied it wrong. The parameter is about the data type and not the name of the data instantiation.

"cannot do" - Yes, I can do. The dev provides the data type "struct whatever_dev_needs". The template decides on the name and provides the name as a macro parameter ${DEVICE_DATA}. Both get what they need in a simple and easy way.

Your suggestion needs a lot more parameters that are not necessary. The template always replaces the macro parameters by the actual values of the specific driver instantiation - no need for any index.

Another one is the macro names your are talking about: these are totally unrelated to DTS. $BASE_ADDRESS, $CLOCK_0_BITS, ..

Thats a matter of naming and not of the principle. In Python string templates you provide a mapping from macro names to values. You can use whatever macro name you like. So best you first decide on how you want to name the properties from the device tree. In my solution I already created an alias for ${BASE_ADDRESS} - you can use ${reg0} instead. You can for shure also add ${base_address} as another alias.

For the rest, we have to stick with device_declare(), fully hiding all the DEVICE_* macros, the irq config function stuff etc... We really want that to disappear

I don´t know why you are regarding driver developers to be so dumb to not know what to put here. Anyway if you wan´t to hide such things you really have to put more restrictions on that. In the end the driver developer has to learn the syntax to fill all the template parameters which in my view is in no way easier than to just use plain C with some macros. I think copy and paste from one driver to another will anyway happen - so no need to invent a new syntax for something that is already documented and available for C. Code generation should be restricted to the parts that can not easiliy be done in C (one of the coding rules).

Personally I like the fact dev does not have to manager #ifdefs by himself and use the 'flag' introduced in this PR used as a condition to generate code.

Depends on what defines you are talking. Defines that are related to the device tree can be resolved by the template (e.g. 'CONFIG_SPI_0'). Defines that reflect application/user decisions (e.g. "SPI_USE_INT") have to be handled in the driver code anyway - there is no benefit to hide these defines only in the driver instantiation when you have to use them in the rest of the code.

tbursztyka · 2018-06-27T08:23:35Z

I don´t know why you are regarding driver developers to be so dumb to not know what to put here.

You are misunderstanding the point here. We want to hide how device instances are actually generated because that will change to address other issues (unrelated to code generation, sure). And you don't want to change that in every drivers (changing or removing DEVICE_* macros etc etc...). In any case: what's generic and found in every driver (as generated code that is) should just disappear from there and put in the device_declare (or else) directly.

erwango · 2018-06-27T08:29:10Z

@b0661 @dbkinder, I've just rebased with the following changes:

Rebased onto latest scripts: add Zephyr inline code generation with Python #6762
Removed doc commits as I think this is only noise to review doc while we still don't know where we want to go (and review could happen in scripts: add Zephyr inline code generation with Python #6762 if needed)

I didn't had time to update the solution vs the new cogen version so integration is somewhat worst than previously even if still working.

b0661 · 2018-06-27T08:34:27Z

We want to hide how device instances are actually generated

Provide the parameters the new device instantiation needs. Without that we are spinning wheels forever.

erwango · 2018-06-27T08:43:59Z

@b0661

In my solution I already created an alias for ${BASE_ADDRESS} - you can use ${reg0} instead. You can for shure also add ${base_address} as another alias.

One of the goal we'd like to achieve is to avoid hardcoded references to device tree property, solution should be flexible enough to get these as inputs of whatever API will be used. And parameters encoding should be close enough to device tree syntax to make its use instinctive.
This is what I provided in this PR with function device_tree_node_property.

Then, I have one specific comment to recent #6762, that I will do in #6762 to avoid mixing topics (we discuss "what" here and "how" in #6762)

b0661 · 2018-06-27T08:44:56Z

cmake/dts.cmake

@@ -108,6 +109,7 @@ if(CONFIG_HAS_DTS)
    ${DTS_FIXUPS}
    --keyvalue ${GENERATED_DTS_BOARD_CONF}
    --include ${GENERATED_DTS_BOARD_H}
+    --structs ${GENERATED_DTS_STRUCTS}


Can we get rid of structs as a name. I know this has historical reasons but now it is about to generate a database and not of structs only. So what about GENERATED_DTS_BOARD. This is also the name of the generated file.

ok to remove structs, though I'd like we can identify this is a('the") database.
What about GENERATED_DTS_BOARD_EBD?

b0661 · 2018-06-27T08:48:50Z

scripts/codegen/declare.py

+import pprint
+import json
+
+class DeclareMixin(object):


This shall become a template. There might be several ways how to instantiate a driver in the future. The core of codegen should not be affected by this.

Cf, my comment here (#6762 (comment)).
I agree to find a solution to isolate these modules from core of codegen. Though, I don't think we should use templates for python code.

Now codegen modules are available. Please make this a module.

b0661 · 2018-06-27T08:51:50Z

scripts/codegen/devicetree.py

+import pprint
+import json
+
+class DeviceTreeMixin(object):


Please reduce this class to the bare minimum to access the json file and get device tree properties. Move the rest to a template. Same reason as above to keep the core of codegen stable and provide the ability for different solutions by templates.

b0661 · 2018-06-27T09:01:17Z

cmake/extensions.cmake

+      -D "CMAKE_COMPILER_IS_GNUCXX=${CMAKE_COMPILER_IS_GNUCXX}"
+      -D "GENERATED_DTS_BOARD_H=${GENERATED_DTS_BOARD_H}"
+      -D "GENERATED_DTS_BOARD_CONF=${GENERATED_DTS_BOARD_CONF}"
+      -D "GENERATED_DTS_STRUCTS=${GENERATED_DTS_STRUCTS}"


GENERATE_DTS_BOARD

b0661 · 2018-06-27T09:03:50Z

drivers/spi/spi_ll_stm32.c

+ * @code{.codegen}
+ * codegen.device_tree_set_compatible('st,stm32-spi')
+ *
+ * # static struct spi_stm32_data st_stm32_spi_<node_idx>_data = {


It is not necessary to let the dev define the name of the data instantiation (here "st_stm32_spi_<node_idx>_data"). Data can and shall always be referenced by the device structure.

Right, comment is actually misleading here as this code will indeed be generated from compat + 'data'

b0661 · 2018-06-27T09:06:42Z

drivers/spi/spi_ll_stm32.c

+ * # };
+ *
+ * device_data = ( ['spi_stm32_data',
+ *      {'string': ['SPI_CONTEXT_INIT_LOCK(st_stm32_spi_', 'index', '_data, ctx)']},


As long as @tbursztyka does not provide info on the planned new device instantiation I regard this interface as too complex and be easily replaced by a template string with macros.

@b0661

As long as @tbursztyka does not provide info on the planned new device instantiation

Even in a new instantiation solution, devices will be provided by sepcific _data and _config structures, so see these structs as an immutable in the API. User should be able to provide his own custom structs with 'free text' and access to device tree edb.

I regard this interface as too complex and be easily replaced by a template string with macros.

'Too complex', I agree.
'Easily replaced', this is where the codegen solution provided is maybe not as easy to understand as you might think. I might be slow learner, but I'm having hard time to see how it should be used and (what I see as) abusive use of templates does not help.

I might be slow learner, but I'm having hard time to see how it should be used and (what I see as) abusive use of templates does not help.

My ideal world is a C code string with placeholders where a template script replaces the placeholders by the values from device tree, autoconf, ... for a device driver. The template script repeats that for all instantiations of a device driver.

Only thing to know for a dev are the placeholder names that relate to device tree, autoconf, ... and the C syntax to write the data definitions and the interrupt initialisation.

The ideal world has to be augmented by some control data to know about the CONFIG vairables of the device driver, the api struct, ...

Your aim is different; you wan´t something where the code generator is in control how the data definition and the interrupt initialisation will be coded. Only by this you can make it independent of the device initialisation principle. In my view this independence is not worth the effort and the additional complexity. The initialisation principle will not be changed very often. If it is changed you may even use sematic patching to apply the change.

b0661 · 2018-06-27T09:36:45Z

@erwango

One of the goal we'd like to achieve is to avoid hardcoded references to device tree property,

You always have to state the device tree property you want to use. How the value for this property is found can be decided by a template. There is nothing hardcoded here.

Using Python template strings the mapping between names and values is completely controlled by the snippet code. The value can e.g. be retrieved by device_tree_node_property().

The important thing is to define a scheme for the names of properties that can easily used by the devs.

SebastianBoe · 2018-06-27T10:13:04Z

drivers/spi/spi_ll_stm32.c

+ * codegen.if_device_tree_node_compatible('st,stm32-spi')
+ * @endcode{.codegen}
+ */
+/** @code{.codeins}@endcode */


As mentioned earlier, this is not ok.

See the original PR for the reasoning.

codegen.if_device_tree_node_compatible() stops code generation if there is no compatible device activated. This was regarded not acceptable as the file is generated but it does have no content. The solution was to make the decision what file to generate in CMakeList.txt using a config var.

This is a copy of old stuff that needs to be adpated by @erwango. See https://github.com/b0661/zephyr/blob/1e68884e8b1505cb236b5af595a91fc55712f337/drivers/spi/spi_stm32.c in #5799 for a version that does not use codegen.if_device_tree_node_compatible().

SebastianBoe · 2018-06-27T10:16:40Z

This PR has copied code from other PR's without addressing the review comments in the original PR's.

How do we deal with that? Repeat ourselves? Add links to original PR? Block this PR on the original PR's?

Replace device instances code by codegen solution. Since there are 2 compatibles for STM32 SPI, declare both compatibles in device_declare API. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

Replace device instances code by codegen solution. Since there are 2 compatibles for STM32 i2c, specify 2 different compatibles are supported by the driver Change the isr name, with isr name as postfix (instead being in the middle of the function name) in order to make it easier to be generated. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

To align with requirement about split of responsibility between Kconfig and device tree, don't condition instance code generation on matching CONFIG flag value. Codegen should generate the code configured in device tree. Kconfig is responible for its final compilation. This way codegen is independant from Kconfig and could be executed before it. Reported misconfiguration should not be possible if a 'compatible' check is done beforehand. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

Depending on the caller, edtsdatabase should import EDTSDevice class from edtsdevice or dts.edtsdevice. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

In codegen devicedeclare module, use EDTSDevice API to access EDTS data. To simplify things, 'device-name' derivates (device-data, device-config-info and device-config-irq) are reworked to use unique_id field. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

This change is only to demonstrate how we could declare common template for a driver compatible with 2 compatible variants. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

erwango · 2018-11-09T14:54:32Z

drivers/sensor/lsm6dsl/lsm6dsl_devices.c.in

+ *
+ * device_config_spi = ('lsm6dsl_config',
+ * """
+ *      .comm_master_dev_name = ${get_property('bus/master')},


Need an EDTSDevice api to get bus/master name

erwango · 2018-11-09T14:56:05Z

drivers/sensor/lsm6dsl/lsm6dsl_devices.c.in

+ * device_config_spi = ('lsm6dsl_config',
+ * """
+ *      .comm_master_dev_name = ${get_property('bus/master')},
+ *      .irq_gpio_port = ${get_property('irq-gpios/0/controller')},


Need an EDTSDevice api to get controller name

erwango · 2018-11-09T14:56:10Z

drivers/sensor/lsm6dsl/lsm6dsl_devices.c.in

+ *      .comm_master_dev_name = ${get_property('bus/master')},
+ *      .irq_gpio_port = ${get_property('irq-gpios/0/controller')},
+ *      .irq_gpio_pin = ${get_property('irq-gpios/0/pin')},
+ *      .cs_gpio_port = ${get_property('cs-gpios-controller/0/controller')},


Need an EDTSDevice api to get controller name

erwango · 2018-11-09T14:56:20Z

drivers/sensor/lsm6dsl/lsm6dsl_devices.c.in

+ *
+ * device_config_i2c = ('lsm6dsl_config',
+ * """
+ *      .comm_master_dev_name = ${get_property('bus/master')},


Update devicedecmare to work with parents and controllers. Since they now can be returned thanks to EDTSDevice methods returning EDTSDevice Object, remove the additional code to do this treatment inside devicedeclare. Add support for '.' inside pattern _DeviceCustomTemplate to enable treatment of such partens: get_controller('irq-gpios/0').get_property('label') Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

erwango · 2018-11-12T09:59:00Z

@galak

-Add example of sensor device generation with get_parent and get_controller methods returning EDTSDevice object

Needs error checking, and handling of different 'types' (interrupt, gpio, etc). Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Add get_controller that return controller EDTSDevice Object Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

Update driver to support code generation. Note that this driver change is not functional but done to provide example on how to demonstrate several possibilities of device_declare api: -access bus-master information (implicit in device tree, build thanks to yaml) -access irq/cs gpios and pin information -For a single driver specify 2 types of config struct and build them depending on the compatible present in the board Remove from build_all test to pass CI Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

Do Not Merge: Add LSM6DSL I2C sensor Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

erwango added RFC Request For Comments: want input from the community DNM This PR should not be merged (Do Not Merge) labels Jun 26, 2018

erwango requested review from mbolivar, nashif, SebastianBoe, tbursztyka, b0661, carlescufi, galak and agross-oss June 26, 2018 14:08

erwango requested review from dbkinder and superna9999 as code owners June 26, 2018 14:08

erwango force-pushed the dev_codegen_driver_instantiation branch from 33d5c20 to 106cce5 Compare June 26, 2018 15:48

dbkinder previously requested changes Jun 26, 2018

View reviewed changes

carlescufi reviewed Jun 26, 2018

View reviewed changes

b0661 suggested changes Jun 26, 2018

View reviewed changes

erwango force-pushed the dev_codegen_driver_instantiation branch from 106cce5 to 72b4953 Compare June 27, 2018 08:16

b0661 suggested changes Jun 27, 2018

View reviewed changes

SebastianBoe requested changes Jun 27, 2018

View reviewed changes

erwango added 6 commits November 9, 2018 15:41

drivers/spi: stm32: use codegen driver instantiation

3fab258

Replace device instances code by codegen solution. Since there are 2 compatibles for STM32 SPI, declare both compatibles in device_declare API. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

scripts/dts: edtsdatabase.py: provide alternate path for edtsdevice

d617e19

Depending on the caller, edtsdatabase should import EDTSDevice class from edtsdevice or dts.edtsdevice. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

drivers/i2c: stm32: Split v1 and v2 compatibles templates

55c15fb

This change is only to demonstrate how we could declare common template for a driver compatible with 2 compatible variants. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

erwango force-pushed the dev_codegen_driver_instantiation branch 4 times, most recently from 13026b2 to 867bd1c Compare November 9, 2018 14:52

erwango commented Nov 9, 2018

View reviewed changes

erwango force-pushed the dev_codegen_driver_instantiation branch from 867bd1c to fcde762 Compare November 12, 2018 09:47

erwango force-pushed the dev_codegen_driver_instantiation branch from fcde762 to f9e720c Compare November 12, 2018 09:50

erwango force-pushed the dev_codegen_driver_instantiation branch 2 times, most recently from b4cc89f to 2733355 Compare November 20, 2018 10:48

erwango force-pushed the dev_codegen_driver_instantiation branch from 2733355 to 6f6c34e Compare November 21, 2018 11:00

galak and others added 4 commits November 21, 2018 17:11

scripts/dts: add initial cut at get_parent()

4fb3dd8

Needs error checking, and handling of different 'types' (interrupt, gpio, etc). Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

scripts/dts: edtsdevice: Method to get controller EDTSDevice object

91e81be

Add get_controller that return controller EDTSDevice Object Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

boards: 96b_argonkey: Add LSM6DSL I2C sensor for codegen demo [DNM]

043c929

Do Not Merge: Add LSM6DSL I2C sensor Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

erwango force-pushed the dev_codegen_driver_instantiation branch from 6f6c34e to 043c929 Compare November 21, 2018 16:12

carlescufi mentioned this pull request Nov 29, 2018

ELCE: DT, Kconfig, EDTS path forward #10821

Closed

erwango mentioned this pull request Nov 29, 2018

[RFC]: moving to eliminate dts_fixup files #11737

Closed

galak closed this Aug 13, 2019

erwango deleted the dev_codegen_driver_instantiation branch September 3, 2019 12:04


		Keep the code as simple as possible.

		Zephyr allows to generate code by tools. Keep this to a minimum and use the tool


		.. function:: codegen.if_device_tree_controller_compatible(device_type_name, compatible)

		Stop code generation if there is no activated device that is compatible.


		.. function:: codegen.prerr(s [, end="\n"])

		Code generation in the build process

Generated driver instantiation prototype #8561

Generated driver instantiation prototype #8561

Conversation

erwango commented Jun 26, 2018 • edited Loading

Goal

Concept

Oustanding points

Database

Device declaration API

Next steps

codecov-io commented Jun 26, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b0661 Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Jun 26, 2018

b0661 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbursztyka commented Jun 27, 2018 • edited Loading

b0661 commented Jun 27, 2018

tbursztyka commented Jun 27, 2018 • edited Loading

erwango commented Jun 27, 2018

b0661 commented Jun 27, 2018

erwango commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

b0661 Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

b0661 commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe commented Jun 27, 2018

erwango Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango commented Nov 12, 2018

erwango commented Jun 26, 2018 •

edited

Loading

codecov-io commented Jun 26, 2018 •

edited

Loading

b0661 Jun 29, 2018 •

edited

Loading

tbursztyka commented Jun 27, 2018 •

edited

Loading

tbursztyka commented Jun 27, 2018 •

edited

Loading

erwango commented Jun 27, 2018 •

edited

Loading

erwango Jun 27, 2018 •

edited

Loading

b0661 Jun 27, 2018 •

edited

Loading

erwango Nov 9, 2018 •

edited

Loading