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.

Sign up for GitHub

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

Jump to bottom

Adds RendezvousSession inside src/transport #2508

Merged
merged 1 commit into from
Sep 14, 2020
Merged

Adds RendezvousSession inside src/transport #2508

merged 1 commit into from
Sep 14, 2020

Conversation

vivien-apple
Copy link
Contributor

RendezvousSession manages the initial connection between a commissioner and a device.
During rendezvous a key pair is generated by SecurePairingSession under the hood.

Embedders can listen to the different states of Rendezvous by implementing
the RendezvousSessionCallback interface. The differents events are:

  • OnRendezvousConnectionOpened();
  • OnRendezvousConnectionClosed();
  • OnRendezvousError(CHIP_ERROR err);
  • OnRendezvousMessageReceived(System::PacketBuffer * buffer);

fixes #2394

@vivien-apple vivien-apple self-assigned this Sep 8, 2020
@restyled-io restyled-io bot mentioned this pull request Sep 8, 2020
Closed
@vivien-apple vivien-apple force-pushed the Rendezvous_SessionSecurePairingLand branch from 44c9814 to 4abce27 Compare September 8, 2020 16:21
rwalker-apple
rwalker-apple reviewed Sep 8, 2020
View reviewed changes
examples/wifi-echo/server/esp32/main/RendezvousDeviceDelegate.cpp Outdated

mRendezvousSession = new RendezvousSession(this, RendezvousParameters(setupPINCode).SetLocalNodeId(kLocalNodeId));
err = mRendezvousSession->Init();
SuccessOrExit(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a no-op because the very next instruction is exit: ? I'm ok with this for uniformity's sake, but would you also do this at line 66 in such case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like to do it at the end in case someone is adding code after it. But you're right that I need to stay consistent. Will fix this one or line 66.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

rwalker-apple
rwalker-apple reviewed Sep 8, 2020
View reviewed changes
src/transport/SecurePairingSession.h Outdated
*
* @return Optional<NodeId> The associated peer NodeId
*/
const Optional<NodeId> & GetPeerNodeId() const { return mPeerNodeId; };
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unfamiliar with this syntax, is better than returning nullptr if mPeerNodeId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there will be more code changes as I would need to change mPeerNodeId to be a pointer otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@vivien-apple
Copy link
Contributor Author

LGTM fails on ../../src/platform/Linux/ThreadStackManagerImpl.h:26:10: fatal error: dbus/client/thread_api_dbus.hpp: No such file or directory
Sounds unrelated to my changes.

@vivien-apple vivien-apple force-pushed the Rendezvous_SessionSecurePairingLand branch from 4abce27 to 2c4e7a4 Compare September 10, 2020 08:28
sagar-apple
sagar-apple reviewed Sep 10, 2020
View reviewed changes
examples/wifi-echo/server/esp32/main/include/RendezvousDeviceDelegate.h

using namespace ::chip;

class RendezvousDeviceDelegate : public RendezvousSessionDelegate
Copy link
Contributor

@sagar-apple sagar-apple Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Might be outside the scope of this PR, but I'm wondering if this class should purely be getPinCode(), onOpen(), onClose(), and onError() callbacks. The Sending and Receiving of Messages can eventually be handled internally within CHIP as it might be common for most accessories right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's mainly driven by the separation of handling of WiFi credentials on the device side.

@pan-apple
Copy link
Contributor

@vivien-apple we should eventually move the logic to handle IP address processing (on controller side) to RendezvousSession class. It's currently being passed to the application layer, where it calls ConnectDevice to associate the IP address with the session.

@vivien-apple
Copy link
Contributor Author

vivien-apple commented Sep 10, 2020 •
edited
Loading

@vivien-apple we should eventually move the logic to handle IP address processing (on controller side) to RendezvousSession class. It's currently being passed to the application layer, where it calls ConnectDevice to associate the IP address with the session.

The controller side code that received the IP will be removed once there is a mechanism to discover the device on the network.

@vivien-apple vivien-apple force-pushed the Rendezvous_SessionSecurePairingLand branch from 2c4e7a4 to f3345a9 Compare September 11, 2020 16:14
@rwalker-apple
Copy link
Contributor

rwalker-apple commented Sep 11, 2020 •
edited
Loading

LGTM fails on ../../src/platform/Linux/ThreadStackManagerImpl.h:26:10: fatal error: dbus/client/thread_api_dbus.hpp: No such file or directory
Sounds unrelated to my changes.

I wonder if a rebase will address that. We don't have a way to land this without Admin help unless LGTM passes.

@vivien-apple
Copy link
Contributor Author

vivien-apple commented Sep 11, 2020 •
edited
Loading

LGTM fails on ../../src/platform/Linux/ThreadStackManagerImpl.h:26:10: fatal error: dbus/client/thread_api_dbus.hpp: No such file or directory
Sounds unrelated to my changes.

I wonder if a rebase will address that. We don't have a way to land this without Admin help unless LGTM passes.

Let me have a look. It may be my bad after all. It seems like the inclusion of CHIPDeviceLayer in src/transport/BLE.cpp trigger that.

 In file included from ../../src/include/platform/ThreadStackManager.h:167,
 from ../../src/include/platform/internal/GenericConnectivityManagerImpl_Thread.h:28,
                  from ../../src/platform/Linux/ConnectivityManagerImpl.h:30,
                  from ../../src/include/platform/ConnectivityManager.h:250,
                  from ../../src/include/platform/CHIPDeviceLayer.h:28,
                  from ../../src/transport/BLE.cpp:31:
 ../../src/platform/Linux/ThreadStackManagerImpl.h:26:10: fatal error: dbus/client/thread_api_dbus.hpp: No such file or directory
    26 | #include "dbus/client/thread_api_dbus.hpp"

@vivien-apple vivien-apple force-pushed the Rendezvous_SessionSecurePairingLand branch from f3345a9 to 611d988 Compare September 14, 2020 10:31
@vivien-apple
Adds RendezvousSession inside src/transport
e8acc46 
RendezvousSession manages the initial connection between a commissioner and a device.
During rendezvous a key pair is generated by SecurePairingSession under the hood.

Embedders can listen to the different states of Rendezvous by implementing
the RendezvousSessionCallback interface. The differents events are:
 * OnRendezvousConnectionOpened();
 * OnRendezvousConnectionClosed();
 * OnRendezvousError(CHIP_ERROR err);
 * OnRendezvousMessageReceived(System::PacketBuffer * buffer);
@vivien-apple vivien-apple force-pushed the Rendezvous_SessionSecurePairingLand branch from 611d988 to e8acc46 Compare September 14, 2020 19:06
@rwalker-apple rwalker-apple merged commit f12bd0f into project-chip:master Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pan-apple pan-apple pan-apple approved these changes

@jelderton jelderton jelderton approved these changes

@rwalker-apple rwalker-apple rwalker-apple approved these changes

@sagar-apple sagar-apple sagar-apple approved these changes

@BroderickCarlin BroderickCarlin BroderickCarlin approved these changes

@andy31415 andy31415 Awaiting requested review from andy31415

@bzbarsky-apple bzbarsky-apple Awaiting requested review from bzbarsky-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@hawk248 hawk248 Awaiting requested review from hawk248

@mspang mspang Awaiting requested review from mspang

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

@saurabhst saurabhst Awaiting requested review from saurabhst

@shana-apple shana-apple Awaiting requested review from shana-apple

Assignees

@vivien-apple vivien-apple

Labels
controller darwin examples transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add RendezvousSession under src/transport
6 participants
@vivien-apple @pan-apple @rwalker-apple @BroderickCarlin @sagar-apple @jelderton