zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.17k stars 6.23k forks source link

board revisions: It should be possible to use lower cap for board revision #33921

Open erwango opened 3 years ago

erwango commented 3 years ago

Is your enhancement proposal related to a problem? Please describe. When defining board revisions using letters, only capital letters could be used to define revisions (for instance stm32f3_disco@E). This is not consistent with Zephyr coding style where lower caps is the default rule

Describe the solution you'd like Allow to use lower caps for board revisions (stm32f3_disco@e)

tejlmand commented 3 years ago

Thanks for this. ~I would recommend that stm32f3_disco@e be treated equally to stm32f3_disco@E.~

but let me me know if you think this should be defined by the board implementer, so that if revision is a b c d e f ...then stm32f3_disco@E will fail, and similar for A B C D E F ... where stm32f3_disco@e would be considered invalid revision.

Update: based on further discussions and because board names are case-sensitive and usually lower case only, I suggest we only allow @<lower case letter>, such as @a, @b, ...

Users can still make a custom revision handling, as described: https://docs.zephyrproject.org/latest/guides/porting/board_porting.html#custom-revision-cmake-files

erwango commented 3 years ago

I would recommend that stm32f3_disco@e be treated equally to stm32f3_disco@E.

I think it would be fine as well.

pabigot commented 3 years ago

Is the board name case-sensitive?

Is the version (variant) considered to be part of the board name from a user's perspective?

The answers should inform the decision of how to handle this. (I suspect the best approach is that neither should be case-sensitive, and that making that reality will require fixes to more than just the version fragment, including normalization of the user input.)

tejlmand commented 3 years ago

@pabigot Thanks for you comments.

Is the board name case-sensitive?

It is.

Is the version (variant) considered to be part of the board name from a user's perspective?

Besides the fact that a board maintainer may decide to default to a specific variant when no variant is provided, then I think the variant seen from user can be considered part of the board name.

The answers should inform the decision of how to handle this. (I suspect the best approach is that neither should be case-sensitive, and that making that reality will require fixes to more than just the version fragment, including normalization of the user input.)

Not only that. Today, the board provided by user goes to the CMakeCache along with an extra internal variable to ensure user cannot modify the BOARD / SHIELD variable. So would also require some more work there, as board nRF52840dk would need to be identical to nRf52840dk if user later rebuilds and changes upper to lower case letters.

Noticing that all boards today are using lower case letters only, then maybe the cleanest thing to do is to deprecate @E and instead only support @e, of course with a proper warning for 2 releases if E.conf/overlay is detected instead of e.conf/overlay.

Using pure lowercase will make it clearer to users that Kconfig fragments and DT overlays must also be named in lower case.

pabigot commented 3 years ago

Using pure lowercase will make it clearer to users that Kconfig fragments and DT overlays must also be named in lower case.

Is that true? If so, why?

Or is it that the names are case sensitive, and the (strongly) recommended practice is that lower case be used. (I can see a vendor wanting to use the correctly capitalized name of their product for branding purposes.)

erwango commented 3 years ago

Noticing that all boards today are using lower case letters only, then maybe the cleanest thing to do is to deprecate @E and instead only support @e, of course with a proper warning for 2 releases if E.conf/overlay is detected instead of e.conf/overlay.

The board was just added 2 weeks ago in current release. If the modification is done soon enough, I think it is "safe" to notify current user and just clean up the E.

tejlmand commented 3 years ago

Using pure lowercase will make it clearer to users that Kconfig fragments and DT overlays must also be named in lower case.

Is that true? If so, why?

Today, all boards are case-sensitive and Kconfig fragments / DT overlays must be named according to the board name and variant. So if a board is named: my_board, then the corresponding Kconfig fragment must be named: my_board.conf.

If we start normalizing board, shield and variant names, then what should be the name of the Kconfig fragment or DT overlay ?

Normalizing board name would mean -DBOARD=My_Board is equal to specifying -DBOARD=my_board but then what is the correct name for the fragment or overlay ? my_board.conf vs. My_Board.conf or both ?

When the name of the file must follow the name of the board, and the board is case-sensitive, then it's clear how to name the fragment or overlay, cause only one form is allowed when using -DBOARD=my_board is my_board.conf.

And thus, if board revision of type letter is always lower case, then a.conf, b.conf, and so on will be the only form, as board revision letter is also case sensitive.

I prefer lower case, because of the fact that all boards today are lower case, but users can still define My_Board in their own projects in which case the fragment must be name My_Board.conf. But the revision scheme letter would still be lower case, for example: My_Board@b.

Users who wants @B can still do so using: custom revision.cmake. This is only a question of what Zephyr revision letter form should do.

tejlmand commented 3 years ago

Noticing that all boards today are using lower case letters only, then maybe the cleanest thing to do is to deprecate @e and instead only support @e, of course with a proper warning for 2 releases if E.conf/overlay is detected instead of e.conf/overlay.

The board was just added 2 weeks ago in current release. If the modification is done soon enough, I think it is "safe" to notify current user and just clean up the E.

There could be downstream users who are using custom boards and letter revision, so therefore I think a proper warning is in place.

erwango commented 3 years ago

There could be downstream users who are using custom boards and letter revision, so therefore I think a proper warning is in place.

Ok, if not only for this particular one, this makes sense indeed.

pabigot commented 3 years ago

Today, all boards are case-sensitive and Kconfig fragments / DT overlays must be named according to the board name and variant.

But above you have (italics added):

Update: based on further discussions and because board names are case-insensitive and usually lower case only, I suggest we only allow @, such as @a, @b, ...

Which is it? I think it's that they're case sensitive.

I prefer lower case, because of the fact that all boards today are lower case, but users can still define My_Board in their own projects in which case the fragment must be name My_Board.conf. But the revision scheme letter would still be lower case, for example: My_Board@b.

Users who wants @B can still do so using: custom revision.cmake. This is only a question of what Zephyr revision letter form should do.

This is all complex, and there are distinctions being made that don't seem justified to me, so I'll leave my position as this:

The simplest and most flexible solution seems to be have all components of board identifiers be case sensitive. Then the only thing that has to change (if it does have to change) is whatever restriction on board versions forced use of E instead of e in #33851. Instead of adding a different arbitrary restriction that requires board revision letters to be lower case.

Then everything (including the file system objects that use these names) are consistently case-sensitive, and we can continue to use lower case as the preferred (but not mandated) spelling, without forcing vendors who chose to use upper case to do weird cmake overrides.

Unless, of course, all this is happening because cmake went with their own version identifier ordering that's case-insensitive, in which case I guess we get what we get and like it.

tejlmand commented 3 years ago

Which is it? I think it's that they're case sensitive.

Sorry, I meant case-sensitive. Also the example should make that clear as it stated:

I suggest we only allow @<lower case letter>, such as @a, @b, ..

i'm not sure why you use @A in your quote, as I didn't wrote that above in that sentence. I used @a <-- lower case.

The simplest and most flexible solution seems to be have all components of board identifiers be case sensitive.

Which is the case today and I'm arguing that we do not change that.

Zephyr offers two generic revision formats, numeric and letter, as described: https://docs.zephyrproject.org/latest/guides/porting/board_porting.html#multiple-board-revisions

Which can be used by calling board_check_revision()

FORMAT LETTER: matches single letter revisions from A to Z only FORMAT MAJOR.MINOR.PATCH: matches exactly three digits.

so the question basically is, should we change from:

FORMAT LETTER: matches single letter revisions from A to Z only to: FORMAT LETTER: matches single letter revisions from a to z only

or also allow @A to be used for @a for a limited time, as we are breaking existing naming scheme that could be used downstream.

pabigot commented 3 years ago

i'm not sure why you use @A in your quote, as I didn't wrote that above in that sentence. I used @a <-- lower case.

Because I didn't restore the single-quotes around those to make them code, so they became mentions. (at)a names a github user whose handle is A and (at)b names a github user whose handle is b.

so the question basically is, should we change from:

FORMAT LETTER: matches single letter revisions from A to Z only

to:

FORMAT LETTER: matches single letter revisions from a to z only

Why can't we change it to:

FORMAT LETTER: matches single letter revisions from A to Z or a to z only

tejlmand commented 3 years ago

Why can't we change it to:

FORMAT LETTER: matches single letter revisions from A to Z or a to z only

Before answering, so a user doing: board_check_revision(FORMAT LETTER) can do both: a.conf and A.conf, or how does he choose which format to be used ?

If <board>@a can match A.conf then they become case-insensitive, which I thought we agreed we didn't want.

The simplest and most flexible solution seems to be have all components of board identifiers be case sensitive.

Note, I wrote above:

but let me me know if you think this should be defined by the board implementer, so that if revision is a b c d e f ... then stm32f3_disco@E will fail, and similar for A B C D E F ... where stm32f3_disco@e would be considered invalid revision.

But unless there is a real wish for having the upper case version, I would suggest to just change the generic revision to lower case and keep things simple.

pabigot commented 3 years ago

Why can't we change it to:

FORMAT LETTER: matches single letter revisions from A to Z or a to z only

Before answering, so a user doing: board_check_revision(FORMAT LETTER) can do both: a.conf and A.conf, or how does he choose which format to be used ?

If <board>@a can match A.conf then they become case-insensitive, which I thought we agreed we didn't want.

I certainly don't want that, and only suggested support for it if everything related to board identification was to become case insensitive (normalized). If a.conf and A.conf exist, they're two different versions, and should co-exist as that. (That actually having both is probably a bad idea is true, but not something Zephyr should be enforcing.)

The simplest and most flexible solution seems to be have all components of board identifiers be case sensitive.

Note, I wrote above:

but let me me know if you think this should be defined by the board implementer, so that if revision is a b c d e f ... then stm32f3_disco@E will fail, and similar for A B C D E F ... where stm32f3_disco@e would be considered invalid revision.

I think I missed that because I don't get notifications when the comment changes, and the original comment content (still present in strikeout) suggested case-insensitive board versions. If there's a proposal to just keep case sensitive and not reject certain cases that'd be the minimal change and what I'd expect.

tejlmand commented 3 years ago

If a.conf and A.conf exist, they're two different versions, and should co-exist as that. (That actually having both is probably a bad idea is true, but not something Zephyr should be enforcing.)

I completely agree, having both is a bad idea. When it comes to variants, then Zephyr is not enforcing anything. board_check_revision() is simply a helper function which provides some common revision patterns that can be used for common cases, and from that view point I would prefer to keep just one of type when that helper function is used with LETTER so users don't mix up A.conf and a.conf by mistake.

I'm open to your suggestion but would like to hear more opinions.

I think I missed that because I don't get notifications when the comment changes, and the original comment content (still present in strikeout) suggested case-insensitive board versions.

Actually that sentence was present in the original comment. Only the strike-out was changed, and then Update: ..... part was added at bottom of comment to describe why the line was striked-out.

but good that your intentions are clear :+1: