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

NRF52 debug tools update #297

Merged
merged 2 commits into from
Jun 22, 2023
Merged

NRF52 debug tools update #297

merged 2 commits into from
Jun 22, 2023

Conversation

szszszsz
Copy link
Member

@szszszsz szszszsz commented Jun 21, 2023

This PR concerns debug tooling for NRF52 hardware:

  • select bootloader by name, to make sure other device is not connected to during flashing (evaluated lazily at the call moment)
  • add simple tracing and graphing script for stack memory diagnostics

Connected: #118

Further work ideas:

  • ideally gdb would step out automatically, while being in the Rust core libraries (e.g. memcpy) - should be achievable with Python.

Example chart:
image

@@ -66,7 +66,9 @@ UPLOAD_SH = $(BL_DIR)/upload.sh

FW_SIGNED = fw_signed_update.zip
FW_RAW = fw_unsigned.hex
TTYDEV = /dev/ttyACM1
TTYDEV := $(wildcard /dev/serial/by-id/usb-Nitrokey_Nitrokey_3_Bootloader*)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if there are multiple devices plugged?

Copy link
Member Author

@szszszsz szszszsz Jun 21, 2023

Choose a reason for hiding this comment

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

Both will be supplied as arguments probably (so the flashing should fail?). I did not consider this to be a use case, but perhaps you could provide the TTYDEV instead while calling Make, if you have multiple devices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it that case it should bail. Having multiple devices plugged while flashing is likely an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but I do not want to support that. It's out of scope for me. What's the use case for having multiple bootloaders connected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having multiple bootloaders connected is most likely an error and should abort.
It would be a nice to have safety feature.

Copy link
Member Author

@szszszsz szszszsz Jun 21, 2023

Choose a reason for hiding this comment

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

I understand the motivation, but I do not see how that can happen in the daily use, hence it looks like imaginary case for me, wasting time needlessly. As in, we do not check for free RAM memory or disk space either.
Can we fix that in another PR? Alternatively, I will be fine with a suggestion.
I do not think it can be done in one line (unless switching back to shell with some tee and pipes), so the count will have to be done separately on the actual use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could easily happen by mistake. Previously worst case scenario it would only flash one, now it would run a command with multiple unexpected arguments, so at least we should be aware of the consequence.

Another option would be to take only the first result of the wildcard with $(world 1, $(TTYDEV)): https://stackoverflow.com/questions/39674277/random-access-arrays-in-makefile

Copy link
Member Author

Choose a reason for hiding this comment

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

It should abort, or otherwise we are not fixing the main problem this patch was meant to fix (to have a properly selected target device).

Copy link
Collaborator

@sosthene-nitrokey sosthene-nitrokey Jun 22, 2023

Choose a reason for hiding this comment

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

nrf-builder does it this way:

TTY := $(shell ls -1rt /dev/ttyACM* | tail -n 1 | xargs)

The other solution for me would be to use

TTYDEV := $(wildcard /dev/serial/by-id/usb-Nitrokey_Nitrokey_3_Bootloader*)

in nrf-builder

Actually nrf-bootloader/upload.sh already fail if it has more than 2 arguments, so the answer to my initial question was "it falis" which is a satisfying answer.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Yes, basically it selects the last ttyACM device, assuming alphabetic order, which does not always mean the last connected device. And we still do not know, to what we have connected to. It should fail with multiple devices ideally, as you said. Let's keep this in mind for the next update.
  2. Fails right now, but it may accept more arguments in the future :-) The solution is probably to quote the result, so when multiple paths will be returned by wildcard, it won't be a correct path when parsed as a single argument.

@szszszsz szszszsz merged commit 218cdb6 into main Jun 22, 2023
@szszszsz szszszsz deleted the sz-nrf52-debug branch June 22, 2023 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants