Implement debug feature

[rsora]

This PR Implements the debug functionality, in gRPC and "Command Line Interface" flavor (see #567).

IMPORTANT NOTES:

1. The debug functionality expects that a core is configured properly to expose a debug recipe. ATM the only Arduino core that will support the debug feature is the arduino:samd core (see https://github.com/arduino/ArduinoCore-samd). Other Arduino cores will follow.

2. In most cases you will need a debugger hardware tool to use the debug feature (i.e. to debug a mkr1000 you can use an Atmel ICE tool). Another option is to use boards like the Arduino Zero that has an additional debugging port.

gdb Command Generation

This PR implements a commands/debug/debug.go module that wraps an interactive gdb session, this makes the Arduino CLI to act as a StdIn and StdOut proxy both in gRPC and "Command Line Interface" mode. The CLI is also responsible to manage the gdb process life-cycle gracefully.

The creation of the board specific gdb command with all its options is made with the same "recipe building" approach as the compile and upload command, resolving groups of config parameters stored in the boards.txt and platform.txt core files

The command creation is tested in the commands/debug/debug_test.go file in a unit-test fashion. Proper e2e testing for both the gRPC and CLI interfaces will be implemented in a separate PR.

gRPC Interface

A new rpc/debug module is created to implement a gRPC "debug service" in streaming mode, and the client_example code is refreshed to implement a sample of use of the debug gRPC interface (please note that you will need a debugger tool to properly use the example)

CLI Interface

The CLI interface is basically a stdIn/Out proxy for the gdb command

Additional Details

arduino/utils/stream.go contains Pipe helpers used to implement StdIn and StdOut forwarding for shelled-out commands

[masci]

I'm not sure I understand the need of this Kill. io.Copy is supposed to block until it gets an EOF, does this mean the process executed hangs in some way that we have to kill it?

[cmaglie]

Yes, it happens when the pipe is not gracefully terminated (for example if we kill the client with CTRL-C), in this case we do not receive the EOF and the process is left hanging.
We may catch this error and kill the process only in that case, but in the end we opted to kill the process anyway (after a second of delay) to avoid unforeseen cases like this one.
If the process terminates normally, the Kill will just fail silently.

[rsora]

Basically the only case of graceful gdb process close is when a quit command is sent.
Other cases has to be managed by the cli.

[matthijskooijman]

I'm not sure I understand this entirely. If the process is terminated ungracefully, why kill it (it will be already dead). Also, zombie processes are processes which are actually dead, but are still waiting for their parent to reap them (e.g. read their exit status). This should be done using a "wait" call, which I suppose kill might do internally, but which is also done a few lines below, so I wonder why this kill is really needed, then.

[cmaglie]

The process is not zombie, is just "hanging" (this is a mistake in the comment).
In other words if gdb doesn't receive the quit command it just sits there: it's not dead, it's not zombie... it's just running waiting for input.

If something goes wrong on the IDE side or on the OpenOCD side, for any reason, gdb will not receive quit and will not close itself gracefully, the only way out is to kill it. But we have no way to understand if everything went smooth unless we inspect the stdin/out traffic...

That's the reason why we kill it anyway. We could check if the process is still running and kill it only if still running, but it will just complicate things without a real benefit.

This should be done using a "wait" call, which I suppose kill might do internally

No kill doesn't wait, the wait syscall is made here https://github.com/arduino/arduino-cli/pull/590/files#diff-ec656dd0bf60c33c4e4144e7d5e9508aR86 outside of the go-routine, basically it waits for the subprocess to terminate to finish the external debug call.

[matthijskooijman]

I just had a look over this PR, looks ok overall. Probably a good approach to make arduino-cli as transparent as possible (just exposing the gdb I/O, rather than exposing some higher level debug commands as we did with the older arduino-debugger prototype).

I do wonder about the configuration of the debugger itself. This is probably just a first version, but I do wonder about a few things:

• The handling of the tmp_file property and the importpath seems a bit fragile. What seems to be needed is the name of the .elf file produced by the build, so maybe that should just be exposed (by platform.txt) in a new property somehow for cli to read and re-exposed (by cli to platform.txt) or possibly replaced by the specified importPath? Or alternatively, some import_file property can be set and used by platform.txt (setting it to the .elf file generated by the build) for when no importFile was specified on the commandline, and this property can then be overwritten by cli when an importFile is specified on the commandline?
• Related, I'm not sure I understand the name "importFile". I can imagine that using elfFile is too arch-specific, but I'm not sure what "import" means in this sense?
• Currently, the board explicitly selects the debug tool to use with it, which works for e.g. the Arduino Zero that has a debug tool built in. However, when using an external debugger (e.g. the atmel ICE, or JTAGICE3, etc.), the board definition cannot know which tool is selected, so some external selection must be used (I think this is analogous to the "Upload" vs "Upload using programmer" distinction. Any thoughts on how to handle this yet?
• Selecting the board to debug might also need some additional thought. I think (mostly based on the java IDE, maybe arduino-cli can do more) that board selection currently happens exclusively based on serial ports (the java IDE can also do network ports, but I'm not quite sure how that works internaly). This works well for serial uploads, and could work well enough as long as the board to use exposes a serial port. However, some boards (or programmers/debuggers) do not expose a serial port (or, the tool that uploads does not know about serial ports). I actually suspect that this is already the case for the Arduino zero, that it just uploads to any cmsis-dap USB device it finds, completely ignoring the selected serial port. For this to work, there should be some way to select USB devices (probably based on vidpid or other filters) and expose them to tools (which might need multiple and platform-specific identifiers, e.g. Linux can specify as "bus 1, device 23", but also a port-path like "1-3.2:1.3"). This is probably an issue completely separate from debugging, though.
• The gdb command might need an extra --nx to prevent reading the .gdbinit file (or files maybe). Maybe also --quiet to prevent printing startup messages?
• In my original prototype, I also used --interpreter=mi2 to switch gdb from its normal "human readable" command mode into a slightly more machine-readable format (which also has better defined semantics for connecting commands and replies, IIRC). This is probably not suitable for direct commandline usage, but I can imagine that mi2 might be easier to work with for IDEs that will run on top of this. Maybe both would need to be supported? Or maybe the IDE can switch to mi2 mode using a normal mode command? Or is normal mode good enough in practice (i.e. does the IDE the Pro IDE is based on use it by default)?

[matthijskooijman]

I'm not sure I understand this entirely. If the process is terminated ungracefully, why kill it (it will be already dead). Also, zombie processes are processes which are actually dead, but are still waiting for their parent to reap them (e.g. read their exit status). This should be done using a "wait" call, which I suppose kill might do internally, but which is also done a few lines below, so I wonder why this kill is really needed, then.

[matthijskooijman]

This says "to be uploaded", but I did not see any code that actually uploads? Instead, I think it assumes that this .elf file is already uploaded?

[cmaglie]

Hey @matthijskooijman,

yes this is a prototype version, we will surely iterate on this one. BTW it has been very helpful to find critical issues and to see if the whole thing works... :-)

The handling of the tmp_file property and the importpath seems a bit fragile. What seems to be needed is the name of the .elf file produced by the build, so maybe that should just be exposed (by platform.txt) in a new property somehow for cli to read and re-exposed (by cli to platform.txt) or possibly replaced by the specified importPath? Or alternatively, some import_file property can be set and used by platform.txt (setting it to the .elf file generated by the build) for when no importFile was specified on the commandline, and this property can then be overwritten by cli when an importFile is specified on the commandline?

The filename of the .elf is already (almost) exposed in platform.txt for example samd has:

build.preferred_out_format=bin
## Save hex
recipe.output.tmp_file={build.project_name}.{build.preferred_out_format}
recipe.output.save_file={build.project_name}.{build.variant}.{build.preferred_out_format}

We assume that there is a .elf equally named as the corresponding .bin.
The problem is that this is produced in the temp folder after the build, so the cli copies it inside the sketch when compiled. The remainder of the importpath handling is to retrieve the correct build artifact in all cli use-cases, this piece has been copied from the upload command and is duplicated code, it must be factored into a general purpose function.

Related, I'm not sure I understand the name "importFile". I can imagine that using elfFile is too arch-specific, but I'm not sure what "import" means in this sense?

I think it comes from the Java IDE where you "Export .hex file", this being the opposite we used import. Nothing stop us from changing it BTW.

Currently, the board explicitly selects the debug tool to use with it, which works for e.g. the Arduino Zero that has a debug tool built in. However, when using an external debugger (e.g. the atmel ICE, or JTAGICE3, etc.), the board definition cannot know which tool is selected, so some external selection must be used (I think this is analogous to the "Upload" vs "Upload using programmer" distinction. Any thoughts on how to handle this yet?

This is something to prepare for the next iteration together with the "Upload using programmer" feature, those are strictly correlated. This could be the chance also to clarify a bit the programmers.txt definitions.

Selecting the board to debug might also need some additional thought. I think (mostly based on the java IDE, maybe arduino-cli can do more) that board selection currently happens exclusively based on serial ports (the java IDE can also do network ports, but I'm not quite sure how that works internaly). This works well for serial uploads, and could work well enough as long as the board to use exposes a serial port. However, some boards (or programmers/debuggers) do not expose a serial port (or, the tool that uploads does not know about serial ports). I actually suspect that this is already the case for the Arduino zero, that it just uploads to any cmsis-dap USB device it finds, completely ignoring the selected serial port. For this to work, there should be some way to select USB devices (probably based on vidpid or other filters) and expose them to tools (which might need multiple and platform-specific identifiers, e.g. Linux can specify as "bus 1, device 23", but also a port-path like "1-3.2:1.3"). This is probably an issue completely separate from debugging, though.

I think we just need to provide a port parameter (port to the programmer of course, not the board), or none if the programmer is autodetected. About the different "kinds" of ports the pluggable discovery mechanism will allow to discover different types of ports with their properties attached to them. The pluggable discovery is another thing planned for the release 1.0 of the arduino-cli...

The gdb command might need an extra --nx to prevent reading the .gdbinit file (or files maybe). Maybe also --quiet to prevent printing startup messages?

--nx may be added for sure, --quiet I don't know, maybe it could be helpful to have the startup message in some cases.

In my original prototype, I also used --interpreter=mi2 to switch gdb from its normal "human readable" command mode into a slightly more machine-readable format (which also has better defined semantics for connecting commands and replies, IIRC). This is probably not suitable for direct commandline usage, but I can imagine that mi2 might be easier to work with for IDEs that will run on top of this. Maybe both would need to be supported? Or maybe the IDE can switch to mi2 mode using a normal mode command? Or is normal mode good enough in practice (i.e. does the IDE the Pro IDE is based on use it by default)?

we added --interpreter=mi2 to the recipe in the platform.txt, this works for the IDE but not so much if you run the debug interactively by command-line. Maybe we can add a command line switch, or better, try to switch mode from the IDE by sending command in normal mode as you suggested. Again, room for improvements on the next iterations.

Thanks again for your comments @matthijskooijman, now that I re-read it there is a lot going on, it may be better to open separate issues for each part...