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.

Sign up for GitHub

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
Merged

Refactor to use common routines/macros to handle bit manipulation. #934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3624a23
Basic function to get/setBits.
crankyoldgit Sep 23, 2019
e5e2121
Add setBit() via pointer
crankyoldgit Sep 24, 2019
ea4d67b
Improve bit functions
crankyoldgit Sep 24, 2019
f7dc6c0
Use std routines for all single bit operations for A/Cs.
crankyoldgit Sep 25, 2019
b7fdac9
Add irutils::setBits()
crankyoldgit Sep 25, 2019
14138fd
Improvements to setBits() and GETBIT*() handling.
crankyoldgit Sep 28, 2019
8221112
Modify existing a/c code to use setBits()/GETBITS().
crankyoldgit Sep 28, 2019
bb7a36d
Correct issues introduced after merge.
crankyoldgit Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion keywords.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,7 @@ kElectraAcMessageGap LITERAL1
kElectraAcMinRepeat LITERAL1
kElectraAcMinTemp LITERAL1
kElectraAcModeMask LITERAL1
kElectraAcOffsetTemp LITERAL1
kElectraAcTempDelta LITERAL1
kElectraAcOneSpace LITERAL1
kElectraAcPowerMask LITERAL1
kElectraAcStateLength LITERAL1
Expand Down
148 changes: 148 additions & 0 deletions src/IRutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,4 +1033,152 @@ namespace irutils {
if (integer > 99) return 255; // Too big.
return ((integer / 10) << 4) + (integer % 10);
}

// Return the value of `position`th bit of `data`.
// Args:
// data: Value to be examined.
// position: Nr. of the nth bit to be examined. `0` is the LSB.
// size: Nr. of bits in data.
bool getBit(const uint64_t data, const uint8_t position, const uint8_t size) {
if (position >= size) return false; // Outside of range.
return data & (1ULL << position);
}

// Return the value of `position`th bit of `data`.
// Args:
// data: Value to be examined.
// position: Nr. of the nth bit to be examined. `0` is the LSB.
bool getBit(const uint8_t data, const uint8_t position) {
if (position >= 8) return false; // Outside of range.
return data & (1 << position);
}

// Return the value of `data` with the `position`th bit changed to `on`
// Args:
// data: Value to be changed.
// position: Nr. of the bit to be changed. `0` is the LSB.
// on: Value to set the position'th bit to.
// size: Nr. of bits in data.
uint64_t setBit(const uint64_t data, const uint8_t position, const bool on,
const uint8_t size) {
if (position >= size) return data; // Outside of range.
uint64_t mask = 1ULL << position;
if (on)
return data | mask;
else
return data & ~mask;
}

// Return the value of `data` with the `position`th bit changed to `on`
// Args:
// data: Value to be changed.
// position: Nr. of the bit to be changed. `0` is the LSB.
// on: Value to set the position'th bit to.
uint8_t setBit(const uint8_t data, const uint8_t position, const bool on) {
if (position >= 8) return data; // Outside of range.
uint8_t mask = 1 << position;
if (on)
return data | mask;
else
return data & ~mask;
}

// Change the value at the location `data_ptr` with the `position`th bit
// changed to `on`
// Args:
// data: Ptr to the data to be changed.
// position: Nr. of the bit to be changed. `0` is the LSB.
// on: Value to set the position'th bit to.
void setBit(uint8_t * const data, const uint8_t position, const bool on) {
uint8_t mask = 1 << position;
if (on)
*data |= mask;
else
*data &= ~mask;
}

// Change the value at the location `data_ptr` with the `position`th bit
// changed to `on`
// Args:
// data: Ptr to the data to be changed.
// position: Nr. of the bit to be changed. `0` is the LSB.
// on: Value to set the position'th bit to.
void setBit(uint32_t * const data, const uint8_t position, const bool on) {
uint32_t mask = (uint32_t)1 << position;
if (on)
*data |= mask;
else
*data &= ~mask;
}

// Change the value at the location `data_ptr` with the `position`th bit
// changed to `on`
// Args:
// data: Ptr to the data to be changed.
// position: Nr. of the bit to be changed. `0` is the LSB.
// on: Value to set the position'th bit to.
void setBit(uint64_t * const data, const uint8_t position, const bool on) {
uint64_t mask = (uint64_t)1 << position;
if (on)
*data |= mask;
else
*data &= ~mask;
}

// Change the uint8_t pointed to by `dst` starting at the `offset`th bit
// and for `nbits` bits, with the contents of `data`.
// Args:
// dst: Ptr to the uint8_t to be changed.
// offset: Nr. of bits from the Least Significant Bit to be ignored.
// nbits: Nr of bits of `data` to be placed into the destination uint8_t.
// data: Value to be placed into dst.
void setBits(uint8_t * const dst, const uint8_t offset, const uint8_t nbits,
const uint8_t data) {
if (offset >= 8 || !nbits) return; // Short circuit as it won't change.
// Calculate the mask for the supplied value.
uint8_t mask = UINT8_MAX >> (8 - ((nbits > 8) ? 8 : nbits));
// Calculate the mask & clear the space for the data.
// Clear the destination bits.
*dst &= ~(uint8_t)(mask << offset);
// Merge in the data.
*dst |= ((data & mask) << offset);
}

// Change the uint32_t pointed to by `dst` starting at the `offset`th bit
// and for `nbits` bits, with the contents of `data`.
// Args:
// dst: Ptr to the uint32_t to be changed.
// offset: Nr. of bits from the Least Significant Bit to be ignored.
// nbits: Nr of bits of `data` to be placed into the destination uint32_t.
// data: Value to be placed into dst.
void setBits(uint32_t * const dst, const uint8_t offset, const uint8_t nbits,
const uint32_t data) {
if (offset >= 32 || !nbits) return; // Short circuit as it won't change.
// Calculate the mask for the supplied value.
uint32_t mask = UINT32_MAX >> (32 - ((nbits > 32) ? 32 : nbits));
// Calculate the mask & clear the space for the data.
// Clear the destination bits.
*dst &= ~(mask << offset);
// Merge in the data.
*dst |= ((data & mask) << offset);
}

// Change the uint64_t pointed to by `dst` starting at the `offset`th bit
// and for `nbits` bits, with the contents of `data`.
// Args:
// dst: Ptr to the uint64_t to be changed.
// offset: Nr. of bits from the Least Significant Bit to be ignored.
// nbits: Nr of bits of `data` to be placed into the destination uint64_t.
// data: Value to be placed into dst.
void setBits(uint64_t * const dst, const uint8_t offset, const uint8_t nbits,
const uint64_t data) {
if (offset >= 64 || !nbits) return; // Short circuit as it won't change.
// Calculate the mask for the supplied value.
uint64_t mask = UINT64_MAX >> (64 - ((nbits > 64) ? 64 : nbits));
// Calculate the mask & clear the space for the data.
// Clear the destination bits.
*dst &= ~(mask << offset);
// Merge in the data.
*dst |= ((data & mask) << offset);
}
} // namespace irutils
38 changes: 38 additions & 0 deletions src/IRutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
#include "IRremoteESP8266.h"
#include "IRrecv.h"

const uint8_t kNibbleSize = 4;
const uint8_t kLowNibble = 0;
const uint8_t kHighNibble = 4;
const uint8_t kModeBitsSize = 3;
uint64_t reverseBits(uint64_t input, uint16_t nbits);
String uint64ToString(uint64_t input, uint8_t base = 10);
String typeToString(const decode_type_t protocol,
Expand Down Expand Up @@ -65,5 +69,39 @@ namespace irutils {
const uint8_t init = 0);
uint8_t bcdToUint8(const uint8_t bcd);
uint8_t uint8ToBcd(const uint8_t integer);
bool getBit(const uint64_t data, const uint8_t position,
const uint8_t size = 64);
bool getBit(const uint8_t data, const uint8_t position);
#define GETBIT8(a, b) (a & ((uint8_t)1 << b))
#define GETBIT16(a, b) (a & ((uint16_t)1 << b))
#define GETBIT32(a, b) (a & ((uint32_t)1 << b))
#define GETBIT64(a, b) (a & ((uint64_t)1 << b))
#define GETBITS8(data, offset, size) \
(((data) & (((uint8_t)UINT8_MAX >> (8 - (size))) << (offset))) >> (offset))
#define GETBITS16(data, offset, size) \
(((data) & (((uint16_t)UINT16_MAX >> (16 - (size))) << (offset))) >> \
(offset))
#define GETBITS32(data, offset, size) \
(((data) & (((uint32_t)UINT32_MAX >> (32 - (size))) << (offset))) >> \
(offset))
#define GETBITS64(data, offset, size) \
(((data) & (((uint64_t)UINT64_MAX >> (64 - (size))) << (offset))) >> \
(offset))
uint64_t setBit(const uint64_t data, const uint8_t position,
const bool on = true, const uint8_t size = 64);
uint8_t setBit(const uint8_t data, const uint8_t position,
const bool on = true);
void setBit(uint8_t * const data, const uint8_t position,
const bool on = true);
void setBit(uint32_t * const data, const uint8_t position,
const bool on = true);
void setBit(uint64_t * const data, const uint8_t position,
const bool on = true);
void setBits(uint8_t * const dst, const uint8_t offset, const uint8_t nbits,
const uint8_t data);
void setBits(uint32_t * const dst, const uint8_t offset, const uint8_t nbits,
const uint32_t data);
void setBits(uint64_t * const dst, const uint8_t offset, const uint8_t nbits,
const uint64_t data);
} // namespace irutils
#endif // IRUTILS_H_
58 changes: 27 additions & 31 deletions src/ir_Amcor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ using irutils::addBoolToString;
using irutils::addModeToString;
using irutils::addFanToString;
using irutils::addTempToString;
using irutils::setBits;

#if SEND_AMCOR
// Send a Amcor HVAC formatted message.
Expand Down Expand Up @@ -145,49 +146,45 @@ void IRAmcorAc::on(void) { setPower(true); }
void IRAmcorAc::off(void) { setPower(false); }

void IRAmcorAc::setPower(const bool on) {
remote_state[kAmcorPowerByte] &= ~kAmcorPowerMask;
remote_state[kAmcorPowerByte] |= (on ? kAmcorPowerOn : kAmcorPowerOff);
setBits(&remote_state[kAmcorPowerByte], kAmcorPowerOffset, kAmcorPowerSize,
on ? kAmcorPowerOn : kAmcorPowerOff);
}

bool IRAmcorAc::getPower(void) {
return ((remote_state[kAmcorPowerByte] & kAmcorPowerMask) == kAmcorPowerOn);
return GETBITS8(remote_state[kAmcorPowerByte], kAmcorPowerOffset,
kAmcorPowerSize) == kAmcorPowerOn;
}

// Set the temp in deg C
void IRAmcorAc::setTemp(const uint8_t degrees) {
uint8_t temp = std::max(kAmcorMinTemp, degrees);
temp = std::min(kAmcorMaxTemp, temp);

temp <<= 1;
remote_state[kAmcorTempByte] &= ~kAmcorTempMask;
remote_state[kAmcorTempByte] |= temp;
setBits(&remote_state[kAmcorTempByte], kAmcorTempOffset, kAmcorTempSize,
temp);
}

uint8_t IRAmcorAc::getTemp(void) {
return (remote_state[kAmcorTempByte] & kAmcorTempMask) >> 1;
return GETBITS8(remote_state[kAmcorTempByte], kAmcorTempOffset,
kAmcorTempSize);
}

// Maximum Cooling or Hearing
void IRAmcorAc::setMax(const bool on) {
if (on) {
switch (getMode()) {
case kAmcorCool:
setTemp(kAmcorMinTemp);
break;
case kAmcorHeat:
setTemp(kAmcorMaxTemp);
break;
default: // Not allowed in all other operating modes.
return;
case kAmcorCool: setTemp(kAmcorMinTemp); break;
case kAmcorHeat: setTemp(kAmcorMaxTemp); break;
// Not allowed in all other operating modes.
default: return;
}
remote_state[kAmcorSpecialByte] |= kAmcorMaxMask;
} else {
remote_state[kAmcorSpecialByte] &= ~kAmcorMaxMask;
}
setBits(&remote_state[kAmcorSpecialByte], kAmcorMaxOffset, kAmcorMaxSize,
on ? kAmcorMax : 0);
}

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.


// Set the speed of the fan
Expand All @@ -197,36 +194,35 @@ void IRAmcorAc::setFan(const uint8_t speed) {
case kAmcorFanMin:
case kAmcorFanMed:
case kAmcorFanMax:
remote_state[kAmcorModeFanByte] &= ~kAmcorFanMask;
remote_state[kAmcorModeFanByte] |= speed << 4;
setBits(&remote_state[kAmcorModeFanByte], kAmcorFanOffset, kAmcorFanSize,
speed);
break;
default:
setFan(kAmcorFanAuto);
}
}

uint8_t IRAmcorAc::getFan(void) {
return (remote_state[kAmcorModeFanByte] & kAmcorFanMask) >> 4;
return GETBITS8(remote_state[kAmcorModeFanByte], kAmcorFanOffset,
kAmcorFanSize);
}

uint8_t IRAmcorAc::getMode(void) {
return remote_state[kAmcorModeFanByte] & kAmcorModeMask;
return GETBITS8(remote_state[kAmcorModeFanByte], kAmcorModeOffset,
kAmcorModeSize);
}

void IRAmcorAc::setMode(const uint8_t mode) {
remote_state[kAmcorSpecialByte] &= ~kAmcorVentMask; // Clear the vent setting
switch (mode) {
case kAmcorFan:
remote_state[kAmcorSpecialByte] |= kAmcorVentMask; // Set the vent option
// FALL-THRU
case kAmcorCool:
case kAmcorHeat:
case kAmcorDry:
case kAmcorAuto:
// Mask out bits
remote_state[kAmcorModeFanByte] &= ~kAmcorModeMask;
// Set the mode at bit positions
remote_state[kAmcorModeFanByte] |= mode;
setBits(&remote_state[kAmcorSpecialByte], kAmcorVentOffset,
kAmcorVentSize, (mode == kAmcorFan) ? kAmcorVentOn : 0);
setBits(&remote_state[kAmcorModeFanByte], kAmcorModeOffset,
kAmcorModeSize, mode);
return;
default:
this->setMode(kAmcorAuto);
Expand Down
26 changes: 17 additions & 9 deletions src/ir_Amcor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,36 +35,44 @@ const uint8_t kAmcorFanMin = 0b001;
const uint8_t kAmcorFanMed = 0b010;
const uint8_t kAmcorFanMax = 0b011;
const uint8_t kAmcorFanAuto = 0b100;
const uint8_t kAmcorFanMask = 0b01110000;
const uint8_t kAmcorFanOffset = 4;
const uint8_t kAmcorFanSize = 3;
// Modes
const uint8_t kAmcorCool = 0b001;
const uint8_t kAmcorHeat = 0b010;
const uint8_t kAmcorFan = 0b011; // Aka "Vent"
const uint8_t kAmcorDry = 0b100;
const uint8_t kAmcorAuto = 0b101;
const uint8_t kAmcorModeMask = 0b00000111;
const uint8_t kAmcorModeOffset = 0;
const uint8_t kAmcorModeSize = 3;

// state[2]
const uint8_t kAmcorTempByte = 2;
// Temperature
const uint8_t kAmcorMinTemp = 12; // Celsius
const uint8_t kAmcorMaxTemp = 32; // Celsius
const uint8_t kAmcorTempMask = 0b01111110;
const uint8_t kAmcorTempOffset = 1;
const uint8_t kAmcorTempSize = 6; // Bits

// state[5]
// Power
const uint8_t kAmcorPowerByte = 5;
const uint8_t kAmcorPowerMask = 0b11110000;
const uint8_t kAmcorPowerOn = 0b00110000; // 0x30
const uint8_t kAmcorPowerOff = 0b11000000; // 0xC0
const uint8_t kAmcorPowerOffset = 4;
const uint8_t kAmcorPowerSize = 4;
const uint8_t kAmcorPowerOn = 0b0011; // 0x3
const uint8_t kAmcorPowerOff = 0b1100; // 0xC

// state[6]
const uint8_t kAmcorSpecialByte = 6;
// Max Mode (aka "Lo" in Cool and "Hi" in Heat)
const uint8_t kAmcorMaxMask = 0b00000011; // 0x03
// "Vent" Mode
const uint8_t kAmcorVentMask = 0b11000000; // 0xC0
const uint8_t kAmcorMax = 0b11;
const uint8_t kAmcorMaxOffset = 0;
const uint8_t kAmcorMaxSize = 2;

// "Vent" Mode
const uint8_t kAmcorVentOn = 0b11;
const uint8_t kAmcorVentOffset = 6;
const uint8_t kAmcorVentSize = 2;
// state[7]
// Checksum byte.
const uint8_t kAmcorChecksumByte = kAmcorStateLength - 1;
Expand Down
Loading