Skip to content

Fix for negative temp in Eddystone TLM; solving #7618 #7791

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

Merged
merged 16 commits into from
Feb 20, 2023
Merged

Fix for negative temp in Eddystone TLM; solving #7618 #7791

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ddd6496
Changed data type of temperature
PilnyTomas Feb 3, 2023
1f3a941
Changed data type in EddystoneTLM class and example
PilnyTomas Feb 3, 2023
cc893d5
Revert "Changed data type in EddystoneTLM class and example"
PilnyTomas Feb 8, 2023
f677580
Draft of Eddystone TLM example
PilnyTomas Feb 9, 2023
50e1c7e
Adds MACROs to convert beacon temperature
SuGlider Feb 13, 2023
3ee9d53
Fixed temp
PilnyTomas Feb 13, 2023
125cdee
Merge branch 'espressif:master' into beacon_scanner_fix
PilnyTomas Feb 17, 2023
96faa77
Changed to conform with PR comments
PilnyTomas Feb 17, 2023
0980a21
Fixed comment on closing bracket
PilnyTomas Feb 17, 2023
a42a513
Prints negative TEMP big endian as just 2 bytes
SuGlider Feb 18, 2023
1f406c6
Extacts correct Eddyston Service Data
SuGlider Feb 18, 2023
f638759
Fixes BLEEddystoneTLM::toString() negative temp
SuGlider Feb 18, 2023
c9bf68d
Fixes URL field length
SuGlider Feb 18, 2023
aa5486b
Fixes Eddystone URL decoding
SuGlider Feb 18, 2023
a64613d
Fixes MSB for iBeacon UUID
SuGlider Feb 19, 2023
5529457
Fix to detect iBeacon that also has Service UUID
SuGlider Feb 19, 2023
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 libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class MyAdvertisedDeviceCallbacks : public BLEAdvertisedDeviceCallbacks
foundEddyURL.setData(eddyContent);
Serial.printf("Reported battery voltage: %dmV\n", foundEddyURL.getVolt());
Serial.printf("Reported temperature from TLM class: %.2fC\n", (double)foundEddyURL.getTemp());
int temp = (int)payLoad[16] + (int)(payLoad[15] << 8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just changed Line 118 to:
int16_t temp = payLoad[22+5] + (payLoad[22+4] << 8);

int16_t temp = (int16_t)payLoad[16] + (int16_t)(payLoad[15] << 8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is OK. Another way to do the same:
int16_t temp = (payLoad[16] + (payLoad[15] << 8);

We could improve it by testing if the beacon really supports temperature measuring :

 // BIG ENDIAN payload in signed 8.8 fixed-point notation. Unit is Celsius. 
 int16_t temp_payload = payLoad[16] + (payLoad[15] << 8);
 if (payLoad[15] == 0x80 && payLoad[16] == 0) {
    Serial.printf("This device can't measure temperatures.\n");
 } else {
    float calcTemp = temp_payload / 256.0f;
    Serial.printf("Reported temperature from data: %.2fC\n", calcTemp);
 }

float calcTemp = temp / 256.0f;
Serial.printf("Reported temperature from data: %.2fC\n", calcTemp);
Serial.printf("Reported advertise count: %d\n", foundEddyURL.getCount());
Expand Down
2 changes: 1 addition & 1 deletion libraries/BLE/examples/BLE_EddystoneTLM_Beacon/BLE_EddystoneTLM_Beacon.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ void setBeacon()
uint16_t volt = random(2800, 3700); // 3300mV = 3.3V
float tempFloat = random(2000, 3100) / 100.0f;
Serial.printf("Random temperature is %.2fC\n", tempFloat);
int temp = (int)(tempFloat * 256); //(uint16_t)((float)23.00);
int16_t temp = (int16_t)(tempFloat * 256);
Copy link
Collaborator

Choose a reason for hiding this comment

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

float can't be converted to uint16_t in this way...
float has 4 bytes and it uses IEEE-754 single precision representation.
https://www.c-programming-simple-steps.com/c-float.html

The original code is correct. Please revert the change.
https://en.wikipedia.org/wiki/Fixed-point_arithmetic

The float temperature is converted into 8.8 fixed point notation and then inverted to become a bigendian data in order to be placed in the Eddystone Beacon advertising information, as done in the sketch.

Serial.printf("Converted to 8.8 format %0X%0X\n", (temp >> 8), (temp & 0xFF));

BLEAdvertisementData oAdvertisementData = BLEAdvertisementData();
Expand Down
2 changes: 1 addition & 1 deletion libraries/BLE/src/BLEEddystoneTLM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ void BLEEddystoneTLM::setVolt(uint16_t volt) {
} // setVolt

void BLEEddystoneTLM::setTemp(float temp) {
m_eddystoneData.temp = (uint16_t)temp;
m_eddystoneData.temp = (int16_t)temp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the C struct with raw data.
m_eddystoneData.temp shall be a uint16_t

BUT.... this data shall be 2 bigendian bytes in Fixed Point Notation 8.8 that represents the value of the float...
Therefore, it is necessary to calculate it instead of just casting a single precision float (32 bits) to a uint16_t.

The original code is completely wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we will leave the data model in its original type casting, it is now necessary to cast m_eddystoneData.temp to int16_t in the float BLEEddystoneTLM::getTemp() function.

https://github.com/espressif/arduino-esp32/blob/master/libraries/BLE/src/BLEEddystoneTLM.cpp#L49-L51

It is always better to leave the data processing logic in the API code that returns/gets and sets the information.

float BLEEddystoneTLM::getTemp() {
	return ((int16_t)ENDIAN_CHANGE_U16(m_eddystoneData.temp)) / 256.0f;
} // getTemp

Copy link
Collaborator

Choose a reason for hiding this comment

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

Line
https://github.com/espressif/arduino-esp32/blob/master/libraries/BLE/src/BLEEddystoneTLM.cpp#L28

is completely wrong as well.
It shall be the 2 bytes BigEndian representation of a 8.8 fixed point notation of the 23.00 float number...

Running that original code line, m_eddystoneData.temp will value zero.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Line
https://github.com/espressif/arduino-esp32/blob/master/libraries/BLE/src/BLEEddystoneTLM.cpp#L76

also needs to be casted to int16_t after having its endianness solved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Final suggestion:

Create a static BLEEddystoneTLM::float2temp(float fTemp) or just a macro to convert a float to BigEndian 2 bytes in 8.8 fixed point notation. It will be used to fix the code.

} // setTemp

void BLEEddystoneTLM::setCount(uint32_t advCount) {
Expand Down
2 changes: 1 addition & 1 deletion libraries/BLE/src/BLEEddystoneTLM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class BLEEddystoneTLM {
uint8_t frameType;
uint8_t version;
uint16_t volt;
uint16_t temp;
int16_t temp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a packed C struct with raw data from BLE advertising package.
Temperature is a bigendian data in fixed point notation 8.8.

Better leave it like in the original code as uint16_t
Please revert it.

uint32_t advCount;
uint32_t tmil;
} __attribute__((packed)) m_eddystoneData;
Expand Down