[FEATURE REQUEST] Publish MQTT update on SET commands

[fl42]

Hello,

Is your feature request related to a problem? Please describe. When updating a parameter (e.g., changing parameter 701 using HTTP), systems that rely on MQTT (e.g., Home Assistant) don't have immediate update about the new state (they have to wait up to log_interval).

Describe the solution you'd like I think it makes sense to publish a MQTT update before the next scheduled update (defined by log_interval) if a SET command was issued (i.e., a command that may change the value of a parameter).

Describe alternatives you've considered Reduce log interval parameter.

Describe your own contribution I had a look to the code but I'm not sure what is the best way to implement it. Setting lastMQTTTime to 0 when a SET command is issued is one option.

Additional context Thanks for your great work!

[fredlcore]

@thorbennehmer or @Luposoft63, would this be doable?

[Luposoft63]

I think this is doable. @fl42 But, why don't you use the set command via MQTT? With it you have the inbuilt answer via MQTT. This were the best way for all. If we change 'lastMQTTTime' for this event I'm not shure wether other user get any problems...

[fl42]

Hello @Luposoft63 @fredlcore

Thanks for your quick answer,

Let me detail my use case: I'm using Home Assistant (HA) to control my heater using BSB LAN. My sensors in HA are MQTT sensors and I'm interacting with BSB LAN using REST queries.

sensor:
  - platform: mqtt
    state_topic: "bsb_lan/json"
    name: Status circuit 1
    value_template: >
      {% if '8000' in value_json['BSB-LAN']['status'] %}
        {{ value_json['BSB-LAN']['status']['8000'] }}
      {% else %}
        {{ states('sensor.status_circuit_1') }}
      {% endif %}

rest_command:
  set_comfort_mode:
    url: http://IP/S701=2

When I'm calling the service set_comfort_mode (i.e. calling the URL http://IP/S701=2), my interface in HA that shows the sensor "Status circuit 1" is not updated immediately.

Basically the MQTT instead of publishing every log_interval will publish an update every [min_interval, log_interval] where min_interval is an interval to avoid being flooded if the user is doing multiple updates in a short period (e.g., 1s)

Or even just an URL to trigger a MQTT update would be fine too, if it's easier to implement. A call to http://IP/MQTT would publish immediately a MQTT update. This approach does not change how MQTT behaves, but adds a new feature..

Is the need clearer? :)

[Luposoft63]

Yes, I think so. After thinking about it, I see an timing problem. It is not shure, when the heater has completed your requested switch by http. So it's not exactly clear, when MQTT should update the values. I've another idea: Some times ago I'm fighting whith similar things. The best option for me was an own custom code in BSB-LAN. I think this could be the right way for you too. Ask in an very short loop (500ms) after your value via MQTT. Only if the value has changed publish it. Otherwise were too much traffic. This should solve your problem.

[fredlcore]

Not sure if you need a minimum interval because a SET command takes about 1-2 seconds anyways. And since BSB-LAN ist not multi-threaded, subsequent SET commands won't run concurrently, so the MQTT publishing takes some time as well. So if you set various setpoint temperatures in one go, you'll get several MQTT publishes anyways. So probably you'd just need to set the MQTT timer to zero for each SET command.

[fredlcore]

@Luposoft63: Once the set() function is finished, we will have the ACK from the heater (unless it's an INF telegram such as for room temperature, but we won't be able to query that one anyways). So the value will be stored (and is query-/publish-able). @fl42: Make sure you only set the timer to zero if the set command is actually a SET and not an INF!

[fl42]

@fredlcore I agree with you, I updated my code in #410 I'll test and let you know

[fl42]

#410 works for me

[Luposoft63]

lastMQTTTime=0 Mmhh, is it the exactest way? I think: lastMQTTTime=millis()-log_interval*1000; should be better, you catch an overflow with it I'm I right? @fredlcore In my opinion, a minimum interval ensured to get the new value as soon as possible. If you wait for a second you must hope the switch has done already, but you can't be shure. But, if fl42 is happy, than is it surely ok. I just wanted to say...

[fl42]

In fact lastMQTTTime=millis()-log_interval*1000; could be problematic if millis() < log_interval*1000 (e.g., after a restart if you have large log_interval) #410 runs for me for few days now without major issue

You are right that some parameters are not updated immediately in the microcontroller of the heater. For instance updating heater mode from Off to Automatic: sensor of "mode" is updated immediately, while sensor of "state circuit 1" still have the old value for few seconds. So it makes sense to wait 1 or 2 sec before publishing the update.

Something like the code below could be better:

  if (setcmd) {  // Only for SET messages
    lastMQTTTime = millis()-log_interval*1000+1000;
  }

You have to check if millis()-log_interval*1000+1000 is positive...

@Luposoft63 This is what you mean?

[Luposoft63]

I thought you are looking for an immediate update. Now you are happy with 1 or 2 seconds. I understand, we have different meanings about immediately. But don't worry. That's ok.

lastMQTTTime is unsigned. lastMQTTTime = millis()-log_interval will work every time without any check.

the condition in the loop for mqtt-update is millis() - lastMQTTTime >= (log_interval * 1000) You don't need +1000

[fl42]

I thought you are looking for an immediate update. Now you are happy with 1 or 2 seconds.

Actually I don't have the choice: some parameters are not updated immediately in micro-controller, please refer to my example when switching from Off to Automatic.

I'll update update my PR accordingly

[fredlcore]

PR #410 still has the +1000 in there. Do you intend to keep it, @fl42 or do you want to change it according to @Luposoft63's suggestion?

[fl42]

Hello @fredlcore @Luposoft63,

IMO the +1000 is required.

You are right that some parameters are not updated immediately in the microcontroller of the heater. For instance updating heater mode from Off to Automatic: sensor of "mode" is updated immediately, while sensor of "state circuit 1" still have the old value for few seconds. So it makes sense to wait 1 or 2 sec before publishing the update.

Then the MQTT update should not be triggered immediately (i.e., on next loop) but a little later to allow the microcontroller to update all values. Experimentally 1s looks good. @Luposoft63 I'm sorry but I didn't understand why the +1000 would be an issue?

lastMQTTTime is unsigned. lastMQTTTime = millis()-log_interval will work every time without any check.

When unsigned variables are made to exceed their maximum capacity they "roll over" back to 0, and also the other way around (docs) Then you should check if the value is > 0 before assigning it to an unsigned int millis() - log_interval * 1000 may be negative just after boot, this is why I added a check How does it sound to you?

[Luposoft63]

@fl42 please read this: https://arduino.stackexchange.com/questions/12587/how-can-i-handle-the-millis-rollover

[fredlcore]

@fl42: I suggest you adjust your code as per @Luposoft63's suggestions. If you look around in the code, you'll find that this is the way it is handled everywhere else, too (I hope). The link to stackexchange explains this well. As for the 1s delay, I don't think it is necessary, because the query of the parameter alone (after it has been set) takes at least dozens of milliseconds just for the telegram to be sent out on the bus (2.1ms for each byte). If what you describe above is true, it would have to be the same when setting a parameter via the webinterface. Because if you issue a /S700=1 for example to change the mode of HC1, what you see in the resulting page is the query of the parameter right after it has been set. So if your controller really takes so long, then you would be seeing the old value when issueing the command. If that's the case, then please post a serial monitor log of setting that parameter because that would really be an important information. If what you see on the resulting page is the new value, then you don't need a delay because the MQTT function uses exactly the same logic.

If my assumptions are correct, then all you'd need is this:

#ifdef MQTT
  // Force to publish MQTT update as state may have been modified by this SET command
  if (setcmd) {  // Only for SET messages
    lastMQTTTime = millis() - log_interval * 1000;
  }
#endif

Do you agree, @Luposoft63?

[fredlcore]

Ok, I got confirmation by another user that some parameters are only updated rather late, so adding one second may be the way to go. Therefore I'd suggest this:

#ifdef MQTT
  // Force to publish MQTT update in 1s as state may have been modified by this SET command
  // Wait 1s to ensure all values are updated in the microcontroller
  // (e.g., moving from Off to Automatic: state circuit 1 is updated after dozen of ms)
  if (setcmd) {  // Only for SET messages
    lastMQTTTime = millis() - log_interval * 1000 + 1000;
  }
#endif

Everyone agree?

[fl42]

I'm glad to hear that another user was able to reproduce the issue.

Just be clear, what would happen if millis() returns 100000 (i.e., 100s after boot) and log_interval = 1800? The result would be negative, right?

When unsigned variables are made to exceed their maximum capacity they "roll over" back to 0, and also the other way around

docs

Then lastMQTTTime will roll over and be close the maximum, no? It's not the same as the rollover of millis(), which is a different issue.

[fredlcore]

Let's go through this with actual values, with millis() = 100000 and log_interval = 1800, so then it becomes clearer: lastMQTTTime = 100000 - (1800*1000) + 1000 i.e.: lastMQTTTime = -1699000 = 4239268296 unsigned The condition check is as follows: millis() - lastMQTTTime >= (log_interval * 1000) i.e.: 100000 - 4239268296 >= (1800 * 1000) i.e.: -4293168296 >= 1800000 i.e.: 1799000 >= 1800000 This is false for exactly 1000ms or 1 second, so exactly the intended delay. Once millis() is no longer 100000 but 101000, the condition will evaluate true and mqtt_sendtoBroker() will be called.

So in the end, it's the same mechanism that prevents the millis() rollover in the example above.

EDIT: Updated with the corresponding unsigned values

[fl42]

i.e.: lastMQTTTime = -1699000

lastMQTTTime cannot be negative as it's an unsigned long So lastMQTTTime = 100000 - (1800*1000) + 1000 will result to lastMQTTTime = 4294967295 + -1699000 = 4293268295

There is an example in the docs link above:

unsigned int x; x = 0; x = x - 1; // x now contains 65535 - rolls over in neg direction

This is why I added a check to ensure the result is not negative Do you agree?

[fredlcore]

You're right about the unsigned negatives, but if you convert the negatives to the corresponding unsigned representations, it's still true. I updated my post above and let me know if you're still not convinced ;)...

[fl42]

Is relying on out-of-bound roll over a good idea? It's not HW dependent? For me adding a condition to ensure the result is positive (or set to 0) sounds safer, but I'm ok with that if for you is fine ;)

[fredlcore]

Where do you see the difference of what is described here: https://arduino.stackexchange.com/questions/12587/how-can-i-handle-the-millis-rollover and what I described above? What should be hardware dependet with this? Every unsigned long will roll over at some point, whether it is 32, 64 or 2048 bits. The math in the above calculation will always work the same.

The thing is that your code actually does the same as mine, because millis() - log_interval * 1000 + 1000 > 0 will always be true for an unsigned long. Or under what condition would lastMQTTTime ever be set to zero?

[fl42]

I was thinking if some hw catches the out of bound and set to 0 or whatever instead of "normal" roll over But that's fine, thanks for the explanation :)

[fl42]

The thing is that your code actually does the same as mine, because millis() - log_interval * 1000 + 1000 > 0 will always be true for an unsigned long. Or under what condition would lastMQTTTime ever be set to zero?

The condition would be if millis() + 1000 < log_interval * 1000 then set lastMQTTTime to 0

[fredlcore]

But that's a completely different condition than the one in your PR, right? And if the condition would fire if millis() + 1000 is less than log_interval * 1000, it would mean that with a log_interval of 1800 as mentioned in your example, lastMQTTTime would be set to 0 for up to 1700 seconds (i.e. 29:59 minutes) after boot, so the the update wouldn't be sent to the broker with a 1 second delay, but with a 30 minute delay. That's certainly not what you want, is it?

Out of bound does not apply here. That's if you address an array with an index larger than the array's size and that will be caught by some hardware (ESP32) more strictly than with other (Arduino Due). But here we deal with rollovers. These occur all the time (not only with millis()) In machine language there are even processor flags that notify you of such a rollover so that you can handle them appropriately (for example increase the next counter by one). But of course you have to handle them appropriately, otherwise you might end up in a mess...

[fl42]

You are totally right: the condition in my PR is wrong (always true).

lastMQTTTime would be set to 0 for up to 1700 seconds

Yes, but it's not a big deal as if you set log_interval to 1800 you don't expect an update before log_interval. However as you mentioned no update will be published on set command before log_interval, not sure if it's a big issue (in practice log_interval is often small)

Let's go with your proposal, I'm ok to test as soon as merged

[fredlcore]

if you set log_interval to 1800 you don't expect an update before log_interval

I thought the whole idea of the PR was to publish the changes after a set command by setting lastMQTTTime accordingly? Even with a correct condition, setting it to 0 will effectively delay publishing to the broker by (almost) twice the duration of log_duration (if the condition fires one millisecond before millis() is equal to log_duration*1000 after boot).

Anyway, I've added the code suggested above, let us know if it works as expected.

[fredlcore]

@fl42: Can we consider this closed or is there still something to do in your opinion?

[fl42]

Works as expected for me, so IMO you can close. Thanks!