Added default temperature for ESP32 and NRF52 repeaters in telemetry

[IoTThinks]

Hi all,
I have added a default temperature for ESP32 and NRF52 repeaters in telemetry.

• The default temperature is from built-in sensor in MCUs.
• I have added a parent object NRF52Board.h and NRF52Board.cpp for all NRF52 boards. The same way we did for ESP32Board.

This is to know the temperature on the repeater box.
I have tested for Heltec v3/V4, RAK4631 and T-echo Lite under room and outdoor roof.
Quite accurate. 25C at 6am and 54C at 2pm.

[IoTThinks]

STM32 based boards have built-in temperature too.
But may be it is in TODO list for you.

The result is like below.

[IoTThinks]

RAK4631 is tested.

[jbrazio]

As it is, this will create confusion: "Temperature" is used for "environmental temperature" we should not show "CPU temperature" under the same label. Nevertheless this is interesting information available "for-free", it would be great to work with @liamcottle so the app could represent this under a different label.

[IoTThinks]

@jbrazio If any sensor regardless built-in or external sensor can give a meaningful value, then it would be useful.
The built-in temperature of NRF52 and ESP32 is even more accurate than the sensor of a T1000-e.
The correctness is due to the heat from NRF52 and ESP32 is not much compared to the environment.

The default temperature should be available to all repeaters as they all need to be temperature monitored under the sun.
External temperature sensors is just addon and I believe not many repeaters have external sensors.

[IoTThinks]

I will add a BME680 to ESP32 and RAK repeaters.
Then we can compare the built-in temperature sensors and BME sensors.

It would be interesting.

[oltaco]

I have no opinion on whether or not this is a PR that we want, but can I suggest it would be cleaner to do a rebase squash first so that the PR is not 21 commits 🙏

[IoTThinks]

Yes, besides temperature, I created NRF52Board parent object like ESP32Board.

So we do not need to copy & paste the same code 10+ times to 10+ NRF52 boards.

The PR alone is just around 20 lines.

They should share the same feature and consistent look&feel.

[IoTThinks]

@jbrazio I fully understand the two links you gave me. Thanks a lot.
I would be happy to make this Telemetry as MCU Temperature, too. Rest assured on this point.

However, MeshCore uses CayenneLPP as the framework for telemetry.

However, as I know (correct me if I'm wrong), CayenneLPP has pre-defined and fixed telemetry label like Temperature and Humidity...
There can not be Room Temperature or Roof temperature or MCU temperature, just....temperature.

This will be a great limitation if later we come to Meshcore Sensor.
For sensors, we may have a lot sensors in the same Meshcore sensor node.

So if we can not change / switch CayenneLPP now.
We can use MCU Temperature as the default temperature in Channel 1.
Other temperature is well, optional and can override this temperature in Channel 2...

This is in my room now.

This is from BME680, trusted by many friends.

From Heltec v3, 30.9*C from built-in temperature sensor

This is from RAK4631.

The difference is 1-2 degree. This is close and useful enough.
I'm pretty sure when we have 10 temperature sensors, they will show different values even if they are industrial sensors.
Actually, I feel the temperature in Heltec v3 is more accurate than a BME680.
I have a few more temperature sensors around as a referees.

[IoTThinks]

From friends in Denmark, he said the temperature is more or less correct for his Heltec v3.
Well, 3.9*C.

No idea why wrong battery voltage, I didn't touch the battery voltage.
I saw there were some changes for ADC reading in v1.11.0 or the ADC reading is impacted by the coldness.
It means the built-in temperature is quite useful. Especially when the Heltec v3 is in light sleep to cool down most of the time.

[wel97459]

I like having this on my repeaters, and I have added it myself in my own mod of the firmware. I just pulled this into my branch, you did it cleaner.

[4np]

I was also looking at the code yesterday to add Air Quality Index from BME68X, and running into the same limitations with CayenneLPP. There are just fixed types in there and not much flexibility.

It would help if the telemetry channels could be named. Then Channel 1 could be CPU / Radio, Channel 2 could be Environment Sensor, etcetera. Ideally CayenneLPP should be more flexible, but I guess that deviates from the entire point of the framework - to send telemetry over LoRa with minimal footprint. If we need to be able to support more telemetry readings, perhaps a fork is necessary?

Note: As a side note, I am also getting the impression that some values cause output in the app to halt / break. For example, in the BME680 implementation, altitude based on barometric pressure (which is pretty inaccurate as you need to know the current barometric pressure at sea level for your geolocation and weather conditions) will never show in the app. If you shuffle the way the CayenneLPP is constructed (e.g. altitude first, temperature next, etcetera) you won't see any values at all so somehow altitude (incorrectly computed to 122m in my location) 'breaks' the implementation. I am not sure if that's caused by CayenneLPP or the app as I have no clue how to debug either one of them apart from flashing a newly built firmware.

[IoTThinks]

@4np I think we need to leave CayenneLPP in very near future.

May be for repeaters, it can use CayenneLPP for basic sensors. Well, repeaters do not need to have many sensors.
We can have Channel 1 (Built-in sensors) and Channel 2 (External sensors).

However, for MeshCore sensors, it will be a different story.
In LoRaWAN, we just use bytes[] array and let the application decides how to decode the bytes.
So may be, for MeshCore sensors, I will just send bytes[] array or base64 messages.

[wel97459]

With CatenneLPP you can only have one of any value type in a channel, I ran into this with monitoring battery voltage and solar panel voltage in channel 1 only the last value written is sent. Channel naming could be handled by the client app.

I was also looking at the code yesterday to add Air Quality Index from BME68X, and running into the same limitations with CayenneLPP. There are just fixed types in there and not much flexibility.

It would help if the telemetry channels could be named. Then Channel 1 could be CPU / Radio, Channel 2 could be Environment Sensor, etcetera. Ideally CayenneLPP should be more flexible, but I guess that deviates from the entire point of the framework - to send telemetry over LoRa with minimal footprint. If we need to be able to support more telemetry readings, perhaps a fork is necessary?

Note: As a side note, I am also getting the impression that some values cause output in the app to halt / break. For example, in the BME680 implementation, altitude based on barometric pressure (which is pretty inaccurate as you need to know the current barometric pressure at sea level for your geolocation and weather conditions) will never show in the app. If you shuffle the way the CayenneLPP is constructed (e.g. altitude first, temperature next, etcetera) you won't see any values at all so somehow altitude (incorrectly computed to 122m in my location) 'breaks' the implementation. I am not sure if that's caused by CayenneLPP or the app as I have no clue how to debug either one of them apart from flashing a newly built firmware.

[jbrazio]

The difference is 1-2 degree. This is close and useful enough.

I always said it's useful, specially when it's "for free", the hardware is there.. there is no reason not to publish it's values.

Nevertheless, we should use a different tag/channel/whatever, the point being: do not show it's value where a user expects to see environmental temperature. It's on the product manufacturer's data sheet for both MCU the intended purpose of that sensor.

[IoTThinks]

@jbrazio True.
I suggest to use channel 1 for anything built-in.
Such as battery voltage and MCU temperature.

Channel 2 for external sensors.

Best if we can name the channel to something meaningful to users.

[wel97459]

I could add a channel abbreviation that would fit in 4 byte of the telemetry.

@jbrazio True.
I suggest to use channel 1 for anything built-in.
Such as battery voltage and MCU temperature.

Channel 2 for external sensors.

Best if we can name the channel to something meaningful to users.

[4np]

Ah, I observed earlier the Telemetry output would be cut off if you add altitude.

In the release notes for App version 1.35.0 @liamcottle wrote:

• add support for standalone altitude in the telemetry response

Does that mean that any telemetry data that is supported by CayenneLPP but not handled in the mobile apps will break telemetry rendering? Shouldn't the mobile apps cover all possible CayenneLPP values?

[liamcottle]

Does that mean that any telemetry data that is supported by CayenneLPP but not handled in the mobile apps will break telemetry rendering?

@4np yeah, unfortunately there's no length info in cayenne lpp, you have to know how to parse the telemetry type ahead of time.

So, if the app doesn't know about a specific cayenne lpp type, it can't parse any fields after the unknown field, even if it knows how to parse the latter fields.

For now, you'd have to make sure unknown or custom fields are at the end of the telemetry payload, otherwise as you saw, known telemetry is not parsed.

I think we will have to move away from cayenne, or implement custom telemetry types that include length info in the payload. Otherwise this is going to be painful when adding support for new types down the road.

Shouldn't the mobile apps cover all possible CayenneLPP values?

Currently, the app doesn't parse all lpp types, because they were never used officially.

[ripplebiz]

while loops like this make me nervous. Prefer to also have a timeout check

[IoTThinks]

Ok, let me replace by some small delay and test.

Thanks a lot.

[liamcottle]

Did you try using the soft device service call for this instead? Might be safer, and it already handles the delay internally. According to the docs, the call takes roughly 50us.

#include "nrf_temp.h"

int32_t temp = 0;
if(sd_temp_get(&temp) == NRF_SUCCESS){
  double temperature = temp / 4.0;
}

/**@brief Get the temperature measured on the chip
 *
 * This function will block until the temperature measurement is done.
 * It takes around 50 us from call to return.
 *
 * @param[out] p_temp Result of temperature measurement. Die temperature in 0.25 degrees Celsius.
 *
 * @retval ::NRF_SUCCESS A temperature measurement was done, and the temperature was written to temp
 */
SVCALL(SD_TEMP_GET, uint32_t, sd_temp_get(int32_t * p_temp));

[IoTThinks]

@liamcottle Let me try and test with my Heltec v3, RAK and T-Echo Lite.

[IoTThinks]

@liamcottle This code will always return 0. E.g temp = 0
AFAIK, in repeaters, we do not use Soft Device (BLE), so this call sd_... are not usable.

Are we using Soft Device?

#include "nrf_temp.h"

int32_t temp = 0;
if(sd_temp_get(&temp) == NRF_SUCCESS){
  double temperature = temp / 4.0;
}

[IoTThinks]

Tested working with RAK4631 and T-Echo Lite.
I submitted the code change at d9a02b2

// Temperature from NRF52 MCU
float NRF52Board::getMCUTemperature() {
  NRF_TEMP->TASKS_START = 1; // Start temperature measurement

  long startTime = millis();  
  while (NRF_TEMP->EVENTS_DATARDY == 0) { // Wait for completion. Should complete in 50us
    if(millis() - startTime > 1) {  // To wait 1ms
      NRF_TEMP->TASKS_STOP = 1;
      return NAN;
    }
  }
  
  NRF_TEMP->EVENTS_DATARDY = 0; // Clear event flag

  int32_t temp = NRF_TEMP->TEMP; // In 0.25°C units
  NRF_TEMP->TASKS_STOP = 1;

  return temp * 0.25f; // Convert to °C
}

[ripplebiz]

@jbrazio True. I suggest to use channel 1 for anything built-in. Such as battery voltage and MCU temperature.

Channel 2 for external sensors.

Best if we can name the channel to something meaningful to users.

Yeah, I like that suggestion. Channel 1 for internal/device, 2 for external/environ

[redfast00]

Yeah, I like that suggestion. Channel 1 for internal/device, 2 for external/environ

I've ordered a (self-designed) custom board that has 3 voltage & current measurement channels, measured by the INA3221 (battery, solar panel and vsys). What CayenneLPP channels should I use for this?

[redfast00]

Nitpick: if you're unlucky, millis returns 0 in the first call (while being internally at 0.9999999 ms) and then instantly rolls over to 1 in the next call, without 50us having passed.

Additionally, what happens if millis() rolls over (after 49 days)?

[IoTThinks]

@redfast00

1. Well, a repeater takes several seconds even minutes to finish initiazliation.
   When you manage to do remote management to a repeater, it takes even a few minutes to get to this point.
   How possible this millis returns 0?

2. The NRF_TEMP->EVENTS_DATARDY is expected to complete within 50us. This is a commitment fron NRF52.
   If this is happened, the NRF52 MCU is faulty and it should be replaced before we talk about this temperature.
   Furthermore, the timeout is an additional protection.
   The chance for both conditions to hold true in this case is much lower than winning a lottery in a lifetime.

[redfast00]

@IoTThinks good point on point 2 :)

On point 1, I think I should have clarified further: the granularity of millis() is not enough if you want to be guaranteed to wait at least 50us by setting a timeout of 1ms. Another example: if you are at 50000.999999999 ms since boot, the first millis call will return 50000. Before 50us has passed, the next millis call will already return 50001.

This could be fixed by changing the timeout to 2 milliseconds instead, that way, you're guaranteed to wait at least 1 full millisecond instead of having no minimal guaranteed timeout.

[redfast00]

Point 1 actually wasn't as nitpicky as I though: if you keep the code as-is, you'll see the timeout code trigger before 50us 5% of all calls, on average.

[IoTThinks]

@redfast00 How do you calculate the 5%?
Just keen to know.

1ms or even 10ms is fine for me.

The app is not required to be responsive at ms anyway.

[redfast00]

How did I calculate the 5%?

At the end of each millisecond real time, the millis() function return value will increase by one. If this happens before 50us of real time has passed, we say the code 'fails' (since it aborts the measurement without 50us having passed). So if the code starts at the end of n ms - 20 us real time for example, it fails, since it will already roll over at n ms. This roll-over happens every millisecond, and the unsafe window is between n ms - 50us to n ms - 0.0000000..1us. So for 50us every millisecond, we're 'unsafe', so this is 50us / 1ms = 5%.

[oltaco]

I like what you're trying to do here, I think it is useful. 👍

The PR says it's for repeaters but doesn't it get included for companions as well? For NRF52 you should probably be checking if softdevice is enabled and using the appropriate sd call if it is:

uint8_t sd_enabled = 0;
sd_softdevice_is_enabled(&sd_enabled);
if (sd_enabled) {

Also please can you do a squash rebase once the code is deemed ready for merge? There are 17 separate commits with the same message "To extend NRF52Board", imho these should just be one commit. To be honest it looks like the whole thing should probably be just 3 commits.

[IoTThinks]

@oltaco The code can be reused for companion later.

This code works regardless if we are using Soft Device or not.

True, I keep improving the code so having many commits.
Sorry about that.

[IoTThinks]

Let me cleanup the commit and open another clean PR.

[fschrempf]

Let me cleanup the commit and open another clean PR.

You don't need a new PR for that. You can use for example git rebase -i to adjust your commits and then git push --force to overwrite the previous version of the branch. The PR gets updated automatically and GitHub lets you compare the versions of the PR and makes it easy for others to track the progress.

Opening a new PR can even be counterproductive as it makes it hard to track your changes and follow the development.

[IoTThinks]

New PR is here.
Clean and clear.
#1206