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

makefiles/tools/serial.inc.mk: Allow detection of debug adapter #19119

Merged
merged 3 commits into from
Feb 24, 2023

Conversation

maribu
Copy link
Member

@maribu maribu commented Jan 10, 2023

Contribution description

This PR adds the ability to automatically detect the debug adapter for boards with an integrated programmer/debugger, if that debugger also provides the TTY.

This extends the TTY detection that can be enabled with MOST_RECENT_PORT=1 to set DEBUG_ADAPTER_ID to the TTY's serial, but only if DEBUG_ADAPTER_ID_IS_TTY_SERIAL is set to 1 by the board (as not all boards have an integrated programmer/debugger).

Testing procedure

Connect a HiFive-1B and a nRF52840DK at the same time and try make BOARD=<nrf52840dk|hifive1b> MOST_RECENT_PORT=1 -C examples/default flash term for both. The programmer will not reliably select the correct programmer in master. With this PR, it will.

Issues/PRs references

None

@maribu maribu added Area: build system Area: Build system Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools labels Jan 10, 2023
@maribu maribu requested review from chrysn and bergzand January 10, 2023 08:44
@github-actions github-actions bot added Area: boards Area: Board ports Area: doc Area: Documentation labels Jan 10, 2023
@riot-ci
Copy link

riot-ci commented Jan 10, 2023

Murdock results

✔️ PASSED

64d4aec boards: Provide debug adapter ID from serial where possible

Success Failures Total Runtime
6865 0 6865 08m:22s

Artifacts

@chrysn
Copy link
Member

chrysn commented Feb 24, 2023

AIU to test this I need a device that is programmed through the very UART the device exposes as its terminal port. What confuses me is that already on master, if ttyACM0 is taken by somethign different, I can already run make BOARD=nrf52840dongle all flash term MOST_RECENT_PORT=1, and the programmign step will run nrfutil dfu usb-serial --port=/dev/ttyACM1.

Does that already use a different mechanism, or it it actually doing things wrong by uncoordinatedly using ${PORT} as a flag in boards/nrf52840dongle/Makefile.include instead of DEBUG_ADAPTER_ID?

@chrysn
Copy link
Member

chrysn commented Feb 24, 2023

Ah, OK, now I see: It would work fine already if the programmer is PORT based, but not if it's serial based.

I have no boards to test this with (the one I did was PORT based), but anyhow, that case didn't get worse, and I trust your testing with the boards that use it.

PORT_DETECTED := $(shell $(TTY_SELECT_CMD) || echo 'no-tty-detected')
PORT ?= $(PORT_DETECTED)
TTY_DETECTED := $(shell $(TTY_SELECT_CMD) || echo 'no-tty-detected no-serial-detected')
PORT_DETECTED := $(firstword $(TTY_DETECTED))
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever expect OSes to assign TTY names that contain whitespace, or devices to expose serials that do?

I don't see any direct security concerns, so I'll just leave that as a note and not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

ttys.py will quote any output that would contain the "seperation char", which by default is the space. I have to admit that I don't know for sure if $(firstword "foo bar" "blah blah") does indeed yield "foo bar", or just "foo.

@benpicco
Copy link
Contributor

This needs to be squashed

For scripts it can be useful to output not only one, but multiple
formats (e.g. to obtain both path and serial of a TTY). The script
now support passing multiple formats.

Note that only simple formats can be combined, as the JSON and markdown
table won't mix well with any other format.
Boards with an integrated debugger/programmer that also provides the
serial as UART <--> USB adapter, the TTY serial matches the serial of
the programmer.

This adapts the `serial.inc.mk` to set the `DEBUG_ADAPTER_ID` to the
TTY serial if (and only if) `MOST_RECENT_PORT` *and*
`DEBUG_ADAPTER_ID_IS_TTY_SERIAL` both have a value of `1`. Boards with
an integrated programmer are expected to set
`DEBUG_ADAPTER_ID_IS_TTY_SERIAL` to `1` in their `Makefile.include`.
Set `DEBUG_ADAPTER_ID_IS_TTY_SERIAL` to `1` for those boards to allow
automatic detection of the debug adapter with `MOST_RECENT_PORT=1`.
@maribu maribu force-pushed the dist/tools/usb-serial branch from 2ae1a62 to 64d4aec Compare February 24, 2023 15:51
@maribu
Copy link
Member Author

maribu commented Feb 24, 2023

Done. I also updated the microbit-v2, as it also contains an embedded debugger where the debug adapter ID equals the serial ID.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 24, 2023

Build succeeded:

@bors bors bot merged commit 9d1d4bb into RIOT-OS:master Feb 24, 2023
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
@maribu maribu deleted the dist/tools/usb-serial branch April 25, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: doc Area: Documentation Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants