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

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.

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

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

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
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

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
Contributor

The test cases need updates too.

@henrikbrixandersen
Copy link
Member

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
Contributor

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

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.

@carlescufi carlescufi added the dev-review To be discussed in dev-review meeting label Aug 11, 2021
@mbolivar-nordic
Copy link
Contributor

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
Collaborator

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

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
Collaborator

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 To be discussed in dev-review meeting label Aug 12, 2021
@carlescufi
Copy link
Member Author

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

@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.

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

@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 Compare October 6, 2021 12:54
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

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
Comment on lines +84 to +85
action=partial(depr_action,
replacement='-i/--dev-id'),
Copy link
Contributor

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

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 :)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: West West utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants