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

scripts: dts: Format multi-line comments nicely #20185

Merged
merged 1 commit into from
Oct 30, 2019

Conversation

ulfalizer
Copy link
Collaborator

@ulfalizer ulfalizer commented Oct 28, 2019

Multi-line comments were stuck as-is between /* and */ in the generated
header, which looks ugly and confusing e.g. for multi-line
binding/property descriptions.

Use this format for multi-line comments in the header instead:

/*
 * First line
 * Second line
 *
 * Line after blank line
 */

Also clean up the output a bit by turning some things that were separate
comments into a single multi-line comment. Add some air and reduce line
lengths too.

Before:

/*  Generated by gen_defines.py  */
/*  DTS input file: hifive1.dts.pre.tmp  */
/*  Directories with bindings: $ZEPHYR_BASE/dts/bindings  */

/*  Devicetree node: /cpus/cpu@0/interrupt-controller  */
/*  Binding (compatible = riscv,cpu-intc): $ZEPHYR_BASE/... */
/*  Binding description: This binding describes the RISC-V ...

Some extra lines
for testing  */
#define DT_INST_0_RISCV_CPU_INTC                    1

After:

/*
 * Generated by gen_defines.py
 *
 * DTS input file:
 *   hifive1.dts.pre.tmp
 *
 * Directories with bindings:
 *   $ZEPHYR_BASE/dts/bindings
 */

/*
 * Devicetree node:
 *   /cpus/cpu@0/interrupt-controller
 *
 * Binding (compatible = riscv,cpu-intc):
 *   $ZEPHYR_BASE/dts/bindings/interrupt-controller/...
 *
 * Description:
 *   This binding describes the RISC-V CPU Interrupt Controller
 *
 *   Some extra lines
 *   for testing
 */
#define DT_INST_0_RISCV_CPU_INTC                    1

Also tweak Node.description and Property.description in edtlib to be
strip()ed instead of rstrip()ed. There's probably no reason to
preserving leading whitespace in them either.

@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Oct 28, 2019

This is why I keep nagging about the "...provides a base representation of..." boilerplate btw:

 *
 * Description:
 *   This binding describes the RISC-V CPU Interrupt Controller
 *

It would be nicer if it just said "RISC-V CPU Interrupt Controller" there (described the device without mentioning the binding).

Multi-line comments were stuck as-is between /* and */ in the generated
header, which looks ugly and confusing e.g. for multi-line
binding/property descriptions.

Use this format for multi-line comments in the header instead:

    /*
     * First line
     * Second line
     *
     * Line after blank line
     */

Also clean up the output a bit by turning some things that were separate
comments into a single multi-line comment. Add some air and reduce line
lengths too.

Before:

    /*  Generated by gen_defines.py  */
    /*  DTS input file: hifive1.dts.pre.tmp  */
    /*  Directories with bindings: $ZEPHYR_BASE/dts/bindings  */

    /*  Devicetree node: /cpus/cpu@0/interrupt-controller  */
    /*  Binding (compatible = riscv,cpu-intc): $ZEPHYR_BASE/... */
    /*  Binding description: This binding describes the RISC-V ...

    Some extra lines
    for testing  */
    #define DT_INST_0_RISCV_CPU_INTC                    1

After:

    /*
     * Generated by gen_defines.py
     *
     * DTS input file:
     *   hifive1.dts.pre.tmp
     *
     * Directories with bindings:
     *   $ZEPHYR_BASE/dts/bindings
     */

    /*
     * Devicetree node:
     *   /cpus/cpu@0/interrupt-controller
     *
     * Binding (compatible = riscv,cpu-intc):
     *   $ZEPHYR_BASE/dts/bindings/interrupt-controller/...
     *
     * Description:
     *   This binding describes the RISC-V CPU Interrupt Controller
     *
     *   Some extra lines
     *   for testing
     */
    #define DT_INST_0_RISCV_CPU_INTC                    1

Also tweak Node.description and Property.description in edtlib to be
strip()ed instead of rstrip()ed. There's probably no reason to
preserving leading whitespace in them either.

Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

For:

  dpd-wakeup-sequence:
    type: array
    required: false
    description: >
      Specifies wakeup durations for devices without RDPD.

      Some devices (Macronix MX25R in particular) wake from deep power
      down by a timed sequence of CSn assertion rather than the RDPD
      command.  This property specifies three durations measured in
      nanoseconds: tDPDD (Delay Time for Release from Deep Power-Down
      Mode), tCDRP (CSn Toggling Time before Release from Deep
      Power-Down Mode), antRDP (Recovery Time for Release from Deep
      Power-Down Mode).

      Absence of this property indicates that the RDPD command should be
      used to wake the chip from Deep Power-Down mode.

I would previously get:

/*  Specifies wakeup durations for devices without RDPD.
Some devices (Macronix MX25R in particular) wake from deep power down by a timed sequence of CSn assertion rather than the RDPD command.  This property specifies three durations measured in nanoseconds: tDPDD (Delay Time for Release from Deep Power-Down Mode), tCDRP (CSn Toggling Time before Release from Deep Power-D
own Mode), antRDP (Recovery Time for Release from Deep Power-Down Mode).
Absence of this property indicates that the RDPD command should be used to wake the chip from Deep Power-Down mode.  */

I now get:

/*
 * Specifies wakeup durations for devices without RDPD.
 * Some devices (Macronix MX25R in particular) wake from deep power down by a timed sequence of CSn assertion rather than the RDPD command.  This property specifies three durations measured in nanoseconds: tDPDD (Delay Time for Release from Deep Power-Down Mode), tCDRP (CSn Toggling Time before Release from Deep Power-Down Mode), antRDP (Recovery Time for Release from Deep Power-Down Mode).
 * Absence of this property indicates that the RDPD command should be used to wake the chip from Deep Power-Down mode.
 */

If we're going for readability I'd rather see this, with some reasonable wrap length like 78 characters.

/*
 * Specifies wakeup durations for devices without RDPD.
 *
 * Some devices (Macronix MX25R in particular) wake from deep power
 * down by a timed sequence of CSn assertion rather than the RDPD
 * command.  This property specifies three durations measured in
 * nanoseconds: tDPDD (Delay Time for Release from Deep Power-Down
 * Mode), tCDRP (CSn Toggling Time before Release from Deep Power-Down
 * Mode), antRDP (Recovery Time for Release from Deep Power-Down
 * Mode).
 *
 * Absence of this property indicates that the RDPD command should be
 * used to wake the chip from Deep Power-Down mode.
 */

@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Oct 28, 2019

@pabigot
What does the .yaml file look like? That second one is what I intended.

Oh... nm, you pasted it there. Does changing > to | change anything?

I'll experiment a bit.

@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Oct 28, 2019

Ah, yeah, it's because > removes newlines. | fixes it.

See the documentation updates in this PR as well. I'll go through all the bindings and change > to |.

This is why consistency is overrated. Better to slightly improve everything after you copy it. Then other people will copy the improved version around in turn. ;)

@pabigot
Copy link
Collaborator

pabigot commented Oct 28, 2019

@pabigot
What does the .yaml file look like?

I provided it at the top of my comment.

That second one is what I intended.

OK, but I think it's unreadable having one line of unbounded length per paragraph. I want to see the third option where paragraphs are separated and wrap at a reasonable length.

However, it appears I can get that by using the literal style, as you just noted too.

I'd intended to propose that you format each line as a paragraph, but that would break the literal style. So I think this is fine as is---and I'm not really in favor of you doing a global change of > with | in everybody's binding file. I'll use it in the cases where I think it's important.

ulfalizer added a commit to ulfalizer/zephyr that referenced this pull request Oct 28, 2019
With zephyrproject-rtos#20185, multi-line
descriptions will be formatted nicely, but using '>' breaks it, because
it removes internal newlines (including between paragraphs).

See https://yaml-multiline.info/.

Replace 'description: >' with 'description: |' to encourage '|'. That'll
prevent '>' from getting copied around and messing up long descriptions.

This will lead to some extra newlines in the output, but it's fine.
Line-wrapping messes up any manual formatting.

The replacement was done with

    $ git ls-files 'dts/bindings/*.yaml' | \
          xargs sed -i 's/description:\s*>/description: |/'

Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
galak pushed a commit that referenced this pull request Oct 30, 2019
With #20185, multi-line
descriptions will be formatted nicely, but using '>' breaks it, because
it removes internal newlines (including between paragraphs).

See https://yaml-multiline.info/.

Replace 'description: >' with 'description: |' to encourage '|'. That'll
prevent '>' from getting copied around and messing up long descriptions.

This will lead to some extra newlines in the output, but it's fine.
Line-wrapping messes up any manual formatting.

The replacement was done with

    $ git ls-files 'dts/bindings/*.yaml' | \
          xargs sed -i 's/description:\s*>/description: |/'

Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
@galak galak merged commit f3f88a8 into zephyrproject-rtos:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants