Skip to content

Conversation

jthm-ot
Copy link
Collaborator

@jthm-ot jthm-ot commented Feb 4, 2025

No description provided.

@jthm-ot jthm-ot requested review from babrsn and larsgk February 4, 2025 14:02
return;
}

if (bt_vcp_vol_ctlr_conn_get(vol_ctlr, &conn) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to check if conn == vcs_conn and return early if they are not equal

Copy link
Contributor

@babrsn babrsn left a comment

Choose a reason for hiding this comment

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

Your changes look good to me. However, I have a comment:
If a new connection starts immediately after another is established, we might lose the VCS and CSIS discovery for the second one. It might be better to store all connections and use a discovery flag to ensure VCS and CSIS discovery for each connection, making the implementation clearer and more robust.

@jthm-ot
Copy link
Collaborator Author

jthm-ot commented Feb 5, 2025

Your changes look good to me. However, I have a comment: If a new connection starts immediately after another is established, we might lose the VCS and CSIS discovery for the second one. It might be better to store all connections and use a discovery flag to ensure VCS and CSIS discovery for each connection, making the implementation clearer and more robust.

Yes. Good point

@babrsn
Copy link
Contributor

babrsn commented Feb 7, 2025

Your changes look good to me. However, I have a comment: If a new connection starts immediately after another is established, we might lose the VCS and CSIS discovery for the second one. It might be better to store all connections and use a discovery flag to ensure VCS and CSIS discovery for each connection, making the implementation clearer and more robust.

Yes. Good point

As this issue is not directly related to your changes in this PR, I have created a new issue (issue #69) to address it.

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