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

Consider removing .nfc namespace #67

Closed
kenchris opened this issue Oct 15, 2015 · 39 comments
Closed

Consider removing .nfc namespace #67

kenchris opened this issue Oct 15, 2015 · 39 comments
Labels
F2F tag-tracker Group bringing to attention of the TAG, or tracked by the TAG but not needing response.

Comments

@kenchris
Copy link
Contributor

https://github.com/w3ctag/spec-reviews/blob/master/2015/10/nfc-feedback.md

The NFC adapter is two-levels away from the navigator object. Given that the NFC container object only has the single method, this could be collapsed a level to avoid extra verbosity, and to avoid use of the word "Adapter" (which felt foreign to me). Suggestion: navigator.requestNFC() --- instead of navigator.nfc.requestAdapter()

@zolkis
Copy link
Contributor

zolkis commented Oct 15, 2015

Same as #66

@kenchris kenchris added the F2F label Oct 15, 2015
@alexshalamov
Copy link

@kenchris Someting like this?

partial interface Navigator {
  Promise<NFC> requestNFC();
};

interface NFC {
  push
  cancel
  watch
  unwatch
};

How would you check if UA has WebNFC implemented?

if (!navigator.requestNFC) {
...
}

@zolkis
Copy link
Contributor

zolkis commented Oct 23, 2015

Works for me. If Web NFC is not implemented, requestNFC() would reject with NotSupportedError.

@zolkis
Copy link
Contributor

zolkis commented Oct 23, 2015

Note that in the example above NFC is the same thing as NFCAdapter used to be. If there are multiple NFC adapters, implementations MAY choose one of them based on settings/heuristics, or pop up a dialog for choosing one of the adapters.

Implementations will need to do things with this multiplicity in mind. I think the spec should include a note saying that the NFC object should be a singleton per physical NFC adapter and script execution context. Subsequent calls on requestNFC() would return the same object if it was requested before, or otherwise construct it again.
This solves the problem of dangling watches for bad client code.

@kenchris
Copy link
Contributor Author

I would really like to discuss this at the F2F... requestNFC sounds weird to me. Maybe adapter is not the best word, but maybe we can find something better than "NFC" for describing it.

@zolkis
Copy link
Contributor

zolkis commented Oct 23, 2015

Replacing navigator.nfc with navigator.requestNFC() does simplify things. If you don't like the NFC name, it could be requestNFCAdapter, and stick with NFCAdapter instead of NFC. I don't care about the names that much :). But this really needs to be discussed and synchronized with the Bluetooth guys, since it's the same problem for them. @jyasskin

@alexshalamov
Copy link

@zolkis
I think we need to create an issue for "multiple nfc adapters case".

To my knowledge, there are no known platforms that have such HW configuration (>1 nfc adapter).
This issue increases complexity of API implementation as well as usability of the API by the developers.

Here is an example:

var nfcAdapter;
var watchId;

// UA shows dialog (select adapter => adapter 1 is selected)
navigatior.nfc.requestAdapter().then( (adapter) => {nfcAdapter = adapter; } );
nfcAdapter.watch(...).then((id) => {watchId = id;})
nfcAdapter.pushMesage(...);

// UA shows dialog (select adapter => adapter 2 is selected)
navigatior.nfc.requestAdapter().then( (adapter) => {nfcAdapter = adapter; } );
nfcAdapter.pushMesage(...);
nfcAdapter.unwatch(watchId); // Side effects?
  1. What should we do with first nfcAdapter? Should we release HW? Should we suspend watch / push?
  2. Should we reject promise for first pushMessage call?
  3. Should we move watch/push operations to new adapter?
  4. Adapter doesn't have an id, therefore developer is not even aware that HW was changed => security issue?
  5. Other Device APIs does not expose HW "adapter" multiplicity to the spec, NFC should not be exception.
    Battery (some laptops have 2)
    Geolocation (can have USB GPS dongle)
    Bluetooth (can be internal + USB)

Proposal:
I would propose to remove "hw adapter multiplicity" complexity from initial specification and add enumeration later if requested by real life use-cases.

@zolkis
Copy link
Contributor

zolkis commented Oct 23, 2015

We should avoid enumeration... Supporting multiplicity does not change the API with the pattern above.

As for the example, we have 2 different adapters that can work in parallel.

  1. we should not release nfcAdapter, unless the underlying platform can only work with one.
  2. we don't reject promise for first push
  3. we don't move watch/push to the new adapter
  4. developer has chosen to write this code. It is legal, but not wise. Instead of id, they get an object. If they lose the object, it will leak until the page is closed/reloaded.
  5. I think we could have internal + USB adapter.

However: if we decide to not support multiple adapters, no need to change the API. Even in that case, it should be noted in the spec that an NFC adapter is a singleton per physical adapter+script execution context.

@kenchris
Copy link
Contributor Author

You can just say, that first invocation of requestAdapter per browsing context MAY show a selector dialog allowing the user to pick an adapter, but after that it will never change. I think that is a fine compromise.

@alexshalamov
Copy link

@kenchris Fine with me. After first requestAdapter, NFC adapter should not be changed during lifetime of browsing context.

@zolkis
Copy link
Contributor

zolkis commented Oct 23, 2015

Basically it is implementation choice whether:

  1. always return the same (internal) adapter
  2. choose a policy (e.g. based on user setting) to select one of the adapter
  3. pop up a dialog.

In all of these cases the implementation should still do the same thing, expose a singleton for NFC adapter, or return an error. Disposal of the adapter is described in the spec.

@zolkis
Copy link
Contributor

zolkis commented Oct 23, 2015

If adapter is changed to a second one, we have the following options:

  1. suspend the first adapter
  2. release the first adapter
  3. reject if this is not the first invocation of requestAdapter() (or requestNFC)
  4. allow them working in parallel.

I see more problems/inconsistencies with policy 3 than either with policy 1 or 2.

In addition, we discussed with @alexshalamov that a non-fingerprintable id (like "nfc1", "nfc2" etc) could be included to an NFCAdapter, in order that clients could tell them apart.

@kenchris
Copy link
Contributor Author

Well, the id etc could always be added later in the spec if needed. Let's just keep it open and make sure it is always a singleton

@zolkis
Copy link
Contributor

zolkis commented Oct 23, 2015

I would suggest policy 2 from above. I think it has least amount of problems. Also, an id is not needed in that case. If you agree with that, I can include this in the requestAdapter() steps.

@kenchris
Copy link
Contributor Author

2 is fine

zolkis added a commit to zolkis/web-nfc that referenced this issue Oct 23, 2015
zolkis added a commit to zolkis/web-nfc that referenced this issue Oct 23, 2015
@jyasskin
Copy link
Member

In the F2F, I proposed moving the contents of the NFCAdapter interface onto navigator.nfc, and treating them as operating on a "merged" system adapter.

That is, navigator.nfc.watch() would watch for the next peer/tag near any of the system's adapters, and resolve the promise with that peer/tag's data.

navigator.nfc.pushMessage() would publish the data on all of the system's adapters, and resolve or reject based on the first adapter to see a nearby peer/tag (at which point it would cancel the publication on the other adapters). The user might simultaneously tap multiple devices on the system's multiple adapters and get multiple writes, but they kinda asked for that outcome.

If we eventually find a need for the developer to distinguish between multiple adapters, we can add that notion into the API then, but leaving it out could make the initial API simpler.

@zolkis
Copy link
Contributor

zolkis commented Oct 26, 2015

When pushing, it is non-deterministic on which adapter the push/write happened, and the web page would not know either. Basically that is not such a big problem. If it was the wrong one, and timeout happens, the page can ask again, and again,... until the user realizes the wrong adapter was used. Which is not much different from the case when we would have had 2 adapter objects.

When reading from 2 adapters far enough to not have interference/race, we don't have a problem merging them. When they are too close, it gets non-deterministic, but similarly as above, in the end the use case is handled the same way.

In summary, we can try to implement this, and modify the spec accordingly.

This would also have the advantage of attaching permissions to the leaves, i.e. navigator.nfc.watch() and navigator.nfc.push().

@zolkis
Copy link
Contributor

zolkis commented Oct 26, 2015

However, it is tricky to handle errors with the merged model. If you have multiple adapters, and a push, or setting a watch succeeds on some of them and fails on others, how do we handle the situation?

If we say succeeding on any adapter is success, from where would the user know which adapter is usable?

Without explicit adapter enumeration and management (which I would like to avoid), what is better:

  1. making the user, or the UA select an adapter, then every NFC operation belongs to that adapter, or
  2. using all adapters transparently (those that have a connected device), and consider any success a success and resolve the Promise, or
  3. using all adapters transparently (those that have a connected device), and consider any failure a failure and reject the Promise.

Note that 1. is possible with the navigator.nfc model as well, e.g.

1.a.) by using a Web NFC specific policy, like:

  • select the built-in adapter if no external is connected and there is only one built-in adapter, or
  • select the connected adapter if only one external adapter is connected,
  • otherwise show a dialog when nfc.push() or nfc.watch() is invoked first time (but then you cannot change it until the page is reloaded),

or

1.b.) by using an explicit (new) nfc.selectAdapter() method which triggers a selection dialog, but the default policy would be active (i.e. there always would be a default adapter, if there is any).

Please choose between 1.a., 1.b., 2, 3, or propose another policy.

Personally I would be most comfortable with 1.b. (or 1.a. in the first version), and I would like to avoid using all the adapters in parallel (2 and 3).

In this case the API would look like:

partial interface Navigator {
    readonly attribute NFC nfc;
};

interface NFC {
    Promise<void> push(NFCMessage message, optional NFCPushOptions options);
    void cancelPush(NFCPushTarget target);

    Promise<long> watch(NFCWatchOptions options, MessageCallback callback);
    Promise<void> unwatch(long id);

   // Promise<void> selectAdapter();  // triggers a dialog
};

(NB. The other proposed changes, e.g. content handling and removing push target etc. will be discussed separately)

@jyasskin
Copy link
Member

I think it makes sense to take the first result from any adapter and resolve the promise with it. In most cases, that would mean option (3), that if any adapter fails to set a watch or publish a message, the overall promise would reject with that adapter's result, but we wouldn't want to wait for a failure past any adapter receiving or successfully pushing a message.

With this model, a user with multiple adapters, one of which is broken, may have some trouble figuring out which one to disconnect (although we might be able to help with the rejection message), but I expect that to be less common than a user with multiple working adapters who's confused by which one was picked as the default or by the selection dialog. (And both of these will be much less common than the user with just one adapter, who's the only person developers will actually write code for.)

@zolkis
Copy link
Contributor

zolkis commented Oct 27, 2015

I agree that in 95+% of the cases there will be one adapter. However, the policy you suggest is too obscure. Probably hard to formulate as exact algorithmic steps, but beyond that I am more scared of letting developers and users deal with all possible combination of errors that may occur with various scenarios of I/O and radio races, without any slight hint where it went wrong. IMO that would bring a really bad user, and developer experience.

A priority list like external, internal, and then (if ever needed) a selection dialog for any other case would make it very clear to the user which adapter to use. Also, when something goes wrong, both users and developers know where and what happened.

If there is one adapter, or even when there is one internal and one external adapter, this works as well as your proposal, and covers a tiny bit more use cases, say 99+% of them :).

Indeed we'd lose the ability to handle concurrent writes and reads across all connected adapters, but I doubt we have a use case for that. Quite the contrary, with NFC use cases (and not only payment, though it's in future versions) you want to be certain which adapter you are using.

Also, it leaves an open path forward to support adapter selection on user demand, while keeping the simple interface which was the primary goal after all.

@alexshalamov
Copy link

I'm in favor of option (3).
As I mentioned before, "multiple adapters" scenario is highly improbable and degrades usability of API for 99.9% of use-cases.

Promise selectAdapter(); function also degrades API usability.
Developers would need to think about things like:

  1. Should it be called before push / watch?
  2. If promise is resolved => new adapter selected or same adapter selected?
  3. If promise is resolved and new adapter is selected what happens with push / watch that were invoked for old adapter?
  4. If promise rejected => dialog was cancelled? Can developer still call push / watch?
  5. If external usb adapter was selected and then physically removed, do we switch back to internal, do we reject all pending promises?

I'm for simple API that doesn't have quirks due to unrealistic scenarios.

@zolkis
Copy link
Contributor

zolkis commented Oct 27, 2015

I did not propose selectAdapter() at this step, only a policy to select a default adapter, which sometimes would pop up a selection dialog if there are multiple external adapters. Not very likely to occur, but this would make a clear situation: we use one adapter, and we know which one.

But if we speak about selectAdapter():

  1. you can call it any time you choose, probably create a selection button that the user can use
  2. new or old adapter can be selected, according to user choice; if the same is selected, nothing changes.
  3. If another adapter is selected, the current one needs to be released, but the watches, and even push buffers can relate to the new adapter.
  4. When the dialog is rejected, the current adapter is not changed. The developer can still push/watch.
  5. Yes we should switch back to the internal. It would probably be good to reject the pending promises with an error message that tells the adapter was plugged out/changed. Or, we could transparently switch and maintain the watches and push buffer(s). This is not different from how you would handle concurrent adapters. But you get rid of the nightmare of undeterministic error handling of concurrent adapters with only one promise/error message to give. It is wrong in principle, though you don't meet the error condition often.

The API is the same simple in all cases, and all options will have quirks. I am for the quirks which can be resolved, not the ones for which we hope for the best.

@zolkis
Copy link
Contributor

zolkis commented Oct 27, 2015

We can also have a 4th option: forget the user dialog, and just have a policy with a priority list. Select the internal adapter, unless there is an external, and in case there are more, select the last which was plugged in. The essence here is to always have one adapter active with Web NFC, and to know which, by policy in this case.

The concurrent use of adapters has the problems described above. I wonder how should we write the steps vs error handling. If you are adamant on this is what you want, try writing the push algorithm with this in mind and I will be happy to review it. I can also give it a try.

Watches and push buffers can be made independent from the adapter, reflecting developer choice.

It ultimately boils down to whether is having a user selection dialog a problem, and whether we explicitly want concurrent handling of multiple adapters.

@kenchris
Copy link
Contributor Author

Let's not add a selectAdapter or similar method. Developers would rarely use it, which means that it is as good as not having it in the first place and as good as not supporting multiple adapters

@zolkis
Copy link
Contributor

zolkis commented Oct 28, 2015

We can do without selectAdapter, and we can support multiple adapters as well as one with a priority list. No change to the API is needed. But an algorithm on how to handle multiple adapters when they are present should be in the spec, even if we restrict functionality to one and the same adapter per browsing session.

@zolkis
Copy link
Contributor

zolkis commented Oct 28, 2015

So I have got answer to one of my key questions: do we want/tolerate selection dialog. The quite unanimous answer has been no. We can take that as a group decision. If ever the need arises, it is quite easy to add.

The other key question on which we need group decision: in the rare case we have multiple adapters, should the NFC object be bound to only one of them, or allow working with all of them in parallel?

In native world, adapters work in parallel. Any of the adapters which get in proximity with an NFC device (peer or tag) will work both on pushing and reading. However, they will have different buffers and will not present problems that we would have with Web NFC if we allowed parallelism.

I do not like the narrative that "let's disregard the simultaneous push error mess since it's not likely to happen". That may be fine in an implementation, but not in a spec.

We could say in the spec that implementations are free to choose the policy of handling multiple adapters, either by exposing only one of them and make the selection, or by handling in parallel.

However, even then the spec should have an algorithm either for selecting an adapter (standardize the priority list), or on how to handle errors. I do not have a good solution for the latter. I have tried to write the relevant push steps and I am stuck.

So if you would like to see multiple adapters in work, please specify the push steps.
We can give it some time.

Unless we can do that with reasonable correctness, IMO we have no other choice than selecting an adapter. We could do that the first time NFC functionality is accessed, either through push or watch calls. Then, we could support reacting to plugged in new adapters by applying the priority list based policy, or just say in the spec the page should be reloaded for applying that policy.

Make your choices.

@zolkis
Copy link
Contributor

zolkis commented Oct 28, 2015

One correction to the above: in native, multiple adapters likely won't actually work in the same time and we will get errors from all of them, which is a clean situation: we reject the push Promise. The question is the following: can some of them succeed and some of them fail, because that is the main problem in resolving or rejecting the push Promise.

If we choose option 3 and reject on any error, it can be misleading, since some of the writes might have happened, and while we report error, we have already changed the data on some of the tags, so we lied. Option 2 is the reverse: we report success, whereas some of the writes didn't happen.

While you can say we don't care about that case because it is unlikely, that is to say we don't care if the algorithm is correct, I think it would be better then to expose only one adapter at any given time.

It is true this would limit read functionality, and add more things to implement (priority list policy).

If someone can formulate reasonably correct push steps, I would be fine with the parallel adapters idea as well. But we need to make a choice, and soon.

@alexshalamov
Copy link

For multiple adapters (should be applicable for N adapters):

One pending push operation: push(tag)

Case 1: One physical NFC tag OR one NFC tag + one peer

  • One of adapters pushes => resolve / reject

Case 2: Two physical NFC tags are activated simultaneously OR one tag is being written while another enters RF field generated by second adapter

  • When push completes by both adapters => resolve
  • Any of push operations fails => reject

Two pending push operations: push(tag) + push(peer)

Case 3: One physical NFC tag

  • When push suceeds resolve promise for push(tag)
  • Keep push(peer) unresolved

Case 4: Two tags (similar to Case 2)

  • When push completes by both adapters => resolve push(tag)
  • Any of pushes fails => reject push(tag)
  • Keep push(peer) unresolved

Case 5: One peer

  • Resolve / reject push(peer)
  • Keep push(tag) unresolved

Case 6: Two peers (similar to Case 2, but with NFC peers)

  • When push completes on both adapters => resolve push(peer)
  • Otherwise reject push(peer)
  • Keep push(tag) unresolved

At the moment I'm rewriting POC with following interface:

partial interface Navigator {
  readonly attribute NFC nfc;
};

interface NFC {
  Promise<void> push (sequence<NFCRecord> records, optional NFCPushOptions options);
  Promise<void> cancelPush (NFCPushTarget target);
  Promise<long> watch (MessageCallback callback, optional NFCWatchOptions options);
  Promise<void> unwatch (long id);
}

Note that cancelPush returns promise, since operation is asynchronous and I need to check from platform whether it is possible to cancel push.
Also watch(...) has reversed parameters, IMO it is easier to write:

// Watch options are defaulted
navigator.nfc.watch(callback);

@zolkis
Copy link
Contributor

zolkis commented Oct 28, 2015

The NFC interface looks good to me. It is also in line with Travis' comments. Actually I have already updated the spec with that in my fork, except the watch parameter reversal.

I have a question regarding your examples.

In Case 2 for instance, we have one Promise returned. Now say one push (write) succeeded and one failed. We reject the promise. The developer may think the tags were not modified, but one of them was. Therefore unless we can unroll partial success as it were a transaction we cannot say the push failed. Nor can we say the push succeeded. One of them succeeded and the other one failed. We would need 2 Promise objects to convey that. If we would return a sequence of Promise objects, one per adapter, we would need to identify them. That is not trivial to do without fingerprintable information that are visible to the page (or if I am wrong here, suggest a solution). Even then I'd not go there. If we choose to not identify the adapters, that may also work, at least the developer knows what is the state, even though it cannot be mapped exactly to adapters. Anyway, it kind of sucks returning an array of Promises.

I see only 2 acceptable ways out: 1. undo the successful write (transaction model) and reject, 2. work with only one adapter. If we choose 1, we need to specify the transaction.

I would like to understand, is it more complex to work with one adapter than with multiple? Or we don't want to lose the parallel reading functionality?

@kenchris
Copy link
Contributor Author

I don't think that anyone cares about unroll. Why not just create an exception/error like PartialWriteException or similar

@zolkis
Copy link
Contributor

zolkis commented Oct 29, 2015

Yeah, transactions and rollback look like overkill, considering the very small likelyhood we run into this corner case: they are not worth implementing.

Then, Jeffrey's suggestion of handling adapters in parallel has more appeal than selecting a single adapter and deal with multiplicity by applying a policy (or selection dialog), and definitely better than not dealing with multiplicity at all. I think that was a nice cut through, kudos. I just have a problem with writing the push steps for it.

Since I cannot imagine a use case which would require simultaneous tapping of multiple tags, could we say in the spec that multiple adapters are supported as long as only one of them is used by the user at any given time, and concurrent use will likely work with reads, but may result in push errors in some of the adapters which means partial success? And in the case of an error, we reject, and the user should try again?

Then we could take option 3, as it got most votes so far.

@kenchris
Copy link
Contributor Author

I am fine with that. I also like this option the most

@zolkis
Copy link
Contributor

zolkis commented Oct 29, 2015

Good. Then we arrived to a consensus and we can use the simple API that @alexshalamov suggested. I will prepare the spec change with these.

Thanks @jyasskin for the "merged" adapter suggestion; once its details were sorted out, it settled a long debate.

@jyasskin
Copy link
Member

In case 2, I'd probably have the UA take the following steps, if the underlying APIs make it possible.

  1. Publish the message to all adapters.
  2. When one adapter discovers a tag/peer and starts writing to it, attempt to unpublish the message from all other adapters.
  3. If any of the other adapters has already started writing by the time the unpublish request arrives, allow it to continue.
  4. Wait until all of the active writes attempts succeed or fail.
  5. If any succeeded, fulfil the promise. Otherwise, reject it.

I'm suggesting to fulfil if any succeed because that tells the developer whether the state of the external world changed.

@zolkis
Copy link
Contributor

zolkis commented Oct 29, 2015

Now that is option 2 (resolve if any pushes succeed), whereas previously you supported option 3, i.e. reject if any of the writes failed.

If we resolve when any success happen, we have no way telling the user something went wrong and please repeat the operation (and if possible, not concurrently). So at the moment I find option 3 a bit more useful because that feedback, though it is inconsistent on errors as well as option 2.

@jyasskin
Copy link
Member

In #67 (comment), I missed the fact that, even if adapter A starts writing before adapter B starts writing, it might be impossible to cancel the publication on adapter B between when adapter A starts writing and when adapter B finishes writing. My guideline of "take the first result" stands, but since there may be many results, we have to fall back to some other guideline.

It's not really important which option you pick for this race condition: it's not going to happen a significant amount out in the wild. I'd still lean toward reporting that a side-effect happened, but going the other way isn't going to hurt anyone.

@zolkis
Copy link
Contributor

zolkis commented Oct 29, 2015

Then we could use the suggestion from @kenchris : reject the Promise with a new error type like PartialWriteException (find a good name) or DataCloneError, and then we know it is a partial failure (or success). When all operations fail, we reject with NetworkError (which is the error we use for regular push failure).

@jyasskin
Copy link
Member

Another principle is to avoid making developers write code to handle cases that almost never happen. They mostly won't do it. Try to make the code they will write Just Work.

zolkis added a commit that referenced this issue Nov 3, 2015
Fix #65. Improve requestAdapter() steps based on #67 discussion.
zolkis added a commit to zolkis/web-nfc that referenced this issue Nov 4, 2015
…. Use the merged NFC adapter concept for multiple adapters. Update examples.
@zolkis
Copy link
Contributor

zolkis commented Nov 6, 2015

Fixed by #88.

@zolkis zolkis closed this as completed Nov 6, 2015
@plehegar plehegar added the tag-tracker Group bringing to attention of the TAG, or tracked by the TAG but not needing response. label Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F2F tag-tracker Group bringing to attention of the TAG, or tracked by the TAG but not needing response.
Projects
None yet
Development

No branches or pull requests

5 participants