implement time command reply

[cristianchelu]

This PR adds support for the time MCU command, in both posix and local time formats.

The posix response is the UTC timestamp, and the simple command is in the local time, as defined by the timezone in the configuration.

I tried to make it work without the miot_time configuration variable, but I couldn't find a way to get a reference to the time otherwise, in the style of your ota::global_ota_component->add_on_state_callback. Hints appreciated.

Example YAML config:

time:
  - platform: homeassistant
    id: homeassistant_time
    timezone: "Europe/Bucharest"

miot:
  id: miot_main
  miot_time: homeassistant_time

Example verbose log:

[16:55:08][V][miot:091]: Received MCU message 'time posix'
[16:55:08][D][miot:294]: MCU time request: sending posix time "1708174508"
[16:55:08][V][miot:183]: Sending reply '1708174508' to MCU

Although my MCU doesn't send the non-posix time command, I assume it will work per the blakadder specs. Here's an on-device log of how that looks:

[17:04:26][D][miot:332]: MCU time request: sending time "2024-02-17 17:04:25"

Disclaimer: I'm not a C++ guy, so any style/syntax/code nasties or other foot-guns you spot, I can change. :)

Hopefully the code is in good enough shape to merge, but I've had fun nonetheless implementing this.

[dhewg]

Nice!

We could just reuse CONF_TIME_ID from esphome.const instead of CONF_MIOT_TIME, since it's just an ordinary rtc reference without anything specific to miot?

Untested, but using that I guess this should work? cv.GenerateID(CONF_TIME_ID): cv.use_id(time.RealTimeClock)

(similar to how the miot/* components reference the main miot component)

When using all that, can time_ really be nullptr then (the 1st check in your function)?

And I'd not drag the whole strtok thing in your time_reply function. It basically just cares about "is the only argument 'posix' or not?", so maybe just:

std::string Miot::get_timereply(bool posix)

And in processmessage: sendreply(get_timereply(std::strcmp(strtok_r(nullptr, " ", &saveptr), "posix") == 0).c_str());

(completely untested, just written in my email client :P)

[cristianchelu]

We could just reuse CONF_TIME_ID from esphome.const instead of CONF_MIOT_TIME, since it's just an ordinary rtc reference without anything specific to miot?

Sweet! This is just what I was missing! I've just had to make it cv.Optional() and add the complementary checks, otherwise this works.

The only highly unlikely situation I can come up with for specifying an ID is 2+ time components,

time:
  - platform: homeassistant
    id: homeassistant_time_home
    timezone: "Europe/Bucharest"
  - platform: homeassistant
    id: homeassistant_time_work
    timezone: "Europe/Budapest"

for which case I'm not sure what the default time choice is, but this still compiles anyway.

When using all that, can time_ really be nullptr then (the 1st check in your function)?

With the revised code time_ shouldn't be nullptr, so I've removed that.

And I'd not drag the whole strtok thing in your time_reply function. It basically just cares about "is the only argument 'posix' or not?"

Alright, fair point. My feeling for the process_message_ function was that it's just a sorting station for incoming cmds and all command processing would happen inside the specialty handlers, but your suggestion makes the code simpler.

I've also removed the MCU time request: unknown request format \"%s\" check with this change, I'm not convinced I should add it back -- except for the cases of corrupted strings (e.g. time po?ix ) there shouldn't be non-standard arguments to this. For the corrupted case, if our reply is not in the expected format, the MCU retries 7 times anyway, so that should be sorted.

The only thing I'd have done is

- send_reply_(get_time_reply_(std::strcmp(strtok_r(nullptr, " ", &saveptr), "posix") == 0).c_str());
+ bool posix = std::strcmp(strtok_r(nullptr, " ", &saveptr), "posix") == 0;
+ send_reply_(get_time_reply_(posix).c_str());

for better readability, with the assumption that the compiler is smart enough to optimize away the one-time-use var, but I don't know how that works, and don't have a strong preference either way.

(Bonus) :facepalm: So the compiler doesn't check the parts of the code not #ifdef'd. Good to know.

-   ESP_LOGCONFIG(TAG, "  Time: UNAVAILABLE";
+   ESP_LOGCONFIG(TAG, "  Time: UNAVAILABLE");

[dhewg]

Does Optional + GenerateID really work? Without the time id, it doesn't look like it would generate the set_time() call?

I find the esphome time code is a bit weird. A real time component basically just sets the time with libc functions. So a time consumer doesn't really require a reference to a esphome time component, one could just access it via libc functions, but it seems other time consumers are all going the reference way. I'll take a look...

Are you sure the posix time need be be in utc?

[dhewg]

This seems to work (but strftime("%s") doesn't when using idf, so I've adapted that too):

std::string Miot::get_time_reply_(bool posix) {
#ifdef USE_TIME
  std::string res;

  if (posix) {
    auto now = ESPTime::from_epoch_utc(::time(nullptr));
    if (now.is_valid())
      res = to_string((int64_t)now.timestamp);
  } else {
    auto now = ESPTime::from_epoch_local(::time(nullptr));
    if (now.is_valid())
      res = now.strftime("%Y-%m-%d %H:%M:%S");
  }

  if (res.empty()) {
    ESP_LOGW(TAG, "MCU time request: time source not ready yet");
    return "0";
  }

  ESP_LOGD(TAG, "MCU time request: sending time \"%s\"", res.c_str());
  return res;
#else
  ESP_LOGW(TAG, "MCU time request: no time source available");
  return "0";
#endif
}

No conf/py stuff required, as we only care about a valid time and not where it came from ;)

- send_reply_(get_time_reply_(std::strcmp(strtok_r(nullptr, " ", &saveptr), "posix") == 0).c_str());
+ bool posix = std::strcmp(strtok_r(nullptr, " ", &saveptr), "posix") == 0;
+ send_reply_(get_time_reply_(posix).c_str());

Yeah, that line got a bit too much, agreed

[cristianchelu]

Does Optional + GenerateID really work? Without the time id, it doesn't look like it would generate the set_time() call?

Without the optional, when I tested with the time: declaration commented out in the yaml I got this error:

Failed config

miot: [source <unicode string>:113]

  Couldn't find any component that can be used for 'time::RealTimeClock'. Are you missing a hub declaration?.
  id: miot_main

Are you sure the posix time need be be in utc?

I just checked and ... it looks like it's also local. :/ Sorry, I should have tested more before opening the PR. This is weird though, because the timestamp by definition is "seconds since midnight 1/1/1970 UTC". We also don't need the from_epoch_utc call, because now.timestamp gives us the correct result regardless of the TZ.

  std::string res;
  auto now = ESPTime::from_epoch_local(::time(nullptr));

  if (now.is_valid()) {
    if (posix) {
      res = to_string((int64_t)now.timestamp);
    } else {
      res = now.strftime("%Y-%m-%d %H:%M:%S");
    }
  }

[12:16:50][D][miot:299]: MCU time request: sending time "1708251409"
// ^ GMT: Sunday, February 18, 2024 10:16:49 AM
[12:16:50][D][miot:299]: MCU time request: sending time "2024-02-18 12:16:49"
// ^ My TZ is GMT+2

The mmgg.feeder.fi1 I'm using has this weird 9:12 phon-time-zone property I can change I assumed was in play, but now that the device got its clock synced and I can test, whatever I set phon-time-zone to, the schedule seems to happen in the TZ of the get_time_reply_. Maybe the mi server artificially adds or removes phon-time-zone hours from the timestamp to make it match?

I'm not sure if it's a different case for other devices, but I wouldn't change this PR just because of one faulty device firmware.

No conf/py stuff required, as we only care about a valid time and not where it came from ;)

Yup. That was also my intention before being sucked in the rabbit hole of fuzzy documentation, stealing code from other components, and lack of experience in the language.

As always, thanks for your patience and insights! (I believe the only pieces of code left from my initial attempt are the log messages :D)

[dhewg]

Sorry, I should have tested more before opening the PR. This is weird though, because the timestamp by definition is "seconds since midnight 1/1/1970 UTC"

Don't worry about it, a PR doesn't need to be perfect when creating it. Often there's more than one way to do something, so it's in its nature to change. And I would be more surprised if the MCU would care about timezones, hence me asking.

[dhewg]

And this was a kinda tricky, esp-idf not supporting %s, esphome api a bit backwards... it's not exactly a nice starting ground to get into c/c++. There's alot of historic baggage and layers of APIs each with their own weirdness. So good work, figuring stuff out often takes more time than writing it in code!

[cristianchelu]

I appreciate the kind words of encouragement. :)

And this was a kinda tricky, esp-idf not supporting %s, esphome api a bit backwards... it's not exactly a nice starting ground to get into c/c++. There's alot of historic baggage and layers of APIs each with their own weirdness.

Haha, yeah, I daily in web development with its own history of dubious choices, and I wasn't expecting c and friends to be any different. I guess I just needed to vent my frustrations a bit.

Thanks for the merge and see you in the next one.