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

Add support for L2CAP and refactor state management #32

Merged
merged 20 commits into from
Sep 6, 2024

Conversation

todd-spruceid
Copy link
Contributor

iOS reader/holder now support L2CAP.

This is a fairly large change; both the holder and the reader have been extended to support L2CAP. In service of this, the original flow has been refactored into more explicit state machines, and the L2CAP flow has been added to those state machines.

Both the reader and holder have useL2CAP booleans; if either is false, the reader and holder will use the old flow to communicate. This should also mean that they will use the old flow when working with other readers or holders that do not support L2CAP.

Some of this work (notably the *Connection.swift files) is derived from Paul Wilkinson's MIT-licensed L2Cap library:

https://github.com/paulw11/L2Cap

This repo is MIT-licensed as well, so the licenses are compatible.

We aren't using the L2Cap library as-is for a variety of reasons, but the main reason is that the behaviour we need differs significantly from the behaviour L2Cap (the library) offers.

There are a couple of places in this change that are hacking around impedance mismatches:

  • we seem to need to have notify on the L2CAP characteristic in order to propagate the PSM to central, though 18013-5 Annex A claims the only required property is read -- we're not out of spec, since we're offering an optional property, but it remains to be seen how that will interact with 3rd party centrals

  • 18013-5 specifies no framing for the request or response sent over the L2CAP stream, so we have to infer when the data has arrived from timing; this seems like a fragile method particularly if confronted by noisy radio environments

sbihel and others added 13 commits August 29, 2024 10:34
Not sure how it was checked out as it is in .gitignore
This is a fairly large change; both the holder and the reader have been
extended to support L2CAP.  In service of this, the original flow has been
refactored into more explicit state machines, and the L2CAP flow has been
added to those state machines.

Both the reader and holder have `useL2CAP` booleans; if either is `false`,
the reader and holder will use the old flow to communicate.  This should
also mean that they will use the old flow when working with other readers
or holders that do not support L2CAP.

Some of this work (notably the `*Connection.swift` files) is derived from
Paul Wilkinson's MIT-licensed L2Cap library:

https://github.com/paulw11/L2Cap

This repo is MIT-licensed as well, so the licenses are compatable.

We aren't using the L2Cap library as-is for a variety of reasons, but the
main reason is that the behaviour we need differs significantly from the
behaviour L2Cap (the library) offers.

There are a couple of places in this change that are hacking around
impedence mismatches:

- we seem to need to have `notify` on the L2CAP characteristic in order
  to propagate the PSM to central, though 18013-5 Annex A claims the
  only required property is `read` -- we're not out of spec, since we're
  offering an optional property, but it remains to be seen how that will
  interact with 3rd party centrals

- 18013-5 specifies no framing for the request or response sent over the
  L2CAP stream, so we have to infer when the data has arrived from timing;
  this seems like a fragile method particularly if confronted by noisy
  radio environments
@todd-spruceid
Copy link
Contributor Author

This needs an up-to-date version of mobile-sdk-rs to work properly.
There are some linter errors I'll look at tomorrow, though some of them look like the linter being foolish; it's complaining about cyclomatic complexity on a UUID to string mapping function, for instance, because it does return switch(uuid) instead of having the switch store a temporary value and returning it.

Copy link
Member

@sbihel sbihel left a comment

Choose a reason for hiding this comment

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

+1 for the use of an internal state machine!

Additional comments:

  1. Could you rebase your branch on the latest main to get rid of the commits from Add MDoc Reader support #22?
  2. It looks like you ran swift-format on the codebase. Currently, we're using SwiftLint and it's unclear to me whether they can be used together, but after a quick test it looks like their default settings are incompatible. Regardless, I'm open to switching to swift-format but it would need to happen in a separate PR and added to the CI.
  3. Your mention of PSM reminded me of this project https://github.com/openwallet-foundation-labs/identity-credential (PR from a while ago, do you think you could look into it and see if we could test our implementation against it?
  4. Did you test the GATT flow to make sure it didn't break?

Sources/MobileSdk/BLEConnection.swift Outdated Show resolved Hide resolved
Sources/MobileSdk/BLEConnection.swift Show resolved Hide resolved
Sources/MobileSdk/MDocHolderBLECentral.swift Outdated Show resolved Hide resolved
Sources/MobileSdk/MDocHolderBLECentral.swift Outdated Show resolved Hide resolved
@sbihel sbihel changed the title Skit 542 add l2cap support for ios Add support for L2CAP and refactor state management Sep 5, 2024
@todd-spruceid
Copy link
Contributor Author

  1. Could you rebase your branch on the latest main to get rid of the commits from Add MDoc Reader support #22?

I'm doing that now.

  1. It looks like you ran swift-format on the codebase. Currently, we're using SwiftLint and it's unclear to me whether they can be used together, but after a quick test it looks like their default settings are incompatible. Regardless, I'm open to switching to swift-format but it would need to happen in a separate PR and added to the CI.

I thought we were using swift-format, so I ran it, but it is quite invasive; it doesn't just change the layout, it also makes syntactic changes. I'm not advocating for its use.

  1. Your mention of PSM reminded me of this project https://github.com/openwallet-foundation-labs/identity-credential (PR from a while ago, do you think you could look into it and see if we could test our implementation against it?

I'll look into it.

  1. Did you test the GATT flow to make sure it didn't break?

I did; that's why there are the two useL2CAP booleans in the reader and holder. Manually setting one (or both) to false makes them use the old flow, and it still works. I tested both with the reader not offering L2CAP and with the holder choosing not to engage via L2CAP, and both completed properly using the old flow. You can see which route is taken in the logs, particularly in how the response is transferred (ie: chunked or not), but also in the state names printed by updateState().

@todd-spruceid todd-spruceid requested a review from sbihel September 5, 2024 17:54
@todd-spruceid todd-spruceid merged commit e1e8d12 into main Sep 6, 2024
1 check passed
@todd-spruceid todd-spruceid deleted the skit-542-add-l2cap-support-for-ios branch September 6, 2024 16:00
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

Successfully merging this pull request may close these issues.

3 participants