-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mqtt][homie] Delete '$name' subscription in discovery service #8017
Conversation
Travis tests have failedHey @bodiroga, |
Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
Hi everyone! I don't understand why is Travis failing 😞 Do you know what is this error about?
I would be awesome if we could merge this fix for openHAB 2.5.7 and have a stable Homie discovery process 😉 Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment. Let's go for 2.5.8!
public static final int HOMIE_DEVICE_TIMEOUT = 5000; | ||
public static final int HOMIE_SUBSCRIBE_TIMEOUT = 500; | ||
public static final int HOMIE_ATTRIBUTE_TIMEOUT = 200; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to put them in constants. Can you add the unit on these constants:
public static final int HOMIE_DEVICE_TIMEOUT = 5000; | |
public static final int HOMIE_SUBSCRIBE_TIMEOUT = 500; | |
public static final int HOMIE_ATTRIBUTE_TIMEOUT = 200; | |
public static final int HOMIE_DEVICE_TIMEOUT_MS = 5000; | |
public static final int HOMIE_SUBSCRIBE_TIMEOUT_MS = 500; | |
public static final int HOMIE_ATTRIBUTE_TIMEOUT_MS = 200; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @Hilbrand, done in the last commit! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intentionally increase the timeout to 15 seconds or was it a mixup with some testing you did?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups, I pushed the changes before writing my "longer" message, sorry. Yes, I have increased the timeout on purpose, because for really complex devices 5 seconds is not enough. That is also the timeout value that was set before #7760, so nothing unexpected. I really hope that this PR can "fix" the Homie bug that we've been suffering from for so long, and I'm really sorry for all my trial and errors, I thought that it would be easier to solve, but it seems that it wasn't true 😞
Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
Hi @Hilbrand! Thank you for your suggestion, I have already changed the names 😉 And I have also increased the device_timeout value, because devices with many many many nodes and properties (50-100) can take more than 10 seconds to be processed. Anyway, this value doesn't impact the discovery time of fast devices, but it is there for those unusual cases. Many thanks for taking care of this PR! |
Travis tests were successfulHey @bodiroga, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com> Signed-off-by: MPH80 <michael@hazelden.me>
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com> Signed-off-by: Daan Meijer <daan@studioseptember.nl>
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
…ab#8017) Closes openhab#7252 Signed-off-by: Aitor Iturrioz <riturrioz@gmail.com>
Yes; I'm still getting this problem in OH3. 2023-02-07 17:22:53.827 [WARN ] [.mqtt.homie.internal.homie300.Device] - Could not subscribe java.util.concurrent.CompletionException: java.lang.Exception: Did not receive mandatory topic value: homie/tode/rctode/$name It seems to be intermittent. Sometimes it works; most of the time it doesn't. After hours of playing around. Getting it to semi-work required.
Finally; Channels started to show up - this just isn't right. |
Context:
Although we thought that #7723 would solve #7252, the truth is that after the release of openHAB 2.5.6, new users started complaining that their Homie devices were not behaving correctly ($name message does not arrive). Taking into account that there are some problems with subscriptions and retained messages (in some circumstances the binding does not receive all the messages it should), it has been decided to remove the subscription to the "homie/xxx/$name" topic in the Homie300Discovery class. The only problem with this solution is that the name of the thing will be the same as the device id of Homie, but I think that this small loss of functionality will compensate for the advantage of fixing the bug. Also, the transport.io.mqtt library is already being improved to fix its bugs (openhab/openhab-core@cc3f140, thanks @jochen314), although these changes are only valid for openHAB Core 3, so I think that, until openHAB3 is released, it is better to opt for this workaround.
Also, in the PR a new timeout has been added in the HomieThingHandler, which represents the maximum time the device can take to be completely processed. This timeout does have a high value (5000 milliseconds), although the other two timeouts still have values below 500 milliseconds. This improvement ends up defining the changes introduced in #7760, since the previous code could fail in devices with many nodes and properties.
What do you think @jochen314, @cpmeister, @J-N-K?
Thanks!
Closes #7252