Skip to content

Conversation

allenporter
Copy link
Contributor

@allenporter allenporter commented Jan 21, 2025

Remove methods that were removed in paho-mqtt v2. This means we no longer change the client id when disconnecting. The client id can be used to resume connections, and it seems worth trying to resolve the root cause of a bug in the future explicitly. e.g. we can create a new client library if we want to reset the entire connection.

This should allow #288 to succeed

Copy link
Collaborator

@Lash-L Lash-L left a comment

Choose a reason for hiding this comment

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

Seems like this still exists - just as a private function?

https://github.com/eclipse-paho/paho.mqtt.python/blob/d45de3737879cfe7a6acc361631fa5cb1ef584bb/src/paho/mqtt/client.py#L407

May be worth still using it. TBH, I'm not sure when this was added. From my understanding of how the Roborock Mqtt server works from, I don't know if the client id really matters.

There's a specific thread that is specific to our client (even different from the app or the vacuum) - so if there is any ratelimiting, happening, I assume it is because of the thread opposed to the client id and when we reconnect, we can probably just sue the same id.

All that to say, I'm fine with removing it, as we want to change how this is working anyways.

@allenporter
Copy link
Contributor Author

Seems like this still exists - just as a private function?

https://github.com/eclipse-paho/paho.mqtt.python/blob/d45de3737879cfe7a6acc361631fa5cb1ef584bb/src/paho/mqtt/client.py#L407

Yes, generally it's not good practice to use private functions like this. I'd prefer if we need something from mqtt we work with the authors to extend it rather than hack around it.

May be worth still using it. TBH, I'm not sure when this was added. From my understanding of how the Roborock Mqtt server works from, I don't know if the client id really matters.

My only knowledge of this is that there is some ability for the server to resume a connection across client IDs.

The commit that added this behavior didn't have any detail about why it exists.

There's a specific thread that is specific to our client (even different from the app or the vacuum) - so if there is any ratelimiting, happening, I assume it is because of the thread opposed to the client id and when we reconnect, we can probably just sue the same id.

Do you mean a thread like a python thread? Or some other notion of a thread as a session object?

All that to say, I'm fine with removing it, as we want to change how this is working anyways.

Yeahy take is there is some risk here, but I don't like having stuff we don't fully understand, and my take is we can add it back if there is a specific problem we want to fix and we can add a comment explaining why we need this, and have some tests to reproduce the behavior. I can defer to y'alls judgment since I'm new here, but that's my rationale.

@Lash-L
Copy link
Collaborator

Lash-L commented Jan 21, 2025

I think go ahead and go for it. I think it would be removed during a larger refactor anyways and we have problems with reconnects now as is.

@allenporter allenporter merged commit 685edc8 into Python-roborock:main Jan 21, 2025
6 checks passed
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