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

Issue with settime validation? #66

Open
borys-wwa opened this issue Dec 29, 2021 · 1 comment
Open

Issue with settime validation? #66

borys-wwa opened this issue Dec 29, 2021 · 1 comment

Comments

@borys-wwa
Copy link

borys-wwa commented Dec 29, 2021

Hi.
I have two ESP32 set up with Your code to monitor and control 7 EQ-3 valves with use of Home Assistant.
For now, HA executes script every few minutes sending "<bleaddr> settime" command for each of the connected EQ3s, to the ESP32 just to trigger a status response, so that it could see, how much valve is open etc.
I noticed, that after few hours o running without issues, one of the ESPs starts throwing following error in the console:
"I (26386629) EQ3_MAIN: Invalid time argument �?M��?ğ�?"

The situation goes away, when I start sending the same command, but with a white space after "settime".
I'm not the great c++ expert, but I tried to analyze the code to see if I can find the root cause, and I believe it might be caused by the fact, that this line is looking beyond actual command string, when the command only contains "settime". I assume, that as long, as the memory behind it is not occupied by anything, all works fine, but as soon as some data is stored just adjacent to the cmdstr, it then messes that check up.
That would (I think) explain, why adding white space to the command fixes it, as it reserves that additional one byte, that this validation routine looks into, preventing it from being allocated to other data.
These are just assumptions, but I thought I would share them with You so that You could look into this.

Kind Regards,
Darek.

PS. Thanks for the great software :)

@softypit
Copy link
Owner

softypit commented Jan 1, 2022

Thanks Darek,
I guess during development this wasn't tested too thoroughly. You are correct that it should be checking cmdptr[7] for a 0 and not cmdptr[8].

I finally got around to pushing a new commit to the repository (v1.63) and included a fix for this. There isn't really a compelling reason to use the new code unless you really need the fix. There are a number of changes including a new esp-idf version, a new networking library and some additional config items although functionally the application works the same.
Regards.
Paul.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants