-
Notifications
You must be signed in to change notification settings - Fork 8.4k
boards: beagle: beagleconnect_freedom: Add rev C5 support #99489
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
base: main
Are you sure you want to change the base?
Conversation
| if("${BOARD_QUALIFIERS}" MATCHES "/cc1352p7$" AND "${BOARD_REVISION}" STREQUAL "C5") | ||
| set(ACTIVE_BOARD_REVISION "C7") | ||
| message(WARNING "C5 only supports CC1352P. Using C7 instead.") | ||
| endif() | ||
|
|
||
| if("${BOARD_QUALIFIERS}" MATCHES "/cc1352p$" AND "${BOARD_REVISION}" STREQUAL "C7") | ||
| set(ACTIVE_BOARD_REVISION "C5") | ||
| message(WARNING "C7 only supports CC1352P7. Using C5 instead.") | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should check from the first character until either the end of the line or until a /, this code prevents users extending a board out of tree, and it should also not be changing the revision, if there is no revision provided then there can be a default set but do not override what the user has put, throw an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, throwing the error breaks CI since there was no way I could find to tie an soc to a specific revision.
It was discussed here: #95065
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses the twister files you have, if there is no .yaml for a board target then twister will not try to use it, and you need to have the revisions in the .yaml files for the twister board name. You should probably not set a default revision anyway since it isn't valid for one of the SoCs, people should provide the correct revision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses the twister files you have, if there is no .yaml for a board target then twister will not try to use it, and you need to have the revisions in the .yaml files for the twister board name. You should probably not set a default revision anyway since it isn't valid for one of the SoCs, people should provide the correct revision
Do you mean this file boards/beagle/beagleconnect_freedom/beagleconnect_freedom_cc1352p7.yaml? It can have a revisions field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, rename and change like so: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/nordic/nrf9160dk/nrf9160dk_nrf9160_0_7_0.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nordicjm I think I might be missing something. I cannot seem to use the names with the revision in platform_allow or exclude lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to have the twister .yaml files, it uses the names exactly as they are from those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nordicjm Can you check the PR now. It seems like I can add variants to board.yml.
Also, you also mentioned the following:
this code prevents users extending a board out of tree
Do you mean I should remove the check that the provided revision is either C5 or C7? Or is that fine?
1db8495 to
8c8ee9e
Compare
05ff41d to
788135b
Compare
| variants: | ||
| - name: beagleconnect_freedom@C5 | ||
| qualifier: "cc1352p" | ||
| - name: beagleconnect_freedom@C7 | ||
| qualifier: "cc1352p7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err this is not what variants are used for no
| variants: | |
| - name: beagleconnect_freedom@C5 | |
| qualifier: "cc1352p" | |
| - name: beagleconnect_freedom@C7 | |
| qualifier: "cc1352p7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I have tried adding a twister.yaml file. However, twister does still try building for wrong variants. So not sure what I am doing incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No not a twister.yaml file, twister yaml files, so beagleconnect_freedom_cc1352p_C5.yaml with the specifiers and details for that then beagleconnect_freedom_cc1352p7_C7 with the specifiers and details for that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to twister internals, *.yaml other than twister.yaml are categorized as legecy files:
| file for file in board_dir.glob("*.yaml") if file.name != "twister.yaml" |
I am pretty sure I tried what you are mentioning here, and it did not work, but let me push that again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PerMac if you cannot fit the behaviour here into twister.yaml files then please remove the comment about multiple twister files being legacy and work towards that because twister.yaml files would then absolutely not fit what is required.
@Ayush1325 use multiple twister files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I stand corrected. It works. Will create a PR to do the same with PocketBeagle 2 revisions.
| revision: | ||
| format: custom | ||
| exact: true | ||
| default: "C7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| default: "C7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| # If BOARD_REVISION not set, use the default revision | ||
| if(NOT DEFINED BOARD_REVISION) | ||
| set(BOARD_REVISION ${LIST_BOARD_REVISION_DEFAULT}) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # If BOARD_REVISION not set, use the default revision | |
| if(NOT DEFINED BOARD_REVISION) | |
| set(BOARD_REVISION ${LIST_BOARD_REVISION_DEFAULT}) | |
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| endif() | ||
|
|
||
| if("${BOARD_QUALIFIERS}" MATCHES "^/cc1352p7$" AND "${BOARD_REVISION}" STREQUAL "C5") | ||
| message(FATAL_ERROR "C5 only supports CC1352P. Using C7 instead.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*use, as for below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
4c5c8d2 to
385bb6b
Compare
|
I don't see any updates to the documentation. Is there any change in building? Can I still do Are you sure the terminology is "CC1352P" and not "CC1352P1"? I know ti.com seems to have it under "cc1352p", but that seems to be before they introduced the others. Looking around for a family overview largely comes up empty, but I did find a hardware architecture section in the SDK docs:
...
I don't see in either of those sections any use of the CC1352P without the CC1352P1. |
Well, TI certainly seems to be only using CC1352P and CC1352P7 in most of the public docs. Additionally, zephyr already has the soc added as CC1352P (and CC1352R): https://github.com/zephyrproject-rtos/zephyr/tree/main/soc/ti/simplelink. So any name change now would involve tree-wide changes to drivers launchxl board.
The product page listss only CC1352P: https://www.ti.com/product/CC1352P. Also in alternatives, it lists CC1352P7, CC1352R (not R1) and CC1312R. |
No, you cannot do |
don't you want to set a default revision though? |
Well, @nordicjm suggested to not have a default revision since the SoCs are different here: #99489 (comment) It would be nice to have C7 as the default, since C5 only existed as prototypes from what I understand. |
There seem to be a decent number of C5 boards around. So add support for it while keeping C7 as the default version. The main difference between C7 and C5 is that C5 uses CC1352P instead of CC1352P7 Signed-off-by: Ayush Singh <ayush@beagleboard.org>
|



There seem to be a decent number of C5 boards around. So add support for it while keeping C7 as the default version.
The main difference between C7 and C5 is that C5 uses CC1352P instead of CC1352P7