openshwprojects / OpenBK7231T_App

Open source firmware (Tasmota/Esphome replacement) for BK7231T, BK7231N, BL2028N, T34, XR809, W800/W801, W600/W601 and BL602
https://openbekeniot.github.io/webapp/devicesList.html
1.39k stars 240 forks source link

MQTT / States are not published when power monitoring is enabled #352

Open iprak opened 1 year ago

iprak commented 1 year ago

I have noticed that when power measurements are enabled, values are published every second. The publish happens in MQTT_PublishTopicToClient which resets g_timeSinceLastMQTTPublish = 0.

The full state publish and commands like publishChannels go through MQTT_RunEverySecondUpdate where if (g_timeSinceLastMQTTPublish > 2) check is performed and so no state publish happens.

valeklubomir commented 1 year ago

Publishing of power data is sent when there is change of voltage, current, power, energycounter beyond threshold level. If there are changing conditions with such intensity then it can be. After all these changes with SDK and APP, I would simple bypass <(g_timeSinceLastMQTTPublish > 2)> and let it transmitt within the power data.

EDIT: when power values are stable my devices are sending this state every minute.

iprak commented 1 year ago

I don't think it is right to publish other values as part of power data publish.

@openshwprojects mentioned that IR can generate lots of MQTT request so it could also cause same issue. Same problem happens if the command publishAll was called; this was added to resolve https://github.com/openshwprojects/OpenBK7231T_App/issues/262.

In my test case, I have Flag2 enabled which is supposed to send out entire state every 60 seconds but it doesn't happen when power data gets published. I was looking into enhancing it to send out availability status (https://github.com/openshwprojects/OpenBK7231T_App/issues/333).

valeklubomir commented 1 year ago

Availability status for topic clientID/connected is designed only to send it when connection is established and as last will message.

valeklubomir commented 1 year ago

I think we could do it. Now message publish puts messages into ringbuf and sending is executed by LWIP stack when socket is ready to accept data. Adding another message will work.

iprak commented 1 year ago

Availability status for topic clientID/connected is designed only to send it when connection is established and as last will message.

Yes .. well that was my working theory that last status message was not sufficient. I need to look more into this. But I think something needs to be done to make publishing messages reliable.

I think we could do it. Now message publish puts messages into ringbuf and sending is executed by LWIP stack when socket is ready to accept data. Adding another message will work.

So if I understand you are suggesting taking out that g_timeSinceLastMQTTPublish >2 check completely. I don't know why we have 2 second wait between publishes. Maybe @openshwprojects can shed some light on this.

valeklubomir commented 1 year ago

I could add another state message sending counter of transmitted messages and create listener for counting messages on server side and monitor how often and how many message are lost.

openshwprojects commented 1 year ago

@iprak this is obviously a bug that needs to be fixed

@iprak @valeklubomir the whole story behind that strange MQTT approach, behind that "publish whole device state counter" is that I was never been able to do more than about 3 MQTT publishes per second without problems.

I tried to increase buffers in the past: https://github.com/openshwprojects/OpenBK7231N/commit/be9ad5fd73d3fd903b54e911bf62066387d89651

I also did some debugging. At that time, I arrived at conclusion that increasing buffers does not help because: https://github.com/openshwprojects/OpenBK7231N/commit/9b6a8f673623aa55b85ee03492d8bf895e9dc4e3 ringbuf immediately goes from "almost whole free" to like "-12321 bytes free" (If I remember correctly) , so there must be some other issue, ringbuff handling error or something

It would be great if we would be able to do multiple MQTT publish per second (let's say 10 or so) but at the moment I don't know how hard it would. @valeklubomir already did great job updating LWIP on N so maybe it's more stable now, but we also need the same for T and also don't forget that other platforms (or at least BL602) had similar issue.

PS: we should sync settings on T on N as well: https://github.com/openshwprojects/OpenBK7231T/commit/85b7f32a84dff6e9db2bc3eebf98b27e363aca44

valeklubomir commented 1 year ago

I would like to do it for T. But unfortunately first I thought that I have T device, but was mistaken and it is N device. I have it ready for build but unable to test it, May be somebody can pull from my fork and try it instead?

valeklubomir commented 1 year ago

Created some MQTT benchmark test function into device. So far 14 msg/s is best score in published messages.