Apollon77 / daikin-controller

Control Daikin Air Conditioner devices using nodejs
MIT License
119 stars 29 forks source link

Add support to read demand control data #268

Closed Matze2 closed 1 month ago

Matze2 commented 8 months ago

Closes #267

For now only the minimum needed for later control is exposed:

Not exposed:

Note: DemandControl class is currently trivial, but will be extended later for write support.

Matze2 commented 6 months ago

@Apollon77

I know you are very busy but maybe you find a little bit of time to check the first two PRs (this one and the corresponding one in iobroker.daikin).

I just would like to know if I need to improve something or if they are already in a mergeable quality.

These two PRs cover the "read" part for demand control. The other two (#269 and https://github.com/Apollon77/ioBroker.daikin/pull/185) cover the "write" part and are implemented on top.

The code runs since since six weeks successfully in my iobroker environment with two Stylish devices. Using this enhancement I was able to switch off the daikin-cloud adapter.

Apollon77 commented 5 months ago

Hi @Matze2, sorry for the very late review. One question: Do all devices have this option? Do you know what devices that do not have it return?

ok i checked mine ... I get

curl http://192.168.178.49/aircon/get_demand_control
ret=OK,type=0 

Seems like it always gets returned but "type=0" means not supported?

In fact you now added it in a way that - in case of an error being returned - the sensor infos (which all have) would not be returned. Can you please adjust the order of the calls (put demand control latest and make sure it does not fail the whole chain? So maybe ignore errors there or such?)

Matze2 commented 5 months ago

Hi @Apollon77 , I think I understood. I will check and revise the code when I am back from vacation. Thanks for the feedback so far!

Apollon77 commented 5 months ago

Perfect, no hurries, I'm also on vacation next week :-)

I try to be faster afterwards with reviews :-)

Matze2 commented 1 month ago

Hi @Apollon77 ,

now that the heating season starts, I have better possibilities to test my changes. I worked on all your feedback, let me explain briefly what I've done.

Seems like it always gets returned but "type=0" means not supported?

Could be. But there is no documentation about that "type" field. At least I extended the code to expose also the "type" field. See commit 79afa3c274522ea918b2b11506d5162770f76d4f.

I can use it to disallow write changes on demand control if type != 1.

In fact you now added it in a way that - in case of an error being returned - the sensor infos (which all have) would not be returned. Can you please adjust the order of the calls (put demand control latest...

This one was trivial I just exchanged the order and sensor info is called first again. See commit 45f37d91999d9e1fb55b7612a0cd87eaed345ede.

... and make sure it does not fail the whole chain? So maybe ignore errors there or such?)

For this I first added a new boolean where I remember if "get_demand_control" URL threw an error. In this case I will skip the call fur future update calls. As a result the first update would raise an error (which means no value is updated) but with the next update the demand control part is skipped and everything works like before. See commit 6fccacf8f6d13a3322011a80ef6ee7c755545d93.

If we don't want this one-time-error, then we need to ignore errors for demand control completely. With this change the adapter should work as before even if the AC does not have demand control support and returns with PARAM_NG or similar errors. See commit d195795e0136c146381fb34ab789ec2a394b98ab.

I tested both choices by using an invalid URL "get_demand_control2".

The drawback with the last commit is that even if something else goes wrong, there is no way to escalate that. Currently the callback can only support response or error. But it is safer to keep the old behavior and no sudden new error messages even if only once at adapter start.

I would appreciate if you have time to review. Let me know if you need more explanations. Thanks in advance for your time.

Just to note that I made also some corresponding commits in iobroker. daikin project.

Apollon77 commented 1 month ago

@Matze2 for the type thing: Did yiu tried what happens if you set type to 0 or such ?

Apollon77 commented 1 month ago

Okl this "read" is ok now ... I will merge ... we only need to see if tests run or needs to be enhanced too

Matze2 commented 1 month ago

Thanks for merging. :+1:

Yes this is why I split read and write in different PRs. Please check also the corresponding "read" PR in iobroker.daikin. https://github.com/Apollon77/ioBroker.daikin/pull/184

After that I'll rebase the "write" PRs so they are easier to review.

Apollon77 commented 1 month ago

Lets do the lib first please ... then adapter is easier