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

[Python] Add python commissioning flow #85

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

erjiaqing
Copy link
Owner

This PR introduces manually commissioning flow in Python, with a example of commissioning without autocommissioner. For testing potential commissioning flow changes in the future since it is more flexible. It can also be used to test a few corner cases during commissioning, including bad certificate chain etc.

This PR also splits the Controller in Python into two parts, ControllerBase (like Controller in C++, actually backed with a Commissioner with dummy CertificateIssuer), and Controller (the Commissioner), the ControllerBase can be initialized with a certificate signed by someone else.

This PR only includes changes to Python scripts and necessary C++ interfaces.

The changes to CHIP Tool will be done later in separate PR.

if isinstance(parameter, commissioning.PaseOverBLEParameters):
devCtrl.EstablishPASESessionBLE(parameter.setup_pin, parameter.discriminator, parameter.temporary_nodeid)
elif isinstance(parameter, commissioning.PaseOverIPParameters):
devCtrl.EstablishPASESessionIP(parameter.address, parameter.setup_pin, parameter.temporary_nodeid)

Choose a reason for hiding this comment

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

We should not using directly addresses. We should take CommissionableNode for PaseOverIPParameters.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Resolved, now establish_session will resolved the address.

Comment on lines 109 to 114
self.execute_device_cmd(req_device_id, "pip3 install {}".format(os.path.join(
CHIP_REPO, "out/debug/linux_x64_gcc/controller/python/chip_clusters-0.0-py3-none-any.whl")))
self.execute_device_cmd(req_device_id, "pip3 install {}".format(os.path.join(
CHIP_REPO, "out/debug/linux_x64_gcc/controller/python/chip_core-0.0-cp37-abi3-linux_x86_64.whl")))
self.execute_device_cmd(req_device_id, "pip3 install {}".format(os.path.join(
CHIP_REPO, "out/debug/linux_x64_gcc/controller/python/chip_repl-0.0-py3-none-any.whl")))

Choose a reason for hiding this comment

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

Is there a way to reuse machinery we already have?

Copy link
Owner Author

Choose a reason for hiding this comment

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

We have a few patterns like this in the cirque test, will do a cleanup and add a help function in the CHIPVirtualHome class

Comment on lines -237 to -247
if name is None:
self._name = "caIndex(%x)/fabricId(0x%016X)/nodeId(0x%016X)" % (fabricAdmin.caIndex, fabricId, nodeId)
else:
self._name = name

Choose a reason for hiding this comment

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

This name allocation logic was OK

c_void_p), c_uint64, c_uint64, c_uint16, c_char_p, c_bool, c_bool, POINTER(c_uint32), c_uint32]
self._dmLib.pychip_OpCreds_AllocateController.restype = PyChipError

self._dmLib.pychip_OpCreds_AllocateControllerForCustomCommissioningFlow.argtypes = [

Choose a reason for hiding this comment

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

Why is this a special case? We should be able to make this possible (externally initialized controller credentials) even for non-custom-flow commissioning.

''' A bare device controller without commissioner support.
'''

def __init__(self, ephemeralKey: bytes, noc: bytes, icac: bytes, rcac: bytes, adminVendorId: int, name: str = None):
Copy link

@tcarmelveilleux tcarmelveilleux Jan 9, 2023

Choose a reason for hiding this comment

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

Please wrap the P256Keypair here so it can be passed from a Python bridge, rather than raw bytes. For working with an external CA, we won't have the raw bytes, but sometimes we will. It's easier to abstract away by passing the keypair in.

Also, as mentioned in other comment, we need to include the IPK or add a method to replace the IPK once we have it from the credentials provider.

namespace chip {
namespace Python {

class DLL_EXPORT DummyOperationalCredentialsIssuer : public Controller::OperationalCredentialsDelegate

Choose a reason for hiding this comment

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

Rename Dummy to Placeholder everywhere

Comment on lines 47 to 53
// Constructor to create an instance of this object that vends out operational credentials for a given fabric.
//
// An index should be provided to numerically identify this instance relative to others in a multi-fabric deployment. This is
// needed given the interactions of this object with persistent storage. Consequently, the index is used to scope the entries
// read/written to/from storage.
//
// It is recommended that this index track the fabric index within which this issuer is operating.

Choose a reason for hiding this comment

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

Remove/adapt stale comments.

@erjiaqing erjiaqing force-pushed the repl/python-commissioner branch 2 times, most recently from c42acb7 to b4da241 Compare February 23, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants