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 6 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
33 changes: 10 additions & 23 deletions 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 @@ -99,30 +99,17 @@ class MyAdvertisedDeviceCallbacks : public BLEAdvertisedDeviceCallbacks
Serial.printf("TX power %d\n", foundEddyURL.getPower());
Serial.println("\n");
}
else if (payLoad[11] == 0x20)
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 102 to:
else if (payLoad[22] == 0x20)

in order to make it work.

if (advertisedDevice.getPayloadLength() >= 22 && payLoad[22] == 0x20)
{
Copy link
Collaborator

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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


eddyContent = "01234567890123";

for (int idx = 0; idx < 14; idx++)
{
eddyContent[idx] = payLoad[idx + 11];
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 112 to:
eddyContent[idx] = payLoad[idx + 22];

}

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

float calcTemp = temp / 256.0f;
Serial.printf("Reported temperature from data: %.2fC\n", calcTemp);
Serial.printf("Reported advertise count: %d\n", foundEddyURL.getCount());
Serial.printf("Reported time since last reboot: %ds\n", foundEddyURL.getTime());
Serial.println("\n");
Serial.print(foundEddyURL.toString().c_str());
Serial.println("\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason for removing Lines 123 to 125?

Serial.printf("Found an EddystoneTLM beacon! payload length = %d B; minus 22 = %d\n", advertisedDevice.getPayloadLength(), advertisedDevice.getPayloadLength() - 22);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

BLEEddystoneTLM eddystoneTLM;
//std::string TLMframe(payLoad[22], advertisedDevice.getPayloadLength() - 22);
//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());
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Serial.printf("Reported advertise count: %d\n", eddystoneTLM.getCount());
Serial.printf("Reported time since last reboot: %ds\n", eddystoneTLM.getTime());
}
}
}
Expand Down
22 changes: 10 additions & 12 deletions 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 @@ -13,6 +13,7 @@
5. Stop advertising.
6. deep sleep

To read data advertised by this beacon use second ESP with example sketch BLE_Beacon_Scanner
*/
#include "sys/time.h"

Expand All @@ -22,7 +23,7 @@
#include "BLEUtils.h"
#include "BLEBeacon.h"
#include "BLEAdvertising.h"
#include "BLEEddystoneURL.h"
#include "BLEEddystoneTLM.h"

#include "esp_sleep.h"

Expand All @@ -48,10 +49,10 @@ void setBeacon()
char beacon_data[25];
uint16_t beconUUID = 0xFEAA;
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);
Serial.printf("Converted to 8.8 format %0X%0X\n", (temp >> 8), (temp & 0xFF));
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);
Copy link
Collaborator

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please change it to:

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


BLEAdvertisementData oAdvertisementData = BLEAdvertisementData();
BLEAdvertisementData oScanResponseData = BLEAdvertisementData();
Expand Down Expand Up @@ -82,13 +83,11 @@ void setBeacon()

void setup()
{

Serial.begin(115200);
gettimeofday(&nowTimeStruct, NULL);

Serial.printf("start ESP32 %d\n", bootcount++);

Serial.printf("deep sleep (%lds since last reset, %lds since last boot)\n", nowTimeStruct.tv_sec, nowTimeStruct.tv_sec - last);
Serial.printf("Starting ESP32. Bootcount = %d\n", bootcount++);
Serial.printf("Deep sleep (%lds since last reset, %lds since last boot)\n", nowTimeStruct.tv_sec, nowTimeStruct.tv_sec - last);

last = nowTimeStruct.tv_sec;
lastTenth = nowTimeStruct.tv_sec * 10; // Time since last reset as 0.1 second resolution counter
Expand All @@ -103,12 +102,11 @@ void setup()
setBeacon();
// Start advertising
pAdvertising->start();
Serial.println("Advertizing started for 10s ...");
Serial.println("Advertising started for 10s ...");
delay(10000);
pAdvertising->stop();
Serial.printf("enter deep sleep for 10s\n");
Serial.printf("Enter deep sleep for 10s\n");
esp_deep_sleep(1000000LL * GPIO_DEEP_SLEEP_DURATION);
Serial.printf("in deep sleep\n");
}

void loop()
Expand Down
5 changes: 5 additions & 0 deletions libraries/BLE/src/BLEAdvertising.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,11 @@ void BLEAdvertisementData::setPartialServices(BLEUUID uuid) {
* @brief Set the service data (UUID + data)
* @param [in] uuid The UUID to set with the service data. Size of UUID will be used.
* @param [in] data The data to be associated with the service data advert.
* Data frame:
* | Field || Len | Type | UUID | EddyStone Frame |
* | Offset || 0 | 1 | 2 | 4 |
* | Len || 1 | 1 | 2 | up to 20 |
* | Data || ?? | ?? | 0xAA | 0xFE | ??? |
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 generic BLE advertising API.
Does this commentary relate to this?

*/
void BLEAdvertisementData::setServiceData(BLEUUID uuid, std::string data) {
char cdata[2];
Expand Down
15 changes: 10 additions & 5 deletions libraries/BLE/src/BLEEddystoneTLM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
#include "BLEEddystoneTLM.h"

static const char LOG_TAG[] = "BLEEddystoneTLM";
#define ENDIAN_CHANGE_U16(x) ((((x)&0xFF00)>>8) + (((x)&0xFF)<<8))
#define ENDIAN_CHANGE_U32(x) ((((x)&0xFF000000)>>24) + (((x)&0x00FF0000)>>8)) + ((((x)&0xFF00)<<8) + (((x)&0xFF)<<24))

BLEEddystoneTLM::BLEEddystoneTLM() {
beaconUUID = 0xFEAA;
Expand Down Expand Up @@ -46,10 +44,14 @@ uint16_t BLEEddystoneTLM::getVolt() {
return ENDIAN_CHANGE_U16(m_eddystoneData.volt);
} // getVolt

float BLEEddystoneTLM::getTemp() {
return ENDIAN_CHANGE_U16(m_eddystoneData.temp) / 256.0f;
uint16_t BLEEddystoneTLM::getTemp() {
return ENDIAN_CHANGE_U16(m_eddystoneData.temp);
} // getTemp
Copy link
Collaborator

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be the final code:

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


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

Copy link
Collaborator

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

uint32_t BLEEddystoneTLM::getCount() {
return ENDIAN_CHANGE_U32(m_eddystoneData.advCount);
} // getCount
Expand Down Expand Up @@ -110,6 +112,9 @@ std::string BLEEddystoneTLM::toString() {

/**
* Set the raw data for the beacon record.
* Example:
* uint8_t *payLoad = advertisedDevice.getPayload();
* eddystoneTLM.setData(std::string((char*)payLoad+22, advertisedDevice.getPayloadLength() - 22));
*/
void BLEEddystoneTLM::setData(std::string data) {
if (data.length() != sizeof(m_eddystoneData)) {
Expand All @@ -132,7 +137,7 @@ void BLEEddystoneTLM::setVolt(uint16_t volt) {
} // setVolt

void BLEEddystoneTLM::setTemp(float temp) {
m_eddystoneData.temp = (uint16_t)temp;
m_eddystoneData.temp = EDDYSTONE_TEMP_FLOAT_TO_U16(temp);
} // setTemp

void BLEEddystoneTLM::setCount(uint32_t advCount) {
Expand Down
55 changes: 30 additions & 25 deletions libraries/BLE/src/BLEEddystoneTLM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
#include "BLEUUID.h"

#define EDDYSTONE_TLM_FRAME_TYPE 0x20
#define ENDIAN_CHANGE_U16(x) ((((x)&0xFF00)>>8) + (((x)&0xFF)<<8))
#define ENDIAN_CHANGE_U32(x) ((((x)&0xFF000000)>>24) + (((x)&0x00FF0000)>>8)) + ((((x)&0xFF00)<<8) + (((x)&0xFF)<<24))
#define EDDYSTONE_TEMP_U16_TO_FLOAT(tempU16) (((int16_t)ENDIAN_CHANGE_U16(tempU16)) / 256.0f)
#define EDDYSTONE_TEMP_FLOAT_TO_U16(tempFloat) (ENDIAN_CHANGE_U16(((int)((tempFloat) * 256))))

/**
* @brief Representation of a beacon.
Expand All @@ -18,33 +22,34 @@
*/
class BLEEddystoneTLM {
public:
BLEEddystoneTLM();
std::string getData();
BLEUUID getUUID();
uint8_t getVersion();
uint16_t getVolt();
float getTemp();
uint32_t getCount();
uint32_t getTime();
std::string toString();
void setData(std::string data);
void setUUID(BLEUUID l_uuid);
void setVersion(uint8_t version);
void setVolt(uint16_t volt);
void setTemp(float temp);
void setCount(uint32_t advCount);
void setTime(uint32_t tmil);
BLEEddystoneTLM();
std::string getData();
BLEUUID getUUID();
uint8_t getVersion();
uint16_t getVolt();
uint16_t getTemp();
float getFloatTemp();
Copy link
Collaborator

@SuGlider SuGlider Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be the final code:

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

uint32_t getCount();
uint32_t getTime();
std::string toString();
void setData(std::string data);
void setUUID(BLEUUID l_uuid);
void setVersion(uint8_t version);
void setVolt(uint16_t volt);
void setTemp(float temp);
void setCount(uint32_t advCount);
void setTime(uint32_t tmil);

private:
uint16_t beaconUUID;
struct {
uint8_t frameType;
uint8_t version;
uint16_t volt;
uint16_t temp;
uint32_t advCount;
uint32_t tmil;
} __attribute__((packed)) m_eddystoneData;
uint16_t beaconUUID;
struct {
uint8_t frameType;
uint8_t version;
uint16_t volt;
uint16_t temp;
uint32_t advCount;
uint32_t tmil;
} __attribute__((packed)) m_eddystoneData;

}; // BLEEddystoneTLM

Expand Down