Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console command crashes EMS-ESP #841

Closed
proddy opened this issue Dec 26, 2022 · 6 comments
Closed

console command crashes EMS-ESP #841

proddy opened this issue Dec 26, 2022 · 6 comments
Labels
bug Something isn't working
Milestone

Comments

@proddy
Copy link
Contributor

proddy commented Dec 26, 2022

On 3.5.0b13 and 3.5.0-dev.14

ems-esp:# call thermostat hc1 mode eco

Connection closed by foreign host

The entire device was just restarted automatically.

Originally posted by @akanto in #773 (comment)

@proddy proddy added the bug Something isn't working label Dec 26, 2022
@proddy proddy added this to the v3.5.0 milestone Dec 26, 2022
MichaelDvP added a commit to MichaelDvP/EMS-ESP32 that referenced this issue Dec 26, 2022
MichaelDvP added a commit to MichaelDvP/EMS-ESP32 that referenced this issue Dec 26, 2022
@MichaelDvP
Copy link
Contributor

I'm not sure what is the best fix.
Because the commandline only uses a prefix with no command a nullptr is returned here:

EMS-ESP32/src/command.cpp

Lines 228 to 231 in d08a122

// return null for empty command
if (command[0] == '\0') {
return nullptr;
}

It does not crash if we return the empty string instead.

The crash happens with nullptr here:

uint8_t device_id = EMSESP::device_id_from_cmd(device_type, id, cmd);

in this strcmp:
if ((id < 1 || dv.tag == tag) && dv.has_cmd && strcmp(dv.short_name, cmd) == 0) {

We can check for nullptr or use std::string instead of const char* (like in is_readonly here:
if (dv.has_cmd && std::string(dv.short_name) == cmd && (dv.tag < DeviceValueTAG::TAG_HC1 || dv.tag == tag || id == -1)) {
)

But with other commandlines like call thermostat hc1 it also crashes here:

return EMSESP::get_device_value_info(output, cmd, id, device_type) ? CommandRet::OK : CommandRet::ERROR; // entity = cmd

I think an easy fix is inserting at the start of Command::call:

    if (cmd == nullptr) {
        return CommandRet::NOT_FOUND;
    }

and the critical (crashing) functions are not called.

@proddy
Copy link
Contributor Author

proddy commented Dec 27, 2022

thanks for hunting that one down. Yes, the easiest fix is as you suggested. Do you mind fixing it?

MichaelDvP added a commit to MichaelDvP/EMS-ESP32 that referenced this issue Dec 27, 2022
proddy added a commit that referenced this issue Dec 27, 2022
@proddy proddy closed this as completed Dec 27, 2022
@proddy proddy reopened this Dec 30, 2022
@proddy
Copy link
Contributor Author

proddy commented Dec 30, 2022

seeing similar behaviour with testing the mixer

ems-esp:# call mixer info
{
  "wwc1 current temperature": 56.5,
  "wwc1 pump status in assigned wwc (PC1)": "off",
  "wwc1 temperature switch in assigned wwc (MC1)": 0,
  "wwc2 current temperature": 62.0,
  "wwc2 pump status in assigned wwc (PC1)": "off",
  "wwc2 temperature switch in assigned wwc (MC1)": 0,
  "hc1 mixing valve actuator (VC1)": 0,
  "hc1 setpoint flow temperature": 0,
  "hc1 pump status (PC1)": "off"
}

ems-esp:# call mixer hc1 info
make: *** [run] Segmentation fault: 11

@MichaelDvP
Copy link
Contributor

Hmm.

ems-esp:# call mixer hc1 info
Unknown command
Available commands are: activated commands entities flowsettemp info pumpstatus valvesettime

ems-esp:# call mixer hc2/info
{
  "Vorlauftemperatur HK (TC1)": 45.5,
  "Mischerventil Position (VC1)": 48,
  "Sollwert Vorlauftemperatur": 45,
  "Pumpenstatus HK (PC1)": "an",
  "Aktiviert": "an",
  "Zeit zum Einstellen des Ventils": 120
}

ems-esp:# call mixer info 2
{
  "Vorlauftemperatur HK (TC1)": 45.5,
  "Mischerventil Position (VC1)": 48,
  "Sollwert Vorlauftemperatur": 45,
  "Pumpenstatus HK (PC1)": "an",
  "Aktiviert": "an",
  "Zeit zum Einstellen des Ventils": 120
}

ems-esp:# call mixer info
{
  "HK2 Vorlauftemperatur HK (TC1)": 46.2,
  "HK2 Mischerventil Position (VC1)": 45,
  "HK2 Sollwert Vorlauftemperatur": 45,
  "HK2 Pumpenstatus HK (PC1)": "an",
  "HK2 Aktiviert": "an",
  "HK2 Zeit zum Einstellen des Ventils": 120
}

@proddy
Copy link
Contributor Author

proddy commented Dec 30, 2022

I'm running from standalone simulating a mixer. Maybe that's the issue

@MichaelDvP
Copy link
Contributor

No, i'm wrong, it crashes, my version was different ,i've tested with this

EMS-ESP32/src/command.cpp

Lines 229 to 231 in 6c09134

if (command[0] == '\0') {
return nullptr;
}

removed to give empty string back.

With nullptr returned it crashes here in strcmp

if (strncmp(cmd, "info", 4) == 0) {

MichaelDvP added a commit to MichaelDvP/EMS-ESP32 that referenced this issue Dec 31, 2022
@proddy proddy closed this as completed Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants