Skip to content

Improve 'subscribe_command` callbacks (CON-1960)#1690

Closed
tomasmcguinness wants to merge 2 commits into
espressif:mainfrom
tomasmcguinness:main
Closed

Improve 'subscribe_command` callbacks (CON-1960)#1690
tomasmcguinness wants to merge 2 commits into
espressif:mainfrom
tomasmcguinness:main

Conversation

@tomasmcguinness
Copy link
Copy Markdown
Contributor

@tomasmcguinness tomasmcguinness commented Feb 3, 2026

Description

This PR makes two main changes to the subscribe_command

First, a new subscription_established_cb_t has been added along with a new parameter established_cb.
Second, the existing subscribe_done_cb_t has been renamed to subscription_terminated_cb to better reflect what it means.

The new established_cb allows a controller to know when the subscribe_command has been sent and processed.

Renaming done_cb -> terminated_cb makes the callback's use much clearer. Done usually means success, but in this case it refers to subscriptions being terminated.

The existing constructors remain so no breaking changes should be introduced.

Testing

I have tested this in my controller:

 subscribe_command *cmd = chip::Platform::New<subscribe_command>(node->node_id,
                                                                            std::move(attr_paths),
                                                                            std::move(event_paths),
                                                                            min_interval,
                                                                            max_interval,
                                                                            false,
                                                                            attribute_data_cb,
                                                                            nullptr,
                                                                            node_subscription_established_cb,
                                                                            node_subscription_terminated_cb,
                                                                            node_subscribe_failed_cb,
                                                                            false);

Session establishment is reported

I (214853) chip[DMG]: SubscribeResponse is received
I (214853) chip[DMG]: Subscription established in 1508ms with SubscriptionID = 0x029ec2e9 MinInterval = 0s MaxInterval = 300s Peer = 01:00000000000003E9
I (214853) read_command: Subscription 0x29ec2e9 established
I (214853) app_main: Successfully subscribed, node 1001, subscription id 0x029EC2E9
I (214853) chip[DMG]: Refresh LivenessCheckTime for 344277 milliseconds with SubscriptionId = 0x029ec2e9 Peer = 01:00000000000003E9

Termination is reported

E (559133) chip[DMG]: Subscription Liveness timeout with SubscriptionID = 0x029ec2e9, Peer = 01:00000000000003E9
I (559143) chip[SC]: SecureSession[0x3fce9d4c, LSID:46647]: State change 'kActive' --> 'kDefunct'
E (559143) read_command: Subscribe Error: Error CHIP:0x00000032
I (559143) read_command: Subscription 0x29ec2e9 Done for remote node 0x3e9
I (559143) app_main: Subscription terminated, node 1001, subscription id 0x029EC2E9

Failure to subscribe is reported:

E (512513) chip[EM]: <<5 [E:39943i S:0 M:152332148] (U) Msg Retransmission to 0:0000000000000000 failure (max retries:4)
E (522283) chip[SC]: CASESession timed out while waiting for a response from peer <00000000000003E8, 1>. Current state was 1
E (522283) app_main: Failed to subscribe (context: 0x3fcf1be4)

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@github-actions github-actions Bot changed the title Improve 'subscribe_command` callbacks Improve 'subscribe_command` callbacks (CON-1960) Feb 3, 2026
@tomasmcguinness tomasmcguinness marked this pull request as ready for review February 3, 2026 13:32
Copy link
Copy Markdown
Collaborator

@shubhamdp shubhamdp left a comment

Choose a reason for hiding this comment

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

@tomasmcguinness Thanks for the PR, LGTM! Could you also update the RELEASE_NOTES.md stating the rename and additional callback?

@tomasmcguinness
Copy link
Copy Markdown
Contributor Author

@tomasmcguinness Thanks for the PR, LGTM! Could you also update the RELEASE_NOTES.md stating the rename and additional callback?

Great. I've added two bullets to RELEASE_NOTES.md as requested. Please let me know if they are acceptable. Thx.

@shubhamdp
Copy link
Copy Markdown
Collaborator

@tomasmcguinness could you please fix the restyling errors and codespell errors flagged by pre-commit hook? You can find the documentation here: https://github.com/espressif/esp-matter/blob/main/DEVELOPER_GUIDE.md

Also, please squash the commits.

@tomasmcguinness
Copy link
Copy Markdown
Contributor Author

tomasmcguinness commented Mar 4, 2026

@shubhamdp I'm not 100% sure how to squash the commits as there are other merges from main and a conflict with the README. Can you advise?

I gave it a try and it only seems to have duplicated my commits 😕

@tomasmcguinness
Copy link
Copy Markdown
Contributor Author

I cleaned the branch! All the workflows have passed.

@shubhamdp
Copy link
Copy Markdown
Collaborator

I cleaned the branch! All the workflows have passed.

I was wondering why you closed it 😄

LGMT!

@tomasmcguinness
Copy link
Copy Markdown
Contributor Author

Sorry @shubhamdp I did that automatically when I reset my branch (no more differences!)

Do I need to do anything further to get this merged?

@shubhamdp
Copy link
Copy Markdown
Collaborator

I think this would be all!

@tomasmcguinness
Copy link
Copy Markdown
Contributor Author

Do you have an ETA on the merge of this PR?

@shubhamdp
Copy link
Copy Markdown
Collaborator

@tomasmcguinness your changes was merged with merge-commit: 8428176

I think since you updated this branch with the merge-commit, may be it may have missed out on detecting and marking this as merged!

@tomasmcguinness
Copy link
Copy Markdown
Contributor Author

@tomasmcguinness your changes was merged with merge-commit: 8428176

I think since you updated this branch with the merge-commit, may be it may have missed out on detecting and marking this as merged!

Excellent. Can I just close this PR then?

@shubhamdp
Copy link
Copy Markdown
Collaborator

Yes Please!

@tomasmcguinness
Copy link
Copy Markdown
Contributor Author

tomasmcguinness commented Mar 17, 2026

Closing as this has been merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants