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 to use common routines/macros to handle bit manipulation. #934

Merged
merged 8 commits into from
Oct 7, 2019

Conversation

crankyoldgit
Copy link
Owner

@crankyoldgit crankyoldgit commented Sep 30, 2019

  • Add setBit(), setBits() methods to simplify & standardise bit manipulation.
    o Unit tests for above.

  • Add macros for GETBIT8() -> GETBIT64() & GETBITS8() -> GETBITS64()for standardising bit retrieval.
    o Macro used instead of a function as the overhead was too expensive. i.e. program space got bigger in most cases.

  • Improved bit handling for some protocols.
    o Changes the library values of some settings in a few protocols.
    o This required some minor changes to Unit Tests to match new values.

  • Refactor all the A/C protocols to use the above.
    o Above routines covered 99+% of use cases.

  • Passes all the existing unit tests without modification (see above for exceptions) so this is pretty much safe and should not affect functionality.

  • Fixes a number of potential issues where poor bit-wise math was used previously.

  • Program size remains roughly the same.
    o Some gains and some losses. Overall it's worth it.

  • Code style/formatting changes & improvements.

Note to reviewers:

  • Sorry this is a massive code review. It's mostly mechanical. It took almost a week to write. Take solace in the fact the the unit test coverage works as expected. It saved me more times than I can easily count when writing this.

@crankyoldgit crankyoldgit added enhancement Hacktoberfest Hacktoberfest participation labels Sep 30, 2019
@crankyoldgit crankyoldgit self-assigned this Sep 30, 2019
}

bool IRAmcorAc::getMax(void) {
return ((remote_state[kAmcorSpecialByte] & kAmcorMaxMask) == kAmcorMaxMask);
return GETBITS8(remote_state[kAmcorSpecialByte], kAmcorMaxOffset,
kAmcorMaxSize) == kAmcorMax;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all masks changed, or just specific ones?
I find the the new way of using offsets much harder to understand compared to using masks - this might lead to it being harder for others to understand the code, and contribute. - I do clearly see the benefits in other places.
It might however be better to have functions that works with mask directly, instead of introducing offsets.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Almost all masks have been changed. The exceptions at this stage are masks like 0b00100100 where the mask isn't contiguous.
Personally, I preferred simple bit masks. but a lot of people had trouble understanding them. When refactoring the code, I found a few examples of either poor masks that I had done, or ones that were wrong.
I often get them wrong when writing the code but people don't see that as I write unit tests to make sure things behave the way I expect, not the way I've initially written the code. :-)

You can still use the old ways of doing it.
I'll probably add some routines to allow arbitrary masks to this PR.

* Allow setting multiple bits at time.
* Unit tests for the above.
* Improve certain setting values in a couple of protocols after using 
narrower bit patterns.
* Lots of code/style formatting issues improved for code readablity.
* Reduce some duplicate code instances.
* Minor tweaks here and there.

* Except for some value changes, all unit tests passed and remain 
(largely) unmodified.
@crankyoldgit crankyoldgit merged commit 95a2563 into master Oct 7, 2019
@crankyoldgit crankyoldgit deleted the std_bit_handling branch October 7, 2019 14:42
crankyoldgit added a commit that referenced this pull request Oct 30, 2019
_v2.7.0 (20191030)_

**[Bug Fixes]**
- auto_analyse: Fix > 64 bit send code generation. (#976)
- auto_analyse: Fix missing arguments in generated code for send64+ (#972)
- IRsendProntoDemo: Fix compile issue on ESP32 platform. (#938)
- IRMQTTServer: Fix compile error when `MQTT_ENABLE` is false. (#933)

**[Features]**
- Add Hitachi 424 bit A/C support. (#975, #980, #981)
- Experimental detailed support for `DAIKIN152` (#971)
- Mitsubishi 112bit A/C support (#947, #968)
- gc_decode: Adding Support for Decoding codes in raw code format (#963)
- Refactor to use common routines/macros to handle bit manipulation. (#934)
- Use centralised common strings. Saves ~1.5k of program space. (#946)
- Add Internationalisation (i18n) / Locale support. (#946, #955, #966)
  - `de-CH`: Swiss German. (#949, #954)
  - `de-DE`: German. (#946, #950, #952)
  - `en-AU`: English/Australian (Default locale) (#946)
  - `en-IE`: English/Irish (#946)
  - `en-UK`: English/United Kingdom (#946)
  - `en-US`: English/United States (#946)
  - `es-ES`: Spanish. (#953)
  - `fr-FR`: French. (#962)
- Port CI pipeline to PlatformIO (#936)

**[Misc]**
- Add DAIKIN128 & DAIKIN152 to `decodeToState()` (#982)
- auto_analyse: Produce better code when leader is detected. (#977)
- Coolix A/C improvements (#944)
- A/C setRaw/getRaw/stateReset() cleanup. (#967)
- Add documentation on how to use & support the i18n aspects of the library.
- Make travis checks faster. (#957)
- Translate README.md to french (#959)
- Fixed Coolix kCoolixDefaultState (#941)
- Improve generation of list of pio projects. (#940)
@crankyoldgit crankyoldgit mentioned this pull request Oct 30, 2019
crankyoldgit added a commit that referenced this pull request Oct 30, 2019
_v2.7.0 (20191030)_

**[Bug Fixes]**
- auto_analyse: Fix > 64 bit send code generation. (#976)
- auto_analyse: Fix missing arguments in generated code for send64+ (#972)
- IRsendProntoDemo: Fix compile issue on ESP32 platform. (#938)
- IRMQTTServer: Fix compile error when `MQTT_ENABLE` is false. (#933)

**[Features]**
- Add Hitachi 424 bit A/C support. (#975, #980, #981)
- Experimental detailed support for `DAIKIN152` (#971)
- Mitsubishi 112bit A/C support (#947, #968)
- gc_decode: Adding Support for Decoding codes in raw code format (#963)
- Refactor to use common routines/macros to handle bit manipulation. (#934)
- Use centralised common strings. Saves ~1.5k of program space. (#946)
- Add Internationalisation (i18n) / Locale support. (#946, #955, #966)
  - `de-CH`: Swiss German. (#949, #954)
  - `de-DE`: German. (#946, #950, #952)
  - `en-AU`: English/Australia (Default locale) (#946)
  - `en-IE`: English/Ireland (#946)
  - `en-UK`: English/United Kingdom (#946)
  - `en-US`: English/United States (#946)
  - `es-ES`: Spanish. (#953)
  - `fr-FR`: French. (#962)
- Port CI pipeline to PlatformIO (#936)

**[Misc]**
- Add DAIKIN128 & DAIKIN152 to `decodeToState()` (#982)
- auto_analyse: Produce better code when leader is detected. (#977)
- Coolix A/C improvements (#944)
- A/C setRaw/getRaw/stateReset() cleanup. (#967)
- Add documentation on how to use & support the i18n aspects of the library.
- Make travis checks faster. (#957)
- Translate README.md to french (#959)
- Fixed Coolix kCoolixDefaultState (#941)
- Improve generation of list of pio projects. (#940)
crankyoldgit added a commit that referenced this pull request Jul 4, 2021
* Add Fan low and high speeds to setFan
* Adjust unit tests accordingly.
* Fixes a regression introduced in #934 

Co-authored-by: David Conran <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Hacktoberfest Hacktoberfest participation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants