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

Add option to build OTBR and CLI references separately, and add support for 1.3.x nrf52840 type fw #58

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sunytt
Copy link
Contributor

@sunytt sunytt commented Jun 14, 2023

This PR introduces the option REFERENCE_TYPE=(OTBR|CLI) so it's possible to build CLI reference release and OTBR(host+RCP) reference release separately. If this option is not provided all types of reference release will be built as before.

ot-br-posix commit id is added to the output path.

Added support for building 1.3.1 reference release with REFERENCE_PLATFORM=nrf52840. It can be used when building OTBR reference release so the same openthread commit is used to build RCP and host. (If we use ncs as platform then the openthread commit is hardcoded in sdk-nrf so it may differ with the host image)

@sunytt sunytt requested review from superwhd and librasungirl June 14, 2023 05:23
run: |
export PATH=/tmp/gcc-arm-none-eabi-9-2019-q4-major/bin:$PATH
REFERENCE_PLATFORM=nrf52840 REFERENCE_RELEASE_TYPE=1.3 ./script/make-reference-release.bash
REFERENCE_TYPE=CLI REFERENCE_PLATFORM=nrf52840 REFERENCE_RELEASE_TYPE=1.3 ./script/make-reference-release.bash
Copy link
Contributor

Choose a reason for hiding this comment

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

REFERENCE_TYPE and REFERENCE_RELEASE_TYPE may be ambiguous when put together. Shall we rename one of them? Note that renaming REFERENCE_RELEASE_TYPE may break backward compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about REFERENCE_TYPE -> REFERENCE_RELEASE_TARGET? or any better suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

REFERENCE_RELEASE_TARGET looks good to me. On the other hand I'm afraid in the end we have to rename REFERENCE_RELEASE_TYPE anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had an offline discussion with @superwhd , I'll rename REFERENCE_RELEASE_TYPE to REFERENCE_THREAD_VERSION to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rename REFERENCE_RELEASE_TYPE to REFERENCE_THREAD_VERSION in a separate PR to limit the scope of this PR.

@@ -292,6 +307,23 @@ build()
thread_version=1.2 build_ot "-DBOARD=brd4166a" "${options[@]}" "$@"
;;
esac
elif [ "${REFERENCE_RELEASE_TYPE}" = "1.3.1" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's still possible to merge these two if branches into one.

You already have the options for different platforms of the same version number like build_${version}_options_${platform}. Maybe you can build a string to refer to the variable name. I'm not super familiar with this shell syntax but it seems to work.

Just an example:

build=build_1_3_1
build_1_3_1_options="-DOT_VERSION=1.3.1"
options_var_name="$build"_options
options=${!options_var_name}

reference: https://stackoverflow.com/questions/8515411/what-is-indirect-expansion-what-does-var-mean

Copy link
Contributor

@superwhd superwhd left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sunytt sunytt force-pushed the refactor-build-options branch from 3df9d7c to 8033a37 Compare June 19, 2023 03:02
@sunytt sunytt requested review from jwhui and removed request for jwhui June 19, 2023 07:06
@sunytt sunytt force-pushed the refactor-build-options branch from 8033a37 to 80346af Compare June 21, 2023 06:56
script/make-firmware.bash Outdated Show resolved Hide resolved
@sunytt
Copy link
Contributor Author

sunytt commented Jun 26, 2023

Disabled building for ncs type reference release for now due to #61

@sunytt sunytt requested a review from jwhui June 26, 2023 06:02
@sunytt sunytt changed the title Adapt the script to build OTBR and CLI references separately Add option to build OTBR and CLI references separately, and add support for 1.3.x nrf52840 type fw Jul 4, 2023
@sunytt sunytt removed the request for review from jwhui July 17, 2023 06:37
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.

3 participants