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

[Rendezvous] Add BLEEndPoint support to the iOS app #1460

Conversation

vivien-apple
Copy link
Contributor

Problem

This patch add BlePlatformDelegate and BleApplicationDelegate capabilities to the iOS app and allow the app to send a ping to an esp32 with a BLE echo server (#1458).

The current code of the patch basically setup a BLE connection if the QR code specify a BLE rendezvous and send a ping to the peripheral to check that the BLEEndPoint code is working.

The current patch still has some warnings about retain cycles and nil property that should not be assigned. That said those warnings comes from part of the code that I took from OW and I would like to fix them in followups since I would like to have this patch landed to move forwards.

That's a good base to move forward and the followups that I can foresee are:

  • Clean the selfwarnings
  • clean the nilwarnings
  • Move the ble callbacks one level up so the app code access them
  • Add a view to send custom ble messages instead of the raw ping
  • show something on the UI saying that we are now connected to the peripheral and/or disconnected from the peripheral
  • while connected regularly send ping like commands to keep the connection active
  • Use the right callback queue
  • Unify the cpp callbacks between BLE and the transport code. Right now most of the callbacks for transports seems to assume IP and/or a secure session. We definitively needs more love there.

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

I believe that if we use BLE as a transport, it should be transparent to the app. I am worried adding extra transport members into the controllers will make it very bloaty. We decided against including the entire weave message layer because of its strong coupling and we should try to not re-write a siumilar coupling in new code.

@gerickson
Copy link
Contributor

I believe that if we use BLE as a transport, it should be transparent to the app. I am worried adding extra transport members into the controllers will make it very bloaty. We decided against including the entire weave message layer because of its strong coupling and we should try to not re-write a siumilar coupling in new code.

Is there any reason to not have @vivien-apple start here and then refactor, as we've done with UDP, to fold BLE into the overall transport abstraction? If we can quickly refactor up front and then have @vivien-apple leverage that, that's fine too.

@@ -85,6 +87,11 @@ - (instancetype)init

_lock = [[NSRecursiveLock alloc] init];

_WorkQueue = dispatch_queue_create(CHIP_WORK_QUEUE, DISPATCH_QUEUE_SERIAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

lowercase _workQueue please :)

src/darwin/Framework/CHIP/ChipBleDelegate.h Show resolved Hide resolved
@andy31415
Copy link
Contributor

andy31415 commented Jul 7, 2020

I believe that if we use BLE as a transport, it should be transparent to the app. I am worried adding extra transport members into the controllers will make it very bloaty. We decided against including the entire weave message layer because of its strong coupling and we should try to not re-write a siumilar coupling in new code.

Is there any reason to not have @vivien-apple start here and then refactor, as we've done with UDP, to fold BLE into the overall transport abstraction? If we can quickly refactor up front and then have @vivien-apple leverage that, that's fine too.

Discussed offline - we will try to see what it takes to make it a transport, but if that seems to take too long we will go back to this commit and add github issues to port over.

My concern is that once this goes in and starts being used, refactor will be difficult due to merge conflicts, hence the request to attempt an actual transport. Agree that if that seems to be too time consuming to just move forward through this PR instead - at least we tried.

@woody-apple
Copy link
Contributor

I believe that if we use BLE as a transport, it should be transparent to the app. I am worried adding extra transport members into the controllers will make it very bloaty. We decided against including the entire weave message layer because of its strong coupling and we should try to not re-write a siumilar coupling in new code.

Is there any reason to not have @vivien-apple start here and then refactor, as we've done with UDP, to fold BLE into the overall transport abstraction? If we can quickly refactor up front and then have @vivien-apple leverage that, that's fine too.

Discussed offline - we will try to see what it takes to make it a transport, but if that seems to take too long we will go back to this commit and add github issues to port over.

My concern is that once this goes in and starts being used, refactor will be difficult due to merge conflicts, hence the request to attempt an actual transport. Agree that if that seems to be too time consuming to just move forward through this PR instead - at least we tried.

I agree with making this more general (so as to handle WiFi only setup as well), but just to be clear - this is pre-secure channel setup, so I don't expect the future Rendezvous/Pairing APIs to have much intersection there. These are different transports.

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2020

CLA assistant check
All committers have signed the CLA.

@andy31415
Copy link
Contributor

I believe that if we use BLE as a transport, it should be transparent to the app. I am worried adding extra transport members into the controllers will make it very bloaty. We decided against including the entire weave message layer because of its strong coupling and we should try to not re-write a siumilar coupling in new code.

Is there any reason to not have @vivien-apple start here and then refactor, as we've done with UDP, to fold BLE into the overall transport abstraction? If we can quickly refactor up front and then have @vivien-apple leverage that, that's fine too.

Discussed offline - we will try to see what it takes to make it a transport, but if that seems to take too long we will go back to this commit and add github issues to port over.
My concern is that once this goes in and starts being used, refactor will be difficult due to merge conflicts, hence the request to attempt an actual transport. Agree that if that seems to be too time consuming to just move forward through this PR instead - at least we tried.

I agree with making this more general (so as to handle WiFi only setup as well), but just to be clear - this is pre-secure channel setup, so I don't expect the future Rendezvous/Pairing APIs to have much intersection there. These are different transports.

@vivien-apple - were you able to attempt to get a BLE transport working? Is it feasable or should we submit this type of integration as an intermediate?

@vivien-apple
Copy link
Contributor Author

I believe that if we use BLE as a transport, it should be transparent to the app. I am worried adding extra transport members into the controllers will make it very bloaty. We decided against including the entire weave message layer because of its strong coupling and we should try to not re-write a siumilar coupling in new code.

Is there any reason to not have @vivien-apple start here and then refactor, as we've done with UDP, to fold BLE into the overall transport abstraction? If we can quickly refactor up front and then have @vivien-apple leverage that, that's fine too.

Discussed offline - we will try to see what it takes to make it a transport, but if that seems to take too long we will go back to this commit and add github issues to port over.
My concern is that once this goes in and starts being used, refactor will be difficult due to merge conflicts, hence the request to attempt an actual transport. Agree that if that seems to be too time consuming to just move forward through this PR instead - at least we tried.

I agree with making this more general (so as to handle WiFi only setup as well), but just to be clear - this is pre-secure channel setup, so I don't expect the future Rendezvous/Pairing APIs to have much intersection there. These are different transports.

@vivien-apple - were you able to attempt to get a BLE transport working? Is it feasable or should we submit this type of integration as an intermediate?

@andy31415 Was OoO for a couple days. French national holidays here. I'm still working on getting BLE working with a different approach. I'm fine to hold off on this PR until I have a proof that a different approach works well / does not work well.

@woody-apple
Copy link
Contributor

@andy31415 is your feedback addressed?

@vivien-apple can you fix the merge conflict here?

@vivien-apple
Copy link
Contributor Author

@andy31415 is your feedback addressed?

@vivien-apple can you fix the merge conflict here?

I did not addressed @andy31415 's feedback in this PR. I went with a different approach in https://github.com/vivien-apple/connectedhomeip-1/commits/Darwin_AddBleEndPoint2 that I still needs to submit as a PR.

@vivien-apple
Copy link
Contributor Author

Now that #1660 has landed we can just close this PR. I will continue to improve the abstraction of the Transport::BLE class in followups.

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.

6 participants