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.

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

Devicetree: edtlib: fix possible AttributeError in Property.description #51139

Merged

Conversation

dottspina
Copy link
Contributor

@dottspina dottspina commented Oct 10, 2022

Attempting to access the Python property Property.description when Property.spec.description is None would raise:

AttributeError: 'NoneType' object has no attribute 'strip'.

Known DT properties that may not have a description (Property.spec.description is None):

  • compatible for nodes such as /, /soc, /soc/timer@e000e010, /leds or /pwmleds
  • reg for nodes such as /soc/timer@e000e010
  • status for nodes such as /soc/timer@e000e010
  • gpios for nodes such as /leds/led_0 or /buttons/button_0
  • pwms for nodes such as /pwmleds/pwm_led_0

This patch checks the PropertySpec.description attribute before calling strip(): will return None, and not raise AttributeError.

NOTE: I'm not aware of any legit edtlib use (e.g. by scripts in zephyr/scripts) that may trigger this issue, in particular I've never faced this AttributeError when building a Zephyr application. My use-case is slightly different (a shell-like interface to a devicetree and its bindings, dtsh)

Thanks.

--
chris

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your fix!

We do not accept PRs with merge commit, though -- please fix your branch so it doesn't have the merge commit, and has a linear history of one commit on top of something in upstream instead.

https://www.youtube.com/watch?v=mnVNLM7ynMQ

Patch itself looks good, thanks

@mbolivar-nordic
Copy link
Contributor

BTW, dtsh looks amazing! Please consider me officially interested in getting support for this into upstream Zephyr when you feel it has matured enough :). west dtsh, anyone?

cc @gmarull

@dottspina dottspina force-pushed the pr-edtlib-property-desc branch from dcdca66 to f47a3cd Compare October 12, 2022 06:59
@dottspina
Copy link
Contributor Author

dottspina commented Oct 12, 2022

@mbolivar-nordic Thanks for the review, and the link: I also prefer a linear git history, but wasn't sure of your policy regarding force push, now I know. I should have red the contribution guidelines more closely, though. Sorry for this.

[Edit: west dtsh: I had the same thought, and have started to read the west commands source code ... but for now I use a shell script that first runs west, then dtsh ;-) ]

@dottspina dottspina force-pushed the pr-edtlib-property-desc branch from 9384b43 to 4a611e0 Compare October 12, 2022 07:49
Attempting to access the property Property.description
when Property.spec.description is None would raise
AttributeError: 'NoneType' object has no attribute 'strip'.

Known properties that may not have a description
(Property.spec.description is None):
- 'compatible' for nodes such as / /soc /soc/timer@e000e010 /leds /pwmleds
- 'reg'        for nodes such as /soc/timer@e000e010
- 'status'     for nodes such as /soc/timer@e000e010
- 'gpios'      for nodes such as /leds/led_0 /buttons/button_0
- 'pwms'       for nodes such as /pwmleds/pwm_led_0

This patch checks the PropertySpec.description attribute before calling
strip(): will return None, and not raise AttributeError.

Signed-off-by: Chris Duf <chris@openmarl.org>

Co-authored-by: Gerard Marull-Paretas <gerard@teslabs.com>
@dottspina dottspina force-pushed the pr-edtlib-property-desc branch from 4a611e0 to 42d321d Compare October 12, 2022 07:53
@mbolivar-nordic
Copy link
Contributor

have started to read the west commands source code

You may find this easier than just heading straight to the source code: https://docs.zephyrproject.org/latest/develop/west/extensions.html#adding-a-west-extension

@dottspina
Copy link
Contributor Author

You may find this easier than just heading straight to the source code: https://docs.zephyrproject.org/latest/develop/west/extensions.html#adding-a-west-extension

Yes, sure, thanks. OTOH, considering my weak Python-Fu, reading some good source code won't hurt ;-)

@stephanosio stephanosio merged commit 230c80e into zephyrproject-rtos:main Oct 13, 2022
@dottspina dottspina deleted the pr-edtlib-property-desc branch October 13, 2022 17:38
@dottspina
Copy link
Contributor Author

BTW, dtsh looks amazing! Please consider me officially interested in getting support for this into upstream Zephyr when you feel it has matured enough :). west dtsh, anyone?

@mbolivar-nordic, thanks for the kind words.

dtsh is slowly transitioning to beta: I plan to add something to show and query the SoC memory layout before the feature freeze for v0.1.0, and then spend some time on code review (as far as I can review Python code) and improving the unit tests coverage.

At this point (or even before), I'd like to have as many bug reports as possible, for as many different configurations as possible: is it appropriate to post an humble announce to main@lists.zephyrproject.org or devel@lists.zephyrproject.org ?

Also at this point, I may open a draft PR to start working on the west dtsh thing: is this the expected workflow to permit early comments without worrying Zephyr maintainers with unwanted notifications ?

Thanks.

--
chris

@mbolivar-nordic
Copy link
Contributor

At this point (or even before), I'd like to have as many bug reports as possible, for as many different configurations as possible: is it appropriate to post an humble announce to main@lists.zephyrproject.org or devel@lists.zephyrproject.org ?

Yep, of course! I would recommend sending email to devel and cc'ing users.

Also at this point, I may open a draft PR to start working on the west dtsh thing: is this the expected workflow to permit early comments without worrying Zephyr maintainers with unwanted notifications ?

Yep, a draft PR is fine.

Here is a (short!) talk which gives an overview of the upstreaming process: https://www.youtube.com/watch?v=mnVNLM7ynMQ

@dottspina
Copy link
Contributor Author

BTW, dtsh looks amazing! Please consider me officially interested in getting support for this into upstream Zephyr when you feel it has matured enough :). west dtsh, anyone?

@mbolivar-nordic , sorry for the delay, I've been overwhelmed at work, then I needed some rest a little farther from keyboards. And apart from a couple updates, mostly to cope with a few edtlib API changes, dtsh did not really mature, until I eventually start to work on the west dtsh RFC.

I've updated design and implementation with some lessons learnt from the prototype, and tried to come to a sizeable PR that still implements enough to prove tasteful or useful: #59863.
Let me known if/when I can drop the Draft flag.

BTW:

  • I've noticed edtlib has gradually added more and more Python type hinting annotations, looks like we all prefer when things are somewhat statically typed
  • is it fine to contact you at your nordicsemi email address, when useful, to avoid spamming PR, issues, etc, with unrelated content ?

Kind regards,
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants