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

west: runners: Add new -i/--dev-id device identifier common runner option #37509

Merged
merged 7 commits into from Oct 12, 2021

Conversation

@carlescufi
Copy link
Member

@carlescufi carlescufi commented Aug 6, 2021

In order to avoid different runners implementing device identification each in their own way, introduce a new -i/--dev-id command-line switch that allows the user to specify which board/debugger/node is to be targeted by the flash or debug operation.

@carlescufi carlescufi requested a review from mbolivar-nordic as a code owner Aug 6, 2021
@carlescufi carlescufi requested review from henrikbrixandersen, mbolivar-nordic and MaureenHelm and removed request for mbolivar-nordic Aug 6, 2021
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Not sure if this is the right approach for all runners. Take for instance the CANopen Node ID; Everybody working with CANopen knows what a Node ID is, but would likely not associate --dev-id with being that from the generic description given. Compare it to e.g. an IP address of a remote debugger - it would not be obvious to pass that in as --dev-id? Same applies to serial numbers, USB VID:PID, board names, etc.

Blocking this to ensure we have a discussion about this.

@carlescufi
Copy link
Member Author

@carlescufi carlescufi commented Aug 6, 2021

Not sure if this is the right approach for all runners. Take for instance the CANopen Node ID; Everybody working with CANopen knows what a Node ID is, but would likely not associate --dev-id with being that from the generic description given. Compare it to e.g. an IP address of a remote debugger - it would not be obvious to pass that in as --dev-id? Same applies to serial numbers, USB VID:PID, board names, etc.

It's a matter of taste more than anything else. Personally I prefer having a single command-line option that is reused by all runners albeit with slight different meanings based on the context, but the opposite argument can be made. For me, anything that identifies a device can fall under the -i/--dev-id usage, and I am more comfortable using -i <node id/vid:pid/serial> than having to remember specific options for each runner.

Copy link
Member

@mbolivar-nordic mbolivar-nordic left a comment

Thanks for finally crossing off this longstanding TODO.

I defer to @henrikbrixandersen on the canopennode runner.

As for the rest, no objections to adding the option, but there's no need to break compatibility. See comments for details.

scripts/west_commands/runners/core.py Outdated Show resolved Hide resolved
scripts/west_commands/runners/nrfjprog.py Show resolved Hide resolved
scripts/west_commands/runners/dfu.py Outdated Show resolved Hide resolved
scripts/west_commands/runners/dfu.py Show resolved Hide resolved
scripts/west_commands/runners/pyocd.py Show resolved Hide resolved
@mbolivar-nordic
Copy link
Member

@mbolivar-nordic mbolivar-nordic commented Aug 6, 2021

The test cases need updates too.

@henrikbrixandersen
Copy link
Member

@henrikbrixandersen henrikbrixandersen commented Aug 9, 2021

I am sorry. I still think these changes are making the command line options less obvious for an end-user. There is no way - except for reading the source code - for en end-user to know what --dev-id expects for a given runner. Serial number? Node ID? IP address? IP:port? USB VID:PID?

Unifying these into one also means that adding more ways of selecting a given instance is even more obscure. Say, a runner that supports selection via either serial number or USB VID:PID.

@mbolivar-nordic
Copy link
Member

@mbolivar-nordic mbolivar-nordic commented Aug 9, 2021

There is no way - except for reading the source code - for en end-user to know what --dev-id expects for a given runner. Serial number? Node ID? IP address? IP:port? USB VID:PID?

We can fix this in both the .rst files and in the command line help.

Unifying these into one also means that adding more ways of selecting a given instance is even more obscure. Say, a runner that supports selection via either serial number or USB VID:PID.

Do you actually have a such a situation? I can imagine IP vs. USB debugging with a J-Link, but that seems solvable with either some additional smarts in the runner ("is this argument an IP:port? Then it's IP. Is it just a number? Then it's USB.") or another option like --id-format.

Assuming we sort these things out, are you still opposed to the idea completely, or are you saying you have reservations about the current status?

@carlescufi
Copy link
Member Author

@carlescufi carlescufi commented Aug 10, 2021

We can fix this in both the .rst files and in the command line help.

I completely agree with @mbolivar-nordic. Documenting this properly and then getting the users to understand that a command-line option might take different formats based on the runner seems simpler for the user to me than having to remember a bunch of different ones. But again, this is purely cosmetic and so subject to individual taste.

@henrikbrixandersen an alternative to generalizing it entirely would be to do this only for those that identify a particular debugger instance. So only jlink, nrfjprog and pyocd would use -i/--dev-id.

@mbolivar-nordic
Copy link
Member

@mbolivar-nordic mbolivar-nordic commented Aug 11, 2021

I think it's time to document the individual runner backends on the .rst side. On the help string side, I think core.py should set the help text for the dev_id option using a self.dev_id_help attribute which subclasses can override. Then each runner can explain what the individual ID contents are via an override of that attribute.

@galak
Copy link
Contributor

@galak galak commented Aug 11, 2021

Would using -u like pyocd be any clearier?

  -u UNIQUE_ID, --uid UNIQUE_ID, --probe UNIQUE_ID
                        Choose a probe by its unique ID or a substring thereof. Optionally prefixed with
                        '<probe-type>:' where <probe-type> is the name of a probe plugin.

@henrikbrixandersen
Copy link
Member

@henrikbrixandersen henrikbrixandersen commented Aug 12, 2021

Would using -u like pyocd be any clearier?

Not as I see it. What I questioning here is what the benefit by combining these different configuration options into one and leave the user to parse a help text/source code in order to find out what kind of argument the given runner expects when specifying that option.

I am all for combining options for common parameters (e.g. --serial-number or --remote-host or similiar`), but I fail to see the benefit of combining them into one, when they do not accept the same arguments.

If I do a west -r canopen -H and see e.g. --node-id I don't need to read any more help text in order to figure out what to pass as argument. If I run the same command and see --dev-id or --uid I would not know what to pass without reading the help text.

We could even (I don't believe we have that today?) have bash completion of runner specific arguments, which would make it even more intuitive to use. Type in west -r canopen --<TAB>, see --node-id in the list and from the option name already there know what to pass as argument - without even looking at the help output or other documentation.

Just to clarify: The above goes for other runner device selection arguments than the CANopen runner --node-id, which is just used as an example.

@galak
Copy link
Contributor

@galak galak commented Aug 12, 2021

dev-review (Aug 12, 2021):

  • suggestion is to support both the old and new option. All runners for debug/flash should support the new option at min going forward. Docs/usage help to clarify.

@galak galak removed the dev-review label Aug 12, 2021
@carlescufi
Copy link
Member Author

@carlescufi carlescufi commented Aug 12, 2021

I think core.py should set the help text for the dev_id option using a self.dev_id_help attribute which subclasses can override

This sounds like a very good idea. I will start with that, given that anyway we are keeping the old options I can actually reuse the same text for both on each runner.

@carlescufi
Copy link
Member Author

@carlescufi carlescufi commented Sep 28, 2021

@mbolivar-nordic

I think it's time to document the individual runner backends on the .rst side

I am not sure about this one for this PR, but I am happy to discuss this with you to see if we can make it happen.

I think core.py should set the help text for the dev_id option using a self.dev_id_help attribute which subclasses can override. Then each runner can explain what the individual ID contents are via an override of that attribute.

This is now done.

@henrikbrixandersen

suggestion is to support both the old and new option.

I did that for the runners that are controversial.

@carlescufi carlescufi requested a review from gmarull Sep 29, 2021
Copy link
Collaborator

@gmarull gmarull left a comment

LGTM, I've added one comment. I think deprecation notice would be a useful addition.

scripts/west_commands/runners/nrfjprog.py Outdated Show resolved Hide resolved
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

A few minor nits, otherwise looks good, thank you for updating.

scripts/west_commands/runners/canopen_program.py Outdated Show resolved Hide resolved
scripts/west_commands/runners/canopen_program.py Outdated Show resolved Hide resolved
scripts/west_commands/runners/nrfjprog.py Show resolved Hide resolved
@carlescufi
Copy link
Member Author

@carlescufi carlescufi commented Oct 5, 2021

@mbolivar-nordic and @henrikbrixandersen could you please take another look?
@gmarull thanks for the suggestion, I implemented the deprecation helper Action

@carlescufi carlescufi force-pushed the runners-devsel branch 3 times, most recently from 8449e4a to eeccf51 Oct 6, 2021
Copy link
Member

@mbolivar-nordic mbolivar-nordic left a comment

Looking good now; just a couple of things. Thanks again.

scripts/west_commands/runners/jlink.py Show resolved Hide resolved
scripts/west_commands/runners/nrfjprog.py Show resolved Hide resolved
action=partial(depr_action,
replacement='-i/--dev-id'),
Copy link
Member

@mbolivar-nordic mbolivar-nordic Oct 11, 2021

Choose a reason for hiding this comment

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

It feels like this should be structured as:

  1. A commit adding DeprecatedAction
  2. A commit adding the new common option
  3. Commits updating the existing runners to use the new option and deprecating the old one using DeprecatedAction (as needed; I know not all runners are deprecating their specific options)

But it's not a huge deal so I'm not blocking on this comment.

Copy link
Member Author

@carlescufi carlescufi Oct 11, 2021

Choose a reason for hiding this comment

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

I agree with you, but honestly I think it's not worth the rebase effort. I need to pick my fights :)

carlescufi added 7 commits Oct 11, 2021
In an effort to standardize the way that a particular debugger or device
instance is identified when there are multiple present, introduce a new
-i/--dev-id option common to all runners that allows the user to specify
which device to interact with when there are multiple connected.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Remove the previous jlink-specific --id option and switch to the common
-i/--dev-id one.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Remove the previous nrfjprog-specific --id option and switch to the
common -i/--dev-id one.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Remove the previous canopen-specific --node-id option and switch to the
common -i/--dev-id one.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Remove the previous dfu-util-specific --pid option and switch to the
common -i/--dev-id one.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Remove the previous pyocd-specific --board-id option and switch to
the common -i/--dev-id one.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
In order to allow for further options to be deprecated with minimal
impact, add a deprecation argparse Action and a callable instantiator
that can be used to deprecate options in favor of new ones.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
@cfriedt cfriedt merged commit 6a3593f into zephyrproject-rtos:main Oct 12, 2021
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants