Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

model: properties always appear to come from the top-level binding file #5

Closed
dottspina opened this issue Mar 13, 2024 · 3 comments
Closed

model: properties always appear to come from the top-level binding file #5

dottspina opened this issue Mar 13, 2024 · 3 comments
Labels
bug The software does not behave as expected or documented

Comments

@dottspina
Copy link
Owner

When using the cat command to show the specifications of node properties, they all appear to be defined (actually last modified) by the node bindings itself, not the YAML file we would expect: e.g. all properties of " nordic,nrf52-flash-controller" devices appear to be defined by nordic,nrf52-flash-controller.yaml, although we should also find flash-controller.yaml, base.yaml and pm.yaml.

An example is the wakeup-source property, which is defined in pm.yaml:

/
> cat &flash_controller$wakeup-source -Bl
           Name: wakeup-source                     
           From: nordic,nrf52-flash-controller.yaml
           Type: boolean                           
       Required: No                                
     Deprecated: No                                
           Enum: Not an enum                       
          Const: Not a const                       
        Default: No default value                  
Specifier Space: No specifier space

This is a known issue in Zephyr's Python devicetree library: edtlib: PropertySpec.path API might be incorrect or misleading #65135

A pull request has been submitted upstream: edtlib: fix last modified semantic in included property specs #65221

While this bug has absolutely no incidence on Zephyr's use of the library, this is an issue for DTSh which aims to help beginners understand Devicetree: a tool that says that the wakeup-source property is defined by the bindings for the "nordic,nrf52-flash-controller" compatible does not.

Since we already have to repackage Zephyr's Python devicetree library (#2, #5e803eb), should we consider applying the above patch ?
@dottspina dottspina added the bug The software does not behave as expected or documented label Mar 13, 2024
@dottspina dottspina mentioned this issue Mar 13, 2024
Merged
dottspina added a commit that referenced this issue Apr 12, 2024
@dottspina
ci: patch upstream python-devicetree library
8b999ab 
The PropertySec.path API in the python-devicetree library
wrongly tells all properties originate from the top-level
binding file, which causes the cat command to show
misleading and confusing paths (see issue #5).

We've proposed a patch upstream, but its review clearly is
not a priority (the underlying issue has no consequence on
Zephyr use of the library).

Applying this patch here:
- does actually fix the cat command, and does not otherwise break DTSh
- can't break Zephyr in any way (will still use its own unpatched
  version of the python-devicetree library, see issue #2)
@dottspina
Copy link
Owner Author

Fixed upstream, patch merged.

dottspina added a commit that referenced this issue Sep 6, 2024
@dottspina
edtlib: preserve the origin of properties from child-bindings
f7bed7a 
All child bindings are initialized at once after included
binding files have been merged, with the including binding
file set as their origin.

As a consequence, properties of child binding appear
to all come from ("last modified" semantic) the top-level file.

For example, properties of a channel child of an ADC controller
with compatible "nordic,nrf-saadc" will all appear last modified
by nordic,nrf-saadc.yaml instead of adc-controller.yaml.

We should use the same approach we've already come to
for property specifications:
- #5
- zephyrproject-rtos/zephyr#65221
dottspina added a commit that referenced this issue Sep 6, 2024
@dottspina
edtlib: preserve the origin of child-bindings definitions
c52ffdc 
All child bindings are initialized at once after included
binding files have been merged, with the top-level binding
file set as their origin.

As a consequence, properties of child bindings appear
to all come from ("last modified" semantic) the top-level file.

For example, properties of a channel child of an ADC controller
with compatible "nordic,nrf-saadc" will all appear last modified
by nordic,nrf-saadc.yaml instead of adc-controller.yaml.

We should use the same approach we've already come to
for property specifications:
- #5
- zephyrproject-rtos/zephyr#65221
dottspina added a commit that referenced this issue Sep 6, 2024
@dottspina
edtlib: preserve the origin of child-bindings definitions
6970a15 
All child bindings are initialized at once after included
binding files have been merged, with the top-level binding
file set as their origin.

As a consequence, properties of child bindings appear
to all come from ("last modified" semantic) the top-level file.

For example, properties of a channel child of an ADC controller
with compatible "nordic,nrf-saadc" will all appear last modified
by nordic,nrf-saadc.yaml instead of adc-controller.yaml.

We should use the same approach we've already come to
for property specifications:
- #5
- zephyrproject-rtos/zephyr#65221
dottspina added a commit that referenced this issue Sep 6, 2024
@dottspina
edtlib: preserve the origin of child-bindings definitions
0625b83 
All child bindings are initialized at once after included
binding files have been merged, with the top-level binding
file set as their origin.

As a consequence, properties of child bindings appear
to all come from ("last modified" semantic) the top-level file.

For example, properties of a channel child of an ADC controller
with compatible "nordic,nrf-saadc" will all appear last modified
by nordic,nrf-saadc.yaml instead of adc-controller.yaml.

We should use the same approach we've already come to
for property specifications:
- #5
- zephyrproject-rtos/zephyr#65221
dottspina added a commit that referenced this issue Sep 6, 2024
@dottspina
edtlib: preserve paths of properties from included child-bindings
9daa38a 
All child bindings are initialized at once after included
binding files have been merged, with the top-level binding
file set as their origin.

As a consequence, properties of child bindings appear
to all come from ("last modified" semantic) the top-level file.

For example, properties of a channel child of an ADC controller
with compatible "nordic,nrf-saadc" will all appear last modified
by nordic,nrf-saadc.yaml instead of adc-controller.yaml.

We should use the same approach we've already come to
for property specifications:
- #5
- zephyrproject-rtos/zephyr#65221
@dottspina
Copy link
Owner Author

The patch didn't work for child-bidings, and introduced regressions.
We decided to revert it upstream (edtlib: Revert a regression affecting property filters at the child-binding level and bellow #80030).

DTSh now implements a workaround, where it retrieves properties lineage a posteriori.
Starting from the "top-level" binding, down to the recursively included YAML files, and honoring filters, we assume:

  • the first binding file that adds some definition (e.g. required: true) to a property specification, is where the property was last modified
  • the file where we find a description for the property is where it's initially specified

I'm reopening this until our workaround proves correct.

@dottspina dottspina reopened this Dec 9, 2024
dottspina added a commit that referenced this issue Dec 9, 2024
@dottspina
utils: provide simple properties lineage from YAMLFilesystem
e7169c2 
Starting from a given "top-level" YAML file,
down to the recursively included files we assume:
- the first binding file that adds some definition
  (e.g. "required: true") to a property with the given name,
  is where the property was last modified
- the first file (should it be the last ?) where we find
  a description for the property is where it's first specified

This is a workaround for issue #5.

The principle can be extended to the entire spcifications
lineage (no only what we assume are the ends) if no issue arise.
@dottspina
Copy link
Owner Author

Closing this: the workaround has been released with DTSh v0.2.4.
Incorrect properties lineage would be a different issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug The software does not behave as expected or documented
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dottspina