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

support 'west flash' with native_sim, add debugserver support #68835

Merged
merged 1 commit into from Mar 12, 2024

Conversation

nashif
Copy link
Member

@nashif nashif commented Feb 10, 2024

  • west: add native_sim flash command
  • west: runner: native_sim: add debugserver support

Fixes #36706

@nashif nashif requested a review from aescolar February 10, 2024 17:13
@nashif nashif marked this pull request as ready for review February 11, 2024 15:55
@zephyrbot zephyrbot added platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: West West utility area: native port Host native arch port (native_posix) labels Feb 11, 2024
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

2 nits:

  • About the rename, given that "native_sim" is the name of one of the boards in tree, and that this works with any other "native" (posix arch based) board in tree including native_posix, I would prefer if we did not call the runner also "native_sim". Maybe just "native" or "native_runner" or "native_run"..
  • You can add the flash target to the other boards' cmake (native_posix and nrf_bsim)

aescolar
aescolar previously approved these changes Feb 12, 2024
aescolar
aescolar previously approved these changes Feb 12, 2024
@alwa-nordic
Copy link
Collaborator

Just my two cents: I think the old name native_gdb makes more sense than just native. From what I understand, the 'runners' are meant to be plug-able targets or environments. This particular runner is for setting up a GDB environment. This is why I named it gdb and not native_posix in the first place. One may want to define a coexisting runner for LLDB or even something with GUI like Ozone or Vscode. It makes sense to want to switch debuggers, just like one may want to switch between debugger probes.

@nashif
Copy link
Member Author

nashif commented Feb 12, 2024

Just my two cents: I think the old name native_gdb makes more sense than just native.

This implements debugserver, debug and flash commands, independent of how you debug or flash etc, debug interfaces can be something else other than gdb, still covered by the same runner, i.e.. through command line arguments. We should not have a runner for gdb and another one for something else.. Also, look at scripts/west_commands/runners/, none of the existing runners is that gdb oriented in the name..

@alwa-nordic
Copy link
Collaborator

alwa-nordic commented Feb 13, 2024

can be something else other than gdb, still covered by the same runner, i.e.. through command line arguments

Sure, 'can'. But it's not. This runner is de-facto GDB-specific. Early abstraction is usually bad abstraction. E.g. You are adding a '-tui' parameter to this runner. This would make no sense in a GUI or server environment.

Btw, it would be nice if we documented how to trigger the '-tui' parameter.

@alwa-nordic
Copy link
Collaborator

I just realized that debugserver is not gdbserver. Now I'm not sure about the naming at all any more. I think I don't understand Zephyr's runner concept.

@alwa-nordic
Copy link
Collaborator

I think it's weird that runners have a separate command for 'debug' and 'debugserver'. I would think that should be two separate runners. What do you think? This may be off-topic.

@nashif
Copy link
Member Author

nashif commented Feb 13, 2024

Btw, it would be nice if we documented how to trigger the '-tui' parameter.

west debug --tui

@alwa-nordic
Copy link
Collaborator

Btw, it would be nice if we documented how to trigger the '-tui' parameter.

west debug --tui

I see you added code to make it work now. Pro tip: Don't rebase before force-pushing changes. Do an extra force-push just for the rebase. This makes the 'Compare'-feature of GitHub work much better.

@nashif
Copy link
Member Author

nashif commented Mar 11, 2024

changing assignee, most related to native targets, no changes to west

aescolar
aescolar previously approved these changes Mar 12, 2024
scripts/west_commands/runners/native.py Outdated Show resolved Hide resolved
else:
assert False

def do_flash(self, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The kwargs are not used. Is it not better to remove them? Same applies to do_debug and do_debugserver.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the "interface" afaik, looking at all the runners:

scripts/west_commands/runners/intel_cyclonev.py:            self.do_flash_elf(**kwargs)
scripts/west_commands/runners/intel_cyclonev.py:            self.do_flash_elf(**kwargs)
scripts/west_commands/runners/intel_cyclonev.py:    def do_flash_elf(self, **kwargs):
scripts/west_commands/runners/native.py:            self.do_flash(**kwargs)
scripts/west_commands/runners/native.py:    def do_flash(self, **kwargs):
scripts/west_commands/runners/nsim.py:            self.do_flash(**kwargs)
scripts/west_commands/runners/nsim.py:    def do_flash(self, **kwargs):
scripts/west_commands/runners/openocd.py:            self.do_flash_elf(**kwargs)
scripts/west_commands/runners/openocd.py:            self.do_flash(**kwargs)
scripts/west_commands/runners/openocd.py:    def do_flash(self, **kwargs):
scripts/west_commands/runners/openocd.py:    def do_flash_elf(self, **kwargs):

No harm in leaving this allowing future expansions

scripts/west_commands/runners/native.py Outdated Show resolved Hide resolved
Add 'west flash' support which in the case of native_sim just start the
built application.
Reuse existing runner and rename it to be more generic as it does more
than just debugging now.

Also add debugserver command.

Fixes zephyrproject-rtos#36706

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif merged commit be60134 into zephyrproject-rtos:main Mar 12, 2024
36 checks passed
@nashif nashif deleted the topic/west/native_sim branch March 12, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: native port Host native arch port (native_posix) area: West West utility platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some standard west commands should Just Work for native_posix board
5 participants