search

project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.48k stars 2k forks source link

Sort out what Advertise should actually be doing #8955

Closed bzbarsky-apple closed 2 years ago

bzbarsky-apple commented 3 years ago

I spent some time staring at Advertise(bool commissionableNode) and I don't understand what this code is even trying to do.... I think the additionalPairing boolean can basically be set to HaveOperationalCredentials(), but what does the modeEnabled arg to CommissionAdvertisingParameters::SetCommissioningMode even mean? Should it just be set to commissionableNode? Something else? None of this stuff is documented and the naming doesn't match up to spec concepts very well. :(

@cecille @andy31415 @chrisdecenzo

_Originally posted by @bzbarsky-apple in https://github.com/project-chip/connectedhomeip/pull/8923#discussion_r687917074_

cecille commented 3 years ago

What is this issue trying to address? Are you looking for more documentation? A re-write? Or are you saying there is a bug?

bzbarsky-apple commented 3 years ago

The context is that the existing code is clearly not correct, because it uses IsFullyProvisioned, which does not correspond to useful provisioning state in practice.

What neither I nor @msandstedt could figure out is exactly what this code is trying to do, which makes it hard to make it do it.

So:

  1. The code is buggy.
  2. There is not enough documentation about what this code is meant to do and what the various APIs involved are meant to do, so we don't know exactly how to fix the bugginess.
stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

bzbarsky-apple commented 2 years ago

We no longer use IsFullyProvisioned here, and the API surface has been changed some and better documented.