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 support for building on Windows and specifying custom boards #72

Merged
merged 6 commits into from
Jul 15, 2024

Conversation

felipejfc
Copy link
Contributor

(Draft PR for us to discuss the changes)

Adjusted paths so that the building will work on Windows/powershell. Also changed the default ncs version to 2.6.1 and fixed Zigbee with changes related to it.
I've included BOARD_ROOT pointing to workdir on the generated CMakeLists so that one can use custom boards by creating a boards dir alongside zigbee.yaml file, I can create example related to it but it's a great way for us to achieve broader compatibility and adoption. I used this capability it to compile the binary to a nice!nano board and it worked great.

Adjusted paths so that building will work on windows/powershell. Also changed default ncs version to 2.6.1 and fixed zigbee with changes related to it.
We're also setting BOARD_ROOT to workdir so that one can use custom board definitions
@felipejfc
Copy link
Contributor Author

The explanation for filepath.Dir to path.Dir in some parts of template can be found in this issue golang/go#44305 (comment)

@ffenix113
Copy link
Owner

ffenix113 commented Jul 13, 2024

Hello @felipejfc,

Thank you for the PR! I looked through it and it does look good to me.
It is interesting that you didn't need to edit this function, as it provides some environment variables as well.

I will do some tests locally as well and post comments, if any, but generally have no objections for this PR.

BOARD_ROOT change is especially nice, as I also have a custom board to develop for, so this will help!

config/device.go Outdated Show resolved Hide resolved
config/device.go Outdated Show resolved Hide resolved
@felipejfc
Copy link
Contributor Author

It is interesting that you didn't need to edit this function, as it provides some environment variables as well.

Thank you very much for reviewing!
Regarding this:

It is interesting that you didn't need to edit this function, as it provides some environment variables as well.

The reason I didn't have problems is probably that whenever I'm running zigbee_home binary, I'm doing it from inside a nrf shell environment with: nrfutil toolchain-manager launch --shell
It would probably be better if we also fix this method for windows so that running from inside the shell is not a requirement. But I'm not sure what the best approach would be, maybe make zb home source all env vars on the file $NCS_TOOLCHAIN_BASE/toolchains/$SELECTED_TOOLCHAIN/environment.json? What do you think?

felipejfc and others added 2 commits July 13, 2024 21:06
Co-authored-by: Yevhen Sylenko <1863154+ffenix113@users.noreply.github.com>
@felipejfc
Copy link
Contributor Author

Update: pushed a new commit fixing the file you pointed out and some more path separators. Now I can compile a firmware successfully even directly in powershell without needing to enter an ncs terminal first

@ffenix113
Copy link
Owner

Could you please remove this, and I am ready to approve & merge this, unless you have anything else to discuss.

Great work!

@felipejfc
Copy link
Contributor Author

Could you please remove this, and I am ready to approve & merge this, unless you have anything else to discuss.

Great work!

done!

Copy link
Owner

@ffenix113 ffenix113 left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@ffenix113 ffenix113 merged commit 76333d0 into ffenix113:develop Jul 15, 2024
1 check passed
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.

2 participants