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

Simplify telink build #9639

Merged
merged 3 commits into from
Sep 13, 2021
Merged

Conversation

andy31415
Copy link
Contributor

Problem

Previous vscode images were missing required build environment variables so build script hardcoded zephyr SDK paths

Change overview

After #9535 we can use environment variables.

Testing

Local build, CI build.

Latest vscode image contains TELINK_ZEPHYR_SDK_DIR, no need
to hardcode the path to it in the build script anymore.
Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

Can you please add this as an argument to the script not as an environment variable?

Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

Can you please add this as an argument to the script not as an environment variable?

@andy31415
Copy link
Contributor Author

Can you please add this as an argument to the script not as an environment variable?

@mspang - I do not think I can - this is zephyr build environment which is cmake based. I am just following what the rest of the build scrips do (or at least used to do).

@mspang
Copy link
Contributor

mspang commented Sep 13, 2021

Can you please add this as an argument to the script not as an environment variable?

@mspang - I do not think I can - this is zephyr build environment which is cmake based. I am just following what the rest of the build scrips do (or at least used to do).

west uses ZEPHYR_BASE not TELINK_ZEPHYR_BASE. I'd rather not add new environment variables. These environment variables will be set in builds other than the telink build which we don't want.

@andy31415
Copy link
Contributor Author

I believe to really fix this, we would need to make all platforms the same. There is an imbalance here: nrf was the first platform and took over setting ZEPHYR_BASE and similar environment variables globally.

Telink is the 2nd zephyr platform, so it uses separate env variables. It seems we want to make nrf not set globals, however that seems to be a larger change.

@mspang
Copy link
Contributor

mspang commented Sep 13, 2021

I believe to really fix this, we would need to make all platforms the same. There is an imbalance here: nrf was the first platform and took over setting ZEPHYR_BASE and similar environment variables globally.

Telink is the 2nd zephyr platform, so it uses separate env variables. It seems we want to make nrf not set globals, however that seems to be a larger change.

There could be --nrfconnect-sdk and --telink-zephyr-sdk options to the build script (and eventually --[upstream-]zephyr-sdk ) ?

Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

Approved given that the environment variables seem to be preexisting issues and this is cleaning it up some.

@github-actions
Copy link

Size increase report for "esp32-example-build" from 92d3a28

File Section File VM
chip-bridge-app.elf .flash.text 85 72
chip-ipv6only-app.elf .flash.text 172 172
chip-shell.elf .flash.text 48 48
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
.flash.text,72,85
[Unmapped],0,-85

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize
[Unmapped],0,3924
.flash.text,172,172

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.flash.text,48,48
[Unmapped],0,-48


@andy31415 andy31415 merged commit 0ca8cbd into project-chip:master Sep 13, 2021
@andy31415 andy31415 deleted the simplify_telink_build branch October 28, 2021 14:03
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.

4 participants