Repository navigation
[SL-UP] Add commissioning event handling in BaseApplication - #1246
Conversation
- Implemented OnCommissioningSessionEstablishmentStarted and logging for commissioning events in BaseApplication. - Updated MQTT example to log commissioning start and failure events. - Added new event types for commissioning to the DeviceEventType enum.
|
BugBot review |
PR SummaryLow Risk Overview BaseApplicationDelegate implements
Reviewed by Cursor Bugbot for commit 755b174. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 755b174. Configure here.
| ChipDeviceEvent event{}; | ||
| event.Type = DeviceEventType::kSLSystemEventCommissioningStarted; | ||
| LogErrorOnFailure(PlatformMgr().PostEvent(&event)); | ||
| } |
There was a problem hiding this comment.
Commissioning events miss session lifecycle
Medium Severity
kSLSystemEventCommissioningStarted is posted from OnCommissioningSessionEstablishmentStarted, and kSLSystemEventCommissioningFailed is posted from every OnCommissioningSessionEstablishmentError. Those callbacks are per PASE attempt: failed still leaves the window open for retries, and closing the window or finishing successfully does not post a matching terminal event (OnCommissioningSessionStopped and OnCommissioningWindowClosed are silent). The names also disagree with isComissioningStarted, which flips only in OnCommissioningSessionStarted. Listeners that pause work on started and resume on failed will flap during retries or stay paused after a window close.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 755b174. Configure here.
There was a problem hiding this comment.
for window close with a success we should be getting kCommissioningComplete event from the CHIPDeviceEvent.h for window close with error we are publishing the corresponding event.
Rohan Sahay (rosahay-silabs)
changed the title
Rohan Sahay (rosahay-silabs)
changed the title
| LEDWidget * BaseApplication::sAppActionLed = nullptr; | ||
| BaseApplicationDelegate BaseApplication::sAppDelegate = BaseApplicationDelegate(); | ||
|
|
||
| void BaseApplicationDelegate::OnCommissioningSessionEstablishmentStarted() |
There was a problem hiding this comment.
Seems ok, but does it need to be a post event? This can already be treated as a event it is already on the same thread as what PlatformMgr().PostEvent will queue it too
There was a problem hiding this comment.
Yes true but only if we use it here right? Since stelpro is looking for the event I am assuming they want to use it somewhere else on the AppTask on somewhere else. In that case we would need the post it to others to react to it?
206f6d4
into
test/feature/add-matter-service
3 participants
Summary
Adds events for Commissioning started and errored events.
Related issues
N/A
Testing
Locally tested the following cases to verify the outcome.