Skip to content

Confusing "Target not supported" errors from absent device_name field in targets.json #3174

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

Closed
sarahmarshy opened this issue Oct 31, 2016 · 16 comments

Comments

@sarahmarshy
Copy link
Contributor

sarahmarshy commented Oct 31, 2016

Since #3173 is the first time it has come up since #2708, but is likely to cause confustion again:

It's obvious that this type of solution needs to be clearer when a user gets a "Target not supported" error. "Target not supported" is potentially ambiguous, as it might mean that the target simply doesn't have a device name, but is described by some CMSIS pack. Though, it could be true that the device is not described in a CMSIS pack (would require user/vendor to manually check?). This will be more of a problem for release 2 targets missed by the device name addition, as the current release targets reliably have a device name if they are supported(as tested in CI), and newly added targets should conform to the spec. @0xc0170 @screamerbg @theotherjimmy: Open to solutions in the interim that won't confuse people.

The checks implemented in the changes #3172 should make it easier to warn a user about absent device names. That can come a follow-up PR. Not sure that will entirely solve the issue though.

@sarahmarshy sarahmarshy changed the title Confucing "Target not supported" errors from absent device_name field in targets.json Confusing "Target not supported" errors from absent device_name field in targets.json Oct 31, 2016
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 1, 2016

First of all, is this documented? How to make new target supported ? cmsis packs - what role do they play here, where are IAR definitions (why not cmsis pack are used) , etc ? I could not locate any readme in export folder neither in the docs.

Where is device name coming from? where to find cmsis-packs? These might be essential information for contributors.

@sarahmarshy
Copy link
Contributor Author

It's in the docs for targets.json, but perhaps it should be relocated.

@sarahmarshy
Copy link
Contributor Author

https://github.com/ARMmbed/mbed-os/blob/master/docs/mbed_targets.md#device_name

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 2, 2016

@sarahmarshy Any proposals for this issue? I checked the referenced PR, and it contains just TargetNotSupportedException that says "not in cmsis-packs". What I see there, if IAR fails it could be because cmsis pack does not support it or IAR definition is missing or IAR not supported toolchain, but can't locate the report message (to be clear what has not been found). Is this issue about it, how to make it more clear?

Should a tool do a better error msg when it fails to export (what I saw in IAR exporter, 3 conditions that can fail):

  • iar toolchain is not supported for the target
  • iar definition in iar_definitions.json is missing for the target
  • cmsis-pack vXXX does not contain this target definition ?

@sarahmarshy
Copy link
Contributor Author

sarahmarshy commented Nov 2, 2016

If a target doesn't have a device_name in targets.json, it is non-deterministic whether that device is supported by a CMSIS pack. That is the case we saw with the microbit. #3182 might describe that a little better than the documentation above. Specifically, this bit.

I guess a warning about a missing device_name should be given, and maybe a link to the documentation somewhere?

@bridadan
Copy link
Contributor

bridadan commented Nov 2, 2016

I'd suggest the following in terms of errors that we give:

  1. From the discussion, it sounds like we are requiring that targets have CMSIS Pack to be supported by the mbed exporters, correct? If that's the case, upon exporting I would expect the tools to check if there is an entry for device_name in targets.json. If it's missing, print an error saying that the device_name field must contain a valid "CMSIS Pack name" (or whatever the appropriate info is called) and then mention the correct document that has more information about it.
    1. Ex. The 'device_name' field is in the target description file ('targets.json') is required for exportring. Please add it with a valid CMSIS Pack name for your target or see the exporter documentation for more details.
  2. If the provided device_name isn't found by the pack manager, then print a message saying so and point them to targets.json to fix it.
  3. If a device_name is specified and valid but the IAR definitions are missing, mention that when exporting to IAR.
    1. Ex. The IAR definitions are needed for this target to export to IAR. Please add a mapping from the target's 'device_name' to the IAR chip name in 'iar_definitions.json'.
    2. Note: I'm sure my vocabulary is all wrong here but hopefully you get the general idea.

@sarahmarshy
Copy link
Contributor Author

sarahmarshy commented Nov 2, 2016

I have made a commit here: sarahmarshy@40918f7

This should make errors a lot more verbose. Additionally, it only determines if the target being used in export is supported, rather than statically determining support for all targets/IDEs. I am waiting for some other PRs to be merged before I make a new one, as I made these changes on top of #3172.

@bridadan
Copy link
Contributor

@sarahmarshy did your commit ever make it into master? If so can you please close this?

@sarahmarshy
Copy link
Contributor Author

No, it did not. I can try to come back to this, but it might be a little while. Maybe @theotherjimmy can comment on any plans for this.

@theotherjimmy
Copy link
Contributor

I don't have any plans to create a PR on your behalf. You could you submit that commit as a PR.

@sarahmarshy
Copy link
Contributor Author

I'm not saying to create a PR on my behalf, but I recall you had some problems with that commit because there were other plans for error messages.

@theotherjimmy
Copy link
Contributor

theotherjimmy commented Jan 18, 2017

My problem with that commit was not related to the errors, it was that the commit removed the static list of computed support that the website depends on.

@sg-
Copy link
Contributor

sg- commented Feb 3, 2017

@sarahmarshy Has this been resolved?

@sarahmarshy
Copy link
Contributor Author

@sg- No, this has not. I'd need to look into this again.

@cmonr
Copy link
Contributor

cmonr commented Jan 23, 2018

@sarahmarshy Is this still an issue?

@sarahmarshy
Copy link
Contributor Author

I don't think so. Presumably, it would be caught by the export-build tests if it were still a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants