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

PilnyTomas · 2023-02-03T14:59:25Z

This PR changes the conversion of 8.8 to float and vise versa to accommodate negative values.

SuGlider · 2023-02-06T21:01:26Z

libraries/BLE/src/BLEEddystoneTLM.h

@@ -41,7 +41,7 @@ class BLEEddystoneTLM {
 		uint8_t frameType;
 		uint8_t version;
 		uint16_t volt;
-		uint16_t temp;
+		int16_t temp;


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.

SuGlider · 2023-02-06T21:03:01Z

libraries/BLE/src/BLEEddystoneTLM.cpp

@@ -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;


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.

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

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.

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.

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.

SuGlider · 2023-02-07T02:46:19Z

libraries/BLE/examples/BLE_EddystoneTLM_Beacon/BLE_EddystoneTLM_Beacon.ino

@@ -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);


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.

SuGlider · 2023-02-07T02:55:37Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

@@ -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);
+          int16_t temp = (int16_t)payLoad[16] + (int16_t)(payLoad[15] << 8);


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); }

This reverts commit 1f3a941.

2 Macros EDDYSTONE_TEMP_U16_TO_FLOAT(tempU16) - takes the TLM BigEndian 8.8 fixed point representation and returns its float value EDDYSTONE_TEMP_FLOAT_TO_U16(tempFloat) - takes a float (temperature) and returns its BigEndian 8.8 fixed point representation

me-no-dev · 2023-02-14T15:09:11Z

@SuGlider PTAL

SuGlider · 2023-02-15T02:03:20Z

@PilnyTomas

I have tested the original version of BLE_EddystoneTLM_Beacon.ino example using the nRF Connect Android APP and it displays the ESP32S3 TLM information when I click on the name of the device.

SuGlider · 2023-02-15T02:10:24Z

For the screenshot above this is the Serial output:

ESP-ROM:esp32s3-20210327
Build:Mar 27 2021
rst:0x5 (DSLEEP),boot:0x8 (SPI_FAST_FLASH_BOOT)
pro cpu reset by JTAG
SPIWP:0xee
mode:DIO, clock div:1
load:0x3fce3808,len:0x44c
load:0x403c9700,len:0xbec
load:0x403cc700,len:0x2920
entry 0x403c98d8
start ESP32 14
deep sleep (284s since last reset, 21s since last boot)
Random temperature is 26.37C
Converted to 8.8 format 1A5E
Advertizing started for 10s ...
enter deep sleep for 10s

When I click on "RAW", I can see the Advertsing block of data which has such information, including the Temperature as 0x1A5E. See the picture below:

SuGlider · 2023-02-15T02:14:38Z

I'll do a full review of your PR by Wednesday.

SuGlider · 2023-02-16T03:54:03Z

libraries/BLE/src/BLEEddystoneTLM.h

+  uint8_t   getVersion();
+  uint16_t  getVolt();
+  uint16_t  getTemp();
+  float     getFloatTemp();


In the Original code there was just
float getTemp();

Now this has changed to float getFloatTemp();
and uint16_t getTemp();

This is a breaking change...
I don't think that it is a good idea.

The suggestion here is to keep the original API float getTemp(); and use the MACROs for converting temperature types whenever necessary.

This should be the final code:

BLEEddystoneTLM(); std::string getData(); BLEUUID getUUID(); uint8_t getVersion(); uint16_t getVolt(); float getTemp();

SuGlider · 2023-02-16T03:57:32Z

libraries/BLE/src/BLEEddystoneTLM.cpp

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


As said, this is a breaking change. I think it should stay the same:
float BLEEddystoneTLM::getTemp()

It should just use the new MACRO to convert u16 to float, considering the signal.
EDDYSTONE_TEMP_U16_TO_FLOAT(tempU16)

Suggestion is to eliminate uint16_t BLEEddystoneTLM::getTemp() and just keep original float BLEEddystoneTLM::getTemp() that converts U16 BigEndian to Float (return value) using the MACRO.

This should be the final code:

float BLEEddystoneTLM::getTemp() { return EDDYSTONE_TEMP_U16_TO_FLOAT(m_eddystoneData.temp); } // getTemp

SuGlider · 2023-02-16T03:58:10Z

libraries/BLE/src/BLEEddystoneTLM.cpp

+float BLEEddystoneTLM::getFloatTemp() {
+  return EDDYSTONE_TEMP_U16_TO_FLOAT(ENDIAN_CHANGE_U16(m_eddystoneData.temp));
+} // getFloatTemp
+


As said, this is a breaking change. I think it should stay the same:
float BLEEddystoneTLM::getTemp()

It should just use the new MACRO to convert u16 to float, considering the signal.

m_eddystoneData.temp is in Bigendian 8.8 fixed point (16 bits) and EDDYSTONE_TEMP_U16_TO_FLOAT() already does all the job.

No need to use ENDIAN_CHANGE_U16() here.

Please eliminate this method because it causes a breaking change to the API.

SuGlider · 2023-02-16T04:05:32Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

@@ -99,30 +99,17 @@ class MyAdvertisedDeviceCallbacks : public BLEAdvertisedDeviceCallbacks
          Serial.printf("TX power %d\n", foundEddyURL.getPower());
          Serial.println("\n");
        }
-        else if (payLoad[11] == 0x20)
+        if (advertisedDevice.getPayloadLength() >= 22 && payLoad[22] == 0x20)
        {


~~Are you sure that this is right? [11] --> [22]?~~ All fine.

OK. I checked it with nRF APP and it is correct. The position is 22.

SuGlider · 2023-02-16T04:07:09Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

+          //eddystoneTLM.setData(TLMFrame);
+          eddystoneTLM.setData(std::string((char*)payLoad+22, advertisedDevice.getPayloadLength() - 22));
+          Serial.printf("Reported battery voltage: %dmV\n", eddystoneTLM.getVolt());
+          Serial.printf("Reported temperature: %.2f°C (raw data=0x%04X)\n", eddystoneTLM.getFloatTemp(), eddystoneTLM.getTemp());


Here it may need a change back using the MACROs...
Please see the comentaries I made in BLEEddystoneTLM.h/.cpp

As suggested in BLEEddystoneTLM.h/.cpp, eddystoneTLM.getTemp() should be kept as in the original API, returning a float.
IF necessary, you can use EDDYSTONE_TEMP_FLOAT_TO_U16() to get the raw data.

SuGlider

In general, I'd say that the less we change the API and the original code, the better.
Changing the API will cause a breaking change, which is not desirable.

SuGlider · 2023-02-16T15:45:50Z

libraries/BLE/src/BLEAdvertising.cpp

+ * | Field  || Len | Type | UUID        | EddyStone Frame |
+ * | Offset || 0   | 1    | 2           | 4               |
+ * | Len    || 1   | 1    | 2           | up to 20        |
+ * | Data   || ??  | ??   | 0xAA | 0xFE | ???             |


This is a generic BLE advertising API.
Does this commentary relate to this?

SuGlider · 2023-02-16T16:10:24Z

libraries/BLE/examples/BLE_EddystoneTLM_Beacon/BLE_EddystoneTLM_Beacon.ino

+  float tempFloat = random(-3000, 3000) / 100.0f;
+  Serial.printf("Random temperature is %.2f°C\n", tempFloat);
+  int temp = EDDYSTONE_TEMP_FLOAT_TO_U16(tempFloat);
+  Serial.printf("Converted to 8.8 format 0x%04X\n", temp);


I tested the proposed new code and it doesn't work.

The problem with the new code is that EDDYSTONE_TEMP_FLOAT_TO_U16() returns a big endian uint16_t and the original code was using temp as little endian which was turned into big endian with the lines:

beacon_data[4] = (temp >> 8); // Beacon temperature beacon_data[5] = (temp & 0xFF); //

Please change it to:

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

SuGlider · 2023-02-16T20:11:43Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

        {
-          Serial.println("Found an EddystoneTLM beacon!");
-          BLEEddystoneTLM foundEddyURL = BLEEddystoneTLM();
-          std::string eddyContent((char *)&payLoad[11]); // incomplete EddystoneURL struct!


I just changed Line 106 to:
std::string eddyContent((char *)&payLoad[22]); // incomplete EddystoneURL struct!

SuGlider · 2023-02-16T20:14:15Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

-
-          for (int idx = 0; idx < 14; idx++)
-          {
-            eddyContent[idx] = payLoad[idx + 11];


I just changed Line 112 to:
eddyContent[idx] = payLoad[idx + 22];

SuGlider · 2023-02-16T20:16:08Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

-          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);


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

SuGlider · 2023-02-16T20:19:58Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

@@ -99,30 +99,17 @@ class MyAdvertisedDeviceCallbacks : public BLEAdvertisedDeviceCallbacks
          Serial.printf("TX power %d\n", foundEddyURL.getPower());
          Serial.println("\n");
        }
-        else if (payLoad[11] == 0x20)


I just changed Line 102 to:
else if (payLoad[22] == 0x20)

in order to make it work.

SuGlider · 2023-02-16T20:21:29Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

-          Serial.println("\n");
-          Serial.print(foundEddyURL.toString().c_str());
-          Serial.println("\n");
+          Serial.printf("Found an EddystoneTLM beacon! payload length = %d B; minus 22 = %d\n", advertisedDevice.getPayloadLength(), advertisedDevice.getPayloadLength() - 22);


I guess that printing the payload length is a left over of debugging, right?

SuGlider · 2023-02-16T20:29:26Z

libraries/BLE/examples/BLE_Beacon_Scanner/BLE_Beacon_Scanner.ino

-          Serial.printf("Reported time since last reboot: %ds\n", foundEddyURL.getTime());
-          Serial.println("\n");
-          Serial.print(foundEddyURL.toString().c_str());
-          Serial.println("\n");


Is there any reason for removing Lines 123 to 125?

SuGlider · 2023-02-16T21:07:34Z

@PilnyTomas

A last VERY important thing... why the original code looks at TLM data in the position [11] and not in the position [22]?
Well... the truth is that this position may change from Beacon to Beacon and it shall be interpreted based on the BLE Advertising Service Data Block information.

So the position 22 works for the BLE_EddystoneTLM_Beacon.ino current example, but if we changed the Beacon name oAdvertisementData.setName("TLMBeacon"); to something else, the postion of TLM data will change in the advertising service data block. You can try it and verify it.

Therefore, this postion shall be calculated based on the interpretation the this frame, looking for the field TYPE 0x16 with LEN = 17 and 0x20 in the 3rd byte of the VALUE.

More information can be found in these links:
https://github.com/google/eddystone/blob/master/eddystone-tlm/tlm-plain.md
https://community.silabs.com/s/article/kba-bt-0201-bluetooth-advertising-data-basics?language=en_US
https://novelbits.io/eddystone-beacons-zephyr-nrf52/

PilnyTomas · 2023-02-17T10:39:40Z

postion shall be calculate

In that case i can fix this in the other PR. But what to do now when we want it ASAP? Just roll back to original?

SuGlider · 2023-02-18T21:58:50Z

postion shall be calculate

In that case i can fix this in the other PR. But what to do now when we want it ASAP? Just roll back to original?

I've added the necessary commit to make it find the Eddystone data dynamicaly.
I shall work fine now, including for the URL beacon.

SuGlider

@PilnyTomas @me-no-dev @VojtechBartoska
I have tested Eddystone TLM and URL.
Everything is working fine.
Ready to merge.

iBeacons use big endian BLE fields.

SuGlider · 2023-02-19T01:31:21Z

Added a FIX (commit a64613d) for issue #7858

SuGlider · 2023-02-19T01:50:27Z

Added a FIX (commit a64613d) for issue #7858

Plus comit 5529457
Tested. Working correctly now. Issue #7858 solved.

This fix makes the BLE_iBeacon.ino to work correctly with the BLE_Beacon_Scanner.ino example

Changed data type of temperature

ddd6496

PilnyTomas added this to the 2.0.7 milestone Feb 3, 2023

PilnyTomas requested a review from SuGlider February 3, 2023 14:59

PilnyTomas self-assigned this Feb 3, 2023

PilnyTomas mentioned this pull request Feb 3, 2023

BLE Beacon Scanner example might not handle negative temperatures? #7618

Closed

1 task

Changed data type in EddystoneTLM class and example

1f3a941

VojtechBartoska added the Type: Example Issue is related to specific example. label Feb 6, 2023

VojtechBartoska requested a review from P-R-O-C-H-Y February 6, 2023 13:03

SuGlider requested changes Feb 7, 2023

View reviewed changes

PilnyTomas and others added 4 commits February 8, 2023 08:57

Revert "Changed data type in EddystoneTLM class and example"

cc893d5

This reverts commit 1f3a941.

Draft of Eddystone TLM example

f677580

Fixed temp

3ee9d53

PilnyTomas changed the title ~~Changed data type of temperature; solving #7618~~ Fix for negative temp in Eddystone TLM; solving #7618 Feb 14, 2023

PilnyTomas requested a review from SuGlider February 14, 2023 09:55

VojtechBartoska removed the request for review from P-R-O-C-H-Y February 15, 2023 13:28

VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Feb 15, 2023

SuGlider reviewed Feb 16, 2023

View reviewed changes

SuGlider requested changes Feb 16, 2023

View reviewed changes

SuGlider reviewed Feb 16, 2023

View reviewed changes

PilnyTomas and others added 5 commits February 17, 2023 11:53

Merge branch 'espressif:master' into beacon_scanner_fix

125cdee

Changed to conform with PR comments

96faa77

Fixed comment on closing bracket

0980a21

Prints negative TEMP big endian as just 2 bytes

a42a513

Extacts correct Eddyston Service Data

1f406c6

SuGlider added 3 commits February 18, 2023 19:29

Fixes BLEEddystoneTLM::toString() negative temp

f638759

Fixes URL field length

c9bf68d

Fixes Eddystone URL decoding

aa5486b

SuGlider self-requested a review February 19, 2023 00:02

SuGlider approved these changes Feb 19, 2023

View reviewed changes

Fixes MSB for iBeacon UUID

a64613d

iBeacons use big endian BLE fields.

SuGlider mentioned this pull request Feb 19, 2023

BLE on ESP32: BLEBeacon::getProximityUUID() returns reversed UUID string compared to expected value #7858

Closed

1 task

SuGlider self-assigned this Feb 19, 2023

SuGlider requested a review from me-no-dev February 19, 2023 02:04

Fix to detect iBeacon that also has Service UUID

5529457

This fix makes the BLE_iBeacon.ino to work correctly with the BLE_Beacon_Scanner.ino example

me-no-dev approved these changes Feb 20, 2023

View reviewed changes

me-no-dev merged commit b8ea455 into espressif:master Feb 20, 2023

VojtechBartoska removed the Status: Review needed Issue or PR is awaiting review label Feb 20, 2023

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

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

Conversation

PilnyTomas commented Feb 3, 2023 • edited by SuGlider Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-no-dev commented Feb 14, 2023

SuGlider commented Feb 15, 2023 • edited Loading

SuGlider commented Feb 15, 2023

SuGlider commented Feb 15, 2023

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuGlider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

SuGlider Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuGlider commented Feb 16, 2023 • edited Loading

PilnyTomas commented Feb 17, 2023

SuGlider commented Feb 18, 2023

SuGlider left a comment

Choose a reason for hiding this comment

SuGlider commented Feb 19, 2023 • edited Loading

SuGlider commented Feb 19, 2023 • edited Loading

PilnyTomas commented Feb 3, 2023 •

edited by SuGlider

Loading

SuGlider commented Feb 15, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider Feb 16, 2023 •

edited

Loading

SuGlider commented Feb 16, 2023 •

edited

Loading

SuGlider commented Feb 19, 2023 •

edited

Loading

SuGlider commented Feb 19, 2023 •

edited

Loading