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

Adding support for WSL #5

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

ShishirPatil
Copy link

@ShishirPatil ShishirPatil commented Apr 15, 2020

Adding make flash support for
a) WSL on Windows, and
b) SES on Windows

@nealjack
Copy link
Member

You don't need to change the SDK makefiles - we have our own set of makefiles that manage flashing in the make directory, specifically Program.mk. We don't use nrfjprog, instead we use the JLink tools directly.

@brghena
Copy link
Member

brghena commented Apr 15, 2020

This definitely doesn't work, right? @nealjack's right that the only makefiles we use from apps are in the make/ directory. If this did work in testing, can you tell us what steps you took?

@ShishirPatil
Copy link
Author

Yeah. I rolled back the SDK changes, and modified the make/Program.mk. This uses nrfjprog for writing to device. Works on my machine.

@ShishirPatil ShishirPatil changed the title [WIP] Adding support for WSL Adding support for WSL Apr 15, 2020
make/README.md Outdated
that doesn't work.
We develop on Linux. This also works on Windows in either of the following ways:
1. Build on WSL (Windows Subsystem for Linux), and `flash` with WSL.
2. Build with SES (Segger Embedded Studio) and `flash` with WSL. Note: For this you will have to manually move the `.hex` file generated from SES to `build/` directory. For e.g. `mv blinky/pca10040/s132/ses/Output/Release/Exe/blinky_pca10040_s132.hex blinky/_build/blink_sdk16_blank.hex`.
Copy link
Member

Choose a reason for hiding this comment

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

What is blinky? It would be better to give instructions for an application in the apps/basic/ directory.

Copy link
Author

Choose a reason for hiding this comment

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

For the nrf52x-base/apps/basic/blink it works as is. You just have to make flash.
The second option is if you are using SES from the SDK.

Copy link
Member

Choose a reason for hiding this comment

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

regardless "blinky" isn't an application we have in the repo

Copy link
Member

Choose a reason for hiding this comment

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

I see. Is there really any reason to list the SES option at all? It doesn't really let you build nrf52x-base applications, just apps in the SDK, which aren't our purview.

Copy link
Author

Choose a reason for hiding this comment

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

I think there is value to provide minimal support to SES.
@nealjack Good point. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

But there isn't really support for SES. To support SES would mean taking an application from the apps/ folder, compiling it with SES, and then flashing it. But that is not going to work.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
make/Program.mk Outdated Show resolved Hide resolved
make/Program.mk Outdated Show resolved Hide resolved
make/Program.mk Outdated Show resolved Hide resolved
else
$(Q)printf "loadfile $(HEX) \nr\ng\nexit\n" >> $(BUILDDIR)flash.jlink
$(Q)$(JLINK) $(JLINK_FLAGS) $(BUILDDIR)flash.jlink
else ifeq ($(USE_BOOTLOADER),0)
Copy link
Member

Choose a reason for hiding this comment

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

We also want to leave this line as just an else with no check. USE_BOOTLOADER may not be defined at all, in which case it won't be equal to zero.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, the better design might just be to leave the logic that creates the flash.jlink file alone. It shouldn't hurt anything to create that file on windows even if you don't use it. Then you can create one if check that decides if you're windows and uses nrfjprog or else falls back on the existing JLINK infrastructure

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, ifeq ($(USE_BOOTLOADER),) will evaluate to true if the variable is undefined

@nealjack
Copy link
Member

@ShishirPatil did you want to keep trying to get this pulled into this repo? If so, what is the current status?

@ShishirPatil
Copy link
Author

@nealjack Thanks for bumping this up. It was functionally ready to merge, but I am yet to incorporate @brghena's recommendations. I'll incorporate them and push.

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.

4 participants