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

refactor ir_Fujitsu #1419

Merged
merged 6 commits into from
Mar 3, 2021
Merged

refactor ir_Fujitsu #1419

merged 6 commits into from
Mar 3, 2021

Conversation

siriuslzx
Copy link
Collaborator

@siriuslzx siriuslzx commented Feb 26, 2021

  1. remove setbit(s), getbit(s) and use bit field instead;
  2. add "const" to get__() api and remove "const bool raw";
  3. remove some private member variable;
  4. split the function buildState() into two.

I use platformIO to build and compare the space size on wemos d1 mini,the configuration is as follows:

CONFIGURATION: https://docs.platformio.org/page/boards/espressif8266/d1_mini.html
PLATFORM: Espressif 8266 (2.6.2) > WeMos D1 R2 and mini
HARDWARE: ESP8266 80MHz, 80KB RAM, 4MB Flash
PACKAGES:

  • framework-arduinoespressif8266 3.20704.0 (2.7.4)
  • tool-esptool 1.413.0 (4.13)
  • tool-esptoolpy 1.20800.0 (2.8.0)
  • toolchain-xtensa 2.40802.200502 (4.8.2)
    Converting IRrecvDumpV2.ino
    LDF: Library Dependency Finder -> http://bit.ly/configure-pio-ldf
    LDF Modes: Finder ~ deep+, Compatibility ~ soft

Building in release mode

and the size(in unit Byte) of object file is as follows:

obj origin new
ir_Fujitsu.cpp.o 41,956 40,660

saving about 1KB.

Then I use platformIO inspect tools to analysis the memory use of examples, and the result(Ram and flash) is as follows:

example origin new
TurnOnFujitsuAC 27.9KB, 268KB 27.9KB, 267.6KB

saving about 400B.

Finally I use Then I use Arduino IDE (1.8.13) to compile the example TurnOnFujitsuAC, and the result is as follows:

IROM : 240184 - code in flash
IROM : 240152 - code in flash

save only 32B. I'm not very satisfied with the result.

Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already had a quick review of it after your second commit (i.e. fixing the fall thru warning.) and I saw the errors in model detection/creation etc.

It looked okay so far, but I need to spend some solid time reviewing this as you know, this is/was a complex beast of a IR class.
i.e. Expect a couple of days or so turn around on the review.

I see you've already fixed the mistakes that the IRac unit tests uncovered.

Anyway. Congrats for tackling this one, its about as tough/complex as they come.

Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As usual, your work is great. Thanks ever so much for do this, and for tackling this horrible code monster.

src/ir_Fujitsu.cpp Outdated Show resolved Hide resolved
// Enable bit for the Off/Sleep timer
setBit(&remote_state[12], 3, getOffSleepTimer());
_.OffTimerEnable = _.OffTimer > 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically I think you can get away with:
_.OffTimerEnable = _.OffTimer; As a non-zero value for a uint is classed as true. But, this is probably safer/more readable ... so treat this is just a FYI. (i.e. no action required)

src/ir_Fujitsu.cpp Show resolved Hide resolved
src/ir_Fujitsu.cpp Outdated Show resolved Hide resolved
src/ir_Fujitsu.cpp Outdated Show resolved Hide resolved
result.quiet = (getFanSpeed() == kFujitsuAcFanQuiet);
result.turbo = getCmd() == kFujitsuAcCmdPowerful;
result.econo = getCmd() == kFujitsuAcCmdEcono;
result.quiet = (_.Fan == kFujitsuAcFanQuiet);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be consistent with the style of the next two statements:

result.quiet = _.Fan == kFujitsuAcFanQuiet;

@siriuslzx
Copy link
Collaborator Author

As usual, your work is great. Thanks ever so much for do this, and for tackling this horrible code monster.

This may be my last PR, and it's been a good time :-)

@siriuslzx siriuslzx merged commit c818091 into crankyoldgit:master Mar 3, 2021
siriuslzx added a commit that referenced this pull request Mar 3, 2021
@siriuslzx
Copy link
Collaborator Author

I accidentally ordered revert, what do we do now?

@NiKiZe
Copy link
Collaborator

NiKiZe commented Mar 3, 2021

I accidentally ordered revert, what do we do now?

Revert was requested after merge, the revert is only in this PR and not in master correct?

It could be just ignored?
Other alternatives is to revert the revert, or force push previous state.

@crankyoldgit
Copy link
Owner

I believe @NiKiZe is correct. As it happened after the merge, it doesn't affect the master branch. So, nothing for us to worry about. i.e. The revert is just in your personal repo.

crankyoldgit added a commit that referenced this pull request Mar 24, 2021
_v2.7.16 (20210324)_

**[Features]**
- ToshibaAC: Swing handling and `setRaw()` improvements. (#1423 #1424 #1425)
- Support for XMP (Xfinity) protocol. (#1414 #1422)
- ToshibaAC: Adjust inter-message gap timing to improve matching. (#1420 #1421)
- Ecoclim: Add detailed A/C support (#1397 #1415)

**[Misc]**
- [ESP32] Fix `addApbChangeCallback(): duplicate func` kernel msgs (#1434 #1435)
- refactor ir_Fujitsu (#1419)
- refactor ir_Whirlpool (#1416)
- refactor ir_Vestel (#1413)
- refactor ir_Trotec (#1412)
@crankyoldgit crankyoldgit mentioned this pull request Mar 24, 2021
crankyoldgit added a commit that referenced this pull request Mar 24, 2021
## _v2.7.16 (20210324)_

**[Features]**
- ToshibaAC: Swing handling and `setRaw()` improvements. (#1423 #1424 #1425)
- Support for XMP (Xfinity) protocol. (#1414 #1422)
- ToshibaAC: Adjust inter-message gap timing to improve matching. (#1420 #1421)
- Ecoclim: Add detailed A/C support (#1397 #1415)

**[Misc]**
- [ESP32] Fix `addApbChangeCallback(): duplicate func` kernel msgs (#1434 #1435)
- refactor ir_Fujitsu (#1419)
- refactor ir_Whirlpool (#1416)
- refactor ir_Vestel (#1413)
- refactor ir_Trotec (#1412)
@crankyoldgit
Copy link
Owner

FYI, the aforementioned changes have been included in the new v2.7.16 release for the library.

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

Successfully merging this pull request may close these issues.

3 participants