Skip to content

Commit

Permalink
Rule triggers should contain up-to-date values
Browse files Browse the repository at this point in the history
  • Loading branch information
@CurlyMoo
CurlyMoo committed Jun 8, 2024
1 parent 8750cb1 commit 9b5e500
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 9 deletions.
2 changes: 0 additions & 2 deletions HeishaMon/HeishaMon.ino
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,6 @@ bool readSerial()
if (data_length == DATASIZE) { //receive a full data block
if (data[3] == 0x10) { //decode the normal data block
decode_heatpump_data(data, actData, mqtt_client, log_message, heishamonSettings.mqtt_topic_base, heishamonSettings.updateAllTime);
memcpy(actData, data, DATASIZE);
{
char mqtt_topic[256];
sprintf(mqtt_topic, "%s/raw/data", heishamonSettings.mqtt_topic_base);
Expand All @@ -370,7 +369,6 @@ bool readSerial()
} else if (data[3] == 0x21) { //decode the new model extra data block
extraDataBlockAvailable = true; //set the flag to true so we know we can request this data always
decode_heatpump_data_extra(data, actDataExtra, mqtt_client, log_message, heishamonSettings.mqtt_topic_base, heishamonSettings.updateAllTime);
memcpy(actDataExtra, data, DATASIZE);
{
char mqtt_topic[256];
sprintf(mqtt_topic, "%s/raw/dataextra", heishamonSettings.mqtt_topic_base);
Expand Down
36 changes: 29 additions & 7 deletions HeishaMon/decode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,44 +285,66 @@ String getSecondByte(byte input) {

// Decode ////////////////////////////////////////////////////////////////////////////
void decode_heatpump_data(char* data, char* actData, PubSubClient &mqtt_client, void (*log_message)(char*), char* mqtt_topic_base, unsigned int updateAllTime) {
bool updatenow = false;
bool updateTime = false;
bool updateTopic[NUMBER_OF_TOPICS] = { false };

if ((lastalldatatime == 0) || ((unsigned long)(millis() - lastalldatatime) > (1000 * updateAllTime))) {
updatenow = true;
updateTime = true;
lastalldatatime = millis();
}
for (unsigned int Topic_Number = 0 ; Topic_Number < NUMBER_OF_TOPICS ; Topic_Number++) {
String Topic_Value;
Topic_Value = getDataValue(data, Topic_Number);

if ((updatenow) || ( getDataValue(actData, Topic_Number) != Topic_Value )) {
if(getDataValue(actData, Topic_Number) != Topic_Value) {
updateTopic[Topic_Number] = true;
}

if (updateTime || updateTopic[Topic_Number]) {
char log_msg[256];
char mqtt_topic[256];
sprintf_P(log_msg, PSTR("received TOP%d %s: %s"), Topic_Number, topics[Topic_Number], Topic_Value.c_str());
log_message(log_msg);
sprintf_P(mqtt_topic, PSTR("%s/%s/%s"), mqtt_topic_base, mqtt_topic_values, topics[Topic_Number]);
mqtt_client.publish(mqtt_topic, Topic_Value.c_str(), MQTT_RETAIN_VALUES);
}
}
memcpy(actData, data, DATASIZE);
for (unsigned int Topic_Number = 0 ; Topic_Number < NUMBER_OF_TOPICS ; Topic_Number++) {
if(updateTopic[Topic_Number]) {
rules_event_cb(_F("@"), topics[Topic_Number]);
}
}
}

void decode_heatpump_data_extra(char* data, char* actDataExtra, PubSubClient &mqtt_client, void (*log_message)(char*), char* mqtt_topic_base, unsigned int updateAllTime) {
bool updatenow = false;
bool updateTime = false;
bool updateTopic[NUMBER_OF_TOPICS_EXTRA] = { false };

if ((lastallextradatatime == 0) || ((unsigned long)(millis() - lastallextradatatime) > (1000 * updateAllTime))) {
updatenow = true;
updateTime = true;
lastallextradatatime = millis();
}
for (unsigned int Topic_Number = 0 ; Topic_Number < NUMBER_OF_TOPICS_EXTRA ; Topic_Number++) {
String Topic_Value;
Topic_Value = getDataValueExtra(data, Topic_Number);

if ((updatenow) || ( getDataValueExtra(actDataExtra, Topic_Number) != Topic_Value )) {

if(getDataValueExtra(actDataExtra, Topic_Number) != Topic_Value) {
updateTopic[Topic_Number] = true;
}

if (updateTime || updateTopic[Topic_Number]) {
char log_msg[256];
char mqtt_topic[256];
sprintf_P(log_msg, PSTR("received XTOP%d %s: %s"), Topic_Number, xtopics[Topic_Number], Topic_Value.c_str());
log_message(log_msg);
sprintf_P(mqtt_topic, PSTR("%s/%s/%s"), mqtt_topic_base, mqtt_topic_xvalues, xtopics[Topic_Number]);
mqtt_client.publish(mqtt_topic, Topic_Value.c_str(), MQTT_RETAIN_VALUES);
}
}
memcpy(actDataExtra, data, DATASIZE);
for (unsigned int Topic_Number = 0 ; Topic_Number < NUMBER_OF_TOPICS_EXTRA ; Topic_Number++) {
if(updateTopic[Topic_Number]) {
rules_event_cb(_F("@"), xtopics[Topic_Number]);
}
}
Expand Down

9 comments on commit 9b5e500

@IgorYbema
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the reason for this change. If you want rules_event_cb to only be called when the value is changed and not every updateAllTime period (due to maybe useless processing) then this could be much simpler

@CurlyMoo
Copy link
Author

Choose a reason for hiding this comment

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

I'm open for suggestions.

@IgorYbema
Copy link
Owner

Choose a reason for hiding this comment

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

First need to understand why this change was needed. Was it, as it looks too me, to prevent rules_event_cb to be callled too often for each (even unchanged) value (each 300 seconds by default)?

@CurlyMoo
Copy link
Author

Choose a reason for hiding this comment

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

No, the issue was that the rules where triggered with the old value instead of the new one. Right @McMagellan

@IgorYbema
Copy link
Owner

Choose a reason for hiding this comment

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

That would not be possible as the pre-commit line would cause the mqtt message but also the rules_event_cb to be called.

if ((updatenow) || ( getDataValue(actData, Topic_Number) != Topic_Value )) {

But it did result in calling rules_event_cb each 300 seconds for each (even unchanged) value. Maybe that had side effects?

@CurlyMoo
Copy link
Author

Choose a reason for hiding this comment

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

I can't find the discussion we had on this. That's why i tagged @McMagellan. He reported it first.

@CurlyMoo
Copy link
Author

@CurlyMoo CurlyMoo commented on 9b5e500 Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see again what the point was. The actData is updated after the rule_event_cb was called. So, the trigger was correct, but when you inside the rule block wanted to retrieve the updated value, it would give you the old one. Because it would try to extract the current value from the actData which wasn't updated yet.

@McMagellan
Copy link

Choose a reason for hiding this comment

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

I asked CurlyMoo whether it was possible to provide the variable trigger with the correct value. So far I have avoided working with triggers.
I'll give you an example: This test rule should add the variable @Quiet_Mode_Level to the current ruleset.
A query takes place within the trigger (#Test1) and the same query one second later from a timer (Test2).
The quiet mode level is then manually changed from 0 to 2 on the remote control.

on System#Boot then
  settimer(1,15);
end

on timer=1 then
  #Test1 = -1;
  #Test2 = -1;
end

on @Quiet_Mode_Level then
  #Test1 = @Quiet_Mode_Level;
  settimer(2,1);
end

on timer=2 then
  #Test2 = @Quiet_Mode_Level;
end

You can see what happens in the log file: The first query returns the value before the change in #Test1, the second query returns the correct value in #Test2.
I asked CurlyMoo to make a change, which he did, thank you very much for that. After that, the triggers for my CoPilot ruleset are perfectly usable.

Screenshot 2024-08-21 141715

CurlyMoo then also turned off the completely pointless triggering every 300 seconds, which makes the console totally confusing and just wastes resources. Thank you very much for that too.
I currently use 7 triggers, all of which work well in Egyras#544. In version 3.62, however, it looks like this.

2Trigger 3.62.txt

@IgorYbema
Copy link
Owner

Choose a reason for hiding this comment

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

Ah that explains a lot. Then the change is indeed necessary. Maybe I'll update it with a improved patch but for now ok.

Please sign in to comment.