I-Feel support - Githubissues

guyco75 commented 7 years ago

Hi!

First, I'm using this library for some time and it works great. So thanks a lot.. I found it ~3 years ago just as I finished parsing the sniffed commands and was about to start implementing it myself - so you saved me that part :) Just FYI, I'm using it on Arduino Nano combined with RPI3/OH2 and based on master from ~3 years ago.

One feature that I was missing in the library is I-Feel. At the time, when we still used the original remote, we always enabled i-feel which made the temperature sensing on the remote itself (which is located in the room) and not internally in the AC unit itself. This made the temperature in the room more stable without long ups and downs and variance in the temperature.

So recently I added the i-feel functionality to this library on top of the revision that I am using. The added support is very simple - it includes the option to set one additional bit in the configuration code that is sent to the AC, but also requires sending every two minutes a room temperature measurement notification (that is using nearly the same IR code structure - but with yet another bit set). I will be happy to share my changes if you'd like to review and consider including them - it's already functional and tested for some time (and it's backward compatible).

Thanks again for this great library! Guy

guyco75 commented 7 years ago

BTW this is the actual temperature in the room (measured every 1 minute) when I set the AC to 24C for few hours, except for half an hour in the middle that the AC was changed to 23C, for the sake of testing.. You can see that the AC responds to the temperature changes quite nicely.. The i-feel feature is mostly needed with a central AC unit (and not in a per-room unit)

barakwei commented 7 years ago

Thanks for the feedback! I always love to hear from people who use the library. It's even better to hear that people added features to it. I'd love to add more features like I-feel! This is the whole point of this repo. Few things:

I want to add I-Feel in any case, but if we're going scientific with the graph, I suggest you attach another graph, without I-Feel.
I'd like to know what changes you did to the library to make it work on Arduino (if any). There are people struggling to make it work (see issues).
I'm very close to merging the version-2.0 branch to master as my changes to arduinoSTL were merged a few hours ago. I believe it should be easier to add new features to remotes on that branch.
The way to contribute code here is to to fork the repo you want to contribute to (so you'll have a copy). Then you commit your changes to your repo, make sure they're working, and create a pull request to the original repo, on the pull request we can discuss the code, interface and such before merging (I assumed you don't know that since you have a new user here, forgive me if that's not the case). There are many guide here on Github and on the internet on how to contribute code in Github. If you don't want to go this way and just paste the code here, that's also fine, but it will take me time to do it.

guyco75 commented 7 years ago

Hi,

I will do the short experiment and upload :)
I remember that enabling Arduino was very easy. I don't remember what was exactly the issue - but this is the diff I see now in my copy. Maybe there was an issue with operator precedence order? Anyway I can enable it on Arduino again once I migrate to 2.0
```
uint64_t IRelectra::EncodeElectra(int power, int mode, int fan, int temperature, int swing, int sleep)
{
 uint64_t num = 0;
+    uint64_t power64 = power;
+    uint64_t mode64 = mode;
```

num |= (((uint64_t)power & 1) << 33);
num |= ((power64 & 1) << 33);
num |= (((uint64_t)mode & 7) << 30);
num |= ((mode64 & 7) << 30);
num |= (((uint64_t)fan & 3) << 28);
num |= ((fan & 3) << 28);
num |= (((uint64_t)swing & 1) << 25);
num |= ((swing & 1) << 25);

temperature -= 15;
num |= (((uint64_t)temperature & 15) << 19);
num |= ((temperature & 15) << 19);
num |= (((uint64_t)sleep & 1) << 18);

num |= ((sleep & 1) << 18);

num |= 2;

return num; }


3. So I guess I better wait till you "release" v2.0, migrate to it myself - as I'm still using a very old revision of your library - and then rebase my changes on top and deliver
4. Indeed I'm new to github as a contributor... I do have a lot of git experience but not here - so thanks for the directions :)

p.s. I will do post the i-feel changes here just for reference and will submit them properly after rebasing.

-uint64_t IRelectra::EncodeElectra(int power, int mode, int fan, int temperature, int swing, int sleep)
+uint64_t IRelectra::EncodeElectra(int power, int mode, int fan, int temp, int swing, int sleep, bool tempNotif, bool iFeel)
 {
     uint64_t num = 0;
-    
-    num |= (((uint64_t)power & 1) << 33);
-    
-    num |= (((uint64_t)mode & 7) << 30);
-    
-    num |= (((uint64_t)fan & 3) << 28);
-
-    num |= (((uint64_t)swing & 1) << 25);
-
-    temperature -= 15;
-    num |= (((uint64_t)temperature & 15) << 19);
-    
-    num |= (((uint64_t)sleep & 1) << 18);
-    
+
+    if (tempNotif)
+      temp -= 5;
+    else
+      temp -= 15;
+
+    num |= (((uint64_t)power     & 1)  << 33);
+    num |= (((uint64_t)mode      & 7)  << 30);
+    num |= (((uint64_t)fan       & 3)  << 28);
+    num |= (((uint64_t)tempNotif & 1)  << 27);
+    num |= (((uint64_t)swing     & 1)  << 25);
+    num |= (((uint64_t)iFeel     & 1)  << 24);
+    num |= (((uint64_t)temp      & 31) << 19);
+    num |= (((uint64_t)sleep     & 1)  << 18);
     num |= 2;

     return num;
 }

Then need to propagate the 2 new parameters through SendElectra and add a default =0 in the h file to both of them so the library can stay backwards compatible for users who don't care about i-feel..

Thanks, Guy.

barakwei commented 7 years ago

Yeah I had a problem with int in Arduino, all the parameters should be uint64_t since its width is platform agnostic. Once arduinoSTL version is out, I'll make sure it compiles and merge to master.

guyco75 commented 7 years ago

BTW - pay attention to the temp&31 (0x1f) vs. temp&15 (0xf) - it's actually a 5 bits field not 4...

barakwei commented 7 years ago

I'm pretty sure it's 4 bits. The remote gives me a range between [16, 31] Celsius which is 4 bits.

guyco75 commented 7 years ago

The 5th bit is indeed always 0 when not using i-feel, but it doesn't mean it's a reserved bit or not part of the temperature field :) When reporting back the temperature measured in the room by the remote (or arduino), the range is larger and uses also the MSBit..

barakwei commented 7 years ago

Sorry I didn't give attention to the implementation. So iFeel has two bits, one is iFeel, presumably to enable the feature, and the second one tempNotif is a boolean stating that the temperature field is the one sensed by the remote. Is this correct? Something seems missing here, I thought there would be two temperature fields, one for the "target" temperature and one for the sampling. If they use the same fields, you must first send a packet with the "target" temperature, followed by packets with iFeel temperatures. Am I missing something? I guess you should explain more about the protocol.

guyco75 commented 7 years ago

You are right - there are actually two separate types of commands sent to the AC, but both are using the same format/structure:

Configuration command - sent when the user is actually making a configuration change (power on/off toggle, changing temp, fan, mode, i-feel on/off, swing, ...) - this is sent once upon a user action
When the i-feel feature is enabled - the remote is sending a local temperature measurement notification every 2 minutes and indeed the same temperature field is reused. In the rest of the fields (fan, mode, ..) the remote sets the same values as in # 1 - I don't know if the AC cares about them or not, but I just do the same in my OH2 implementation anyway and populate them with the existing user configuration.

In # 1 - the remote may set the "i-feel" bit. In # 2 - the remote sets both "i-feel" and "temp-notif" bits (and it's send only if the feature was enabled in # 1 in first place)

barakwei commented 7 years ago

What's status if the iFeel bit in both cases? I'm guessing that in # 2 only tempNotif is ON, is this the case? Looks like these two cases deserve two different methods to send them.

guyco75 commented 7 years ago

In # 1 - the remote may set only the "i-feel" bit out of the two. In # 2 - the remote sets both "i-feel" and "temp-notif" bits (and it's send only if the feature was enabled in # 1 in first place).

In your library I don't think there is a reason to have two separate functions. The only difference is the -15 or -5 in temperature and anyway you don't check input validity in that function (e.g. protect from fan=112 or temp=-47). You just truncate the relevant bits - so the validity enforcement is on the user of your library - which is fare enough.

In my OH implementation of course I have separate code to react to user actions (power on/off toggle, changing temp, fan, mode, i-feel on/off, ...) and then if i-feel was enabled I have a separate code to send the temperature to the AC, triggered every two minutes (actually I do it every 1 minute currently).

You can see it here (if you're familiar with OH): https://github.com/guyco75/openhab-config/blob/master/rules/ac.rules And the arduino code I uploaded recently too: https://github.com/guyco75/arduino1

barakwei commented 7 years ago

The arduinoSTL library now has my changes. If you can, please take the version-2.0-tests-arduino and see if the code works for you, no changes should be necessary. If it doesn't work - let me know. If it works, I'd start on adding iFeel support. Also - if you change anything in the code to make it compile, let me know. The only thing you need is #include <ArduinoSTL.h> before including IRelectra.h.

barakwei / IRelectra

I-Feel support #8