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

Publish updates only on changes #210

Merged
merged 2 commits into from
Jan 5, 2025
Merged

Conversation

melyux
Copy link
Contributor

@melyux melyux commented Jan 4, 2025

There was already a check to only publish new status values when the status changed, but it had a bug: when comparing two nan values for outsideAirTemperature, the equality check would fail because nan == nan always fails in C++.

There was also already a check to only publish new setting values when the settings changed, but the line wasn't included in the otherwise empty if block (maybe by mistake).

Stage, sub mode, and auto sub mode statuses needed new checks, since it appears they were excluded from the settings by commenting out // this->heatpumpUpdate(receivedSettings) and not including them in the heatpumpSettings equality checks. Maybe this was a work in progress, or excluded later.

Closes #206

@echavet
Copy link
Owner

echavet commented Jan 4, 2025

Thanks for your work
I will soon have a look

@echavet
Copy link
Owner

echavet commented Jan 4, 2025

I think I remember why I ended up putting this->publishStateToHA(settings); outside the if brackets.

The connection to the module was systematically lost after a while.
Could you confirm if you're able to maintain a stable connection with this update?

@echavet
Copy link
Owner

echavet commented Jan 4, 2025

For the second commit this->wantedSettings.power it'a a good find!!

@melyux
Copy link
Contributor Author

melyux commented Jan 5, 2025

I think I remember why I ended up putting this->publishStateToHA(settings); outside the if brackets.

The connection to the module was systematically lost after a while.

Could you confirm if you're able to maintain a stable connection with this update?

I've had it running like this for about 24 hours and everything looks good. I tried turning on the unit with the remote control and the ESP received the right settings. Looking good! These changes don't affect the cycle communication with the unit as far as I can tell.

@echavet echavet merged commit 106a2d3 into echavet:main Jan 5, 2025
@flybrys
Copy link

flybrys commented Jan 5, 2025

Not sure if this is related however I recompiled my install to see if it would fix my fan mode errors and now my esp32 crashes after startup. Is there a way I can install a prior version to see if it fixes the issue?

@somlefant
Copy link

Not sure if this is related however I recompiled my install to see if it would fix my fan mode errors and now my esp32 crashes after startup. Is there a way I can install a prior version to see if it fixes the issue?

I had the same issue.
There is a typo on line 168 in hp_readings.cpp after the last commit. I have commented it in the commit and mentioned @echavet to hopefully bring it to his attention.

@melyux
Copy link
Contributor Author

melyux commented Jan 5, 2025

I see the problem. I'll make a new PR with the fix.

@melyux
Copy link
Contributor Author

melyux commented Jan 5, 2025

PR out with #213 should fix the typo

@flybrys
Copy link

flybrys commented Jan 5, 2025

PR out with #213 should fix the typo

figured out how to install this version in esphome and it works great. Thanks!

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

Successfully merging this pull request may close these issues.

Don't send updates if value didn't change
4 participants