bertmelis / VitoWiFi

Communicate with Viessmann boilers using the optolink for ESP8266 and ESP32
MIT License
118 stars 39 forks source link

8 Byte read extension with uint64_t brings inconsistent results #73

Closed GnomiBerlin closed 2 years ago

GnomiBerlin commented 2 years ago

Installation specifics Heating type: Vitotronic 200 KW2 Protocol: KW Board: ESP8266 Wemos D1 mini Hardware: own IR components Symptom

I extended with an 8 Byte part for reading RAW-data "System Time" and the "on/off timer" from the different days of e.g. "Zirkulationspumpe" code changes are at the end listed.

in the main program I used with additional related changes:

DP8Timer getAnlagenzeit("getanlagenzeit", "allgemein", **0x088E**);   //Anlagenzeit  0x088E 
DP8Timer getTimerZirkuSa("gettimerzirkusa", "allgemein", **0x2228**);   //Zirkulationspumpe Zeiten Samstag

Only the address is different. But on MQTT I get back: gettimerzirkusa = FFFFFFFF94803B30 getanlagenzeit = 2092220

Is there anyone who can give an explanation for this. Both should deliver 8 Bytes result?!

DPTypes.cpp:

void conv8Timer::encode(uint8_t* out, DPValue in) {
//zum Schreiben
  uint64_t tmp = in.getU64();
  out[7] = tmp >> 56;
  out[6] = tmp >> 48;
  out[5] = tmp >> 40;
  out[4] = tmp >> 32;
  out[3] = tmp >> 24;
  out[2] = tmp >> 16;
  out[1] = tmp >> 8;
  out[0] = tmp & 0xFF;

}
DPValue conv8Timer::decode(const uint8_t* in) {
//zum Lesen
  uint64_t tmp = in[7] << 56 | in[6] << 48 | in[5] << 40 | in[4] << 32 | in[3] << 24 | in[2] << 16 | in[1] << 8 | in[0];
  DPValue out(tmp);
  return out;
}

DPTypes.hpp

class conv8Timer : public DPType {
 public:
  void encode(uint8_t* out, DPValue in);
  DPValue decode(const uint8_t* in);
  const size_t getLength() const { return 8; }
}; 

Datapoint.hpp

typedef Datapoint<conv8Timer> DP8Timer;

bertmelis commented 2 years ago

I believe 64 bit isn't in the code. where did you define getU64?

GnomiBerlin commented 2 years ago

Hi, thanks for the reply. I added into DPValue.hpp: see below So I think 64 bit was addressed. But I wonder, why with DP8Timer getTimerZirkuSa("gettimerzirkusa", "allgemein", 0x2228); //Zirkulationspumpe Zeiten Samstag --> delivers "FFFFFFFF94803B30" = 6:00-7:30,16:00-18:40,0:00-0:00,0:00-0:00 it is working and with DP8Timer getAnlagenzeit("getanlagenzeit", "allgemein", 0x088E); //Anlagenzeit 0x088E --> delivers "2092220" = only part of the system time 2.09.2022-hours?-minutes?-seconds? not?

enum DPValueType {
  BOOL,
  UINT8_T,
  UINT16_T,
  UINT32_T,
  **UINT64_T,**
  FLOAT,
  PTR
};
class DPValue {
 private:
  union value {
    struct b_t { DPValueType type; bool value; } b;
    struct u8_t { DPValueType type; uint8_t value; } u8;
    struct u16_t { DPValueType type; uint16_t value; } u16;
    struct u32_t { DPValueType type; uint32_t value; } u32;
    **struct u64_t { DPValueType type; uint64_t value; } u64;**
   value(uint32_t u32) : u32{UINT32_T, u32} {}
    **value(uint64_t u64) : u64{UINT64_T, u64} {}**
explicit DPValue(uint32_t u32) : v(u32) {}
  **explicit DPValue(uint64_t u64) : v(u64) {}**
  **uint64_t getU64() {
    if (v.b.type == UINT64_T) {
      return v.u64.value;
    } else {
      return 0;
    }
  }**
 **case UINT64_T:
        snprintf(c, s, "%u", v.u64.value);
        break;**
bertmelis commented 2 years ago

That seems to return a string containing the readable value, not the digital representation.

I'm on phone now so unable to write a correction.

GnomiBerlin commented 2 years ago

Thanks! A hint after the call would also help ;-). I can not find the point of the possible error.

GnomiBerlin commented 2 years ago

So, the time windows can now be written correctly, This means gettimerzirkusa = FFFFFFFF94803B30 (6:00-7:30+16:00-18:40) and also settimerzirkusa are working. For writing only one adaption was missing for the 64 bit adaptation: #include <stdlib.h> /* strtoull */

strtoul() --> strtoull():

if (strcmp(topic, "VITOWIFI/settimerzirkusa") == 0) {
    uint64_t setTimer = strtoull(payload, NULL, 16);
    DPValue value(setTimer);
    VitoWiFi.writeDatapoint(setTimerZirkuSa, value);   // 06000730160018300000000000000000 = FFFFFFFF93803B30
    VitoWiFi.readDatapoint(getTimerZirkuSa);
  }
  if (strcmp(topic, "VITOWIFI/retrievetimerzirkusa") == 0) {
    uint64_t setTimer = strtoull(payload, NULL, 16);
    DPValue value(setTimer);
    VitoWiFi.readDatapoint(getTimerZirkuSa);
  }

getanlagenzeit = 11092220 still gets only 4 Bytes?? with the same routines

bertmelis commented 2 years ago

working on it. I'm unable to test anything though in real life though.

bertmelis commented 2 years ago

Could you test the 8-byte-dp branch with logging enable? As your D1 only has 1 UART, you might need to resort to https://github.com/JoaoLopesF/RemoteDebug (or alternatives)

GnomiBerlin commented 2 years ago

Thank you and I saw, that the extensions are nearly the same with other names as what I did. And you did not add your new rule into Datapoint.hpp like: typedef Datapoint DP8Timer;

Only difference: n.getU16() <-> n.getU64() I tested without any difference in the result. I do not exactly know, what I have to do in the code (at which locations) for using this telnet debugging. yours:

void conv8_1_Timer::encode(uint8_t* out, DPValue in) {
  uint64_t tmp = in.getU16();
  out[7] = tmp >> 56;
  out[6] = tmp >> 48;
  out[5] = tmp >> 40;
  out[4] = tmp >> 32;
  out[3] = tmp >> 24;
  out[2] = tmp >> 16;
  out[1] = tmp >> 8;
  out[0] = tmp & 0xFF;
}
DPValue conv8_1_Timer::decode(const uint8_t* in) {
  uint64_t tmp = in[7] << 56 |
                 in[6] << 48 |
                 in[5] << 40 |
                 in[4] << 32 |
                 in[3] << 24 |
                 in[2] << 16 |
                 in[1] << 8 |
                 in[0];
  DPValue out(tmp);
  return out;
}

my:

void conv8Timer::encode(uint8_t* out, DPValue in) {
//zum Schreiben

  uint64_t tmp = in.getU64();
  out[7] = tmp >> 56;
  out[6] = tmp >> 48;
  out[5] = tmp >> 40;
  out[4] = tmp >> 32;
  out[3] = tmp >> 24;
  out[2] = tmp >> 16;
  out[1] = tmp >> 8;
  out[0] = tmp & 0xFF;
}
DPValue conv8Timer::decode(const uint8_t* in) {
//zum Lesen
  uint64_t tmp = in[7] << 56 | in[6] << 48 | in[5] << 40 | in[4] << 32 | in[3] << 24 | in[2] << 16 | in[1] << 8 | in[0];

  DPValue out(tmp);
  return out;
}
bertmelis commented 2 years ago

Oh, forgot that, indeed + fixed the errors you mentioned

I'll add a small example. Give me a moment...

bertmelis commented 2 years ago

adjust to your needs:

you'll have to connect to your esp8266 using a telnet client (first link in google on how to install: https://petri.com/enable-telnet-client-in-windows-11-and-server-2022/). Check your esp8266's IP address to connect to via your router.

#include <Arduino.h>
#include <VitoWiFi.h>
#include <ESP8266WiFi.h>
#include <RemoteDebug.h>

#define HOST_NAME "VitoWiFi"
const char* ssid = "........";
const char* password = "........";

RemoteDebug debug;
VitoWiFi_setProtocol(P300);

DPTemp outsideTemp("outsideTemp", "boiler", 0x5525);
DPTemp boilerTemp("boilertemp", "boiler", 0x0810);
DPStat pumpStat("pump", "heating1", 0x2906);

void globalCallbackHandler(const IDatapoint& dp, DPValue value) {
  debug.print(dp.getGroup());
  debug.print(" - ");
  debug.print(dp.getName());
  debug.print(" is ");
  char value_str[15] = {0};
  value.getString(value_str, sizeof(value_str));
  debug.println(value_str);
}

void setup() {
  WiFi.begin(ssid, password);
  while (WiFi.status() != WL_CONNECTED) {
    delay(500);
  }
  debug.begin(HOST_NAME);
  debug.setResetCmdEnabled(true); // Enable the reset command
  debug.showColors(true); // Colors

  VitoWiFi.enableLogger();
  VitoWiFi.setLogger(&debug);

  VitoWiFi.setGlobalCallback(globalCallbackHandler);
  VitoWiFi.setup(&Serial);

}

void loop() {
  static unsigned long lastMillis = 0;
  if (millis() - lastMillis > 60 * 1000UL) {  // read all values every 60 seconds
    lastMillis = millis();
    VitoWiFi.readAll();
  }
  VitoWiFi.loop();
  debug.handle();
}
bertmelis commented 2 years ago

In order to use the mentioned library, make sure to use version 2.1.2. Newer versions will not compile.

GnomiBerlin commented 2 years ago

I used putty with Telnet and so was easier than to activate Windows telnet. Telnet is running now but output is not helping me.

Mainly the difference for "getanlagenzeit" (only 4 Byte read) and the working "gettimerzirkusa" (8 Byte time windows for zirkulation pump) is of interest for us now. debug.txt I can put my current VitiWiFi_NodeMCU.ino again on dropbox for you?!

bertmelis commented 2 years ago

yes please. Although the error must be somewhere in the lib...

GnomiBerlin commented 2 years ago

https://www.dropbox.com/sh/iz97u42pn6ci3j7/AACdy5TvW8gvLrIYsngqVjoJa?dl=0

bertmelis commented 2 years ago

I did a small test:

    uint64_t testvar = 0x1234567890123456;
    Serial.printf("%llu\n", testvar);
    Serial.printf("0x%016X\n", testvar);
    Serial.printf("%"PRIu64"\n", testvar);    

The output was surprisingly this:

18446744073709551615
0x00000000FFFFFFFF
18446744073709551615

I thought 64-bit printing was included ages ago in the lib but it's broken apparently.

Since you publish your values in hex, this might be the reason why it fails.

bertmelis commented 2 years ago

You can workaround the issue by doing this: (don't know where you use the timer value, but this was one spot it being used)

void DP8HexCallbackHandler(const IDatapoint& dp, DPValue value) {
  //Umwandeln, und zum schluss per mqtt publish an mqtt-broker senden

  //int64_t nValue = 0x123456789ABCDEF; //(int64_t)value.getU64();
  int64_t nValue = (int64_t)value.getU64();

  char outVal[24];
  sprintf(outVal, "0x%lX%lX", (uint32_t)(nValue>>32), (uint32_t)nValue);

  char outName[40] = "VITOWIFI/";
  strcat(outName, dp.getName());
  mqttClient.publish(outName, 1, true, outVal);
}
bertmelis commented 2 years ago

https://github.com/esp8266/Arduino/issues/8673

bertmelis commented 2 years ago

See linked issue in the esp8266 repo. As far as I can tell, VitoWiFi does fetch 8 bytes but you should "print" it correctly. Don't know if you need the git version or just latest release of the Arduino core.

GnomiBerlin commented 2 years ago

Hello, thanks for the analysis. I added your workaround and now get:

gettimerzirkusa = FFFFFFFF94803B30 --> gettimerzirkusa = 0xFFFFFFFF94803B30 getanlagenzeit = 13092220 --> getanlagenzeit = 0x013092220

I have to analyze, but just as a first feedback.

GnomiBerlin commented 2 years ago

is there a position to debug, what bytes are really sent via the interface in both directions? from the v-control 3.0 logging I saw for the system time:

23-08-2022 08:56:13 DATA TX: 1 F7 8 8E 8 23-08-2022 08:56:13 DATA RX: 20 23-08-2022 08:56:13 DATA RX: 22 23-08-2022 08:56:13 DATA RX: 8 23-08-2022 08:56:13 DATA RX: 23 23-08-2022 08:56:13 DATA RX: 2 23-08-2022 08:56:13 DATA RX: 8 23-08-2022 08:56:13 DATA RX: 31 23-08-2022 08:56:13 DATA RX: 17 23-08-2022 08:56:16 DATA RX: 5

So really 8 Byte?!

bertmelis commented 2 years ago

I've updated the PR: https://github.com/bertmelis/VitoWiFi/pull/76

It now includes full rx/tx logging for the P300 protocol and KW. The latter was not implemented.

GnomiBerlin commented 2 years ago

thanks, how can I see it via the telnet debugging?

bertmelis commented 2 years ago

It's now part of standard logging. Make sure you use the right branch of VitoWiFi: https://github.com/bertmelis/VitoWiFi/tree/8-byte-dp

GnomiBerlin commented 2 years ago

Hi, I can not use the libraries directly, as I made some changes on a private copy.

I updated all your updates but see no optolink interface debug information on the Telnet output. I used your standard OptolinkKW.cpp and OptolinkP300.cpp updates.

I updated all on dropbox what I used: https://www.dropbox.com/sh/iz97u42pn6ci3j7/AACdy5TvW8gvLrIYsngqVjoJa?dl=0

I saw, that your new OptolinkKW.cpp updates from today are files from yesterday late evening?! This is the current debug with some extensions you also see in the code. Mainly again "gettimerzirkusa" and "getanlagenzeit" debug.txt

bertmelis commented 2 years ago
(D) READ gettimerzirkusa
(D) READ 2228
(D) ok
(D) DP gettimerzirkusa succes
(D) [JoP]on_request - gettimerzirkusa is 18446744071889238832 with length 8
(D) [JoP]VITOWIFI/gettimerzirkusa - FFFFFFFF93803B30
...
(D) READ 088e
(D) ok
(D) DP getanlagenzeit succes
(D) [JoP]synchronize - getanlagenzeit is 319365664 with length 8
(D) [JoP]VITOWIFI/getanlagenzeit - 013092220

You sure the uploaded files are used for your firmware? In the OptolinkKW.cpp file it should print the received values (see line 177)

GnomiBerlin commented 2 years ago

the library was used. I tested with adding an error which was found during compilation. Then I changed the order in the .ino file:

now it works and receives: (D) READ f7088e08 (D) ok (D) 2022091302125232 (D) DP getanlagenzeit succes (D) [JoP]synchronize - getanlagenzeit is 319365664 with length 8 (D) [JoP]VITOWIFI/getanlagenzeit - 013092220

So really the preparation of the received 8 Bytes contains an error.

bertmelis commented 2 years ago

I compiled again, don't know why but now I got compiler warnings whereas they didn't show up last time.

Could you change decoding

DPValue conv8_1_Timer::decode(const uint8_t* in) {
  uint64_t tmp = ((uint64_t)in[7]) << 56 |
                 ((uint64_t)in[6]) << 48 |
                 ((uint64_t)in[5]) << 40 |
                 ((uint64_t)in[4]) << 32 |
                 ((uint64_t)in[3]) << 24 |
                 ((uint64_t)in[2]) << 16 |
                 ((uint64_t)in[1]) << 8 |
                 in[0];
  DPValue out(tmp);
  return out;
}
GnomiBerlin commented 2 years ago

I have no compiler errors and used:

DPValue conv8Timer::decode(const uint8_t* in) {
//zum Lesen
  uint64_t tmp = in[7] << 56 | in[6] << 48 | in[5] << 40 | in[4] << 32 | in[3] << 24 | in[2] << 16 | in[1] << 8 | in[0];

  DPValue out(tmp);
  return out;
}

is the error perhaps here in DPValue.hpp due to the long long integer c-error?

case UINT64_T:
        snprintf(c, s, "%llu", v.u64.value); //%lu ?? %u
        break;

from below part ...

void getString(char* c, size_t s) {
    switch (v.b.type) {
    case BOOL:
      snprintf(c, s, "%s", (v.b.value) ? "true" : "false");
      break;
    case UINT8_T:
      snprintf(c, s, "%u", v.u8.value);
      break;
    case UINT16_T:
      snprintf(c, s, "%u", v.u16.value);
      break;
    case UINT32_T:
      snprintf(c, s, "%u", v.u32.value);
      break;
      case UINT64_T:
        snprintf(c, s, "%llu", v.u64.value); //%lu ?? %u
        break;
    case FLOAT:
      snprintf(c, s, "%.1f", v.f.value);
      break;
    case PTR:
      for (uint8_t i = 0; i < v.raw.length; ++i) {
        snprintf(c, s, "%02x", v.raw.value[i]);
        c+=2;
      }
      break;
    }
  }

...

bertmelis commented 2 years ago

warnings, not errors

You have to change the decoding and cast to 64 bit before bitshifting. I've updated the branch.

For the other types, uint8_t is casted to int automatically whereas casting to 64bit has to be done explicitely.

GnomiBerlin commented 2 years ago

it works - champagne!

Thanks for the hard work. I will test later, if writing is also working on 8 Byte fields...