rewrite ir_Gree

[siriuslzx]

make ir_Gree easier to understand and maintain

[crankyoldgit]

Hey ya @siriuslzx,
First off, this is great. I really appreciate the effort. While this works fine on most CPU architectures and compilers, it doesn't work on all of them.
I was initially thrilled to find the "Bit fields" aspects of C and C++ myself, however there are reports of them not producing the same results with different compilers even for the same architecture.
See:

Notes
The following properties of bit fields are implementation-defined

The value that results from assigning or initializing a signed bit field with a value out of range, or from incrementing a signed bit field past its range.
Everything about the actual allocation details of bit fields within the class object
For example, on some platforms, bit fields don't straddle bytes, on others they do
Also, on some platforms, bit fields are packed left-to-right, on others right-to-left

https://en.cppreference.com/w/cpp/language/bit_field

So, it is with a very heavy heart that I'll probably have to decline this PR to try to keep the code as portable as possible. It really is a nice & nice work, and I really want to use it myself.

If you know of a good/clean/nice way to add a check for if the compiler/platform etc decides to swap on us, then I'd entertain it. I'd probably also re-write all the others too.

I'll also need to check if it saves flash space, or costs more too. We get a lot of downward pressure from major library users to reduce space where we can. It sometimes trumps code-readability. :-/

[siriuslzx]

Hey ya @siriuslzx,
First off, this is great. I really appreciate the effort. While this works fine on most CPU architectures and compilers, it doesn't work on all of them.
I was initially thrilled to find the "Bit fields" aspects of C and C++ myself, however there are reports of them not producing the same results with different compilers even for the same architecture.
See:

Notes
The following properties of bit fields are implementation-defined

The value that results from assigning or initializing a signed bit field with a value out of range, or from incrementing a signed bit field past its range.
Everything about the actual allocation details of bit fields within the class object
For example, on some platforms, bit fields don't straddle bytes, on others they do
Also, on some platforms, bit fields are packed left-to-right, on others right-to-left

https://en.cppreference.com/w/cpp/language/bit_field

So, it is with a very heavy heart that I'll probably have to decline this PR to try to keep the code as portable as possible. It really is a nice & nice work, and I really want to use it myself.

If you know of a good/clean/nice way to add a check for if the compiler/platform etc decides to swap on us, then I'd entertain it. I'd probably also re-write all the others too.

I'll also need to check if it saves flash space, or costs more too. We get a lot of downward pressure from major library users to reduce space where we can. It sometimes trumps code-readability. :-/

I'm glad you like it although you won't accept this PR. I test it with Arduino IDE 1.8.13, on WEMOS D1 mini and it works well. Since you say you need to check the flash space, I compile the example TrunOnGreeAC twice and the result is as follow:

the master version:
Executable segment sizes:
IROM : 240452 - code in flash (default or ICACHE_FLASH_ATTR)
IRAM : 27632 / 32768 - code in IRAM (ICACHE_RAM_ATTR, ISRs...)
DATA : 1252 ) - initialized variables (global, static) in RAM/HEAP
RODATA : 1752 ) / 81920 - constants (global, static) in RAM/HEAP
BSS : 25144 ) - zeroed variables (global, static) in RAM/HEAP

the bit field version:
Executable segment sizes:
IROM : 240356 - code in flash (default or ICACHE_FLASH_ATTR)
IRAM : 27632 / 32768 - code in IRAM (ICACHE_RAM_ATTR, ISRs...)
DATA : 1252 ) - initialized variables (global, static) in RAM/HEAP
RODATA : 1752 ) / 81920 - constants (global, static) in RAM/HEAP
BSS : 25144 ) - zeroed variables (global, static) in RAM/HEAP
hope it will be helpful for you :-)

[NiKiZe]

What is the probability that we can write a "startup test" to make sure it works? But then the question becomes, what should happen if it finds issues?

[crankyoldgit]

What is the probability that we can write a "startup test" to make sure it works? But then the question becomes, what should happen if it finds issues?

I personally haven't hit it being "different". All the platforms I have use regularly, it (bit fields) seem to work "as we want it to" on.
When I researched this a while ago for a different PR/merge I think it was Visual Studio's compiler that was different from the Gnu ones used by PlatformIO & Arduino's IDE. So I'm probably making a mountain out of a mole hill.
If I had some solid numbers/platforms it didn't work on, rather than that links's (and comments I researched at the time) statement I'd feel much stronger/safe using it. Especially if we had a check for when it wasn't working the way we expected.
As for how to act, we'd either fail at compile-time if we can, or learn what precompile detection we need and use an alternate structure that resolves down to the exact bit patterning we want. Failing at run-time would be a worst-case solution. People may not see/know why it was failing. Detecting via unit-test is a middle ground but no user is going to run a unit test when trying the library etc.

IROM : 240452 - code in flash (default or ICACHE_FLASH_ATTR)

vs

IROM : 240356 - code in flash (default or ICACHE_FLASH_ATTR)

So it appears the bit field one saves ~100 bytes for a "detailed" protocol. So, all up we might save 2-4k over the entire library.
It might be worth the expense/effort, especial considering the readability trade-off. However, it's a LOT of code/effort.
@siriuslzx Thanks for the data (and saving me the effort! :-) It is tempting.

[Jason2866]

@crankyoldgit

So it appears the bit field one saves ~100 bytes for a "detailed" protocol. So, all up we might save 2-4k over the entire library.
It might be worth the expense/effort, especial considering the readability trade-off. However, it's a LOT of code/effort.

saving about 2-4k would be a great step. The project owner of Tasmota is always asking @s-hadinger about the code size increase when updating to a new version. Without your great lib Tasmota would be not Tasmota ;-)

[crankyoldgit]

@crankyoldgit

So it appears the bit field one saves ~100 bytes for a "detailed" protocol. So, all up we might save 2-4k over the entire library.
It might be worth the expense/effort, especial considering the readability trade-off. However, it's a LOT of code/effort.

saving about 2-4k would be a great step. The project owner of Tasmota is always asking @s-hadinger about the code size increase when updating to a new version. Without your great lib Tasmota would be not Tasmota ;-)

I'm fully aware of how desirable reducing the resources is.
Sadly, it's a lot of work to redo everything, and 1-2 air-conditioning protocols will eat it up.
I'm still trying to work out a portable way of using this safely.

[crankyoldgit]

@siriuslzx I haven't forgotten this. Just trying to find some spare time come up with a safe solution for this so it doesn't bite us later. i.e. I really want to merge it and go this way.

[siriuslzx]

@siriuslzx I haven't forgotten this. Just trying to find some spare time come up with a safe solution for this so it doesn't bite us later. i.e. I really want to merge it and go this way.

I will take part in the work after you merge this

[crankyoldgit]

I've added some sanity that should trigger if we hit some problem with the use of Bit Fields on incompatible environments.
Hopefully that will cover us for finding issues early at least if we encounter them.

When the travis checks finish, I'll merge this PR, and we can start on moving all the protocols over to Bit Fields.

[crankyoldgit]

The code changes mention have now been included in the newly released v2.7.10 of the library.