-
Notifications
You must be signed in to change notification settings - Fork 26
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
Conversation
@@ -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*) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
- 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.
This PR concerns debug tooling for NRF52 hardware:
Connected: #118
Further work ideas:
Example chart: