Fix for Issue 808: Door/Window sensor lost data after deep sleep #1176
Conversation
Queueing sensor state changes while device is offline and publishing them as MQTT connection is established.
|
I think this may be a good solution. I'll give it a chance. |
|
Regarding appendGet, please open another issue and we will investigate. Thanks! |
|
On a second tought, maybe we can improve it a bit futher. Maybe before calling MQTT_QueuePublish we can check if the value is already in queue? Because current version will add a new entry to queue every second before MQTT is on, won;t it? |
* add aht sensor driver * disable by default * revert comment
|
Your suggestion to send to a queue only changes, not the current state at every second, is good. I was thinking the same. The reason I didn't implement it, was that I didn't want to change the existing behavior, where in existing code the state is published every second no matter whether it had changed or not. I think that if we want to publish changes only, we should do it in both cases, when connection has already been established (former lines #85-90) and when events are being queued. Since the code is going to be pretty much the same, I think moving it to a separate function would be great, except that in one case it's being queued, in another -- published directly to the MQTT. Is there any particular reason for it to not being queued when connection is established? If not, than the code could be simplified by simply putting it in the queue in both cases. I think I can do these changes in next few days, if you don't mind. Thank you for your support of this change. |
|
I don't have any active door sensors so take my advice with a grain of salt, I may be wrong, Lots of strangeness of the code comes from the fact that MQTT queue was added a bit later and the queue is limited to (as far as I remember) 16 or so entries, so when not needed, we just do publish directly. Futhermore, we can just publish directly every second without getting into any potential problems and without affecting other parts of the system, while we can't just add to queue blindly, because it has limited size ot (as far as I remember) 16 and it could get clogged very easily My suggestion for the duplicates fix would be something like adding a boolean argument to MQTT_QueuePublish , argument bool bSkipDuplicate, which will skip adding item if the last item in the queue with the same topic has the same value.... I know that lot's of stuff could be improved here, but things like door sensors are very hard for me (as a single person) to test efficiently, that's why I'm trying to be on the safe side with the publishes. I am not sure about using queue in all cases, but I suspect that using a queue may introduce a very little (one "every second" tick?) extra delay in comparison to direct publish call. |
both upload and download artifact should be v4...
* drawers draft * test * support post as well * Update drv_drawers.c * ambient * add led_ambient * foolproof compile * disable drivers before merge
* allow disabling Tasmota JSON API * test enable
* extend pin cfg page: introduce a function te get number of channels for an IO role in JS code generate all elements, but hide and disable unused fields. This way the should not count for POST action * Fix for possible wrong index when pin can't be PWM shorten code --------- Co-authored-by: openshwprojects <85486843+openshwprojects@users.noreply.github.com>
This fixes constants in publishFloat for LN882H and publishInt for BL602 and LN882H
Queueing sensor state changes while device is offline and publishing them as MQTT connection is established.
|
Final fix for Issue 808, tested on 8 door sensors |
|
Looks good for me. Let's see if it compiles and I will merge |
DoorWindowSensor driver has a security problem, which is registered as an issue 808: if the open/close cycle happens fully before the device has been connected to MQTT broker, the event is lost. So, if an intruder opens/closes the door within about 15 seconds when the device was sleeping, this will not be registered. I propose to queue the events until the connection to MQTT is established, so events are not lost.
The fix has been tested on 7 of Door Sensors, and it fully fixes the issue. It had been tested also with MQTT broker being down for 300 seconds to test for the queue overflow, and the memory limit is still very far.
Proposed change leverages the MQTT queue API:
The rest of the behavior is left intact: the device will publish events directly (without queue) every second when it's connected, until it goes to sleep again.
A few things about the queue API itself:
I don't understand why PublishQueuedItems() calls the MQTT_PublishTopicToClient() with appendGet parameter being set to false, effectively forbidding queued messages to be published to the same channel as those that are published directly. I think this is a bug, that requires a fix. For now, to work around this behavior, I am manually adding the suffix "/get" when queueing the event.