Skip to content

Conversation

@janrtvld
Copy link

No description provided.

Copy link
Member

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

Has this been tested with:

  1. Create connection
  2. Send message (arrives)
  3. Disconnect
  4. Send message (error)

for both sides?

for (device in connectedDevices) {
this.gattServer.cancelConnection(device)
}
this.gattServer.close()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we close the gattServer when we stop advertising?

Copy link
Author

Choose a reason for hiding this comment

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

I've moved the close to the stopAdvertising function.


@ReactMethod
fun shutdownCentral(
@Suppress("UNUSED_PARAMETER") options: ReadableMap,
Copy link
Member

Choose a reason for hiding this comment

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

Central needs no changes?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean the same changes as in the peripheralManager? The central shutdown works fine without those changes.

Copy link
Member

Choose a reason for hiding this comment

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

Not the same changes per se. As long as it is tested with the flow I mentioned above it is fine.

Signed-off-by: Jan <[email protected]>
@TimoGlastra
Copy link
Member

Why do we need the empty object?

@berendsliedrecht
Copy link
Member

Why do we need the empty object?

I am not sure on the internals, tried finding information about it before, but a function without any params needs an empty object to work. It might be fixable by tweaking the Objective-C interface, but this seems to be a fine solution to me.

@TimoGlastra
Copy link
Member

Can we add a comment? I would remove this I it wouldn't have a comment 😉

@berendsliedrecht berendsliedrecht merged commit 95f5369 into animo:main Jun 30, 2023
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.

3 participants