zephyrproject-rtos / gsoc-2022-arduino-core

Arduino Core Zephyr Module (GSoC 2022 Project)
Apache License 2.0
43 stars 11 forks source link

Changes necessary for building samples on Windows. #74

Closed ddeletic closed 11 months ago

ddeletic commented 11 months ago

A minor modification to allow samples to build on Windows.

soburi commented 11 months ago

Zephyr not define DTC_OVERLAY_FILE in build system. So, we have no way to select overlay file to load in build system at this time. (This PR may became problem when this repository integrate to main line.) It must pass with -DDTC_OVERLAY_FILE externally as option. For example, west build ... -- -DDTC_OVERLAY_FILE=[overlay file path]. Is it not worked correctly?

ddeletic commented 11 months ago

Note that all CMakeLists.txt files previously had a line

set(DTC_OVERLAY_FILE $ENV{ZEPHYR_BASE}/../modules/lib/Arduino-Zephyr-API/variants/${BOARD}/${BOARD}.overlay)

which is setting the DTC_OVERLAY FILE to the correct overlay file. It works fine when building on Linux as $ENV{ZEPHYR_BASE} resolves to something like

/home/<username/zephyrproject.

However, on Windows, it resolves to something like

C:\Users\<username>\zephyrproject

(notice backslashes used to delimiter directories). When CMake tries to assign a value to DTC_OVERLAY_FILE, it is confused, as it has an invalid string.

In short, this change does not functionally change any aspect of setting DTC_OVERLAY_FILE. It just changes the value that is assigned to it.

DhruvaG2000 commented 11 months ago

Zephyr not define DTC_OVERLAY_FILE in build system.

@soburi , yes this was indeed one of the challenges I faced in the making of this project. You can refer to my blog here to read more on how I solved this issue.

But yes @ddeletic is right here, we can make use of DTC_OVERLAY_FILE in build time in Cmakelists

soburi commented 11 months ago

@ddeletic @DhruvaG2000 Currently, this module is not integrated into the mainline, so I think that's fine for now. However, if it is integrated into the mainline, it will be necessary to reconsider at some point.

DhruvaG2000 commented 11 months ago

@soburi , please excuse me but I don't still understand your exact concern? Is the way it is currently implemented in the main branch of zephyr arduino core api also non ideal? Kindly help me understand the potential issues it could cause

soburi commented 11 months ago

@DhruvaG2000

Simply because it except to this variable set from the command line. If the command line gives the DTC_OVERLAY_FILE option, it will not add but overwrite the value set in CMakeLists.txt. It may cause incorrect behavior.

At the moment, Only some files that are processed in the cmake/modules/configuration_files.cmake can automatically load the overlay.

I think we need to rethink cmake extensions when integrating. (I think the way of the handling shield will be helpful.)

DhruvaG2000 commented 11 months ago

Ah yes got it, when someone wants to build using west build passing another overlay other than the arduino dt overlay.. it will cause issues likely. Maybe something like a += or something may help? Let's think about it.

On Mon, 7 Aug, 2023, 06:05 Tokita, Hiroshi, @.***> wrote:

@DhruvaG2000 https://github.com/DhruvaG2000

Simply because it except to this variable set from the command line. If the command line gives the DTC_OVERLAY_FILE option, it will not add but overwrite the value set in CMakeLists.txt. It may cause incorrect behavior.

At the moment, Only some files that are processed in the cmake/modules/configuration_files.cmake can automatically load the overlay.

I think we need to rethink cmake extensions when integrating. (I think the way of the handling shield will be helpful.)

— Reply to this email directly, view it on GitHub https://github.com/zephyrproject-rtos/gsoc-2022-arduino-core/pull/74#issuecomment-1667024962, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ2S3QACDL6KDWMXJOWOVKDXUAZ6JANCNFSM6AAAAAA3EIVKYU . You are receiving this because you were mentioned.Message ID: @.***>

ddeletic commented 11 months ago

Forgive me, but I still do not understand what the issue is with DTC_OVERLAY_FILE setting.

Here, we are configuring build system for samples. Presumably, this is for people who are trying Zephyr on Arduino for the first time. It is important that samples build and run as smoothly as possible.

If somebody goes around building providing alternative overlay path, that is clearly beyond the purpose of the samples. If they do, they may well have a good reason to do so, and we should not prevent them.

soburi commented 11 months ago

@ddeletic

This way violates Zephyr's expected use of DTC_OVERLAY_FILE. So we should not show a bad example in examples. However, this repository has not yet merged, so we don't need to fix it now. In this case, documentation is a better way to help beginners.

If you want to solve it implicitly, I think proposing a comprehensive solution to handle overlay files is better. (I think it will probably be a set of specs and a framework written in cmake. It is a bit hard work.)

DhruvaG2000 commented 11 months ago

Please rebase on top of latest -next instead of having a "merge" commit in this PR.