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

Fix the commissiong mode of "commissioning for on-network" usecases #8763

Closed
wants to merge 1 commit into from
Closed

Fix the commissiong mode of "commissioning for on-network" usecases #8763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/all-clusters-app/esp32/main/DeviceCallbacks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ void DeviceCallbacks::DeviceEventCallback(const ChipDeviceEvent * event, intptr_
// connectivity. MDNS still wants to refresh its listening interfaces to include the
// newly selected address.
chip::app::Mdns::StartServer();
#ifdef RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE
chip::app::Mdns::AdvertiseCommissionableNode();
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the actual intent of this change? Why do we want to:

  1. Advertise being a commissionable node "unconditionally" (i.e. based on compile-time information only)
  2. But only when RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE, which is generally false

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bzbarsky-apple Intent of this change was to advertise the device as a commissionable node for On-Network Commissioning where RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know what the code is doing. The question is why we want to advertise the device in that way exactly when that compile-time constant is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

IsServiceProvisioned is legacy from OpenWeave data and the "service" provisioning data should not be touched/used.

RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE should not be the gate. It's very clear what should cause commissionable for "already on network": an uncommissioned device but already connected to an IP network (e.g. Thread, Wifi, Ethernet) through non-Matter means, or a device already commissioned after receiving a request to open a commissioning window.

An commissioned device should not be advertising commissionable unless it can be be put into commissioning mode (e.g. some TV usecases).

#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to do this though a CLI or shell command (similar to option present on M5stack through a button) instead of adding it here? Something like:
$ force-wifi-commissioning

}
break;
}
Expand Down
1 change: 1 addition & 0 deletions src/include/platform/internal/GenericConfigurationManagerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,7 @@ bool GenericConfigurationManagerImpl<ImplClass>::_IsFullyProvisioned()
#if CHIP_DEVICE_CONFIG_ENABLE_JUST_IN_TIME_PROVISIONING
(!UseManufacturerCredentialsAsOperational() && _OperationalDeviceCredentialsProvisioned()) &&
#endif
(mFlags.Has(Flags::kIsServiceProvisioned) && mFlags.Has(Flags::kOperationalDeviceCredentialsProvisioned)) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can someone check if this is the correct thing to do and won't break any other platform? cc @cecille @bzbarsky-apple

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change still needed now that #8454 has merged?

In any case, this does not look right because outside of TestConfigurationMgr it looks to me like kIsServiceProvisioned is never set. And kOperationalDeviceCredentialsProvisioned looks like it's never set at all.

Copy link
Contributor Author

@PSONALl PSONALl Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@bzbarsky-apple , actually this change was added because in the case of On-Network commissioning _IsFullyProvisioned is returning true after getting credentials of AP, which causes code to break and Node doesn't advertise Commissionable Mode = 1 after it gets connected to AP. _IsFullyProvisioned should return false as the node is not fully provisioned yet, so to determine if the node is fully provisioned or not, It should check here if operational device credentials are provisioned, is this conclusion 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 would pretty much assume that any use of IsFullyProvisioned is broken.... Whatever code is meant to advertise Commissionable Mode = 1 should presumably just check whether we have operational credentials directly. That's what Mdns::StartServer ends up doing (by calling the kinda-broken GetCurrentNodeId; we should have a way to actually ask the boolean "do we have an opcred?" question we really want to ask.

Does kOperationalDeviceCredentialsProvisioned ever get set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, You are right.. kOperationalDeviceCredentialsProvisioned is never set

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. So fundamentally:

  1. IsFullyProvisioned in its current form can't really be relied on to ever become true; see Operational advertisement on startup is broken on Linux for on-network pairing (and various other cases) #7911 for another example, where any device with thread or wifi support that is actually connecting via ethernet will always have IsFullyProvisioned false.
  2. We should probably not add new conditions to IsFullyProvisioned
  3. Whatever code is misbehaving because it uses IsFullyProvisioned should probably be changed to test the things it actually cares about.

// TODO: Add checks regarding fabric membership (IsMemberOfFabric()) and account pairing (IsPairedToAccount()),
// when functionalities will be implemented.
true;
Expand Down