edtlib: tests: refine coverage of Binding objects initialization

[dottspina]

Bindings initialization supports a rich include mechanism:

• a base binding may be include:ed at any level: binding, child-binding, grandchild-binding, etc, the specified properties being injected (in cascade) at the level of inclusion
• whenever an include: appears, the including binding may filter the properties it chooses to inherit at the binding, child-binding, etc, levels; these filters apply across included files
• when several base bindings are inherited which contain definitions about a same property, sometimes the last one wins, but not always, and quite a few things are not allowed (e.g. downgrading a required: true or changing a const: value)
• some scenarios remain undocumented, e.g. diamond inheritance

All this happens (recursively) in the Binding constructor, independently of any actual devicetree model (EDT instance).

This commit is the first in a series which will try to cover somewhat systematically the possible inputs and what we finally get at the exit of the constructor once the object is completely initialized.

Thanks.

See also: #78095

[benediktibk]

The build failure seems to be unrelated to this change.

/opt/toolchains/zephyr-sdk-0.16.8/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/../../../../arm-zephyr-eabi/bin/ld.bfd: warning: orphan section `DMA_RAM21' from `zephyr/drivers/pwm/libdrivers__pwm.a(pwm_nrfx.c.obj)' being placed in section `DMA_RAM21'

[ghost]

Could you remove this commit from #78095 then? I assume it is verbatim the same that I reviewed there?

[dottspina]

The build failure seems to be unrelated to this change.

I think so too: if this PR were to fail by itself, it would do so in some "Devicetree script tests", since it only adds unit tests to python-devicetree (which are not supposed to fail).

[dottspina]

Could you remove this commit from #78095 then? I assume it is verbatim the same that I reviewed there?

No, it's not. This PR adds only the unit tests that the current implementation of edtlib.Binding will pass, which is a small part (unfortunately).

The other tests will be added as we address the issues raised incidentally while working on #78095:

• ignored or broken property filters when A includes B includes C (see edtlib: indirect "include:"s break property filters #79948)
• failing to overwrite property descriptions in certain circumstances (also relates to properties inheritance, fixing the above issue should also help with this one)
• failing to overwrite binding descriptions in certain circumstances (a different issue, does not relate to properties)
• is diamond inheritance allowed ? if so, what should happen (e.g. what you get may depend on the order in which files are included) ?

The rationale is to merge (and run) unit tests as soon as we can to avoid regressions as we triage/fix the above issues.

[Edit: then we may rebase #78095 onto all this, and rethink the PropertySpec.path thing that brought us there ;-]

[ghost]

Could you remove this commit from #78095 then? I assume it is verbatim the same that I reviewed there?

No, it's not. This PR adds only the unit tests that the current implementation of edtlib.Binding will pass, which is a small part (unfortunately).

The other tests will be added as we address the issues raised incidentally while working on #78095:

• ignored or broken property filters when A includes B includes C (see edtlib: indirect "include:"s break property filters #79948)
• failing to overwrite property descriptions in certain circumstances (also relates to properties inheritance, fixing the above issue should also help with this one)
• failing to overwrite binding descriptions in certain circumstances (a different issue, does not relate to properties)
• is diamond inheritance allowed ? if so, what should happen (e.g. what you get may depend on the order in which files are included) ?

The rationale is to merge (and run) unit tests as soon as we can to avoid regressions as we triage/fix the above issues.

[Edit: then we may rebase #78095 onto all this, and rethink the PropertySpec.path thing that brought us there ;-]

Ok, good. IIRC you wanted to add separate issues for those, right? If you like you could reference PRs with the corresponding failing tests to those issues. I'll try to address those issues as quickly as possible then once we agreed in arch WG what the general direction will be for configuration.

[dottspina]

I think commit eb45da5 has since fixed the unrelated error reported by the CI.

Ok, good. IIRC you wanted to add separate issues for those, right? If you like you could reference PRs with the corresponding failing tests to those issues. I'll try to address those issues as quickly as possible then once we agreed in arch WG what the general direction will be for configuration.

@fgrandel , I pushed a PR reverting #65221, which seems to address all known issues with property filters (#79948): could you please take a look at #80030 ?
Once it's merged, we can add the related unit tests here (before we merge this PR, rather than creating another one on purpose).

The only remaining concerns that I'm aware of are (with failing unit tests at hand):

• inability to overwrite descriptions (bindings as well as properties) in certain circumstances
• diamond inheritance (I1 and I2 include X, B includes I1 and I2)
• I think we can entirely forget all test cases involving the "last modified here" thing until we find a proper implementation for what edtlib: fix last modified semantic in included property specs #65221 tried to achieve

Thanks.

[benediktibk]

Confusing, at least for me with my limited brain capacity. But I'm referring with this to the include and inheritance system itself. The tests themselves are as clear as possible, considering the complexity of the tested system.

[ghost]

Confusing, at least for me with my limited brain capacity. But I'm referring with this to the include and inheritance system itself.

True. Includes is fine but the filters are not.

IMO another instance of how convoluted design generates more convoluted design. Had we made it easier to influence the schema below the parent node in more flexible and obvious ways, we'd probably never had the idea to first design an inappropriate schema just to make it appropriate down the inheritance line by filtering it. :-D

I don't think anyone will want to use filters once we have a proper child definition language in place. I'm quite positive that we'll be able to deprecate filtering down the line.

[ghost]

@dottspina Have you seen that there are failing tests? Guess you'll have to fix them (even though they seem be unrelated) before you get more reviews. ;-) Maybe a rebase helps?

[dottspina]

@dottspina Have you seen that there are failing tests? Guess you'll have to fix them (even though they seem be unrelated) before you get more reviews. ;-) Maybe a rebase helps?

I was thinking of perhaps waiting until #80030 is merged, to be able to add the unit tests for inheritance here (which would otherwise fail).
Of course, I'm fine with doing the other way around, I've rebased this (to trigger the CI, ), I'll add the tests where they fit afterwards (once this is merged).

@fgrandel , I'm afraid you'll have to review this again (even though the PR's content has not changed at all).
[Edit: you don't have to, the CI was smart enough to guess that]

Thanks.

[dottspina]

Confusing, at least for me with my limited brain capacity. But I'm referring with this to the include and inheritance system itself.

True. Includes is fine but the filters are not.

IMO another instance of how convoluted design generates more convoluted design. Had we made it easier to influence the schema below the parent node in more flexible and obvious ways, we'd probably never had the idea to first design an inappropriate schema just to make it appropriate down the inheritance line by filtering it. :-D

Just my two cents: yes, the include and inheritance system is, in a way, confusing by-design, I've felt it more than once too, but I'm not so sure the issue is specifically the use of property filters to describe child bindings.

AFAICT, the property-{allow,block}list: syntax was added to the include: mechanism to allow a device binding to filter the properties it chooses to inherit from a base binding (#29498). Or, paraphrasing #29306, to describe that a property should not exist in the DTS as it doesn't make sense for that specific compatible.
This seems like a legitimate use case to me (note that child bindings support was considered last there, not the initial rationale).

Let me give an example, based on st,stm32-temp-cal.yaml, where you read:

compatible: "st,stm32-temp-cal"

include:
  - name: st,stm32-temp-cal-common.yaml
    property-blocklist:
      - avgslope
      - ntc

Here, property-blocklist filters out, say, the base properties for this kind of device (specified in st,stm32-temp-cal-common.yaml) which this particular device (st,stm32-temp-cal) does not support.
Nothing shocks me so far, the syntax is used for what it's intended.

Same when this happens at the (grand) child binding level, e.g. in nxp,lpc-iocon-pinctrl.yaml (you'll find this structure for most pin configrations in pin groups):

child-binding:
  description: LPC IOCON pin controller pin group
  child-binding:
    description: |
      LPC IOCON pin controller pin configuration node

    include:
      - name: pincfg-node.yaml
        property-allowlist:
          - drive-open-drain
          - bias-pull-up
          - bias-pull-down
          - drive-push-pull
          - input-schmitt-enable

What troubles me is rather reading bindings like bellow (renesas,smartbond-lp-osc.yaml.yaml):

compatible: "renesas,smartbond-lp-osc"

include:
  - name: fixed-clock.yaml
    property-allowlist:
      - status
      - compatible
      - clock-frequency

Here I find confusing that we recursively pull all properties specified in fixed-clock.yaml (of which clock-frequency), in clock-controller.yaml, in base.yaml (of which status and compatible), and in pm.yaml, then eventually filter out everything but three of them:

• the intent is not as clear as it could be: we just wanted to say "this Smartbond low power oscillator is a device (with a status and possibly a compatible string) that provides a fixed-rate clock (the fixed frequency)"
• it's not obvious what we finally inherit and from where [Edit: this example is really simplistic]

If we were able to describe the binding as bellow, everything would be crystal clear:

compatible: "renesas,smartbond-lp-osc"

include:
  - name: device.yaml
  - name: fixed-clock.yaml

The above example is a bit simplistic, just to sketch out my thinking. Now take a look at this (nordic,owned-partitions.yaml):

compatible: "nordic,owned-partitions"

include:
  - name: nordic,owned-memory.yaml
    property-blocklist:
      - reg

child-binding:
  include:
    - name: base.yaml
      property-blocklist:
        - compatible
  properties:
    reg:
      required: true

Try to guess what this binding (compatible) is for without the human readable description:s (which I striped). What properties are allowed, actually supported (useful), or required, and at what level ;-).
Here the use of property filters can indeed seem more like some hack to get all we may need while preventing issues (e.g. possibly overwriting the expected binding if someone absentmindedly set a compatible: to the node in the DTS), than a valuable syntax to actually describe the binding: e.g. do we really intend to say that the child nodes support all properties specified in base.yaml (and therefore pm.yaml) but should not have a compatible string? I'm not sure.

Compared to, e.g.:

compatible: "nordic,owned-partitions"

include: nordic,partition-table.yaml

child-binding:
  include: memory-partition.yaml

To me, these examples don't show that property filters (at any level) are broken per se, but perhaps we're missing the finer-grained basic bindings that would allow us to write more consistent (and readable) device bindings.
Bindings had to be added to support new devices, and a perfect inheritance hierarchy may not always have been the priority (this is not a criticism, just history).

Another aspect of the include: system that I find confusing is the rules that (should) apply when the same property is inherited multiple times, either because the same base binding is included several times, or because different included files specify properties with the same name (both situations actually occur).
I even happened to write tests and try to deduce the expected rules from their results, which didn't always turn out to be consistent.

I don't think anyone will want to use filters once we have a proper child definition language in place. I'm quite positive that we'll be able to deprecate filtering down the line.

As previously mentioned, IMO property filters were introduced to be able to quickly add supported devices without having to first multiply base bindings with stricter separation of concerns, not as something specifically needed (or useful) to describe child bindings (child nodes).
So I'm not entirely convinced that a "proper child definition language" will "deprecate filtering down the line" ;-)

This is not to say that approaches like #79751 are not interesting (or even desirable), just that it does not seem to address the same use cases property-{allow,block}list were added for.

[dottspina]

Updated PR content:

• bindings' descriptions and compatible strings inheritance: add updated unit tests that pass with the current library behavior, documenting this behavior, rather than assuming a different behavior
• property filters when "B filters I includes X": add unit tests documenting the issue, but disabled by default
• Last modified semantic of PropertySpec.path: we decided to forget this issue until we know how to handle it properly, so do not include anything related in this PR

Inheritance of bindings' descriptions and compatible strings

Consider the binding file bellow:

description: Binding's description.

compatible: vnd,device

include:
  - name: base_first.yaml
  - name: base_last.yaml 

Where both base_first.yaml and base_last.yaml set a description: and a compatible: at all levels (binding, child-binding, grandchild-binding, etc).

The rules deduced from the edtlib library behavior are:

• the included bindings (base_first.yaml and base_last.yaml) can't overwrite the compatible: and description: set by the including binding, despite that description: and compatible: appear before include: in the binding file: it seems consistent that the including binding wins
• at the child binding levels (child-binding, grandchild-binding, etc), descriptions and compatible strings will depend on the order in which the binding files are successively included (named:ed): the included first wins
• the rule above would also apply at the binding level if the including binding did not itself contain description: and compatible: elements

With the same inheritance diagram, the included first wins rule also applies to property descriptions inheritance.

I previously assumed that in these later situations the included last would instead win, as long as overwriting descriptions and compatible strings is permitted, and corresponding unit tests were put aside because they were known to fail.
I've updated these tests to match the current behavior of the library: so now we can add (merge) them too.

Indirect property filters

This PR now also includes the unit tests which cover all issues documented in #79948.

These six unit tests are expected to fail until the issues are fixed (see #80030), and disabled by default, e.g.:

# Disable testing a known issue:
# - edtlib: indirect "include:"s break property filters
# - https://github.com/zephyrproject-rtos/zephyr/issues/79948
#
# See also:
# - https://github.com/zephyrproject-rtos/zephyr/pull/80030
TEST_ISSUE_79948: bool = os.environ.get("EDTLIB_TEST_ISSUE_79948") is not None

@pytest.mark.skipif(not TEST_ISSUE_79948, reason="Expected to fail (#79948).")
def test_filter_inherited_propspecs_basics() -> None:
    ...

Rationale:

• this permits to document these issues
• while not causing CI errors (when running the python-devicetree unit tests)
• later enabling these tests without causing errors should allow us to consider the related issues are fixed

Last modified semantic

In #65221 we tried to get more valuable answers from the PropertySpec.path API, which is supposed to tell in which binding file a property's specification was last modified.

This patch:

• failed to achieve its goal (edtlib: Preserve paths of properties from included child bindings #78095)
• introduced a regression when applying child-binding filters to indirectly inherited properties (edtlib: indirect "include:"s break property filters #79948)

We decided to forget all this until we find a proper approach: nothing here tests this last modified thing, and #80030 will also revert the related unit tests.

[mbolivar]

This looks great! Very thorough. I checked about 70% of these in detail and skimmed the rest, LGTM. From your comment in #79948, this needs to be rebased now since the tests that are expected to fail should be fixed, right?

[mbolivar]

nitpick: 'descriptions'

[mbolivar]

'expected' typo

[mbolivar]

#79948 is closed now -- I guess it's time to rebase and remove this? Or did I misunderstand that issue?

Copilot reviewed 18 out of 33 changed files in this pull request and generated no suggestions.

Files not reviewed (15)

• scripts/dts/python-devicetree/tests/test-bindings-init/invalid_child_proptype.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/invalid_grandchild_propdefault.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/base_inherit.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/base_multi.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/compat_desc.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/compat_desc_base.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/compat_desc_multi.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/filter_allows_notblocked.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/invalid_grandchild_propreq.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/base_amend.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/diamond.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/invalid_child_propconst.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/invalid_child_propdefault.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/invalid_grandchild_propenum.yaml: Evaluated as low risk
• scripts/dts/python-devicetree/tests/test-bindings-init/filter_among_allowed.yaml: Evaluated as low risk

[dottspina]

This looks great! Very thorough. I checked about 70% of these in detail and skimmed the rest, LGTM.

@mbolivar , thank you for your careful review.

From your comment in #79948, this needs to be rebased now since the tests that are expected to fail should be fixed, right?

Right, I removed all @pytest.mark.skipif(not TEST_ISSUE_79948) to let these tests run: they pass, as expected.

Note that to comply with the new ruff linter a few other changes were required:

• updating type hinting for Python 3.10+: with Python3.9 you could not subscript dict, and have to use typing.Dict, but starting with 3.10, IIRC, you shall subscript dict and typing.Dict is deprecated, Optional[T] is deprecated in favor of T | None (which I honestly find less intuitive depending on the context), etc
• rewriting Yoda conditions in assertions
• eventually run ruff format on test_edtlib_binding_init.py

So it seems there are a lot of changes, but it's actually only fixing the typo you reported, and removing the TEST_ISSUE_79948 things now that all tests should pass.

Thanks.

[mbolivar]

LGTM